[PATCH 0/2] ublk: fix ublk_queue_rqs() and selftests test_stress_03

[PATCH 0/2] ublk: fix ublk_queue_rqs() and selftests test_stress_03

[Ming Lei]

Hi Jens,

The 1st patch fixes ublk_queue_rqs().

The 2nd patch fixes test_stress_03.sh.

Thanks,


Ming Lei (2):
  ublk: build per-io-ring-ctx batch list
  selftests: ublk: don't take same backing file for more than one ublk
    devices

 drivers/block/ublk_drv.c                       | 17 +++++++++--------
 tools/testing/selftests/ublk/test_stress_03.sh |  5 +++--
 2 files changed, 12 insertions(+), 10 deletions(-)

-- 
2.47.0

[PATCH 1/2] ublk: build per-io-ring-ctx batch list

[Ming Lei]

ublk_queue_cmd_list() dispatches the whole batch list by scheduling task
work via the tail request's io_uring_cmd, this way is fine even though
more than one io_ring_ctx are involved for this batch since it is just
one running context.

However, the task work handler ublk_cmd_list_tw_cb() takes `issue_flags`
of tail uring_cmd's io_ring_ctx for completing all commands. This way is
wrong if any uring_cmd is issued from different io_ring_ctx.

Fixes it by always building per-io-ring-ctx batch list.

For typical per-queue or per-io daemon implementation, this way shouldn't
make difference from performance viewpoint, because single io_ring_ctx is
often taken in each daemon.

Fixes: d796cea7b9f3 ("ublk: implement ->queue_rqs()")
Fixes: ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue daemon")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index c637ea010d34..e79b04e61047 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1336,9 +1336,8 @@ static void ublk_cmd_list_tw_cb(struct io_uring_cmd *cmd,
 	} while (rq);
 }
 
