Re: [PATCH] the dm-loop target

Re: [PATCH] the dm-loop target

[Ming Lei]

On Tue, Mar 11, 2025 at 12:27:23PM +1100, Dave Chinner wrote:
> On Mon, Mar 10, 2025 at 12:18:51PM +0100, Mikulas Patocka wrote:
> > 
> > 
> > On Sun, 9 Mar 2025, Ming Lei wrote:
> > 
> > > On Fri, Mar 07, 2025 at 04:21:58PM +0100, Mikulas Patocka wrote:
> > > > > I didn't say you were. I said the concept that dm-loop is based on
> > > > > is fundamentally flawed and that your benchmark setup does not
> > > > > reflect real world usage of loop devices.
> > > > 
> > > > > Where are the bug reports about the loop device being slow and the
> > > > > analysis that indicates that it is unfixable?
> > > > 
> > > > So, I did benchmarks on an enterprise nvme drive (SAMSUNG 
> > > > MZPLJ1T6HBJR-00007). I stacked ext4/loop/ext4, xfs/loop/xfs (using losetup 
> > > > --direct-io=on), ext4/dm-loop/ext4 and xfs/dm-loop/xfs. And loop is slow.
> > > > 
> > > > synchronous I/O:
> > > > fio --direct=1 --bs=4k --runtime=10 --time_based --numjobs=12 --ioengine=psync --iodepth=1 --group_reporting=1 --filename=/mnt/test2/l -name=job --rw=rw
> > > > raw block device:
> > > >    READ: bw=399MiB/s (418MB/s), 399MiB/s-399MiB/s (418MB/s-418MB/s), io=3985MiB (4179MB), run=10001-10001msec
> > > >   WRITE: bw=399MiB/s (418MB/s), 399MiB/s-399MiB/s (418MB/s-418MB/s), io=3990MiB (4184MB), run=10001-10001msec
> > > > ext4/loop/ext4:
> > > >    READ: bw=223MiB/s (234MB/s), 223MiB/s-223MiB/s (234MB/s-234MB/s), io=2232MiB (2341MB), run=10002-10002msec
> > > >   WRITE: bw=223MiB/s (234MB/s), 223MiB/s-223MiB/s (234MB/s-234MB/s), io=2231MiB (2339MB), run=10002-10002msec
> > > > xfs/loop/xfs:
> > > >    READ: bw=220MiB/s (230MB/s), 220MiB/s-220MiB/s (230MB/s-230MB/s), io=2196MiB (2303MB), run=10001-10001msec
> > > >   WRITE: bw=219MiB/s (230MB/s), 219MiB/s-219MiB/s (230MB/s-230MB/s), io=2193MiB (2300MB), run=10001-10001msec
> > > > ext4/dm-loop/ext4:
> > > >    READ: bw=338MiB/s (355MB/s), 338MiB/s-338MiB/s (355MB/s-355MB/s), io=3383MiB (3547MB), run=10002-10002msec
> > > >   WRITE: bw=338MiB/s (355MB/s), 338MiB/s-338MiB/s (355MB/s-355MB/s), io=3385MiB (3549MB), run=10002-10002msec
> > > > xfs/dm-loop/xfs:
> > > >    READ: bw=375MiB/s (393MB/s), 375MiB/s-375MiB/s (393MB/s-393MB/s), io=3752MiB (3934MB), run=10002-10002msec
> > > >   WRITE: bw=376MiB/s (394MB/s), 376MiB/s-376MiB/s (394MB/s-394MB/s), io=3756MiB (3938MB), run=10002-10002msec
> > > > 
> > > > asynchronous I/O:
> > > > fio --direct=1 --bs=4k --runtime=10 --time_based --numjobs=12 --ioengine=libaio --iodepth=16 --group_reporting=1 --filename=/mnt/test2/l -name=job --rw=rw
> > > > raw block device:
> > > >    READ: bw=1246MiB/s (1306MB/s), 1246MiB/s-1246MiB/s (1306MB/s-1306MB/s), io=12.2GiB (13.1GB), run=10001-10001msec
> > > >   WRITE: bw=1247MiB/s (1308MB/s), 1247MiB/s-1247MiB/s (1308MB/s-1308MB/s), io=12.2GiB (13.1GB), run=10001-10001msec
> > > > ext4/loop/ext4:
> > > >    READ: bw=274MiB/s (288MB/s), 274MiB/s-274MiB/s (288MB/s-288MB/s), io=2743MiB (2877MB), run=10001-10001msec
> > > >   WRITE: bw=275MiB/s (288MB/s), 275MiB/s-275MiB/s (288MB/s-288MB/s), io=2747MiB (2880MB), run=10001-10001msec
> > > > xfs/loop/xfs:
> > > >    READ: bw=276MiB/s (289MB/s), 276MiB/s-276MiB/s (289MB/s-289MB/s), io=2761MiB (2896MB), run=10002-10002msec
> > > >   WRITE: bw=276MiB/s (290MB/s), 276MiB/s-276MiB/s (290MB/s-290MB/s), io=2765MiB (2899MB), run=10002-10002msec
> > > > ext4/dm-loop/ext4:
> > > >    READ: bw=1189MiB/s (1247MB/s), 1189MiB/s-1189MiB/s (1247MB/s-1247MB/s), io=11.6GiB (12.5GB), run=10002-10002msec
> > > >   WRITE: bw=1190MiB/s (1248MB/s), 1190MiB/s-1190MiB/s (1248MB/s-1248MB/s), io=11.6GiB (12.5GB), run=10002-10002msec
> > > > xfs/dm-loop/xfs:
> > > >    READ: bw=1209MiB/s (1268MB/s), 1209MiB/s-1209MiB/s (1268MB/s-1268MB/s), io=11.8GiB (12.7GB), run=10001-10001msec
> > > >   WRITE: bw=1210MiB/s (1269MB/s), 1210MiB/s-1210MiB/s (1269MB/s-1269MB/s), io=11.8GiB (12.7GB), run=10001-10001msec
> > > 
> > > Hi Mikulas,
> > > 
> > > Please try the following patchset:
> > > 
> > > https://lore.kernel.org/linux-block/20250308162312.1640828-1-ming.lei@redhat.com/
> > > 
> > > which tries to handle IO in current context directly via NOWAIT, and
> > > supports MQ for loop since 12 io jobs are applied in your test. With this
> > > change, I can observe similar perf data on raw block device and loop/xfs
> > > over mq-virtio-scsi & nvme in my test VM.
> 
> I'm not sure RWF_NOWAIT is a workable solution.
> 
> Why?

