DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C670265D12
Subject: Re: [PATCH] block: Fix warning when I/O elevator is changed as
 request_queue is being removed
To: Ming Lei <tom.leiming@gmail.com>
Cc: linux-block <linux-block@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Jens Axboe <axboe@kernel.dk>
References: <20170807193818.GA22737@rage>
 <CACVXFVPqY-Sz65uUtWAWu2wSS=Z0nU0NQnTaGmfOdfxcMVnwzQ@mail.gmail.com>
From: David Jeffery <djeffery@redhat.com>
Message-ID: <dada2f16-1c86-e474-a71b-b6d51be9e77a@redhat.com>
Date: Tue, 8 Aug 2017 14:13:14 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <CACVXFVPqY-Sz65uUtWAWu2wSS=Z0nU0NQnTaGmfOdfxcMVnwzQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-MW
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2352
Lines: 64

On 08/07/2017 07:53 PM, Ming Lei wrote:
> On Tue, Aug 8, 2017 at 3:38 AM, David Jeffery <djeffery@redhat.com> wrote:

>>
>> Signed-off-by: David Jeffery <djeffery@redhat.com>
>> ---
>>  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.