On Wed, Aug 23, 2017 at 11:34 AM, Ming Lei [off-list ref] wrote:

On Wed, Aug 9, 2017 at 9:44 AM, Ming Lei [off-list ref] wrote:

quoted

Hi David,

On Wed, Aug 9, 2017 at 2:13 AM, David Jeffery [off-list ref] wrote:

quoted

On 08/07/2017 07:53 PM, Ming Lei wrote:

quoted

On Tue, Aug 8, 2017 at 3:38 AM, David Jeffery [off-list ref] wrote:

quoted

quoted

Signed-off-by: David Jeffery <redacted>
---
 block/blk-sysfs.c |    2 ++
 block/elevator.c  |    4 ++++
 2 files changed, 6 insertions(+)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 27aceab..b8362c0 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c

@@ -931,7 +931,9 @@ void blk_unregister_queue(struct gendisk *disk)
        if (WARN_ON(!q))
                return;

+       mutex_lock(&q->sysfs_lock);
        queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
+       mutex_unlock(&q->sysfs_lock);

Could you share why the lock of 'q->sysfs_lock' is needed here?

As the elevator change is initiated through a sysfs attr file in the
queue directory, the task doing the elevator change already acquires the
q->sysfs_lock before it can try and change the elevator.  Adding the

It is e->sysfs_lock which is held in elv_attr_store(), instead of q->sysfs_lock.

Looks I was wrong, and the store is from queue_attr_store() instead of
elv_attr_store().

I can reproduce the issue and this patch can fix the issue in my test
on scsi_debug,
so:

        Tested-by: Ming Lei [off-list ref]

And it is a typical race between removing queue kobj and adding children of
this queue kobj, we can't acquire q->sysfs_lock in blk_unregister_queue()
because of sysfs's limit(may cause deadlock), so one state has to be used
for this purpose, just like what register/unregister hctx kobjs does,
and it should
be fine to use QUEUE_FLAG_REGISTERED here. More changes are required if
we use e->registered, so this patch looks fine:

       Reviewed-by: Ming Lei [off-list ref]

Hi Jens,

Could you consider this patch for v4.13 or v4.14?


Thanks,
Ming Lei