It is just the sane implementation of Mikulas's static mapping
approach: no need to move to workqueue if the mapping is immutable
or sort of.

Also it matches with io_uring's FS read/write implementation:

- try to submit IO with NOWAIT first

- then fallback to io-wq in case of -EAGAIN

It isn't perfect, sometime it may be slower than running on io-wq
directly.

But is there any better way for covering everything?

I guess no, because FS can't tell us when the IO can be submitted
successfully via NOWAIT, and we can't know if it may succeed without
trying. And basically that is what the interface is designed.

> 
> IO submission is queued to a different thread context because to
> avoid a potential deadlock. That is, we are operating here in the
> writeback context of another filesystem, and so we cannot do things
> like depend on memory allocation making forwards progress for IO
> submission.  RWF_NOWAIT is not a guarantee that memory allocation
> will not occur in the IO submission path - it is implemented as best
> effort non-blocking behaviour.

Yes, that is why BLK_MQ_F_BLOCKING is added.

> 
> Further, if we have stacked loop devices (e.g.
> xfs-loop-ext4-loop-btrfs-loop-xfs) we can will be stacking
> RWF_NOWAIT IO submission contexts through multiple filesystems. This
> is not a filesystem IO path I want to support - being in the middle
> of such a stack creates all sorts of subtle constraints on behaviour
> that otherwise wouldn't exist. We actually do this sort of multi-fs
> stacking in fstests, so it's not a made up scenario.
> 
> I'm also concerned that RWF_NOWAIT submission is not an optimisation
> at all for the common sparse/COW image file case, because in this
> case RWF_NOWAIT failing with EAGAIN is just as common (if not
> moreso) than it succeeding.
> 
> i.e. in this case, RWF_NOWAIT writes will fail with -EAGAIN very
> frequently, so all that time spent doing IO submission is wasted
> time.

Right.

I saw that when I wrote ublk/zoned in which every write needs to
allocate space. It is workaround by preallocating space for one fixed
range or the whole zone.

> 
> Further, because allocation on write requires an exclusive lock and
> it is held for some time, this will affect read performance from the
> backing device as well. i.e. block mapping during a read while a
> write is doing allocation will also return EAGAIN for RWF_NOWAIT.

But that can't be avoided without using NOWAIT, and read is blocked
when write(WAIT) is in-progress.

> This will push the read off to the background worker thread to be
> serviced and so that will go much slower than a RWF_NOWAIT read that
> hits the backing file between writes doing allocation. i.e. read
> latency is going to become much, much more variable.
> 
> Hence I suspect RWF_NOWAIT is simply hiding the underlying issue
> by providing this benchmark with a "pure overwrite" fast path that
> avoids the overhead of queueing work through loop_queue_work()....
> 
> Can you run these same loop dev tests using a sparse image file and
> a sparse fio test file so that the fio benchmark measures the impact
> of loop device block allocation on the test? I suspect the results
> with the RWF_NOWAIT patch will be somewhat different to the fully
> allocated case...

Yes, it will be slower, and io_uring FS IO application is in
the same situation, and usually application doesn't have such
knowledge if RWF_NOWAIT can succeed.

