[PATCH v6.15 0/3] ublk: one selftest fix and two zero copy fixes

[PATCH v6.15 0/3] ublk: one selftest fix and two zero copy fixes

[Ming Lei]

Hello Jens,

The 1st patch fixes UBLK_F_NEED_GET_DATA support in ublk selftest side.

The other two patches enhances check for zero copy feature.

Thanks,
Ming


Ming Lei (3):
  selftests: ublk: fix UBLK_F_NEED_GET_DATA
  ublk: decouple zero copy from user copy
  ublk: enhance check for register/unregister io buffer command

 drivers/block/ublk_drv.c                      | 59 ++++++++++++++-----
 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 +--
 6 files changed, 86 insertions(+), 27 deletions(-)
 create mode 100755 tools/testing/selftests/ublk/test_generic_07.sh

-- 
2.47.0

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

[Ming Lei]

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;
 			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
+ERR_CODE=$?
+if [ "$ERR_CODE" -eq 0 ]; then
+	_mkfs_mount_test /dev/ublkb"${dev_id}"
+	ERR_CODE=$?
+fi
+
+_cleanup_test "generic"
+_show_result $TID $ERR_CODE
diff --git a/tools/testing/selftests/ublk/test_stress_05.sh b/tools/testing/selftests/ublk/test_stress_05.sh
index a7071b10224d..88601b48f1cd 100755
--- a/tools/testing/selftests/ublk/test_stress_05.sh
+++ b/tools/testing/selftests/ublk/test_stress_05.sh
@@ -47,15 +47,15 @@ _create_backfile 0 256M
 _create_backfile 1 256M
 
 for reissue in $(seq 0 1); do
-	ublk_io_and_remove 8G -t null -q 4 -g 1 -r 1 -i "$reissue" &
-	ublk_io_and_remove 256M -t loop -q 4 -g 1 -r 1 -i "$reissue" "${UBLK_BACKFILES[0]}" &
+	ublk_io_and_remove 8G -t null -q 4 -g -r 1 -i "$reissue" &
+	ublk_io_and_remove 256M -t loop -q 4 -g -r 1 -i "$reissue" "${UBLK_BACKFILES[0]}" &
 	wait
 done
 
 if _have_feature "ZERO_COPY"; then
 	for reissue in $(seq 0 1); do
-		ublk_io_and_remove 8G -t null -q 4 -g 1 -z -r 1 -i "$reissue" &
-		ublk_io_and_remove 256M -t loop -q 4 -g 1 -z -r 1 -i "$reissue" "${UBLK_BACKFILES[1]}" &
+		ublk_io_and_remove 8G -t null -q 4 -g -z -r 1 -i "$reissue" &
+		ublk_io_and_remove 256M -t loop -q 4 -g -z -r 1 -i "$reissue" "${UBLK_BACKFILES[1]}" &
 		wait
 	done
 fi
-- 
2.47.0

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

[Caleb Sander Mateos]

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.

>                         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?

Best,
Caleb

> +ERR_CODE=$?
> +if [ "$ERR_CODE" -eq 0 ]; then
> +       _mkfs_mount_test /dev/ublkb"${dev_id}"
> +       ERR_CODE=$?
> +fi
> +
> +_cleanup_test "generic"
> +_show_result $TID $ERR_CODE
> diff --git a/tools/testing/selftests/ublk/test_stress_05.sh b/tools/testing/selftests/ublk/test_stress_05.sh
> index a7071b10224d..88601b48f1cd 100755
> --- a/tools/testing/selftests/ublk/test_stress_05.sh
> +++ b/tools/testing/selftests/ublk/test_stress_05.sh
> @@ -47,15 +47,15 @@ _create_backfile 0 256M
>  _create_backfile 1 256M
>
>  for reissue in $(seq 0 1); do
> -       ublk_io_and_remove 8G -t null -q 4 -g 1 -r 1 -i "$reissue" &
> -       ublk_io_and_remove 256M -t loop -q 4 -g 1 -r 1 -i "$reissue" "${UBLK_BACKFILES[0]}" &
> +       ublk_io_and_remove 8G -t null -q 4 -g -r 1 -i "$reissue" &
> +       ublk_io_and_remove 256M -t loop -q 4 -g -r 1 -i "$reissue" "${UBLK_BACKFILES[0]}" &
>         wait
>  done
>
>  if _have_feature "ZERO_COPY"; then
>         for reissue in $(seq 0 1); do
> -               ublk_io_and_remove 8G -t null -q 4 -g 1 -z -r 1 -i "$reissue" &
> -               ublk_io_and_remove 256M -t loop -q 4 -g 1 -z -r 1 -i "$reissue" "${UBLK_BACKFILES[1]}" &
> +               ublk_io_and_remove 8G -t null -q 4 -g -z -r 1 -i "$reissue" &
> +               ublk_io_and_remove 256M -t loop -q 4 -g -z -r 1 -i "$reissue" "${UBLK_BACKFILES[1]}" &
>                 wait
>         done
>  fi
> --
> 2.47.0
>

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

[Ming Lei]

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

[PATCH v6.15 2/3] ublk: decouple zero copy from user copy

[Ming Lei]

