block: Fix warning when I/O elevator is changed as request_queue is being removed

PATCH In review David Jeffery 3 participants 7 messages Aug 7, 2017 – Aug 28, 2017 Block layer

Share & tools

David Jeffery

author

2017-08-07 19:38 UTC #0 lore raw

PATCH 1/1 block: Fix warning when I/O elevator is changed as request_queue is being removed

There is a race between changing I/O elevator and request_queue removal which can trigger the warning in kobject_add_internal. A program can use sysfs to request a change of elevator at the same time another task is unregistering the request_queue the elevator would be attached to. The elevator's kobject will then attempt to be connected to the request_queue in the object tree when the request_queue has just been removed from sysfs. This triggers the warning in kobject_add_internal as the request_queue no longer has a sysfs directory:

kobject_add_internal failed for iosched (error: -2 parent: queue)

------------[ cut here ]------------
WARNING: CPU: 3 PID: 14075 at lib/kobject.c:244 kobject_add_internal+0x103/0x2d0

To fix this warning, we can check the QUEUE_FLAG_REGISTERED flag when changing the elevator and use the request_queue's sysfs_lock to serialize between clearing the flag and the elevator testing the flag.

Signed-off-by David Jeffery <[email protected]>

---
 block/blk-sysfs.c |    2 ++
 block/elevator.c  |    4 ++++
 2 files changed, 6 insertions(+)

block/blk-sysfs.c +2 -0

index 27aceab..b8362c0 100644
@@ -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);
 
 	wbt_exit(q);

block/elevator.c +4 -0

index 4bb2f0c..51da592 100644
@@ -1055,6 +1055,10 @@ static int __elevator_change(struct request_queue *q, const char *name)
 	char elevator_name[ELV_NAME_MAX];
 	struct elevator_type *e;
 
+	/* Make sure queue is not in the middle of being removed */
+	if (!test_bit(QUEUE_FLAG_REGISTERED, &q->queue_flags))
+		return -ENOENT;
+
 	/*
 	 * Special case for mq, turn off scheduling
 	 */

Ming Lei

2017-08-07 23:53 UTC #1 lore raw

Quoted 9 lines · David Jeffery

There is a race between changing I/O elevator and request_queue removal which can trigger the warning in kobject_add_internal. A program can use sysfs to request a change of elevator at the same time another task is unregistering the request_queue the elevator would be attached to. The elevator's kobject will then attempt to be connected to the request_queue in the object tree when the request_queue has just been removed from sysfs. This triggers the warning in kobject_add_internal as the request_queue no longer has a sysfs directory:

Quoted 8 lines

kobject_add_internal failed for iosched (error: -2 parent: queue)
------------[ cut here ]------------
WARNING: CPU: 3 PID: 14075 at lib/kobject.c:244 kobject_add_internal+0x103/0x2d0
To fix this warning, we can check the QUEUE_FLAG_REGISTERED flag when changing the elevator and use the request_queue's sysfs_lock to serialize between clearing the flag and the elevator testing the flag.

I remember I saw this issue too.

Quoted 13 lines

Signed-off-by David Jeffery <[email protected]>
---
 block/blk-sysfs.c |    2 ++
 block/elevator.c  |    4 ++++
 2 files changed, 6 insertions(+)
block/blk-sysfs.c +0 -0
index 27aceab..b8362c0 100644

block/blk-sysfs.c void blk_unregister_queue(struct gendisk *disk) · line 936

Quoted 6 lines

block/blk-sysfs.c +2 -0

@@ -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?

Quoted 8 lines

block/blk-sysfs.c +0 -0
        wbt_exit(q);
 
block/elevator.c +0 -0
index 4bb2f0c..51da592 100644

block/elevator.c static int __elevator_change(struct request_queue *q, const char *name) · line 1061

Quoted 7 lines

block/elevator.c +4 -0

@@ -1055,6 +1055,10 @@ static int __elevator_change(struct request_queue *q, const char *name)
        char elevator_name[ELV_NAME_MAX];
        struct elevator_type *e;
 
