On 2/25/25 1:28 PM, Hannes Reinecke wrote:
> On 2/24/25 14:30, Nilay Shroff wrote:
>> The bdi->ra_pages could be updated under q->limits_lock because it's
>> usually calculated from the queue limits by queue_limits_commit_update.
>> So protect reading/writing the sysfs attribute read_ahead_kb using
>> q->limits_lock instead of q->sysfs_lock.
>>
>> Signed-off-by: Nilay Shroff <nilay@xxxxxxxxxxxxx>
>> ---
>> block/blk-sysfs.c | 16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>> index 8f47d9f30fbf..228f81a9060f 100644
>> --- a/block/blk-sysfs.c
>> +++ b/block/blk-sysfs.c
>> @@ -93,9 +93,9 @@ static ssize_t queue_ra_show(struct gendisk *disk, char *page)
>> {
>> ssize_t ret;
>> - mutex_lock(&disk->queue->sysfs_lock);
>> + mutex_lock(&disk->queue->limits_lock);
>> ret = queue_var_show(disk->bdi->ra_pages << (PAGE_SHIFT - 10), page);
>> - mutex_unlock(&disk->queue->sysfs_lock);
>> + mutex_unlock(&disk->queue->limits_lock);
>> return ret;
>> }
>> @@ -111,12 +111,15 @@ queue_ra_store(struct gendisk *disk, const char *page, size_t count)
>> ret = queue_var_store(&ra_kb, page, count);
>> if (ret < 0)
>> return ret;
>> -
>> - mutex_lock(&q->sysfs_lock);
>> + /*
>> + * ->ra_pages is protected by ->limits_lock because it is usually
>> + * calculated from the queue limits by queue_limits_commit_update.
>> + */
>> + mutex_lock(&q->limits_lock);
>> memflags = blk_mq_freeze_queue(q);
>> disk->bdi->ra_pages = ra_kb >> (PAGE_SHIFT - 10);
>> + mutex_unlock(&q->limits_lock);
>> blk_mq_unfreeze_queue(q, memflags);
>> - mutex_unlock(&q->sysfs_lock);
>>
>
> Cf my comments to the previous patch: Ordering.
>
> Here we take the lock _before_ 'freeze', with the previous patch we took
> the lock _after_ 'freeze'.
> Why?
>
Yes this is ->limits_lock which is different from ->elevator_lock. The ->limits_lock
is used by atomic update APIs queue_limits_start_update() and helpers. Here, the
order we follow is : acquire ->limits_lock followed by queue-freeze.
So even here in sysfs attribute store method we follow the same locking order.
Thanks,
--Nilay
