I'd think it'd be an improvement, yes.
Please see the reason why it is put back of hctx in
073196787727("blk-mq: Reduce blk_mq_hw_ctx size").
I know why it is there, just was saying that having an additional
pointer is fine. But the discussion is moot.
.q_usage_counter should have been put in the 1st cacheline of
request queue. If it is moved to the 1st cacheline of request queue,
we shouldn't put 'dispatch_counter' there, because it may hurt other
non-blocking drivers.
q_usage_counter currently there, and the two will always be taken
together, and there are several other stuff that we can remove from
that cacheline without hurting performance for anything.
And when q_usage_counter is moved to the first cacheline, then I'd
expect that the dispatch_counter also moves to the front (maybe not
the first if it is on the expense of other hot members, but definitely
it should be treated as a hot member).
Anyways, I think that for now we should place them together.
Then it may hurt non-blocking.
Each hctx has only one run-work, if the hctx is blocked, no other request
may be queued to hctx any more. That is basically sync run queue, so I
am not sure good enough perf can be expected on blocking.
I don't think that you should assume that a blocking driver will block
normally, it will only rarely block (very rarely).
So it may not be worth of putting the added .dispatch_counter together
with .q_usage_counter.
I happen to think it would. Not sure why you resist so much given how
request_queue is arranged currently.
Also maybe a better name is needed here since it's just
for blocking hctxs.
+ wait_queue_head_t mq_quiesce_wq;
+
struct dentry *debugfs_dir;
#ifdef CONFIG_BLK_DEBUG_FS
What I think is needed here is at a minimum test quiesce/unquiesce loops
during I/O. code auditing is not enough, there may be driver assumptions
broken with this change (although I hope there shouldn't be).
We have elevator switch / updating nr_request stress test, and both relies
on quiesce/unquiesce, and I did run such test with this patch.
You have a blktest for this? If not, I strongly suggest that one is
added to validate the change also moving forward.
There are lots of blktest tests doing that, such as block/005,
block/016, block/021, ...
Good, but I'd also won't want to get this without making sure the async
quiesce works well on large number of namespaces (the reason why this
is proposed in the first place). Not sure who is planning to do that...
