Re: [PATCH 1/4] selftests: ublk: fix UBLK_F_NEED_GET_DATA

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

On Sat, Apr 26, 2025 at 01:15:58PM -0700, Caleb Sander Mateos wrote:
> On Sat, Apr 26, 2025 at 2:41 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.
> 
> Looks good to me, just a few minor comments.
> 
> 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          | 11 +++++---
> >  tools/testing/selftests/ublk/kublk.h          |  1 +
> >  .../testing/selftests/ublk/test_generic_07.sh | 25 +++++++++++++++++++
> >  4 files changed, 35 insertions(+), 3 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..701b47f98902 100644
> > --- a/tools/testing/selftests/ublk/kublk.c
> > +++ b/tools/testing/selftests/ublk/kublk.c
> > @@ -538,10 +538,12 @@ int ublk_queue_io_cmd(struct ublk_queue *q, struct ublk_io *io, unsigned tag)
> >
> >         /* we issue because we need either fetching or committing */
> >         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)))
> 
> Comment could use an update
> 
> >                 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 +660,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 +1318,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:g:az",
> >                                   longopts, &option_idx)) != -1) {
> 
> It's a little strange for -g to take an argument since it's basically
> a boolean flag. But it looks like several other flags behave the same
> way.

Yeah, we can make it one bool flag.


> 
> >                 switch (opt) {
> >                 case 'a':
> > 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..5d82b5955006
> > --- /dev/null
> > +++ b/tools/testing/selftests/ublk/test_generic_07.sh
> > @@ -0,0 +1,25 @@
> > +#!/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 1 "${UBLK_BACKFILES[0]}")
> > +_check_add_dev $TID $?
> > +
> > +# run fio over the ublk disk
> > +if ! _run_fio_verify_io --filename=/dev/ublkb"${dev_id}" --size=256M; then
> > +       _cleanup_test "generic"
> > +       _show_result $TID 255
> 
> Propagate the return code from fio?

OK.

> 
> > +fi
> 
> All the other tests that use _run_fio_verify_io appear to check
> _have_program fio first. Is that necessary here too?

Will add the check.


Thanks, 
Ming