On 08/07/2017 07:53 PM, Ming Lei wrote: > On Tue, Aug 8, 2017 at 3:38 AM, David Jeffery <djeffery@xxxxxxxxxx> wrote: >> >> Signed-off-by: David Jeffery <djeffery@xxxxxxxxxx> >> --- >> 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 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. > >> >> wbt_exit(q); >> >> diff --git a/block/elevator.c b/block/elevator.c >> index 4bb2f0c..51da592 100644 >> --- a/block/elevator.c >> +++ b/block/elevator.c >> @@ -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. > 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.