UBLK_F_USER_COPY and UBLK_F_SUPPORT_ZERO_COPY are two different
features, and shouldn't be coupled together.

Commit 1f6540e2aabb ("ublk: zc register/unregister bvec") enables
user copy automatically in case of UBLK_F_SUPPORT_ZERO_COPY, this way
isn't correct.

So decouple zero copy from user copy, and use independent helper to
check each one.

Fixes: 1f6540e2aabb ("ublk: zc register/unregister bvec")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 40f971a66d3e..0a3a3c64316d 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -205,11 +205,6 @@ static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
 static inline unsigned int ublk_req_build_flags(struct request *req);
 static inline struct ublksrv_io_desc *ublk_get_iod(struct ublk_queue *ubq,
 						   int tag);
-static inline bool ublk_dev_is_user_copy(const struct ublk_device *ub)
-{
-	return ub->dev_info.flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
-}
-
 static inline bool ublk_dev_is_zoned(const struct ublk_device *ub)
 {
 	return ub->dev_info.flags & UBLK_F_ZONED;
@@ -609,14 +604,19 @@ static void ublk_apply_params(struct ublk_device *ub)
 		ublk_dev_param_zoned_apply(ub);
 }
 
+static inline bool ublk_support_zero_copy(const struct ublk_queue *ubq)
+{
+	return ubq->flags & UBLK_F_SUPPORT_ZERO_COPY;
+}
+
 static inline bool ublk_support_user_copy(const struct ublk_queue *ubq)
 {
-	return ubq->flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
+	return ubq->flags & UBLK_F_USER_COPY;
 }
 
 static inline bool ublk_need_map_io(const struct ublk_queue *ubq)
 {
-	return !ublk_support_user_copy(ubq);
+	return !ublk_support_user_copy(ubq) && !ublk_support_zero_copy(ubq);
 }
 
 static inline bool ublk_need_req_ref(const struct ublk_queue *ubq)
@@ -624,8 +624,11 @@ static inline bool ublk_need_req_ref(const struct ublk_queue *ubq)
 	/*
 	 * read()/write() is involved in user copy, so request reference
 	 * has to be grabbed
+	 *
+	 * for zero copy, request buffer need to be registered to io_uring
+	 * buffer table, so reference is needed
 	 */
-	return ublk_support_user_copy(ubq);
+	return ublk_support_user_copy(ubq) || ublk_support_zero_copy(ubq);
 }
 
 static inline void ublk_init_req_ref(const struct ublk_queue *ubq,
@@ -2245,6 +2248,9 @@ static struct request *ublk_check_and_get_req(struct kiocb *iocb,
 	if (!ubq)
 		return ERR_PTR(-EINVAL);
 
+	if (!ublk_support_user_copy(ubq))
+		return ERR_PTR(-EACCES);
+
 	if (tag >= ubq->q_depth)
 		return ERR_PTR(-EINVAL);
 
@@ -2783,13 +2789,18 @@ static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header)
 	ub->dev_info.flags |= UBLK_F_CMD_IOCTL_ENCODE |
 		UBLK_F_URING_CMD_COMP_IN_TASK;
 
-	/* GET_DATA isn't needed any more with USER_COPY */
-	if (ublk_dev_is_user_copy(ub))
+	/* GET_DATA isn't needed any more with USER_COPY or ZERO COPY */
+	if (ub->dev_info.flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY))
 		ub->dev_info.flags &= ~UBLK_F_NEED_GET_DATA;
 
-	/* Zoned storage support requires user copy feature */
+	/*
+	 * Zoned storage support requires reuse `ublksrv_io_cmd->addr` for
+	 * returning write_append_lba, which is only allowed in case of
+	 * user copy or zero copy
+	 */
 	if (ublk_dev_is_zoned(ub) &&
-	    (!IS_ENABLED(CONFIG_BLK_DEV_ZONED) || !ublk_dev_is_user_copy(ub))) {
+	    (!IS_ENABLED(CONFIG_BLK_DEV_ZONED) || !(ub->dev_info.flags &
+	     (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY)))) {
 		ret = -EINVAL;
 		goto out_free_dev_number;
 	}
-- 
2.47.0

Re: [PATCH v6.15 2/3] ublk: decouple zero copy from user copy

[Caleb Sander Mateos]

On Sun, Apr 27, 2025 at 6:49 AM Ming Lei <ming.lei@redhat.com> wrote:
>
> UBLK_F_USER_COPY and UBLK_F_SUPPORT_ZERO_COPY are two different
> features, and shouldn't be coupled together.
>
> Commit 1f6540e2aabb ("ublk: zc register/unregister bvec") enables
> user copy automatically in case of UBLK_F_SUPPORT_ZERO_COPY, this way
> isn't correct.
>
> So decouple zero copy from user copy, and use independent helper to
> check each one.

I agree this makes sense.

