On Mon, Aug 24, 2020 at 01:19:56AM -0700, Sagi Grimberg wrote:
>
> > > > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > > > index bb5636cc17b9..5fa8bc1bb7a8 100644
> > > > --- a/include/linux/blkdev.h
> > > > +++ b/include/linux/blkdev.h
> > > > @@ -572,6 +572,10 @@ struct request_queue {
> > > > struct list_head tag_set_list;
> > > > struct bio_set bio_split;
> > > > + /* only used for BLK_MQ_F_BLOCKING */
> > > > + struct percpu_ref dispatch_counter;
> > >
> > > Can this be moved to be next to the q_usage_counter? they
> > > will be taken together for blocking drivers...
> >
> > I don't think it is a good idea, at least hctx->srcu is put at the end
> > of hctx, do you want to move it at beginning of hctx?
>
> 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").
>
> > .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.
So it may not be worth of putting the added .dispatch_counter together
with .q_usage_counter.
>
> > > 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, ...
Thanks,
Ming
