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@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.
>
> Looks good to me, just a few minor comments.
>
> 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 | 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