>
> Fixes: 1f6540e2aabb ("ublk: zc register/unregister bvec")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/block/ublk_drv.c | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 40f971a66d3e..0a3a3c64316d 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -205,11 +205,6 @@ static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
>  static inline unsigned int ublk_req_build_flags(struct request *req);
>  static inline struct ublksrv_io_desc *ublk_get_iod(struct ublk_queue *ubq,
>                                                    int tag);
> -static inline bool ublk_dev_is_user_copy(const struct ublk_device *ub)
> -{
> -       return ub->dev_info.flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
> -}
> -
>  static inline bool ublk_dev_is_zoned(const struct ublk_device *ub)
>  {
>         return ub->dev_info.flags & UBLK_F_ZONED;
> @@ -609,14 +604,19 @@ static void ublk_apply_params(struct ublk_device *ub)
>                 ublk_dev_param_zoned_apply(ub);
>  }
>
> +static inline bool ublk_support_zero_copy(const struct ublk_queue *ubq)
> +{
> +       return ubq->flags & UBLK_F_SUPPORT_ZERO_COPY;
> +}
> +
>  static inline bool ublk_support_user_copy(const struct ublk_queue *ubq)
>  {
> -       return ubq->flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
> +       return ubq->flags & UBLK_F_USER_COPY;
>  }
>
>  static inline bool ublk_need_map_io(const struct ublk_queue *ubq)
>  {
> -       return !ublk_support_user_copy(ubq);
> +       return !ublk_support_user_copy(ubq) && !ublk_support_zero_copy(ubq);
>  }
>
>  static inline bool ublk_need_req_ref(const struct ublk_queue *ubq)
> @@ -624,8 +624,11 @@ static inline bool ublk_need_req_ref(const struct ublk_queue *ubq)
>         /*
>          * read()/write() is involved in user copy, so request reference
>          * has to be grabbed
> +        *
> +        * for zero copy, request buffer need to be registered to io_uring
> +        * buffer table, so reference is needed
>          */
> -       return ublk_support_user_copy(ubq);
> +       return ublk_support_user_copy(ubq) || ublk_support_zero_copy(ubq);
>  }
>
>  static inline void ublk_init_req_ref(const struct ublk_queue *ubq,
> @@ -2245,6 +2248,9 @@ static struct request *ublk_check_and_get_req(struct kiocb *iocb,
>         if (!ubq)
>                 return ERR_PTR(-EINVAL);
>
> +       if (!ublk_support_user_copy(ubq))
> +               return ERR_PTR(-EACCES);

This partly overlaps with the existing ublk_need_req_ref() check in
__ublk_check_and_get_req() (although that allows
UBLK_F_SUPPORT_ZERO_COPY too). Can that check be removed now that the
callers explicitly check ublk_support_user_copy() or
ublk_support_zero_copy()?

> +
>         if (tag >= ubq->q_depth)
>                 return ERR_PTR(-EINVAL);
>
> @@ -2783,13 +2789,18 @@ static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header)
>         ub->dev_info.flags |= UBLK_F_CMD_IOCTL_ENCODE |
>                 UBLK_F_URING_CMD_COMP_IN_TASK;
>
> -       /* GET_DATA isn't needed any more with USER_COPY */
> -       if (ublk_dev_is_user_copy(ub))
> +       /* GET_DATA isn't needed any more with USER_COPY or ZERO COPY */
> +       if (ub->dev_info.flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY))
>                 ub->dev_info.flags &= ~UBLK_F_NEED_GET_DATA;
>
> -       /* Zoned storage support requires user copy feature */
> +       /*
> +        * Zoned storage support requires reuse `ublksrv_io_cmd->addr` for
> +        * returning write_append_lba, which is only allowed in case of
> +        * user copy or zero copy

Thanks, this comment is much more helpful.

Best,
Caleb

> +        */
>         if (ublk_dev_is_zoned(ub) &&
> -           (!IS_ENABLED(CONFIG_BLK_DEV_ZONED) || !ublk_dev_is_user_copy(ub))) {
> +           (!IS_ENABLED(CONFIG_BLK_DEV_ZONED) || !(ub->dev_info.flags &
> +            (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY)))) {
>                 ret = -EINVAL;
>                 goto out_free_dev_number;
>         }
> --
> 2.47.0
>

Re: [PATCH v6.15 2/3] ublk: decouple zero copy from user copy

[Ming Lei]

