Cool, thanks. I'll poke around some more next week, but sounds good, let's go ahead with 667ea36378 for 6.6 and 6.1 then.
- leah
On Thu, Oct 3, 2024 at 10:58 PM Christoph Hellwig hch@lst.de wrote:
Hi Leah,
On Thu, Oct 03, 2024 at 02:13:52PM -0700, Leah Rumancik wrote:
Hello,
I have been investigating a read performance regression of dm-snapshot on top of loopback in which the read time for a dd command increased from 2min to 40min. I bisected the issue to dc5fc361d89 ("block: attempt direct issue of plug list"). I blktraced before and after this commit and the main difference I saw was that before this commit, when the performance was good, there were a lot of IO unplugs on the loop dev. After this commit, I saw 0 IO unplugs.
/me makes a not that it might make sense to enhance the tracing to show which of the trace_block_unplug call sites did a particular unplug because that might be helpful here, but I suspect the ones you saw the ones from blk_mq_dispatch_plug_list, which now gets bypassed.
I'm not really sure how that changed things, except that I know in old kernels we had issues with reordering I/O in this path, and maybe your workload hit that? Did the issue order change in the blktrace?
On the mainline, I was also able to bisect to a commit which fixed this issue: 667ea36378cf ("loop: don't set QUEUE_FLAG_NOMERGES"). I also blktraced before and after this commit, and unsurprisingly, the main difference was that commit resulted in IO merges whereas previously there were none being.
With QUEUE_FLAG_NOMERGES even very basic merging is enabled, so that would fix any smaller amount of reordering. It is in fact a bit stupid that this ever got set by default on the loop driver.
6.6.y and 6.1.y and were both experiencing the performance issue. I tried porting 667ea36378 to these branches; it applied cleanly and resolved the issue for both. So perhaps we should consider it for the stable trees, but it'd be great if someone from the block layer could chime in with a better idea of what's going on here.
I'm totally fine with backporting the patch.