Re: [PATCH V2 0/2] blk-mq: implement queue quiesce via percpu_ref for BLK_MQ_F_BLOCKING

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

On Thu, Sep 03, 2020 at 05:37:02AM -0700, Keith Busch wrote:
> On Thu, Sep 03, 2020 at 08:35:45AM +0800, Ming Lei wrote:
> > On Wed, Sep 02, 2020 at 11:52:59AM -0600, Jens Axboe wrote:
> > > On 9/1/20 9:11 PM, Ming Lei wrote:
> > > > On Tue, Aug 25, 2020 at 10:17:32PM +0800, Ming Lei wrote:
> > > >> Hi Jens,
> > > >>
> > > >> The 1st patch add .mq_quiesce_mutex for serializing quiesce/unquiesce,
> > > >> and prepares for replacing srcu with percpu_ref.
> > > >>
> > > >> The 2nd patch replaces srcu with percpu_ref.
> > > >>
> > > >> V2:
> > > >> 	- add .mq_quiesce_lock
> > > >> 	- add comment on patch 2 wrt. handling hctx_lock() failure
> > > >> 	- trivial patch style change
> > > >>
> > > >>
> > > >> Ming Lei (2):
> > > >>   blk-mq: serialize queue quiesce and unquiesce by mutex
> > > >>   blk-mq: implement queue quiesce via percpu_ref for BLK_MQ_F_BLOCKING
> > > >>
> > > >>  block/blk-core.c       |   2 +
> > > >>  block/blk-mq-sysfs.c   |   2 -
> > > >>  block/blk-mq.c         | 125 +++++++++++++++++++++++------------------
> > > >>  block/blk-sysfs.c      |   6 +-
> > > >>  include/linux/blk-mq.h |   7 ---
> > > >>  include/linux/blkdev.h |   6 ++
> > > >>  6 files changed, 82 insertions(+), 66 deletions(-)
> > > >>
> > > >> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > > >> Cc: Paul E. McKenney <paulmck@kernel.org>
> > > >> Cc: Josh Triplett <josh@joshtriplett.org>
> > > >> Cc: Sagi Grimberg <sagi@grimberg.me>
> > > >> Cc: Bart Van Assche <bvanassche@acm.org>
> > > >> Cc: Johannes Thumshirn <Johannes.Thumshirn@wdc.com>
> > > >> Cc: Chao Leng <lengchao@huawei.com>
> > > >> Cc: Christoph Hellwig <hch@lst.de>
> > > > 
> > > > Hello Guys,
> > > > 
> > > > Is there any objections on the two patches? If not, I'd suggest to move> on.
> > > 
> > > Seems like the nested case is one that should either be handled, or at
> > > least detected.
> > 
> > Yeah, the 1st patch adds mutex for handling nested case correctly and efficiently.
> 
> That doesn't really do anything about handling nested quiesce/unquiesce.

The mutex is required for avoiding warning in percpu_ref_kill_and_confirm() if
two queue quiesce are nested, and I will comment on this motivation in next
version.

> It just prevents two threads from doing it at the same time, but anyone
> can still undo the other's expected queue state. The following on top of

Right, the patch itself changes nothing wrt. this point, and it breaks nothing
too.

> your series will at least detect the condition:
> 
> ---
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index ef6c6fa8dab0..52b53f2bb567 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -249,6 +249,7 @@ void blk_mq_unquiesce_queue(struct request_queue *q)
>  {
>  	mutex_lock(&q->mq_quiesce_lock);
>  
> +	WARN_ON(!blk_queue_quiesced(q));

We can't do that simply, because queue unquiesce may be called
unconditionally on un-quiesced queue, such as nvme_dev_remove_admin(),
nvme_set_queue_dying(), ...


Thanks,
Ming