Re: [PATCH] blk-mq: implement queue quiesce via percpu_ref for BLK_MQ_F_BLOCKING

[Ming Lei <ming.lei@redhat.com>]

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