+       /* Make sure queue is not in the middle of being removed */
+       if (!test_bit(QUEUE_FLAG_REGISTERED, &q->queue_flags))
+               return -ENOENT;
+

I suggest to check 'e->registered' here, which should be more reasonable or straightforward.

Signature · 1 line

Ming Lei

David Jeffery

author

2017-08-08 18:13 UTC #2 lore raw

Quoted 18 lines · Ming Lei

Quoted 16 lines · David Jeffery

Signed-off-by David Jeffery <[email protected]>

---
 block/blk-sysfs.c |    2 ++
 block/elevator.c  |    4 ++++
 2 files changed, 6 insertions(+)

block/blk-sysfs.c +0 -0

index 27aceab..b8362c0 100644
@@ -931,7 +931,9 @@ void blk_unregister_queue(struct gendisk *disk)
        if (WARN_ON(!q))
                return;

Quoted 5 lines

block/blk-sysfs.c void blk_unregister_queue(struct gendisk *disk) · line 936
Quoted 3 lines
block/blk-sysfs.c +2 -0
+       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 lock around clearing QUEUE_FLAG_REGISTERED ensures that the queue state will be stable while the elevator is being changed. It prevents a race condition where the bit is checked but then cleared and queue removed from sysfs before the elevator change completes.

Quoted 12 lines

block/blk-sysfs.c +0 -0

        wbt_exit(q);

block/elevator.c +0 -0

index 4bb2f0c..51da592 100644
@@ -1055,6 +1055,10 @@ static int __elevator_change(struct request_queue *q, const char *name)
        char elevator_name[ELV_NAME_MAX];
        struct elevator_type *e;

Quoted 8 lines

block/elevator.c static int __elevator_change(struct request_queue *q, const char *name) · line 1061
Quoted 4 lines
block/elevator.c +4 -0
+       /* Make sure queue is not in the middle of being removed */
+       if (!test_bit(QUEUE_FLAG_REGISTERED, &q->queue_flags))
+               return -ENOENT;
+
I suggest to check 'e->registered' here, which should be more reasonable or straightforward.

e->registered is not the state needing to be checked. We need to know the state of the associated request queue.

Before changing the elevator, we need to ensure the request queue is still connected to sysfs. i.e. We need to know that kobject_del has not been called on the request queue. When QUEUE_FLAG_REGISTERED is not set it means the request queue either has had kobject_del called or will have it called soon, so we should fail the elevator change attempt.

Ming Lei

2017-08-09 01:44 UTC #3 lore raw

Hi David,

Quoted 25 lines · David Jeffery

Quoted 18 lines · Ming Lei
Quoted 16 lines · David Jeffery
Signed-off-by David Jeffery <[email protected]>
---
 block/blk-sysfs.c |    2 ++
 block/elevator.c  |    4 ++++
 2 files changed, 6 insertions(+)
block/blk-sysfs.c +0 -0
index 27aceab..b8362c0 100644
@@ -931,7 +931,9 @@ void blk_unregister_queue(struct gendisk *disk)
        if (WARN_ON(!q))
                return;
 
Quoted 5 lines
block/blk-sysfs.c void blk_unregister_queue(struct gendisk *disk) · line 936
Quoted 3 lines
block/blk-sysfs.c +2 -0
+       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?

Quoted 3 lines

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.

Quoted 26 lines

lock around clearing QUEUE_FLAG_REGISTERED ensures that the queue state will be stable while the elevator is being changed. It prevents a race condition where the bit is checked but then cleared and queue removed from sysfs before the elevator change completes.
Quoted 12 lines
Quoted 12 lines
block/blk-sysfs.c +0 -0
        wbt_exit(q);
 
block/elevator.c +0 -0
index 4bb2f0c..51da592 100644
@@ -1055,6 +1055,10 @@ static int __elevator_change(struct request_queue *q, const char *name)
        char elevator_name[ELV_NAME_MAX];
        struct elevator_type *e;
 
