Re: [PATCH v6.15 1/3] selftests: ublk: fix UBLK_F_NEED_GET_DATA

[Ming Lei <ming.lei@redhat.com>]

On Mon, Apr 28, 2025 at 08:51:03AM -0700, Caleb Sander Mateos wrote:
> On Sun, Apr 27, 2025 at 6:49 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > Commit 57e13a2e8cd2 ("selftests: ublk: support user recovery") starts to
> > support UBLK_F_NEED_GET_DATA for covering recovery feature, however the
> > ublk utility implementation isn't done correctly.
> >
> > Fix it by supporting UBLK_F_NEED_GET_DATA correctly.
> >
> > Also add test generic_07 for covering UBLK_F_NEED_GET_DATA.
> >
> > Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
> > Fixes: 57e13a2e8cd2 ("selftests: ublk: support user recovery")
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  tools/testing/selftests/ublk/Makefile         |  1 +
> >  tools/testing/selftests/ublk/kublk.c          | 20 ++++++++++------
> >  tools/testing/selftests/ublk/kublk.h          |  1 +
> >  .../testing/selftests/ublk/test_generic_07.sh | 24 +++++++++++++++++++
> >  .../testing/selftests/ublk/test_stress_05.sh  |  8 +++----
> >  5 files changed, 43 insertions(+), 11 deletions(-)
> >  create mode 100755 tools/testing/selftests/ublk/test_generic_07.sh
> >
> > diff --git a/tools/testing/selftests/ublk/Makefile b/tools/testing/selftests/ublk/Makefile
> > index ec4624a283bc..f34ac0bac696 100644
> > --- a/tools/testing/selftests/ublk/Makefile
> > +++ b/tools/testing/selftests/ublk/Makefile
> > @@ -9,6 +9,7 @@ TEST_PROGS += test_generic_03.sh
> >  TEST_PROGS += test_generic_04.sh
> >  TEST_PROGS += test_generic_05.sh
> >  TEST_PROGS += test_generic_06.sh
> > +TEST_PROGS += test_generic_07.sh
> >
> >  TEST_PROGS += test_null_01.sh
> >  TEST_PROGS += test_null_02.sh
> > diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c
> > index e57a1486bb48..3afd45d7f989 100644
> > --- a/tools/testing/selftests/ublk/kublk.c
> > +++ b/tools/testing/selftests/ublk/kublk.c
> > @@ -536,12 +536,17 @@ int ublk_queue_io_cmd(struct ublk_queue *q, struct ublk_io *io, unsigned tag)
> >         if (!(io->flags & UBLKSRV_IO_FREE))
> >                 return 0;
> >
> > -       /* we issue because we need either fetching or committing */
> > +       /*
> > +        * we issue because we need either fetching or committing or
> > +        * getting data
> > +        */
> >         if (!(io->flags &
> > -               (UBLKSRV_NEED_FETCH_RQ | UBLKSRV_NEED_COMMIT_RQ_COMP)))
> > +               (UBLKSRV_NEED_FETCH_RQ | UBLKSRV_NEED_COMMIT_RQ_COMP | UBLKSRV_NEED_GET_DATA)))
> >                 return 0;
> >
> > -       if (io->flags & UBLKSRV_NEED_COMMIT_RQ_COMP)
> > +       if (io->flags & UBLKSRV_NEED_GET_DATA)
> > +               cmd_op = UBLK_U_IO_NEED_GET_DATA;
> > +       else if (io->flags & UBLKSRV_NEED_COMMIT_RQ_COMP)
> >                 cmd_op = UBLK_U_IO_COMMIT_AND_FETCH_REQ;
> >         else if (io->flags & UBLKSRV_NEED_FETCH_RQ)
> >                 cmd_op = UBLK_U_IO_FETCH_REQ;
> > @@ -658,6 +663,9 @@ static void ublk_handle_cqe(struct io_uring *r,
> >                 assert(tag < q->q_depth);
> >                 if (q->tgt_ops->queue_io)
> >                         q->tgt_ops->queue_io(q, tag);
> > +       } else if (cqe->res == UBLK_IO_RES_NEED_GET_DATA) {
> > +               io->flags |= UBLKSRV_NEED_GET_DATA | UBLKSRV_IO_FREE;
> > +               ublk_queue_io_cmd(q, io, tag);
> >         } else {
> >                 /*
> >                  * COMMIT_REQ will be completed immediately since no fetching
> > @@ -1313,7 +1321,7 @@ int main(int argc, char *argv[])
> >
> >         opterr = 0;
> >         optind = 2;
> > -       while ((opt = getopt_long(argc, argv, "t:n:d:q:r:e:i:az",
> > +       while ((opt = getopt_long(argc, argv, "t:n:d:q:r:e:i:gaz",
> >                                   longopts, &option_idx)) != -1) {
> >                 switch (opt) {
> >                 case 'a':
> > @@ -1351,9 +1359,7 @@ int main(int argc, char *argv[])
> >                                 ctx.flags |= UBLK_F_USER_RECOVERY | UBLK_F_USER_RECOVERY_REISSUE;
> >                         break;
> >                 case 'g':
> > -                       value = strtol(optarg, NULL, 10);
> > -                       if (value)
> > -                               ctx.flags |= UBLK_F_NEED_GET_DATA;
> > +                       ctx.flags |= UBLK_F_NEED_GET_DATA;
> 
> The help text in __cmd_create_help() should be updated accordingly.

Good catch!

> 
> >                         break;
> >                 case 0:
> >                         if (!strcmp(longopts[option_idx].name, "debug_mask"))
> > diff --git a/tools/testing/selftests/ublk/kublk.h b/tools/testing/selftests/ublk/kublk.h
> > index 918db5cd633f..44ee1e4ac55b 100644
> > --- a/tools/testing/selftests/ublk/kublk.h
> > +++ b/tools/testing/selftests/ublk/kublk.h
> > @@ -115,6 +115,7 @@ struct ublk_io {
> >  #define UBLKSRV_NEED_FETCH_RQ          (1UL << 0)
> >  #define UBLKSRV_NEED_COMMIT_RQ_COMP    (1UL << 1)
> >  #define UBLKSRV_IO_FREE                        (1UL << 2)
> > +#define UBLKSRV_NEED_GET_DATA           (1UL << 3)
> >         unsigned short flags;
> >         unsigned short refs;            /* used by target code only */
> >
> > diff --git a/tools/testing/selftests/ublk/test_generic_07.sh b/tools/testing/selftests/ublk/test_generic_07.sh
> > new file mode 100755
> > index 000000000000..e3ad36ef7b9a
> > --- /dev/null
> > +++ b/tools/testing/selftests/ublk/test_generic_07.sh
> > @@ -0,0 +1,24 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +. "$(cd "$(dirname "$0")" && pwd)"/test_common.sh
> > +
> > +TID="generic_07"
> > +ERR_CODE=0
> > +
> > +_prep_test "generic" "test UBLK_F_NEED_GET_DATA"
> > +
> > +_create_backfile 0 256M
> > +dev_id=$(_add_ublk_dev -t loop -q 2 -g "${UBLK_BACKFILES[0]}")
> > +_check_add_dev $TID $?
> > +
> > +# run fio over the ublk disk
> > +_run_fio_verify_io --filename=/dev/ublkb"${dev_id}" --size=256M
> 
> I thought you were planning to add a _have_program fio check?

Indeed, don't know how the check wasn't added, :-(


thanks,
Ming