On Mon, Apr 28, 2025 at 09:01:04AM -0700, Caleb Sander Mateos wrote:
> On Sun, Apr 27, 2025 at 6:49 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > UBLK_F_USER_COPY and UBLK_F_SUPPORT_ZERO_COPY are two different
> > features, and shouldn't be coupled together.
> >
> > Commit 1f6540e2aabb ("ublk: zc register/unregister bvec") enables
> > user copy automatically in case of UBLK_F_SUPPORT_ZERO_COPY, this way
> > isn't correct.
> >
> > So decouple zero copy from user copy, and use independent helper to
> > check each one.
> 
> I agree this makes sense.
> 
> >
> > Fixes: 1f6540e2aabb ("ublk: zc register/unregister bvec")
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/block/ublk_drv.c | 35 +++++++++++++++++++++++------------
> >  1 file changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 40f971a66d3e..0a3a3c64316d 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -205,11 +205,6 @@ static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> >  static inline unsigned int ublk_req_build_flags(struct request *req);
> >  static inline struct ublksrv_io_desc *ublk_get_iod(struct ublk_queue *ubq,
> >                                                    int tag);
> > -static inline bool ublk_dev_is_user_copy(const struct ublk_device *ub)
> > -{
> > -       return ub->dev_info.flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
> > -}
> > -
> >  static inline bool ublk_dev_is_zoned(const struct ublk_device *ub)
> >  {
> >         return ub->dev_info.flags & UBLK_F_ZONED;
> > @@ -609,14 +604,19 @@ static void ublk_apply_params(struct ublk_device *ub)
> >                 ublk_dev_param_zoned_apply(ub);
> >  }
> >
> > +static inline bool ublk_support_zero_copy(const struct ublk_queue *ubq)
> > +{
> > +       return ubq->flags & UBLK_F_SUPPORT_ZERO_COPY;
> > +}
> > +
> >  static inline bool ublk_support_user_copy(const struct ublk_queue *ubq)
> >  {
> > -       return ubq->flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
> > +       return ubq->flags & UBLK_F_USER_COPY;
> >  }
> >
> >  static inline bool ublk_need_map_io(const struct ublk_queue *ubq)
> >  {
> > -       return !ublk_support_user_copy(ubq);
> > +       return !ublk_support_user_copy(ubq) && !ublk_support_zero_copy(ubq);
> >  }
> >
> >  static inline bool ublk_need_req_ref(const struct ublk_queue *ubq)
> > @@ -624,8 +624,11 @@ static inline bool ublk_need_req_ref(const struct ublk_queue *ubq)
> >         /*
> >          * read()/write() is involved in user copy, so request reference
> >          * has to be grabbed
> > +        *
> > +        * for zero copy, request buffer need to be registered to io_uring
> > +        * buffer table, so reference is needed
> >          */
> > -       return ublk_support_user_copy(ubq);
> > +       return ublk_support_user_copy(ubq) || ublk_support_zero_copy(ubq);
> >  }
> >
> >  static inline void ublk_init_req_ref(const struct ublk_queue *ubq,
> > @@ -2245,6 +2248,9 @@ static struct request *ublk_check_and_get_req(struct kiocb *iocb,
> >         if (!ubq)
> >                 return ERR_PTR(-EINVAL);
> >
> > +       if (!ublk_support_user_copy(ubq))
> > +               return ERR_PTR(-EACCES);
> 
> This partly overlaps with the existing ublk_need_req_ref() check in
> __ublk_check_and_get_req() (although that allows
> UBLK_F_SUPPORT_ZERO_COPY too). Can that check be removed now that the
> callers explicitly check ublk_support_user_copy() or
> ublk_support_zero_copy()?

Yeah, it can be removed.


Thanks, 
Ming

Re: [PATCH v6.15 2/3] ublk: decouple zero copy from user copy

[Ming Lei]

On Tue, Apr 29, 2025 at 08:55:48AM +0800, Ming Lei wrote:
> On Mon, Apr 28, 2025 at 09:01:04AM -0700, Caleb Sander Mateos wrote:
> > On Sun, Apr 27, 2025 at 6:49 AM Ming Lei <ming.lei@redhat.com> wrote:
> > >
> > > UBLK_F_USER_COPY and UBLK_F_SUPPORT_ZERO_COPY are two different
> > > features, and shouldn't be coupled together.
> > >
> > > Commit 1f6540e2aabb ("ublk: zc register/unregister bvec") enables
> > > user copy automatically in case of UBLK_F_SUPPORT_ZERO_COPY, this way
> > > isn't correct.
> > >
> > > So decouple zero copy from user copy, and use independent helper to
> > > check each one.
> > 
> > I agree this makes sense.
> > 
> > >
> > > Fixes: 1f6540e2aabb ("ublk: zc register/unregister bvec")
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > ---
> > >  drivers/block/ublk_drv.c | 35 +++++++++++++++++++++++------------
> > >  1 file changed, 23 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index 40f971a66d3e..0a3a3c64316d 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -205,11 +205,6 @@ static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> > >  static inline unsigned int ublk_req_build_flags(struct request *req);
> > >  static inline struct ublksrv_io_desc *ublk_get_iod(struct ublk_queue *ubq,
> > >                                                    int tag);
> > > -static inline bool ublk_dev_is_user_copy(const struct ublk_device *ub)
> > > -{
> > > -       return ub->dev_info.flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
> > > -}
> > > -
> > >  static inline bool ublk_dev_is_zoned(const struct ublk_device *ub)
> > >  {
> > >         return ub->dev_info.flags & UBLK_F_ZONED;
> > > @@ -609,14 +604,19 @@ static void ublk_apply_params(struct ublk_device *ub)
> > >                 ublk_dev_param_zoned_apply(ub);
> > >  }
> > >
> > > +static inline bool ublk_support_zero_copy(const struct ublk_queue *ubq)
> > > +{
> > > +       return ubq->flags & UBLK_F_SUPPORT_ZERO_COPY;
> > > +}
> > > +
> > >  static inline bool ublk_support_user_copy(const struct ublk_queue *ubq)
> > >  {
> > > -       return ubq->flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
> > > +       return ubq->flags & UBLK_F_USER_COPY;
> > >  }
> > >
> > >  static inline bool ublk_need_map_io(const struct ublk_queue *ubq)
> > >  {
> > > -       return !ublk_support_user_copy(ubq);
> > > +       return !ublk_support_user_copy(ubq) && !ublk_support_zero_copy(ubq);
> > >  }
> > >
> > >  static inline bool ublk_need_req_ref(const struct ublk_queue *ubq)
> > > @@ -624,8 +624,11 @@ static inline bool ublk_need_req_ref(const struct ublk_queue *ubq)
> > >         /*
> > >          * read()/write() is involved in user copy, so request reference
> > >          * has to be grabbed
> > > +        *
> > > +        * for zero copy, request buffer need to be registered to io_uring
> > > +        * buffer table, so reference is needed
> > >          */
> > > -       return ublk_support_user_copy(ubq);
> > > +       return ublk_support_user_copy(ubq) || ublk_support_zero_copy(ubq);
> > >  }
> > >
> > >  static inline void ublk_init_req_ref(const struct ublk_queue *ubq,
> > > @@ -2245,6 +2248,9 @@ static struct request *ublk_check_and_get_req(struct kiocb *iocb,
> > >         if (!ubq)
> > >                 return ERR_PTR(-EINVAL);
> > >
> > > +       if (!ublk_support_user_copy(ubq))
> > > +               return ERR_PTR(-EACCES);
> > 
> > This partly overlaps with the existing ublk_need_req_ref() check in
> > __ublk_check_and_get_req() (although that allows
> > UBLK_F_SUPPORT_ZERO_COPY too). Can that check be removed now that the
> > callers explicitly check ublk_support_user_copy() or
> > ublk_support_zero_copy()?
> 
> Yeah, it can be removed.

Actually the removal can only be done after the 3rd patch is applied with
zero copy check is added.



Thanks,
Ming

Re: [PATCH v6.15 2/3] ublk: decouple zero copy from user copy

[Caleb Sander Mateos]

On Mon, Apr 28, 2025 at 6:36 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Tue, Apr 29, 2025 at 08:55:48AM +0800, Ming Lei wrote:
> > On Mon, Apr 28, 2025 at 09:01:04AM -0700, Caleb Sander Mateos wrote:
> > > On Sun, Apr 27, 2025 at 6:49 AM Ming Lei <ming.lei@redhat.com> wrote:
> > > >
> > > > UBLK_F_USER_COPY and UBLK_F_SUPPORT_ZERO_COPY are two different
> > > > features, and shouldn't be coupled together.
> > > >
> > > > Commit 1f6540e2aabb ("ublk: zc register/unregister bvec") enables
> > > > user copy automatically in case of UBLK_F_SUPPORT_ZERO_COPY, this way
> > > > isn't correct.
> > > >
> > > > So decouple zero copy from user copy, and use independent helper to
> > > > check each one.
> > >
> > > I agree this makes sense.
> > >
> > > >
> > > > Fixes: 1f6540e2aabb ("ublk: zc register/unregister bvec")
> > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > ---
> > > >  drivers/block/ublk_drv.c | 35 +++++++++++++++++++++++------------
> > > >  1 file changed, 23 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > > index 40f971a66d3e..0a3a3c64316d 100644
> > > > --- a/drivers/block/ublk_drv.c
> > > > +++ b/drivers/block/ublk_drv.c
> > > > @@ -205,11 +205,6 @@ static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> > > >  static inline unsigned int ublk_req_build_flags(struct request *req);
> > > >  static inline struct ublksrv_io_desc *ublk_get_iod(struct ublk_queue *ubq,
> > > >                                                    int tag);
> > > > -static inline bool ublk_dev_is_user_copy(const struct ublk_device *ub)
> > > > -{
> > > > -       return ub->dev_info.flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
> > > > -}
> > > > -
> > > >  static inline bool ublk_dev_is_zoned(const struct ublk_device *ub)
> > > >  {
> > > >         return ub->dev_info.flags & UBLK_F_ZONED;
> > > > @@ -609,14 +604,19 @@ static void ublk_apply_params(struct ublk_device *ub)
> > > >                 ublk_dev_param_zoned_apply(ub);
> > > >  }
> > > >
> > > > +static inline bool ublk_support_zero_copy(const struct ublk_queue *ubq)
> > > > +{
> > > > +       return ubq->flags & UBLK_F_SUPPORT_ZERO_COPY;
> > > > +}
> > > > +
> > > >  static inline bool ublk_support_user_copy(const struct ublk_queue *ubq)
> > > >  {
> > > > -       return ubq->flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
> > > > +       return ubq->flags & UBLK_F_USER_COPY;
> > > >  }
> > > >
> > > >  static inline bool ublk_need_map_io(const struct ublk_queue *ubq)
> > > >  {
> > > > -       return !ublk_support_user_copy(ubq);
> > > > +       return !ublk_support_user_copy(ubq) && !ublk_support_zero_copy(ubq);
> > > >  }
> > > >
> > > >  static inline bool ublk_need_req_ref(const struct ublk_queue *ubq)
> > > > @@ -624,8 +624,11 @@ static inline bool ublk_need_req_ref(const struct ublk_queue *ubq)
> > > >         /*
> > > >          * read()/write() is involved in user copy, so request reference
> > > >          * has to be grabbed
> > > > +        *
> > > > +        * for zero copy, request buffer need to be registered to io_uring
> > > > +        * buffer table, so reference is needed
> > > >          */
> > > > -       return ublk_support_user_copy(ubq);
> > > > +       return ublk_support_user_copy(ubq) || ublk_support_zero_copy(ubq);
> > > >  }
> > > >
> > > >  static inline void ublk_init_req_ref(const struct ublk_queue *ubq,
> > > > @@ -2245,6 +2248,9 @@ static struct request *ublk_check_and_get_req(struct kiocb *iocb,
> > > >         if (!ubq)
> > > >                 return ERR_PTR(-EINVAL);
> > > >
> > > > +       if (!ublk_support_user_copy(ubq))
> > > > +               return ERR_PTR(-EACCES);
> > >
> > > This partly overlaps with the existing ublk_need_req_ref() check in
> > > __ublk_check_and_get_req() (although that allows
> > > UBLK_F_SUPPORT_ZERO_COPY too). Can that check be removed now that the
> > > callers explicitly check ublk_support_user_copy() or
> > > ublk_support_zero_copy()?
> >
> > Yeah, it can be removed.
>
> Actually the removal can only be done after the 3rd patch is applied with
> zero copy check is added.

Right, I just meant it can be removed in this series.

Thanks,
Caleb

[PATCH v6.15 3/3] ublk: enhance check for register/unregister io buffer command

[Ming Lei]

The simple check of UBLK_IO_FLAG_OWNED_BY_SRV can avoid incorrect
register/unregister io buffer easily, so check it before calling
starting to register/un-register io buffer.

Also only allow io buffer register/unregister uring_cmd in case of
UBLK_F_SUPPORT_ZERO_COPY.

Also mark argument 'ublk_queue *' of ublk_register_io_buf as const.

Fixes: 1f6540e2aabb ("ublk: zc register/unregister bvec")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 0a3a3c64316d..c624d8f653ae 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -201,7 +201,7 @@ struct ublk_params_header {
 static void ublk_stop_dev_unlocked(struct ublk_device *ub);
 static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq);
 static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
-		struct ublk_queue *ubq, int tag, size_t offset);
+		const struct ublk_queue *ubq, int tag, size_t offset);
 static inline unsigned int ublk_req_build_flags(struct request *req);
 static inline struct ublksrv_io_desc *ublk_get_iod(struct ublk_queue *ubq,
 						   int tag);
@@ -1949,13 +1949,20 @@ static void ublk_io_release(void *priv)
 }
 
 static int ublk_register_io_buf(struct io_uring_cmd *cmd,
-				struct ublk_queue *ubq, unsigned int tag,
+				const struct ublk_queue *ubq, unsigned int tag,
 				unsigned int index, unsigned int issue_flags)
 {
 	struct ublk_device *ub = cmd->file->private_data;
+	const struct ublk_io *io = &ubq->ios[tag];
 	struct request *req;
 	int ret;
 
+	if (!ublk_support_zero_copy(ubq))
+		return -EINVAL;
+
+	if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
+		return -EINVAL;
+
 	req = __ublk_check_and_get_req(ub, ubq, tag, 0);
 	if (!req)
 		return -EINVAL;
@@ -1971,8 +1978,17 @@ static int ublk_register_io_buf(struct io_uring_cmd *cmd,
 }
 
 static int ublk_unregister_io_buf(struct io_uring_cmd *cmd,
+				  const struct ublk_queue *ubq, unsigned int tag,
 				  unsigned int index, unsigned int issue_flags)
 {
+	const struct ublk_io *io = &ubq->ios[tag];
+
+	if (!ublk_support_zero_copy(ubq))
+		return -EINVAL;
+
+	if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
+		return -EINVAL;
+
 	return io_buffer_unregister_bvec(cmd, index, issue_flags);
 }
 
@@ -2076,7 +2092,7 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
 	case UBLK_IO_REGISTER_IO_BUF:
 		return ublk_register_io_buf(cmd, ubq, tag, ub_cmd->addr, issue_flags);
 	case UBLK_IO_UNREGISTER_IO_BUF:
-		return ublk_unregister_io_buf(cmd, ub_cmd->addr, issue_flags);
+		return ublk_unregister_io_buf(cmd, ubq, tag, ub_cmd->addr, issue_flags);
 	case UBLK_IO_FETCH_REQ:
 		ret = ublk_fetch(cmd, ubq, io, ub_cmd->addr);
 		if (ret)
@@ -2128,7 +2144,7 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
 }
 
 static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
-		struct ublk_queue *ubq, int tag, size_t offset)
+		const struct ublk_queue *ubq, int tag, size_t offset)
 {
 	struct request *req;
 
-- 
2.47.0

Re: [PATCH v6.15 3/3] ublk: enhance check for register/unregister io buffer command

[Caleb Sander Mateos]

On Sun, Apr 27, 2025 at 6:50 AM Ming Lei <ming.lei@redhat.com> wrote:
>
> The simple check of UBLK_IO_FLAG_OWNED_BY_SRV can avoid incorrect
> register/unregister io buffer easily, so check it before calling
> starting to register/un-register io buffer.
>
> Also only allow io buffer register/unregister uring_cmd in case of
> UBLK_F_SUPPORT_ZERO_COPY.
>
> Also mark argument 'ublk_queue *' of ublk_register_io_buf as const.
>
> Fixes: 1f6540e2aabb ("ublk: zc register/unregister bvec")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/block/ublk_drv.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 0a3a3c64316d..c624d8f653ae 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -201,7 +201,7 @@ struct ublk_params_header {
>  static void ublk_stop_dev_unlocked(struct ublk_device *ub);
>  static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq);
>  static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> -               struct ublk_queue *ubq, int tag, size_t offset);
> +               const struct ublk_queue *ubq, int tag, size_t offset);
>  static inline unsigned int ublk_req_build_flags(struct request *req);
>  static inline struct ublksrv_io_desc *ublk_get_iod(struct ublk_queue *ubq,
>                                                    int tag);
> @@ -1949,13 +1949,20 @@ static void ublk_io_release(void *priv)
>  }
>
>  static int ublk_register_io_buf(struct io_uring_cmd *cmd,
> -                               struct ublk_queue *ubq, unsigned int tag,
> +                               const struct ublk_queue *ubq, unsigned int tag,
>                                 unsigned int index, unsigned int issue_flags)
>  {
>         struct ublk_device *ub = cmd->file->private_data;
> +       const struct ublk_io *io = &ubq->ios[tag];
>         struct request *req;
>         int ret;
>
> +       if (!ublk_support_zero_copy(ubq))
> +               return -EINVAL;
> +
> +       if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
> +               return -EINVAL;

I would still prefer to see this common UBLK_IO_FLAG_OWNED_BY_SRV
check moved to __ublk_ch_uring_cmd() along with the existing flag
checks. Something like this:

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index a183aa7648c3..79ef2580ddcc 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -2055,6 +2055,11 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
                goto out;
        }

