| Message ID | 20250325-ublk_timeout-v1-0-262f0121a7bd@purestorage.com |
|---|---|
| Headers | show |
| Series | ublk: improve handling of saturated queues when ublk server exits | expand |
On Tue, Mar 25, 2025 at 04:19:31PM -0600, Uday Shankar wrote: > There are a couple of places in the kublk selftests ublk server which > use the legacy ublk opcodes. These operations fail (with -EOPNOTSUPP) on > a kernel compiled without CONFIG_BLKDEV_UBLK_LEGACY_OPCODES set. We > could easily require it to be set as a prerequisite for these selftests, > but since new applications should not be using the legacy opcodes, use > the ioctl-encoded opcodes everywhere in kublk. Is it required to allow for the building of old userspace code (using legacy opcodes) against new kernel headers? Or do we only need to guarantee that old userspace code using legacy opcodes that is already compiled continues to work against newer ublk_drv? If it's the latter case, maybe we can consider removing the legacy opcode definitions from the userspace header as a follow-on change?
On Tue, Mar 25, 2025 at 04:19:31PM -0600, Uday Shankar wrote: > There are a couple of places in the kublk selftests ublk server which > use the legacy ublk opcodes. These operations fail (with -EOPNOTSUPP) on > a kernel compiled without CONFIG_BLKDEV_UBLK_LEGACY_OPCODES set. We > could easily require it to be set as a prerequisite for these selftests, > but since new applications should not be using the legacy opcodes, use > the ioctl-encoded opcodes everywhere in kublk. > > Signed-off-by: Uday Shankar <ushankar@purestorage.com> Reviewed-by: Ming Lei <ming.lei@redhat.com>
On Tue, Mar 25, 2025 at 04:19:32PM -0600, Uday Shankar wrote: > When doing io_uring operations using liburing, errno is not used to > indicate errors, so the %m format specifier does not provide any > relevant information for failed io_uring commands. Fix a log line > emitted on get_params failure to translate the error code returned in > the cqe->res field instead. > > Signed-off-by: Uday Shankar <ushankar@purestorage.com> > --- > tools/testing/selftests/ublk/kublk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c > index b17eee643b2dbfd59903b61718afcbc21da91d97..ded1b93e7913011499ae5dae7b40f0e425982ee4 100644 > --- a/tools/testing/selftests/ublk/kublk.c > +++ b/tools/testing/selftests/ublk/kublk.c > @@ -215,7 +215,7 @@ static void ublk_ctrl_dump(struct ublk_dev *dev) > > ret = ublk_ctrl_get_params(dev, &p); > if (ret < 0) { > - ublk_err("failed to get params %m\n"); > + ublk_err("failed to get params %d %s\n", ret, strerror(-ret)); > return; Reviewed-by: Ming Lei <ming.lei@redhat.com>
On Tue, Mar 25, 2025 at 04:19:33PM -0600, Uday Shankar wrote: > SIGCHLD from exiting children can arrive during io_uring_wait_cqe and > cause it to return early with -EINTR. Since we don't have a handler for Probably -EINTR needs to be handled, and libublksrv retry in case of -EINTR. > SIGCHLD, avoid this issue by ignoring SIGCHLD. > > Signed-off-by: Uday Shankar <ushankar@purestorage.com> > --- > tools/testing/selftests/ublk/kublk.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c > index ded1b93e7913011499ae5dae7b40f0e425982ee4..064a5bb6f12f35892065b8dfacb6f57f6fc16aee 100644 > --- a/tools/testing/selftests/ublk/kublk.c > +++ b/tools/testing/selftests/ublk/kublk.c > @@ -890,6 +890,7 @@ static int cmd_dev_add(struct dev_ctx *ctx) > exit(-1); > } > > + signal(SIGCHLD, SIG_IGN); Reviewed-by: Ming Lei <ming.lei@redhat.com> BTW, the SIGCHLD signal is ignored by default, looks it is good to do it explicitly, if the -EINTR from io_uring_enter() can be avoided in this way. Thanks, Ming
On Wed, Mar 26, 2025 at 11:23:25AM +0800, Ming Lei wrote:
> BTW, the SIGCHLD signal is ignored by default
You're right, that's a good point, this change is actually a noop. I
think I got confused during test development and the -EINTR probably
came from something else. I see no issues when reverting this change in
my tests now, so I will drop it.
This set aims to reduce the long delay in applications reacting to ublk server exit in the case of a "fully saturated" queue, i.e. one for which all I/Os are outstanding to the ublk server. The first few patches fix some minor issues in the ublk selftests, and the last patch contains the main work and a test to validate it. Signed-off-by: Uday Shankar <ushankar@purestorage.com> --- Uday Shankar (4): selftests: ublk: kublk: use ioctl-encoded opcodes selftests: ublk: kublk: fix an error log line selftests: ublk: kublk: ignore SIGCHLD ublk: improve handling of saturated queues when ublk server exits drivers/block/ublk_drv.c | 40 +++++++++++------------ tools/testing/selftests/ublk/Makefile | 1 + tools/testing/selftests/ublk/kublk.c | 10 ++++-- tools/testing/selftests/ublk/kublk.h | 3 ++ tools/testing/selftests/ublk/null.c | 4 +++ tools/testing/selftests/ublk/test_generic_02.sh | 43 +++++++++++++++++++++++++ 6 files changed, 76 insertions(+), 25 deletions(-) --- base-commit: 648154b1c78c9e00b6934082cae48bb38714de20 change-id: 20250325-ublk_timeout-b06b9b51c591 Best regards,