Re: [PATCH V2 6/6] block: split .sysfs_lock into two locks

From: Ming Lei <ming.lei@redhat.com>
To: Bart Van Assche <bvanassche@acm.org>
Cc: Jens Axboe <axboe@kernel.dk>,
	linux-block@vger.kernel.org,
	Christoph Hellwig <hch@infradead.org>,
	Hannes Reinecke <hare@suse.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	Mike Snitzer <snitzer@redhat.com>
Subject: Re: [PATCH V2 6/6] block: split .sysfs_lock into two locks
Date: Fri, 23 Aug 2019 09:08:05 +0800	[thread overview]
Message-ID: <20190823010804.GA16810@ming.t460p> (raw)
In-Reply-To: <04b567f5-df49-3d44-1707-14fe8445843e@acm.org>

On Thu, Aug 22, 2019 at 12:52:54PM -0700, Bart Van Assche wrote:
> On 8/21/19 6:28 PM, Ming Lei wrote:
> > On Wed, Aug 21, 2019 at 09:18:08AM -0700, Bart Van Assche wrote:
> > > On 8/21/19 2:15 AM, Ming Lei wrote:
> > > > diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
> > > > index 31bbf10d8149..a4cc40ddda86 100644
> > > > --- a/block/blk-mq-sysfs.c
> > > > +++ b/block/blk-mq-sysfs.c
> > > > @@ -247,7 +247,7 @@ void blk_mq_unregister_dev(struct device *dev, struct request_queue *q)
> > > >    	struct blk_mq_hw_ctx *hctx;
> > > >    	int i;
> > > > -	lockdep_assert_held(&q->sysfs_lock);
> > > > +	lockdep_assert_held(&q->sysfs_dir_lock);
> > > >    	queue_for_each_hw_ctx(q, hctx, i)
> > > >    		blk_mq_unregister_hctx(hctx);
> > > > @@ -297,7 +297,7 @@ int __blk_mq_register_dev(struct device *dev, struct request_queue *q)
> > > >    	int ret, i;
> > > >    	WARN_ON_ONCE(!q->kobj.parent);
> > > > -	lockdep_assert_held(&q->sysfs_lock);
> > > > +	lockdep_assert_held(&q->sysfs_dir_lock);
> > > >    	ret = kobject_add(q->mq_kobj, kobject_get(&dev->kobj), "%s", "mq");
> > > >    	if (ret < 0)
> > > 
> > > blk_mq_unregister_dev and __blk_mq_register_dev() are only used by
> > > blk_register_queue() and blk_unregister_queue(). It is the responsibility of
> > > the callers of these function to serialize request queue registration and
> > > unregistration. Is it really necessary to hold a mutex around the
> > > blk_mq_unregister_dev and __blk_mq_register_dev() calls? Or in other words,
> > > can it ever happen that multiple threads invoke one or both functions
> > > concurrently?
> > 
> > hctx kobjects can be removed and re-added via blk_mq_update_nr_hw_queues()
> > which may be called at the same time when queue is registering or
> > un-registering.
> 
> Shouldn't blk_register_queue() and blk_unregister_queue() be serialized
> against blk_mq_update_nr_hw_queues()? Allowing these calls to proceed

It can be easy to say than done. We depends on users for sync
between blk_register_queue() and blk_unregister_queue(), also
there are several locks involved in blk_mq_update_nr_hw_queues().

Now, the sync is done via .sysfs_lock, and so far not see issues in this
area. This patch just converts the .sysfs_lock into .sysfs_dir_lock for
same purpose.

If you have simple and workable patch to serialize blk_register_queue() and
blk_unregister_queue() against blk_mq_update_nr_hw_queues(), I am happy to
review. Otherwise please consider to do it in future, and it shouldn't a
blocker for fixing this deadlock, should it?

Thanks,
Ming