+       /* only FETCH_REQ is allowed if io is not OWNED_BY_SRV */
+       if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV) &&
+           _IOC_NR(cmd_op) != UBLK_IO_FETCH_REQ)
+               goto out;
+
        /*
         * ensure that the user issues UBLK_IO_NEED_GET_DATA
         * iff the driver have set the UBLK_IO_FLAG_NEED_GET_DATA.
@@ -2080,10 +2085,6 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
                break;
        case UBLK_IO_COMMIT_AND_FETCH_REQ:
                req = blk_mq_tag_to_rq(ub->tag_set.tags[ub_cmd->q_id], tag);
-
-               if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
-                       goto out;
-
                if (ublk_need_map_io(ubq)) {
                        /*
                         * COMMIT_AND_FETCH_REQ has to provide IO buffer if
@@ -2105,8 +2106,6 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
                ublk_commit_completion(ub, ub_cmd);
                break;
        case UBLK_IO_NEED_GET_DATA:
-               if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
-                       goto out;
                ublk_fill_io_cmd(io, cmd, ub_cmd->addr);
                req = blk_mq_tag_to_rq(ub->tag_set.tags[ub_cmd->q_id], tag);
                ublk_dispatch_req(ubq, req, issue_flags);


> +
>         req = __ublk_check_and_get_req(ub, ubq, tag, 0);
>         if (!req)
>                 return -EINVAL;
> @@ -1971,8 +1978,17 @@ static int ublk_register_io_buf(struct io_uring_cmd *cmd,
>  }
>
>  static int ublk_unregister_io_buf(struct io_uring_cmd *cmd,
> +                                 const struct ublk_queue *ubq, unsigned int tag,
>                                   unsigned int index, unsigned int issue_flags)
>  {
> +       const struct ublk_io *io = &ubq->ios[tag];
> +
> +       if (!ublk_support_zero_copy(ubq))
> +               return -EINVAL;
> +
> +       if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
> +               return -EINVAL;
> +
>         return io_buffer_unregister_bvec(cmd, index, issue_flags);
>  }
>
> @@ -2076,7 +2092,7 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
>         case UBLK_IO_REGISTER_IO_BUF:
>                 return ublk_register_io_buf(cmd, ubq, tag, ub_cmd->addr, issue_flags);
>         case UBLK_IO_UNREGISTER_IO_BUF:
> -               return ublk_unregister_io_buf(cmd, ub_cmd->addr, issue_flags);
> +               return ublk_unregister_io_buf(cmd, ubq, tag, ub_cmd->addr, issue_flags);
>         case UBLK_IO_FETCH_REQ:
>                 ret = ublk_fetch(cmd, ubq, io, ub_cmd->addr);
>                 if (ret)
> @@ -2128,7 +2144,7 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
>  }
>
>  static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> -               struct ublk_queue *ubq, int tag, size_t offset)
> +               const struct ublk_queue *ubq, int tag, size_t offset)
>  {
>         struct request *req;
>
> --
> 2.47.0
>

Re: [PATCH v6.15 3/3] ublk: enhance check for register/unregister io buffer command

[Ming Lei]

On Mon, Apr 28, 2025 at 09:28:07AM -0700, Caleb Sander Mateos wrote:
> On Sun, Apr 27, 2025 at 6:50 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > The simple check of UBLK_IO_FLAG_OWNED_BY_SRV can avoid incorrect
> > register/unregister io buffer easily, so check it before calling
> > starting to register/un-register io buffer.
> >
> > Also only allow io buffer register/unregister uring_cmd in case of
> > UBLK_F_SUPPORT_ZERO_COPY.
> >
> > Also mark argument 'ublk_queue *' of ublk_register_io_buf as const.
> >
> > Fixes: 1f6540e2aabb ("ublk: zc register/unregister bvec")
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/block/ublk_drv.c | 24 ++++++++++++++++++++----
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 0a3a3c64316d..c624d8f653ae 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -201,7 +201,7 @@ struct ublk_params_header {
> >  static void ublk_stop_dev_unlocked(struct ublk_device *ub);
> >  static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq);
> >  static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> > -               struct ublk_queue *ubq, int tag, size_t offset);
> > +               const struct ublk_queue *ubq, int tag, size_t offset);
> >  static inline unsigned int ublk_req_build_flags(struct request *req);
> >  static inline struct ublksrv_io_desc *ublk_get_iod(struct ublk_queue *ubq,
> >                                                    int tag);
> > @@ -1949,13 +1949,20 @@ static void ublk_io_release(void *priv)
> >  }
> >
> >  static int ublk_register_io_buf(struct io_uring_cmd *cmd,
> > -                               struct ublk_queue *ubq, unsigned int tag,
> > +                               const struct ublk_queue *ubq, unsigned int tag,
> >                                 unsigned int index, unsigned int issue_flags)
> >  {
> >         struct ublk_device *ub = cmd->file->private_data;
> > +       const struct ublk_io *io = &ubq->ios[tag];
> >         struct request *req;
> >         int ret;
> >
> > +       if (!ublk_support_zero_copy(ubq))
> > +               return -EINVAL;
> > +
> > +       if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
> > +               return -EINVAL;
> 
> I would still prefer to see this common UBLK_IO_FLAG_OWNED_BY_SRV
> check moved to __ublk_ch_uring_cmd() along with the existing flag
> checks. Something like this:

This way mixes bug fix with cleanup.

We are close to v6.15-rc5, and bug fix should keep simple for minimizing
regression.

But it is fine to make it one cleanup aiming at v6.16.


thanks,
Ming

Re: [PATCH v6.15 3/3] ublk: enhance check for register/unregister io buffer command

[Caleb Sander Mateos]

On Mon, Apr 28, 2025 at 6:02 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Mon, Apr 28, 2025 at 09:28:07AM -0700, Caleb Sander Mateos wrote:
> > On Sun, Apr 27, 2025 at 6:50 AM Ming Lei <ming.lei@redhat.com> wrote:
> > >
> > > The simple check of UBLK_IO_FLAG_OWNED_BY_SRV can avoid incorrect
> > > register/unregister io buffer easily, so check it before calling
> > > starting to register/un-register io buffer.
> > >
> > > Also only allow io buffer register/unregister uring_cmd in case of
> > > UBLK_F_SUPPORT_ZERO_COPY.
> > >
> > > Also mark argument 'ublk_queue *' of ublk_register_io_buf as const.
> > >
> > > Fixes: 1f6540e2aabb ("ublk: zc register/unregister bvec")
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > ---
> > >  drivers/block/ublk_drv.c | 24 ++++++++++++++++++++----
> > >  1 file changed, 20 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index 0a3a3c64316d..c624d8f653ae 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -201,7 +201,7 @@ struct ublk_params_header {
> > >  static void ublk_stop_dev_unlocked(struct ublk_device *ub);
> > >  static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq);
> > >  static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> > > -               struct ublk_queue *ubq, int tag, size_t offset);
> > > +               const struct ublk_queue *ubq, int tag, size_t offset);
> > >  static inline unsigned int ublk_req_build_flags(struct request *req);
> > >  static inline struct ublksrv_io_desc *ublk_get_iod(struct ublk_queue *ubq,
> > >                                                    int tag);
> > > @@ -1949,13 +1949,20 @@ static void ublk_io_release(void *priv)
> > >  }
> > >
> > >  static int ublk_register_io_buf(struct io_uring_cmd *cmd,
> > > -                               struct ublk_queue *ubq, unsigned int tag,
> > > +                               const struct ublk_queue *ubq, unsigned int tag,
> > >                                 unsigned int index, unsigned int issue_flags)
> > >  {
> > >         struct ublk_device *ub = cmd->file->private_data;
> > > +       const struct ublk_io *io = &ubq->ios[tag];
> > >         struct request *req;
> > >         int ret;
> > >
> > > +       if (!ublk_support_zero_copy(ubq))
> > > +               return -EINVAL;
> > > +
> > > +       if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
> > > +               return -EINVAL;
> >
> > I would still prefer to see this common UBLK_IO_FLAG_OWNED_BY_SRV
> > check moved to __ublk_ch_uring_cmd() along with the existing flag
> > checks. Something like this:
>
> This way mixes bug fix with cleanup.
>
> We are close to v6.15-rc5, and bug fix should keep simple for minimizing
> regression.
>
> But it is fine to make it one cleanup aiming at v6.16.

Okay, I can send out this cleanup for 6.16. In that case,

Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>