-static void ublk_queue_cmd_list(struct ublk_io *io, struct rq_list *l)
+static void ublk_queue_cmd_list(struct io_uring_cmd *cmd, struct rq_list *l)
 {
-	struct io_uring_cmd *cmd = io->cmd;
 	struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
 
 	pdu->req_list = rq_list_peek(l);
@@ -1420,16 +1419,18 @@ static void ublk_queue_rqs(struct rq_list *rqlist)
 {
 	struct rq_list requeue_list = { };
 	struct rq_list submit_list = { };
-	struct ublk_io *io = NULL;
+	struct io_uring_cmd *cmd = NULL;
 	struct request *req;
 
 	while ((req = rq_list_pop(rqlist))) {
 		struct ublk_queue *this_q = req->mq_hctx->driver_data;
-		struct ublk_io *this_io = &this_q->ios[req->tag];
+		struct io_uring_cmd *this_cmd = this_q->ios[req->tag].cmd;
 
-		if (io && io->task != this_io->task && !rq_list_empty(&submit_list))
-			ublk_queue_cmd_list(io, &submit_list);
-		io = this_io;
+		if (cmd && io_uring_cmd_ctx_handle(cmd) !=
+				io_uring_cmd_ctx_handle(this_cmd) &&
+				!rq_list_empty(&submit_list))
+			ublk_queue_cmd_list(cmd, &submit_list);
+		cmd = this_cmd;
 
 		if (ublk_prep_req(this_q, req, true) == BLK_STS_OK)
 			rq_list_add_tail(&submit_list, req);
@@ -1438,7 +1439,7 @@ static void ublk_queue_rqs(struct rq_list *rqlist)
 	}
 
 	if (!rq_list_empty(&submit_list))
-		ublk_queue_cmd_list(io, &submit_list);
+		ublk_queue_cmd_list(cmd, &submit_list);
 	*rqlist = requeue_list;
 }
 
-- 
2.47.0

[PATCH 2/2] selftests: ublk: don't take same backing file for more than one ublk devices

[Ming Lei]

Don't use same backing file for more than one ublk devices, and avoid
concurrent write on same file from more ublk disks.

Fixes: 8ccebc19ee3d ("selftests: ublk: support UBLK_F_AUTO_BUF_REG")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 tools/testing/selftests/ublk/test_stress_03.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/ublk/test_stress_03.sh b/tools/testing/selftests/ublk/test_stress_03.sh
index 6eef282d569f..3ed4c9b2d8c0 100755
--- a/tools/testing/selftests/ublk/test_stress_03.sh
+++ b/tools/testing/selftests/ublk/test_stress_03.sh
@@ -32,22 +32,23 @@ _create_backfile 2 128M
 ublk_io_and_remove 8G -t null -q 4 -z &
 ublk_io_and_remove 256M -t loop -q 4 -z "${UBLK_BACKFILES[0]}" &
 ublk_io_and_remove 256M -t stripe -q 4 -z "${UBLK_BACKFILES[1]}" "${UBLK_BACKFILES[2]}" &
+wait
 
 if _have_feature "AUTO_BUF_REG"; then
 	ublk_io_and_remove 8G -t null -q 4 --auto_zc &
 	ublk_io_and_remove 256M -t loop -q 4 --auto_zc "${UBLK_BACKFILES[0]}" &
 	ublk_io_and_remove 256M -t stripe -q 4 --auto_zc "${UBLK_BACKFILES[1]}" "${UBLK_BACKFILES[2]}" &
 	ublk_io_and_remove 8G -t null -q 4 -z --auto_zc --auto_zc_fallback &
+	wait
 fi
-wait
 
 if _have_feature "PER_IO_DAEMON"; then
 	ublk_io_and_remove 8G -t null -q 4 --auto_zc --nthreads 8 --per_io_tasks &
 	ublk_io_and_remove 256M -t loop -q 4 --auto_zc --nthreads 8 --per_io_tasks "${UBLK_BACKFILES[0]}" &
 	ublk_io_and_remove 256M -t stripe -q 4 --auto_zc --nthreads 8 --per_io_tasks "${UBLK_BACKFILES[1]}" "${UBLK_BACKFILES[2]}" &
 	ublk_io_and_remove 8G -t null -q 4 -z --auto_zc --auto_zc_fallback --nthreads 8 --per_io_tasks &
+	wait
 fi
-wait
 
 _cleanup_test "stress"
 _show_result $TID $ERR_CODE
-- 
2.47.0

Re: [PATCH 1/2] ublk: build per-io-ring-ctx batch list

[Caleb Sander Mateos]

On Sun, Jun 22, 2025 at 6:19 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> ublk_queue_cmd_list() dispatches the whole batch list by scheduling task
> work via the tail request's io_uring_cmd, this way is fine even though
> more than one io_ring_ctx are involved for this batch since it is just
> one running context.
>
> However, the task work handler ublk_cmd_list_tw_cb() takes `issue_flags`
> of tail uring_cmd's io_ring_ctx for completing all commands. This way is
> wrong if any uring_cmd is issued from different io_ring_ctx.
>
> Fixes it by always building per-io-ring-ctx batch list.
>
> For typical per-queue or per-io daemon implementation, this way shouldn't
> make difference from performance viewpoint, because single io_ring_ctx is
> often taken in each daemon.
>
> Fixes: d796cea7b9f3 ("ublk: implement ->queue_rqs()")
> Fixes: ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue daemon")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/block/ublk_drv.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index c637ea010d34..e79b04e61047 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1336,9 +1336,8 @@ static void ublk_cmd_list_tw_cb(struct io_uring_cmd *cmd,
>         } while (rq);
>  }
>
> -static void ublk_queue_cmd_list(struct ublk_io *io, struct rq_list *l)
> +static void ublk_queue_cmd_list(struct io_uring_cmd *cmd, struct rq_list *l)
>  {
> -       struct io_uring_cmd *cmd = io->cmd;
>         struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
>
>         pdu->req_list = rq_list_peek(l);
> @@ -1420,16 +1419,18 @@ static void ublk_queue_rqs(struct rq_list *rqlist)
>  {
>         struct rq_list requeue_list = { };
>         struct rq_list submit_list = { };
> -       struct ublk_io *io = NULL;
> +       struct io_uring_cmd *cmd = NULL;
>         struct request *req;
>
>         while ((req = rq_list_pop(rqlist))) {
>                 struct ublk_queue *this_q = req->mq_hctx->driver_data;
> -               struct ublk_io *this_io = &this_q->ios[req->tag];
> +               struct io_uring_cmd *this_cmd = this_q->ios[req->tag].cmd;
>
> -               if (io && io->task != this_io->task && !rq_list_empty(&submit_list))
> -                       ublk_queue_cmd_list(io, &submit_list);
> -               io = this_io;
> +               if (cmd && io_uring_cmd_ctx_handle(cmd) !=
> +                               io_uring_cmd_ctx_handle(this_cmd) &&
> +                               !rq_list_empty(&submit_list))
> +                       ublk_queue_cmd_list(cmd, &submit_list);

I don't think we can assume that ublk commands submitted to the same
io_uring have the same daemon task. It's possible for multiple tasks
to submit to the same io_uring, even though that's not a common or
performant way to use io_uring. Probably we need to check that both
the task and io_ring_ctx match.

Best,
Caleb

> +               cmd = this_cmd;
>
>                 if (ublk_prep_req(this_q, req, true) == BLK_STS_OK)
>                         rq_list_add_tail(&submit_list, req);
> @@ -1438,7 +1439,7 @@ static void ublk_queue_rqs(struct rq_list *rqlist)
>         }
>
>         if (!rq_list_empty(&submit_list))
> -               ublk_queue_cmd_list(io, &submit_list);
> +               ublk_queue_cmd_list(cmd, &submit_list);
>         *rqlist = requeue_list;
>  }
>
> --
> 2.47.0
>

Re: [PATCH 2/2] selftests: ublk: don't take same backing file for more than one ublk devices

[Caleb Sander Mateos]

On Sun, Jun 22, 2025 at 6:19 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> Don't use same backing file for more than one ublk devices, and avoid
> concurrent write on same file from more ublk disks.
>
> Fixes: 8ccebc19ee3d ("selftests: ublk: support UBLK_F_AUTO_BUF_REG")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  tools/testing/selftests/ublk/test_stress_03.sh | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/ublk/test_stress_03.sh b/tools/testing/selftests/ublk/test_stress_03.sh
> index 6eef282d569f..3ed4c9b2d8c0 100755
> --- a/tools/testing/selftests/ublk/test_stress_03.sh
> +++ b/tools/testing/selftests/ublk/test_stress_03.sh
> @@ -32,22 +32,23 @@ _create_backfile 2 128M
>  ublk_io_and_remove 8G -t null -q 4 -z &
>  ublk_io_and_remove 256M -t loop -q 4 -z "${UBLK_BACKFILES[0]}" &
>  ublk_io_and_remove 256M -t stripe -q 4 -z "${UBLK_BACKFILES[1]}" "${UBLK_BACKFILES[2]}" &
> +wait

Why is wait necessary here? It looks like __run_io_and_remove, which
is called from run_io_and_remove, already ends with a wait. Am I
missing something?

Best,
Caleb

>
>  if _have_feature "AUTO_BUF_REG"; then
>         ublk_io_and_remove 8G -t null -q 4 --auto_zc &
>         ublk_io_and_remove 256M -t loop -q 4 --auto_zc "${UBLK_BACKFILES[0]}" &
>         ublk_io_and_remove 256M -t stripe -q 4 --auto_zc "${UBLK_BACKFILES[1]}" "${UBLK_BACKFILES[2]}" &
>         ublk_io_and_remove 8G -t null -q 4 -z --auto_zc --auto_zc_fallback &
> +       wait
>  fi
> -wait
>
>  if _have_feature "PER_IO_DAEMON"; then
>         ublk_io_and_remove 8G -t null -q 4 --auto_zc --nthreads 8 --per_io_tasks &
>         ublk_io_and_remove 256M -t loop -q 4 --auto_zc --nthreads 8 --per_io_tasks "${UBLK_BACKFILES[0]}" &
>         ublk_io_and_remove 256M -t stripe -q 4 --auto_zc --nthreads 8 --per_io_tasks "${UBLK_BACKFILES[1]}" "${UBLK_BACKFILES[2]}" &
>         ublk_io_and_remove 8G -t null -q 4 -z --auto_zc --auto_zc_fallback --nthreads 8 --per_io_tasks &
> +       wait
>  fi
> -wait
>
>  _cleanup_test "stress"
>  _show_result $TID $ERR_CODE
> --
> 2.47.0
>

Re: [PATCH 2/2] selftests: ublk: don't take same backing file for more than one ublk devices

[Ming Lei]

On Mon, Jun 23, 2025 at 10:54:58AM -0700, Caleb Sander Mateos wrote:
> On Sun, Jun 22, 2025 at 6:19 PM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > Don't use same backing file for more than one ublk devices, and avoid
> > concurrent write on same file from more ublk disks.
> >
> > Fixes: 8ccebc19ee3d ("selftests: ublk: support UBLK_F_AUTO_BUF_REG")
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  tools/testing/selftests/ublk/test_stress_03.sh | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/ublk/test_stress_03.sh b/tools/testing/selftests/ublk/test_stress_03.sh
> > index 6eef282d569f..3ed4c9b2d8c0 100755
> > --- a/tools/testing/selftests/ublk/test_stress_03.sh
> > +++ b/tools/testing/selftests/ublk/test_stress_03.sh
> > @@ -32,22 +32,23 @@ _create_backfile 2 128M
> >  ublk_io_and_remove 8G -t null -q 4 -z &
> >  ublk_io_and_remove 256M -t loop -q 4 -z "${UBLK_BACKFILES[0]}" &
> >  ublk_io_and_remove 256M -t stripe -q 4 -z "${UBLK_BACKFILES[1]}" "${UBLK_BACKFILES[2]}" &
> > +wait
> 
> Why is wait necessary here? It looks like __run_io_and_remove, which
> is called from run_io_and_remove, already ends with a wait. Am I
> missing something?

All tests share the three backing files, this way just avoids concurrent
write to the same file from each test/ublk device.



Thanks,
Ming

Re: [PATCH 1/2] ublk: build per-io-ring-ctx batch list

[Ming Lei]

On Mon, Jun 23, 2025 at 10:51:00AM -0700, Caleb Sander Mateos wrote:
> On Sun, Jun 22, 2025 at 6:19 PM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > ublk_queue_cmd_list() dispatches the whole batch list by scheduling task
> > work via the tail request's io_uring_cmd, this way is fine even though
> > more than one io_ring_ctx are involved for this batch since it is just
> > one running context.
> >
> > However, the task work handler ublk_cmd_list_tw_cb() takes `issue_flags`
> > of tail uring_cmd's io_ring_ctx for completing all commands. This way is
> > wrong if any uring_cmd is issued from different io_ring_ctx.
> >
> > Fixes it by always building per-io-ring-ctx batch list.
> >
> > For typical per-queue or per-io daemon implementation, this way shouldn't
> > make difference from performance viewpoint, because single io_ring_ctx is
> > often taken in each daemon.
> >
> > Fixes: d796cea7b9f3 ("ublk: implement ->queue_rqs()")
> > Fixes: ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue daemon")
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/block/ublk_drv.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index c637ea010d34..e79b04e61047 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -1336,9 +1336,8 @@ static void ublk_cmd_list_tw_cb(struct io_uring_cmd *cmd,
> >         } while (rq);
> >  }
> >
> > -static void ublk_queue_cmd_list(struct ublk_io *io, struct rq_list *l)
> > +static void ublk_queue_cmd_list(struct io_uring_cmd *cmd, struct rq_list *l)
> >  {
> > -       struct io_uring_cmd *cmd = io->cmd;
> >         struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
> >
> >         pdu->req_list = rq_list_peek(l);
> > @@ -1420,16 +1419,18 @@ static void ublk_queue_rqs(struct rq_list *rqlist)
> >  {
> >         struct rq_list requeue_list = { };
> >         struct rq_list submit_list = { };
> > -       struct ublk_io *io = NULL;
> > +       struct io_uring_cmd *cmd = NULL;
> >         struct request *req;
> >
> >         while ((req = rq_list_pop(rqlist))) {
> >                 struct ublk_queue *this_q = req->mq_hctx->driver_data;
> > -               struct ublk_io *this_io = &this_q->ios[req->tag];
> > +               struct io_uring_cmd *this_cmd = this_q->ios[req->tag].cmd;
> >
> > -               if (io && io->task != this_io->task && !rq_list_empty(&submit_list))
> > -                       ublk_queue_cmd_list(io, &submit_list);
> > -               io = this_io;
> > +               if (cmd && io_uring_cmd_ctx_handle(cmd) !=
> > +                               io_uring_cmd_ctx_handle(this_cmd) &&
> > +                               !rq_list_empty(&submit_list))
> > +                       ublk_queue_cmd_list(cmd, &submit_list);
> 
> I don't think we can assume that ublk commands submitted to the same
> io_uring have the same daemon task. It's possible for multiple tasks
> to submit to the same io_uring, even though that's not a common or
> performant way to use io_uring. Probably we need to check that both
> the task and io_ring_ctx match.

Here the problem is in 'issue_flags' passed from io_uring, especially for
grabbing io_ring_ctx lock.

If two uring_cmd are issued via same io_ring_ctx from two tasks, it is
fine to share 'issue_flags' from one of tasks, what matters is that the
io_ring_ctx lock is handled correctly when calling io_uring_cmd_done().



Thanks,
Ming

Re: [PATCH 0/2] ublk: fix ublk_queue_rqs() and selftests test_stress_03

[Jens Axboe]

On Mon, 23 Jun 2025 09:19:25 +0800, Ming Lei wrote:
> The 1st patch fixes ublk_queue_rqs().
> 
> The 2nd patch fixes test_stress_03.sh.
> 
> Thanks,
> 
> 
> [...]

Applied, thanks!

[1/2] ublk: build per-io-ring-ctx batch list
      commit: e867d6c4d51a564943e88e08fb1b27bc68a81b49
[2/2] selftests: ublk: don't take same backing file for more than one ublk devices
      commit: 4cb1a3d63eb7d4d0263b2d3ab4f61555263b69aa

Best regards,
-- 
Jens Axboe

Re: [PATCH 2/2] selftests: ublk: don't take same backing file for more than one ublk devices

[Caleb Sander Mateos]

On Mon, Jun 23, 2025 at 6:13 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Mon, Jun 23, 2025 at 10:54:58AM -0700, Caleb Sander Mateos wrote:
> > On Sun, Jun 22, 2025 at 6:19 PM Ming Lei <ming.lei@redhat.com> wrote:
> > >
> > > Don't use same backing file for more than one ublk devices, and avoid
> > > concurrent write on same file from more ublk disks.
> > >
> > > Fixes: 8ccebc19ee3d ("selftests: ublk: support UBLK_F_AUTO_BUF_REG")
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > ---
> > >  tools/testing/selftests/ublk/test_stress_03.sh | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/ublk/test_stress_03.sh b/tools/testing/selftests/ublk/test_stress_03.sh
> > > index 6eef282d569f..3ed4c9b2d8c0 100755
> > > --- a/tools/testing/selftests/ublk/test_stress_03.sh
> > > +++ b/tools/testing/selftests/ublk/test_stress_03.sh
> > > @@ -32,22 +32,23 @@ _create_backfile 2 128M
> > >  ublk_io_and_remove 8G -t null -q 4 -z &
> > >  ublk_io_and_remove 256M -t loop -q 4 -z "${UBLK_BACKFILES[0]}" &
> > >  ublk_io_and_remove 256M -t stripe -q 4 -z "${UBLK_BACKFILES[1]}" "${UBLK_BACKFILES[2]}" &
> > > +wait
> >
> > Why is wait necessary here? It looks like __run_io_and_remove, which
> > is called from run_io_and_remove, already ends with a wait. Am I
> > missing something?
>
> All tests share the three backing files, this way just avoids concurrent
> write to the same file from each test/ublk device.

Sorry, I didn't see the "&" at the end of the ublk_io_and_remove commands.

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

Re: [PATCH 1/2] ublk: build per-io-ring-ctx batch list

[Caleb Sander Mateos]

On Mon, Jun 23, 2025 at 6:24 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Mon, Jun 23, 2025 at 10:51:00AM -0700, Caleb Sander Mateos wrote:
> > On Sun, Jun 22, 2025 at 6:19 PM Ming Lei <ming.lei@redhat.com> wrote:
> > >
> > > ublk_queue_cmd_list() dispatches the whole batch list by scheduling task
> > > work via the tail request's io_uring_cmd, this way is fine even though
> > > more than one io_ring_ctx are involved for this batch since it is just
> > > one running context.
> > >
> > > However, the task work handler ublk_cmd_list_tw_cb() takes `issue_flags`
> > > of tail uring_cmd's io_ring_ctx for completing all commands. This way is
> > > wrong if any uring_cmd is issued from different io_ring_ctx.
> > >
> > > Fixes it by always building per-io-ring-ctx batch list.
> > >
> > > For typical per-queue or per-io daemon implementation, this way shouldn't
> > > make difference from performance viewpoint, because single io_ring_ctx is
> > > often taken in each daemon.
> > >
> > > Fixes: d796cea7b9f3 ("ublk: implement ->queue_rqs()")
> > > Fixes: ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue daemon")
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > ---
> > >  drivers/block/ublk_drv.c | 17 +++++++++--------
> > >  1 file changed, 9 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index c637ea010d34..e79b04e61047 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -1336,9 +1336,8 @@ static void ublk_cmd_list_tw_cb(struct io_uring_cmd *cmd,
> > >         } while (rq);
> > >  }
> > >
> > > -static void ublk_queue_cmd_list(struct ublk_io *io, struct rq_list *l)
> > > +static void ublk_queue_cmd_list(struct io_uring_cmd *cmd, struct rq_list *l)
> > >  {
> > > -       struct io_uring_cmd *cmd = io->cmd;
> > >         struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
> > >
> > >         pdu->req_list = rq_list_peek(l);
> > > @@ -1420,16 +1419,18 @@ static void ublk_queue_rqs(struct rq_list *rqlist)
> > >  {
> > >         struct rq_list requeue_list = { };
> > >         struct rq_list submit_list = { };
> > > -       struct ublk_io *io = NULL;
> > > +       struct io_uring_cmd *cmd = NULL;
> > >         struct request *req;
> > >
> > >         while ((req = rq_list_pop(rqlist))) {
> > >                 struct ublk_queue *this_q = req->mq_hctx->driver_data;
> > > -               struct ublk_io *this_io = &this_q->ios[req->tag];
> > > +               struct io_uring_cmd *this_cmd = this_q->ios[req->tag].cmd;
> > >
> > > -               if (io && io->task != this_io->task && !rq_list_empty(&submit_list))
> > > -                       ublk_queue_cmd_list(io, &submit_list);
> > > -               io = this_io;
> > > +               if (cmd && io_uring_cmd_ctx_handle(cmd) !=
> > > +                               io_uring_cmd_ctx_handle(this_cmd) &&
> > > +                               !rq_list_empty(&submit_list))
> > > +                       ublk_queue_cmd_list(cmd, &submit_list);
> >
> > I don't think we can assume that ublk commands submitted to the same
> > io_uring have the same daemon task. It's possible for multiple tasks
> > to submit to the same io_uring, even though that's not a common or
> > performant way to use io_uring. Probably we need to check that both
> > the task and io_ring_ctx match.
>
> Here the problem is in 'issue_flags' passed from io_uring, especially for
> grabbing io_ring_ctx lock.
>
> If two uring_cmd are issued via same io_ring_ctx from two tasks, it is
> fine to share 'issue_flags' from one of tasks, what matters is that the
> io_ring_ctx lock is handled correctly when calling io_uring_cmd_done().

Right, I understand the issue you are trying to solve. I agree it's a
problem for submit_list to contain commands from multiple
io_ring_ctxs. But it's also a problem if it contains commands with
different daemon tasks, because ublk_queue_cmd_list() will schedule
ublk_cmd_list_tw_cb() to be called in the *last command's task*. But
ublk_cmd_list_tw_cb() will call ublk_dispatch_req() for all the
commands in the list. So if submit_list contains commands with
multiple daemon tasks, ublk_dispatch_req() will fail on the current !=
io->task check. So I still feel we need to call
ublk_queue_cmd_list(io, &submit_list) if io->task != this_io->task (as
well as if the io_ring_ctxs differ).

Best,
Caleb

Re: [PATCH 1/2] ublk: build per-io-ring-ctx batch list

[Ming Lei]

On Tue, Jun 24, 2025 at 08:26:51AM -0700, Caleb Sander Mateos wrote:
> On Mon, Jun 23, 2025 at 6:24 PM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > On Mon, Jun 23, 2025 at 10:51:00AM -0700, Caleb Sander Mateos wrote:
> > > On Sun, Jun 22, 2025 at 6:19 PM Ming Lei <ming.lei@redhat.com> wrote:
> > > >
> > > > ublk_queue_cmd_list() dispatches the whole batch list by scheduling task
> > > > work via the tail request's io_uring_cmd, this way is fine even though
> > > > more than one io_ring_ctx are involved for this batch since it is just
> > > > one running context.
> > > >
> > > > However, the task work handler ublk_cmd_list_tw_cb() takes `issue_flags`
> > > > of tail uring_cmd's io_ring_ctx for completing all commands. This way is
> > > > wrong if any uring_cmd is issued from different io_ring_ctx.
> > > >
> > > > Fixes it by always building per-io-ring-ctx batch list.
> > > >
> > > > For typical per-queue or per-io daemon implementation, this way shouldn't
> > > > make difference from performance viewpoint, because single io_ring_ctx is
> > > > often taken in each daemon.
> > > >
> > > > Fixes: d796cea7b9f3 ("ublk: implement ->queue_rqs()")
> > > > Fixes: ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue daemon")
> > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > ---
> > > >  drivers/block/ublk_drv.c | 17 +++++++++--------
> > > >  1 file changed, 9 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > > index c637ea010d34..e79b04e61047 100644
> > > > --- a/drivers/block/ublk_drv.c
> > > > +++ b/drivers/block/ublk_drv.c
> > > > @@ -1336,9 +1336,8 @@ static void ublk_cmd_list_tw_cb(struct io_uring_cmd *cmd,
> > > >         } while (rq);
> > > >  }
> > > >
> > > > -static void ublk_queue_cmd_list(struct ublk_io *io, struct rq_list *l)
> > > > +static void ublk_queue_cmd_list(struct io_uring_cmd *cmd, struct rq_list *l)
> > > >  {
> > > > -       struct io_uring_cmd *cmd = io->cmd;
> > > >         struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
> > > >
> > > >         pdu->req_list = rq_list_peek(l);
> > > > @@ -1420,16 +1419,18 @@ static void ublk_queue_rqs(struct rq_list *rqlist)
> > > >  {
> > > >         struct rq_list requeue_list = { };
> > > >         struct rq_list submit_list = { };
> > > > -       struct ublk_io *io = NULL;
> > > > +       struct io_uring_cmd *cmd = NULL;
> > > >         struct request *req;
> > > >
> > > >         while ((req = rq_list_pop(rqlist))) {
> > > >                 struct ublk_queue *this_q = req->mq_hctx->driver_data;
> > > > -               struct ublk_io *this_io = &this_q->ios[req->tag];
> > > > +               struct io_uring_cmd *this_cmd = this_q->ios[req->tag].cmd;
> > > >
> > > > -               if (io && io->task != this_io->task && !rq_list_empty(&submit_list))
> > > > -                       ublk_queue_cmd_list(io, &submit_list);
> > > > -               io = this_io;
> > > > +               if (cmd && io_uring_cmd_ctx_handle(cmd) !=
> > > > +                               io_uring_cmd_ctx_handle(this_cmd) &&
> > > > +                               !rq_list_empty(&submit_list))
> > > > +                       ublk_queue_cmd_list(cmd, &submit_list);
> > >
> > > I don't think we can assume that ublk commands submitted to the same
> > > io_uring have the same daemon task. It's possible for multiple tasks
> > > to submit to the same io_uring, even though that's not a common or
> > > performant way to use io_uring. Probably we need to check that both
> > > the task and io_ring_ctx match.
> >
> > Here the problem is in 'issue_flags' passed from io_uring, especially for
> > grabbing io_ring_ctx lock.
> >
> > If two uring_cmd are issued via same io_ring_ctx from two tasks, it is
> > fine to share 'issue_flags' from one of tasks, what matters is that the
> > io_ring_ctx lock is handled correctly when calling io_uring_cmd_done().
> 
> Right, I understand the issue you are trying to solve. I agree it's a
> problem for submit_list to contain commands from multiple
> io_ring_ctxs. But it's also a problem if it contains commands with
> different daemon tasks, because ublk_queue_cmd_list() will schedule
> ublk_cmd_list_tw_cb() to be called in the *last command's task*. But
> ublk_cmd_list_tw_cb() will call ublk_dispatch_req() for all the
> commands in the list. So if submit_list contains commands with
> multiple daemon tasks, ublk_dispatch_req() will fail on the current !=
> io->task check. So I still feel we need to call
> ublk_queue_cmd_list(io, &submit_list) if io->task != this_io->task (as
> well as if the io_ring_ctxs differ).

Indeed, I will send a V2 for covering different task case.

Jens, can you drop this patch?

Thanks,
Ming

Re: [PATCH 1/2] ublk: build per-io-ring-ctx batch list

[Jens Axboe]

On 6/24/25 7:22 PM, Ming Lei wrote:
> On Tue, Jun 24, 2025 at 08:26:51AM -0700, Caleb Sander Mateos wrote:
>> On Mon, Jun 23, 2025 at 6:24 PM Ming Lei <ming.lei@redhat.com> wrote:
>>>
>>> On Mon, Jun 23, 2025 at 10:51:00AM -0700, Caleb Sander Mateos wrote:
>>>> On Sun, Jun 22, 2025 at 6:19 PM Ming Lei <ming.lei@redhat.com> wrote:
>>>>>
>>>>> ublk_queue_cmd_list() dispatches the whole batch list by scheduling task
>>>>> work via the tail request's io_uring_cmd, this way is fine even though
>>>>> more than one io_ring_ctx are involved for this batch since it is just
>>>>> one running context.
>>>>>
>>>>> However, the task work handler ublk_cmd_list_tw_cb() takes `issue_flags`
>>>>> of tail uring_cmd's io_ring_ctx for completing all commands. This way is
>>>>> wrong if any uring_cmd is issued from different io_ring_ctx.
>>>>>
>>>>> Fixes it by always building per-io-ring-ctx batch list.
>>>>>
>>>>> For typical per-queue or per-io daemon implementation, this way shouldn't
>>>>> make difference from performance viewpoint, because single io_ring_ctx is
>>>>> often taken in each daemon.
>>>>>
>>>>> Fixes: d796cea7b9f3 ("ublk: implement ->queue_rqs()")
>>>>> Fixes: ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue daemon")
>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>>>> ---
>>>>>  drivers/block/ublk_drv.c | 17 +++++++++--------
>>>>>  1 file changed, 9 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>>>>> index c637ea010d34..e79b04e61047 100644
>>>>> --- a/drivers/block/ublk_drv.c
>>>>> +++ b/drivers/block/ublk_drv.c
>>>>> @@ -1336,9 +1336,8 @@ static void ublk_cmd_list_tw_cb(struct io_uring_cmd *cmd,
>>>>>         } while (rq);
>>>>>  }
>>>>>
>>>>> -static void ublk_queue_cmd_list(struct ublk_io *io, struct rq_list *l)
>>>>> +static void ublk_queue_cmd_list(struct io_uring_cmd *cmd, struct rq_list *l)
>>>>>  {
>>>>> -       struct io_uring_cmd *cmd = io->cmd;
>>>>>         struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
>>>>>
>>>>>         pdu->req_list = rq_list_peek(l);
>>>>> @@ -1420,16 +1419,18 @@ static void ublk_queue_rqs(struct rq_list *rqlist)
>>>>>  {
>>>>>         struct rq_list requeue_list = { };
>>>>>         struct rq_list submit_list = { };
>>>>> -       struct ublk_io *io = NULL;
>>>>> +       struct io_uring_cmd *cmd = NULL;
>>>>>         struct request *req;
>>>>>
>>>>>         while ((req = rq_list_pop(rqlist))) {
>>>>>                 struct ublk_queue *this_q = req->mq_hctx->driver_data;
>>>>> -               struct ublk_io *this_io = &this_q->ios[req->tag];
>>>>> +               struct io_uring_cmd *this_cmd = this_q->ios[req->tag].cmd;
>>>>>
>>>>> -               if (io && io->task != this_io->task && !rq_list_empty(&submit_list))
>>>>> -                       ublk_queue_cmd_list(io, &submit_list);
>>>>> -               io = this_io;
>>>>> +               if (cmd && io_uring_cmd_ctx_handle(cmd) !=
>>>>> +                               io_uring_cmd_ctx_handle(this_cmd) &&
>>>>> +                               !rq_list_empty(&submit_list))
>>>>> +                       ublk_queue_cmd_list(cmd, &submit_list);
>>>>
>>>> I don't think we can assume that ublk commands submitted to the same
>>>> io_uring have the same daemon task. It's possible for multiple tasks
>>>> to submit to the same io_uring, even though that's not a common or
>>>> performant way to use io_uring. Probably we need to check that both
>>>> the task and io_ring_ctx match.
>>>
>>> Here the problem is in 'issue_flags' passed from io_uring, especially for
>>> grabbing io_ring_ctx lock.
>>>
>>> If two uring_cmd are issued via same io_ring_ctx from two tasks, it is
>>> fine to share 'issue_flags' from one of tasks, what matters is that the
>>> io_ring_ctx lock is handled correctly when calling io_uring_cmd_done().
>>
>> Right, I understand the issue you are trying to solve. I agree it's a
>> problem for submit_list to contain commands from multiple
>> io_ring_ctxs. But it's also a problem if it contains commands with
>> different daemon tasks, because ublk_queue_cmd_list() will schedule
>> ublk_cmd_list_tw_cb() to be called in the *last command's task*. But
>> ublk_cmd_list_tw_cb() will call ublk_dispatch_req() for all the
>> commands in the list. So if submit_list contains commands with
>> multiple daemon tasks, ublk_dispatch_req() will fail on the current !=
>> io->task check. So I still feel we need to call
>> ublk_queue_cmd_list(io, &submit_list) if io->task != this_io->task (as
>> well as if the io_ring_ctxs differ).
> 
> Indeed, I will send a V2 for covering different task case.
> 
> Jens, can you drop this patch?

Done

-- 
Jens Axboe