However, usually meta IO is much less compared with normal IO, so most
times it will be a win to try NOWAIT first.

> 
> > 
> > Yes - with these patches, it is much better.
> > 
> > > 1) try single queue first by `modprobe loop`
> > 
> > fio --direct=1 --bs=4k --runtime=10 --time_based --numjobs=12 --ioengine=psync --iodepth=1 --group_reporting=1 --filename=/mnt/test2/l -name=job --rw=rw
> > xfs/loop/xfs
> >    READ: bw=302MiB/s (317MB/s), 302MiB/s-302MiB/s (317MB/s-317MB/s), io=3024MiB (3170MB), run=10001-10001msec
> >   WRITE: bw=303MiB/s (317MB/s), 303MiB/s-303MiB/s (317MB/s-317MB/s), io=3026MiB (3173MB), run=10001-10001msec
> > 
> > fio --direct=1 --bs=4k --runtime=10 --time_based --numjobs=12 --ioengine=libaio --iodepth=16 --group_reporting=1 --filename=/mnt/test2/l -name=job --rw=rw
> > xfs/loop/xfs
> >    READ: bw=1055MiB/s (1106MB/s), 1055MiB/s-1055MiB/s (1106MB/s-1106MB/s), io=10.3GiB (11.1GB), run=10001-10001msec
> >   WRITE: bw=1056MiB/s (1107MB/s), 1056MiB/s-1056MiB/s (1107MB/s-1107MB/s), io=10.3GiB (11.1GB), run=10001-10001msec
> 
> Yup, this sort of difference in performance simply from bypassing
> loop_queue_work() implies the problem is the single threaded loop
> device queue implementation needs to be fixed.
> 
> loop_queue_work()
> {
> 	....
> 	spin_lock_irq(&lo->lo_work_lock);
> 	....
> 
>         } else {
>                 work = &lo->rootcg_work;
>                 cmd_list = &lo->rootcg_cmd_list;
>         }
> 	list_add_tail(&cmd->list_entry, cmd_list);
> 	queue_work(lo->workqueue, work);
> 	spin_unlock_irq(&lo->lo_work_lock);
> }
> 
> Not only does every IO that is queued takes this queue lock, there
> is only one work instance for the loop device. Therefore there is
> only one kworker process per control group that does IO submission
> for the loop device. And that kworker thread also takes the work
> lock to do dequeue as well.
> 
> That serialised queue with a single IO dispatcher thread looks to be
> the performance bottleneck to me. We could get rid of the lock by
> using a llist for this multi-producer/single consumer cmd list
> pattern, though I suspect we can get rid of the list entirely...
> 
> i.e. we have a work queue that can run a
> thousand concurrent works for this loop device, but the request
> queue is depth limited to 128 requests. hence we can have a full
> set of requests in flight and not run out of submission worker
> concurrency. There's no need to isolate IO from different cgroups in
> this situation - they will not get blocked behind IO submission
> from a different cgroup that is throttled...
> 
> i.e. the cmd->list_entry list_head could be replaced with a struct
> work_struct and that whole cmd list management and cgroup scheduling
> thing could be replaced with a single call to
> queue_work(cmd->io_work). i.e. the single point that all IO

Then there will be many FS write contention, :-)

> submission is directed through goes away completely.

It has been shown many times that AIO submitted from single or much less
contexts is much more efficient than running IO concurrently from multiple
contexts, especially for sequential IO.

Please see the recent example of zloop vs. ublk/zoned:

https://lore.kernel.org/linux-block/d5e59531-c19b-4332-8f47-b380ab9678be@kernel.org/

When zloop takes single dispatcher just like the in-tree loop, sequential
WRITE performs much better than initial version of ublk/zoned, which just
handles every IO in its own io-wq(one time NOWAIT & -EAGAIN and one time fallback
to io-wq). I tried to submit FS WRITE via io-wq directly and it becomes what
your suggested, the improvement is small, and still much worse than zloop's
single dispatcher.

Later, when I switch to pre-allocate space for each zone or one fixed range,
each write is submitted with NOWAIT successfully, then the sequential write perf
is improved much:

https://lore.kernel.org/linux-block/Z6QrceGGAJl_X_BM@fedora/



Thanks,
Ming

Re: [PATCH] the dm-loop target

[Dave Chinner]

