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
