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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > Fixes: 57e13a2e8cd2 ("selftests: ublk: support user recovery")
> > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > ---
> > 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