On Tue, Mar 11, 2025 at 06:43:22PM +0800, Ming Lei wrote:
> On Tue, Mar 11, 2025 at 12:27:23PM +1100, Dave Chinner wrote:
> > On Mon, Mar 10, 2025 at 12:18:51PM +0100, Mikulas Patocka wrote:
> > > On Sun, 9 Mar 2025, Ming Lei wrote:
> > > > Please try the following patchset:
> > > > 
> > > > https://lore.kernel.org/linux-block/20250308162312.1640828-1-ming.lei@redhat.com/
> > > > 
> > > > which tries to handle IO in current context directly via NOWAIT, and
> > > > supports MQ for loop since 12 io jobs are applied in your test. With this
> > > > change, I can observe similar perf data on raw block device and loop/xfs
> > > > over mq-virtio-scsi & nvme in my test VM.
> > 
> > I'm not sure RWF_NOWAIT is a workable solution.
> > 
> > Why?
> 
> It is just the sane implementation of Mikulas's static mapping
> approach: no need to move to workqueue if the mapping is immutable
> or sort of.
> 
> Also it matches with io_uring's FS read/write implementation:
>
> - try to submit IO with NOWAIT first
> - then fallback to io-wq in case of -EAGAIN

No, it doesn't match what io_uring does. yes, the NOWAIT bit does,
but the work queue implementation is in the loop device is
completely different to the way io_uring dispatches work.

That is, io_uring dispatches one IO per wroker thread context so
they can all run in parallel down through the filesystem. The loop
device has a single worker thread so it -serialises- IO submission
to the filesystem.

i.e. blocking a single IO submission in the loop worker blocks all
IO submission, whereas io_uring submits all IO indepedently so they
only block if serialisation occurs further down the stack.

> It isn't perfect, sometime it may be slower than running on io-wq
> directly.
> 
> But is there any better way for covering everything?

Yes - fix the loop queue workers.

> I guess no, because FS can't tell us when the IO can be submitted
> successfully via NOWAIT, and we can't know if it may succeed without
> trying. And basically that is what the interface is designed.

Wrong.

Speculative non-blocking IO like NOWAIT is the wrong optimisation to
make for workloads that are very likely to block in the IO path. It
just adds overhead without adding any improvement in performance.

Getting rid of the serialised IO submission problems that the loop
device current has will benefit *all* workloads that use the loop
device, not just those that are fully allocated. Yes, it won't quite
show the same performance as NOWAIT in that case, but it still
should give 90-95% of native performance for the static file case.
And it should also improve all the other cases, too, because now
they will only serialise when the backing file needs IO operations to
serialise (i.e. during allocation).

> > IO submission is queued to a different thread context because to
> > avoid a potential deadlock. That is, we are operating here in the
> > writeback context of another filesystem, and so we cannot do things
> > like depend on memory allocation making forwards progress for IO
> > submission.  RWF_NOWAIT is not a guarantee that memory allocation
> > will not occur in the IO submission path - it is implemented as best
> > effort non-blocking behaviour.
> 
> Yes, that is why BLK_MQ_F_BLOCKING is added.
> 
> > 
> > Further, if we have stacked loop devices (e.g.
> > xfs-loop-ext4-loop-btrfs-loop-xfs) we can will be stacking
> > RWF_NOWAIT IO submission contexts through multiple filesystems. This
> > is not a filesystem IO path I want to support - being in the middle
> > of such a stack creates all sorts of subtle constraints on behaviour
> > that otherwise wouldn't exist. We actually do this sort of multi-fs
> > stacking in fstests, so it's not a made up scenario.
> > 
> > I'm also concerned that RWF_NOWAIT submission is not an optimisation
> > at all for the common sparse/COW image file case, because in this
> > case RWF_NOWAIT failing with EAGAIN is just as common (if not
> > moreso) than it succeeding.
> > 
> > i.e. in this case, RWF_NOWAIT writes will fail with -EAGAIN very
> > frequently, so all that time spent doing IO submission is wasted
> > time.
> 
> Right.
> 
> I saw that when I wrote ublk/zoned in which every write needs to
> allocate space. It is workaround by preallocating space for one fixed
> range or the whole zone.

*cough*

You do realise that fallocate() serialises all IO on the file? i.e.
not only does it block all IO submission, mmap IO and other metadata
operations, it also waits for all IO in flight to complete and it
doesn't allow any IO to restart until the preallocation is complete.

i.e. preallocation creates a huge IO submission latency bubble in
the IO path. Hence preallocation is something that should be avoided
at runtime if at all possible.

If you have problems with allocation overhead during IO, then we
have things like extent size hints that allow the per-IO allocation
to be made much larger than the IO itself. This effectively does
preallocation during IO without the IO pipeline bubbles that
preallocation requires to function correctly....

> > Further, because allocation on write requires an exclusive lock and
> > it is held for some time, this will affect read performance from the
> > backing device as well. i.e. block mapping during a read while a
> > write is doing allocation will also return EAGAIN for RWF_NOWAIT.
> 
> But that can't be avoided without using NOWAIT, and read is blocked
> when write(WAIT) is in-progress.

If the loop worker dispatches each IO in it's own task context, then
we don't care if a read IO blocks on some other write in progress.
It doesn't get in the way of any other IO submission...