Quoted 8 lines
block/elevator.c static int __elevator_change(struct request_queue *q, const char *name) · line 1061
Quoted 4 lines
block/elevator.c +4 -0
+       /* Make sure queue is not in the middle of being removed */
+       if (!test_bit(QUEUE_FLAG_REGISTERED, &q->queue_flags))
+               return -ENOENT;
+
I suggest to check 'e->registered' here, which should be more reasonable or straightforward.

Quoted 8 lines

e->registered is not the state needing to be checked. We need to know the state of the associated request queue.
Before changing the elevator, we need to ensure the request queue is still connected to sysfs. i.e. We need to know that kobject_del has not been called on the request queue. When QUEUE_FLAG_REGISTERED is not set it means the request queue either has had kobject_del called or will have it called soon, so we should fail the elevator change attempt.

elv_unregister_queue() is called in blk_unregister_queue() too, that is why I suggest to check e->registered.

Signature · 1 line

Ming Lei

Ming Lei

Tested-by Reviewed-by

2017-08-23 03:34 UTC #4 lore raw

Quoted 28 lines · Ming Lei

Hi David,
Quoted 25 lines · David Jeffery
Quoted 23 lines · Ming Lei
Quoted 13 lines · David Jeffery
Signed-off-by David Jeffery <[email protected]>
---
 block/blk-sysfs.c |    2 ++
 block/elevator.c  |    4 ++++
 2 files changed, 6 insertions(+)
block/blk-sysfs.c +0 -0
index 27aceab..b8362c0 100644
block/blk-sysfs.c void blk_unregister_queue(struct gendisk *disk) · line 936
Quoted 6 lines
block/blk-sysfs.c +2 -0
@@ -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?

Quoted 5 lines

Quoted 3 lines
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 <[email protected]>

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 <[email protected]>

Thanks,
Ming Lei

Ming Lei

2017-08-28 01:36 UTC #5 lore raw

Quoted 52 lines · Ming Lei

Quoted 28 lines · Ming Lei
Hi David,
Quoted 25 lines · David Jeffery
Quoted 23 lines · Ming Lei
Quoted 13 lines · David Jeffery
Signed-off-by David Jeffery <[email protected]>
---
 block/blk-sysfs.c |    2 ++
 block/elevator.c  |    4 ++++
 2 files changed, 6 insertions(+)
block/blk-sysfs.c +0 -0
index 27aceab..b8362c0 100644
block/blk-sysfs.c void blk_unregister_queue(struct gendisk *disk) · line 936
Quoted 6 lines
block/blk-sysfs.c +2 -0
@@ -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?
Quoted 5 lines
Quoted 3 lines
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 <[email protected]>
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:

Quoted 1 line

Reviewed-by Ming Lei <[email protected]>

Hi Jens,

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

Thanks,
Ming Lei

Jens Axboe

maintainer

2017-08-28 16:54 UTC #6 lore raw

Quoted 53 lines · Ming Lei

Quoted 52 lines · Ming Lei
Quoted 28 lines · Ming Lei
Hi David,
Quoted 25 lines · David Jeffery
Quoted 23 lines · Ming Lei
Quoted 13 lines · David Jeffery
Signed-off-by David Jeffery <[email protected]>
---
 block/blk-sysfs.c |    2 ++
 block/elevator.c  |    4 ++++
 2 files changed, 6 insertions(+)
block/blk-sysfs.c +0 -0
index 27aceab..b8362c0 100644
block/blk-sysfs.c void blk_unregister_queue(struct gendisk *disk) · line 936
Quoted 6 lines
block/blk-sysfs.c +2 -0
@@ -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?
Quoted 5 lines
Quoted 3 lines
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 <[email protected]>
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:

Quoted 6 lines

Quoted 1 line
Reviewed-by Ming Lei <[email protected]>
Hi Jens,
Could you consider this patch for v4.13 or v4.14?

Yep, added for 4.14, thanks.

Signature · 1 line

Jens Axboe