| Message ID | 20250224133102.1240146-8-nilay@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | block: fix lock order and remove redundant locking | expand |
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@linux.ibm.com> > --- > 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? Cheers, Hannes
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@linux.ibm.com> >> --- >> 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
On 2/25/25 11:18, Nilay Shroff wrote: > > > 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@linux.ibm.com> >>> --- >>> 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. > Ah. Okay. Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
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); return ret; } @@ -670,7 +673,8 @@ static struct attribute *queue_attrs[] = { &queue_dma_alignment_entry.attr, /* - * attributes protected with q->sysfs_lock + * attributes which require some form of locking + * other than q->sysfs_lock */ &queue_ra_entry.attr,
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@linux.ibm.com> --- block/blk-sysfs.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)