> > This will push the read off to the background worker thread to be
> > serviced and so that will go much slower than a RWF_NOWAIT read that
> > hits the backing file between writes doing allocation. i.e. read
> > latency is going to become much, much more variable.
> > 
> > Hence I suspect RWF_NOWAIT is simply hiding the underlying issue
> > by providing this benchmark with a "pure overwrite" fast path that
> > avoids the overhead of queueing work through loop_queue_work()....
> > 
> > Can you run these same loop dev tests using a sparse image file and
> > a sparse fio test file so that the fio benchmark measures the impact
> > of loop device block allocation on the test? I suspect the results
> > with the RWF_NOWAIT patch will be somewhat different to the fully
> > allocated case...
> 
> Yes, it will be slower, and io_uring FS IO application is in
> the same situation, and usually application doesn't have such
> knowledge if RWF_NOWAIT can succeed.

And that's my point: nothing above the filesystem will have - or
can have - any knowledge of whether NOWAIT IO will succeed or
not.

Therefore we have to use our brains to analyse what the typical
behaviour of the filesystem will be for a given situation to
determine how best to optimise IO submission.

> However, usually meta IO is much less compared with normal IO, so most
> times it will be a win to try NOWAIT first.

Data vs metadata from the upper filesystem doesn't even enter into
the equation here.

Filesystems like XFS, bcachefs and btrfs all dynamically create,
move and remove metadata, so metadata writes are as much of an
allocation problem for sparse loop backing files as write-once user
data IO is.

> > Yup, this sort of difference in performance simply from bypassing
> > loop_queue_work() implies the problem is the single threaded loop
> > device queue implementation needs to be fixed.
> > 
> > loop_queue_work()
> > {
> > 	....
> > 	spin_lock_irq(&lo->lo_work_lock);
> > 	....
> > 
> >         } else {
> >                 work = &lo->rootcg_work;
> >                 cmd_list = &lo->rootcg_cmd_list;
> >         }
> > 	list_add_tail(&cmd->list_entry, cmd_list);
> > 	queue_work(lo->workqueue, work);
> > 	spin_unlock_irq(&lo->lo_work_lock);
> > }
> > 
> > Not only does every IO that is queued takes this queue lock, there
> > is only one work instance for the loop device. Therefore there is
> > only one kworker process per control group that does IO submission
> > for the loop device. And that kworker thread also takes the work
> > lock to do dequeue as well.
> > 
> > That serialised queue with a single IO dispatcher thread looks to be
> > the performance bottleneck to me. We could get rid of the lock by
> > using a llist for this multi-producer/single consumer cmd list
> > pattern, though I suspect we can get rid of the list entirely...
> > 
> > i.e. we have a work queue that can run a
> > thousand concurrent works for this loop device, but the request
> > queue is depth limited to 128 requests. hence we can have a full
> > set of requests in flight and not run out of submission worker
> > concurrency. There's no need to isolate IO from different cgroups in
> > this situation - they will not get blocked behind IO submission
> > from a different cgroup that is throttled...
> > 
> > i.e. the cmd->list_entry list_head could be replaced with a struct
> > work_struct and that whole cmd list management and cgroup scheduling
> > thing could be replaced with a single call to
> > queue_work(cmd->io_work). i.e. the single point that all IO
> 
> Then there will be many FS write contention, :-)

Did you think about how much parallelism your NOWAIT patches are
directing at the loop device backing file before making that
comment?

The fio test that was run had 12 jobs running, there is at least
12-way IO concurrency hitting the backing file directly via the
NOWAIT path.

Yup, the NOWAIT path exposes the filesystem directly to userspace
write IO concurrency.  Yet the filesystem IO scaled to near raw
device speed....

It should be obvious that adding more IO concurrency to loop
device IO submission isn't a problem for the XFS IO path. And if it
proves to be an issue, then that's an XFS problem to solve, not a
generic device scalability issue.

> > submission is directed through goes away completely.
> 
> It has been shown many times that AIO submitted from single or much less
> contexts is much more efficient than running IO concurrently from multiple
> contexts, especially for sequential IO.

Yes, I know that, and I'm not disputing that this is the most
optimal way to submit IO *when it doesn't require blocking to
dispatch*.

However, the fact is this benefit only exists when all IO can be
submitted in a non-blocking manner.

As soon as blocking occurs in IO submission, then AIO submission
either becomes synchronous or is aborted with EAGAIN (i.e. NOWAIT
io).

If we use blocking submission behaviour, then performance tanks.
This is what the loop device does right now.

If we use non-blocking submission and get -EAGAIN, then we still
need to use the slightly less efficient method of per-IO task
contexts to scale out blocking IO submission.

With that in mind, we must accept that loop device IO *frequently
needs to block* and so non-blocking IO is the wrong behaviour to
optimise *exclusively* for.

Hence we need to first make the loop device scale with per-IO task
contexts as this will improve both IO that can be dispatched without
blocking as well as IO that will block during dispatch.  If this
does not bring performance up to acceptible levels, then other
optimisations can then be considered.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] the dm-loop target

[Christoph Hellwig]

On Wed, Mar 12, 2025 at 01:34:02PM +1100, Dave Chinner wrote:
> Wrong.
> 
> Speculative non-blocking IO like NOWAIT is the wrong optimisation to
> make for workloads that are very likely to block in the IO path. It
> just adds overhead without adding any improvement in performance.

Note that I suspect that most or at least many loop workloads are
read-heavy.  And at least for reads NOWAIT makes perfect sense.

> Getting rid of the serialised IO submission problems that the loop
> device current has will benefit *all* workloads that use the loop
> device, not just those that are fully allocated. Yes, it won't quite
> show the same performance as NOWAIT in that case, but it still
> should give 90-95% of native performance for the static file case.
> And it should also improve all the other cases, too, because now
> they will only serialise when the backing file needs IO operations to
> serialise (i.e. during allocation).

And I agree that this should be a first step.

> *cough*

The whole ublk-zoned is a bit of a bullshit thing where Ming wrote
up something that barely works to block inclusion of the zloop driver
we really need for zoned xfs testing.  Please don't take it serious.

Re: [PATCH] the dm-loop target

[Ming Lei]

[-- Attachment #1: Type: text/plain, Size: 9748 bytes --]

On Wed, Mar 12, 2025 at 01:34:02PM +1100, Dave Chinner wrote:
> On Tue, Mar 11, 2025 at 06:43:22PM +0800, Ming Lei wrote:
> > On Tue, Mar 11, 2025 at 12:27:23PM +1100, Dave Chinner wrote:
> > > On Mon, Mar 10, 2025 at 12:18:51PM +0100, Mikulas Patocka wrote:
> > > > On Sun, 9 Mar 2025, Ming Lei wrote:
> > > > > Please try the following patchset:
> > > > > 
> > > > > https://lore.kernel.org/linux-block/20250308162312.1640828-1-ming.lei@redhat.com/
> > > > > 
> > > > > which tries to handle IO in current context directly via NOWAIT, and
> > > > > supports MQ for loop since 12 io jobs are applied in your test. With this
> > > > > change, I can observe similar perf data on raw block device and loop/xfs
> > > > > over mq-virtio-scsi & nvme in my test VM.
> > > 
> > > I'm not sure RWF_NOWAIT is a workable solution.
> > > 
> > > Why?
> > 
> > It is just the sane implementation of Mikulas's static mapping
> > approach: no need to move to workqueue if the mapping is immutable
> > or sort of.
> > 
> > Also it matches with io_uring's FS read/write implementation:
> >
> > - try to submit IO with NOWAIT first
> > - then fallback to io-wq in case of -EAGAIN
> 
> No, it doesn't match what io_uring does. yes, the NOWAIT bit does,
> but the work queue implementation is in the loop device is
> completely different to the way io_uring dispatches work.
> 
> That is, io_uring dispatches one IO per wroker thread context so
> they can all run in parallel down through the filesystem. The loop
> device has a single worker thread so it -serialises- IO submission
> to the filesystem.
> 
> i.e. blocking a single IO submission in the loop worker blocks all
> IO submission, whereas io_uring submits all IO indepedently so they
> only block if serialisation occurs further down the stack.

block layer/storage has many optimization for batching handling, if IOs
are submitted from many contexts:

- this batch handling optimization is gone

- IO is re-ordered from underlying hardware viewpoint

- more contention from FS write lock, because loop has single back file.

That is why the single task context is taken from the beginning of loop aio,
and it performs pretty well for sequential IO workloads, as I shown
in the zloop example.

> 
> > It isn't perfect, sometime it may be slower than running on io-wq
> > directly.
> > 
> > But is there any better way for covering everything?
> 
> Yes - fix the loop queue workers.

What you suggested is threaded aio by submitting IO concurrently from
different task context, this way is not the most efficient one, otherwise
modern language won't invent async/.await.

In my test VM, by running Mikulas's fio script on loop/nvme by the attached
threaded_aio patch:

NOWAIT with MQ 4		:   70K iops(read), 70K iops(write), cpu util: 40%
threaded_aio with MQ 4	:	64k iops(read), 64K iops(write), cpu util: 52% 
in tree loop(SQ)		:   58K	iops(read), 58K iops(write)	

Mikulas, please feel free to run your tests with threaded_aio:

	modprobe loop nr_hw_queues=4 threaded_aio=1

by applying the attached the patch over the loop patchset.

The performance gap could be more obvious in fast hardware.

> 
> > I guess no, because FS can't tell us when the IO can be submitted
> > successfully via NOWAIT, and we can't know if it may succeed without
> > trying. And basically that is what the interface is designed.
> 
> Wrong.
> 
> Speculative non-blocking IO like NOWAIT is the wrong optimisation to
> make for workloads that are very likely to block in the IO path. It
> just adds overhead without adding any improvement in performance.

Mikulas's ->bmap() implies that there isn't block in IO path too.

And many io_uring application shouldn't have idea if the IO will block too,
but they still use NOWAIT...

> 
> Getting rid of the serialised IO submission problems that the loop
> device current has will benefit *all* workloads that use the loop
> device, not just those that are fully allocated. Yes, it won't quite
> show the same performance as NOWAIT in that case, but it still
> should give 90-95% of native performance for the static file case.
> And it should also improve all the other cases, too, because now
> they will only serialise when the backing file needs IO operations to
> serialise (i.e. during allocation).

The above test shows that concurrent submission isn't better than NOWAIT,
and my ublk/zoned examples showed the point too.

> 
> > > IO submission is queued to a different thread context because to
> > > avoid a potential deadlock. That is, we are operating here in the
> > > writeback context of another filesystem, and so we cannot do things
> > > like depend on memory allocation making forwards progress for IO
> > > submission.  RWF_NOWAIT is not a guarantee that memory allocation
> > > will not occur in the IO submission path - it is implemented as best
> > > effort non-blocking behaviour.
> > 
> > Yes, that is why BLK_MQ_F_BLOCKING is added.
> > 
> > > 
> > > Further, if we have stacked loop devices (e.g.
> > > xfs-loop-ext4-loop-btrfs-loop-xfs) we can will be stacking
> > > RWF_NOWAIT IO submission contexts through multiple filesystems. This
> > > is not a filesystem IO path I want to support - being in the middle
> > > of such a stack creates all sorts of subtle constraints on behaviour
> > > that otherwise wouldn't exist. We actually do this sort of multi-fs
> > > stacking in fstests, so it's not a made up scenario.
> > > 
> > > I'm also concerned that RWF_NOWAIT submission is not an optimisation
> > > at all for the common sparse/COW image file case, because in this
> > > case RWF_NOWAIT failing with EAGAIN is just as common (if not
> > > moreso) than it succeeding.
> > > 
> > > i.e. in this case, RWF_NOWAIT writes will fail with -EAGAIN very
> > > frequently, so all that time spent doing IO submission is wasted
> > > time.
> > 
> > Right.
> > 
> > I saw that when I wrote ublk/zoned in which every write needs to
> > allocate space. It is workaround by preallocating space for one fixed
> > range or the whole zone.
> 
> *cough*
> 
> You do realise that fallocate() serialises all IO on the file? i.e.
> not only does it block all IO submission, mmap IO and other metadata
> operations, it also waits for all IO in flight to complete and it
> doesn't allow any IO to restart until the preallocation is complete.
> 

It isn't one issue for zloop & ublk/zoned, each zone is emulated by one
single file, and the zone is usual the parallelism unit.

> i.e. preallocation creates a huge IO submission latency bubble in
> the IO path. Hence preallocation is something that should be avoided
> at runtime if at all possible.
> 
> If you have problems with allocation overhead during IO, then we
> have things like extent size hints that allow the per-IO allocation
> to be made much larger than the IO itself. This effectively does
> preallocation during IO without the IO pipeline bubbles that
> preallocation requires to function correctly....

OK, I will try it later in ublk/zoned and see if the pre-allocation can
be replaced by extent size hints, is it available for all popular FS?

> 
> > > Further, because allocation on write requires an exclusive lock and
> > > it is held for some time, this will affect read performance from the
> > > backing device as well. i.e. block mapping during a read while a
> > > write is doing allocation will also return EAGAIN for RWF_NOWAIT.
> > 
> > But that can't be avoided without using NOWAIT, and read is blocked
> > when write(WAIT) is in-progress.
> 
> If the loop worker dispatches each IO in it's own task context, then
> we don't care if a read IO blocks on some other write in progress.
> It doesn't get in the way of any other IO submission...
> 
> > > This will push the read off to the background worker thread to be
> > > serviced and so that will go much slower than a RWF_NOWAIT read that
> > > hits the backing file between writes doing allocation. i.e. read
> > > latency is going to become much, much more variable.
> > > 
> > > Hence I suspect RWF_NOWAIT is simply hiding the underlying issue
> > > by providing this benchmark with a "pure overwrite" fast path that
> > > avoids the overhead of queueing work through loop_queue_work()....
> > > 
> > > Can you run these same loop dev tests using a sparse image file and
> > > a sparse fio test file so that the fio benchmark measures the impact
> > > of loop device block allocation on the test? I suspect the results
> > > with the RWF_NOWAIT patch will be somewhat different to the fully
> > > allocated case...
> > 
> > Yes, it will be slower, and io_uring FS IO application is in
> > the same situation, and usually application doesn't have such
> > knowledge if RWF_NOWAIT can succeed.
> 
> And that's my point: nothing above the filesystem will have - or
> can have - any knowledge of whether NOWAIT IO will succeed or
> not.
> 
> Therefore we have to use our brains to analyse what the typical
> behaviour of the filesystem will be for a given situation to
> determine how best to optimise IO submission.

Indeed, I think FS typical behaviour for loop case is valuable for
optimizing loop driver, but care to provide more details if you have?

> 
> > However, usually meta IO is much less compared with normal IO, so most
> > times it will be a win to try NOWAIT first.
> 
> Data vs metadata from the upper filesystem doesn't even enter into
> the equation here.
> 
> Filesystems like XFS, bcachefs and btrfs all dynamically create,
> move and remove metadata, so metadata writes are as much of an
> allocation problem for sparse loop backing files as write-once user
> data IO is.

For typical loop workloads, I still see much less metadata io, and that
is why improvement from NOWAIT is big in Mikulas's test.


Thanks,
Ming

[-- Attachment #2: 0001-loop-debug-add-threaded-aio.patch --]
[-- Type: text/plain, Size: 3163 bytes --]

From b3c78b04ac6b80e171271a833c0221b8ab3b2b00 Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei@redhat.com>
Date: Wed, 12 Mar 2025 07:33:55 +0000
Subject: [PATCH] loop: debug: add threaded aio

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/loop.c | 47 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 393f38573169..113e29b3ac80 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -86,6 +86,8 @@ struct loop_cmd {
 	struct bio_vec *bvec;
 	struct cgroup_subsys_state *blkcg_css;
 	struct cgroup_subsys_state *memcg_css;
+
+	struct work_struct work;
 };
 
 #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)
@@ -98,6 +100,8 @@ static DEFINE_IDR(loop_index_idr);
 static DEFINE_MUTEX(loop_ctl_mutex);
 static DEFINE_MUTEX(loop_validate_mutex);
 
+static unsigned int threaded_aio = 0;
+
 /**
  * loop_global_lock_killable() - take locks for safe loop_validate_file() test
  *
@@ -508,10 +512,17 @@ static int lo_rw_aio_prep(struct loop_device *lo, struct loop_cmd *cmd,
 static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd, loff_t pos)
 {
 	unsigned int nr_bvec = lo_cmd_nr_bvec(cmd);
-	int ret;
+	int ret = 0;
+
+	if (threaded_aio) {
+		ret = lo_rw_aio_prep(lo, cmd, nr_bvec);
+		if (ret < 0)
+			goto exit;
+	}
 
 	cmd->iocb.ki_flags &= ~IOCB_NOWAIT;
 	ret = lo_submit_rw_aio(lo, cmd, pos, nr_bvec);
+exit:
 	if (ret != -EIOCBQUEUED)
 		lo_rw_aio_complete(&cmd->iocb, ret);
 	return 0;
@@ -1941,10 +1952,39 @@ static const struct kernel_param_ops loop_nr_hw_q_param_ops = {
 device_param_cb(nr_hw_queues, &loop_nr_hw_q_param_ops, &nr_hw_queues, 0444);
 MODULE_PARM_DESC(nr_hw_queues, "number of hardware queues. Default: " __stringify(LOOP_DEFAULT_NR_HW_Q));
 
+module_param(threaded_aio, uint, 0644);
+MODULE_PARM_DESC(threaded_aio, "threaded aio");
+
 MODULE_DESCRIPTION("Loopback device support");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);
 
+static void loop_aio_workfn(struct work_struct *work)
+{
+	struct loop_cmd *cmd = container_of(work, struct loop_cmd, work);
+	struct request *rq = blk_mq_rq_from_pdu(cmd);
+	struct loop_device *lo = rq->q->queuedata;
+	int ret = do_req_filebacked(lo, rq);
+
+	if (!cmd->use_aio || ret) {
+		if (ret == -EOPNOTSUPP)
+			cmd->ret = ret;
+		else
+			cmd->ret = ret ? -EIO : 0;
+		if (likely(!blk_should_fake_timeout(rq->q)))
+			blk_mq_complete_request(rq);
+	}
+}
+
+static void loop_queue_rq_threaded_aio(struct loop_cmd *cmd)
+{
+	struct request *rq = blk_mq_rq_from_pdu(cmd);
+	struct loop_device *lo = rq->q->queuedata;
+
+	INIT_WORK(&cmd->work, loop_aio_workfn);
+	queue_work(lo->workqueue, &cmd->work);
+}
+
 static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 		const struct blk_mq_queue_data *bd)
 {
@@ -1968,6 +2008,11 @@ static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 		break;
 	}
 
+	if (threaded_aio) {
+		loop_queue_rq_threaded_aio(cmd);
+		return BLK_STS_OK;
+	}
+
 	if (cmd->use_aio) {
 		loff_t pos = ((loff_t) blk_rq_pos(rq) << 9) + lo->lo_offset;
 		int ret = lo_rw_aio_nowait(lo, cmd, pos);
-- 
2.47.1