On Tue, Jan 07, 2025 at 07:30:36AM +0100, Christoph Hellwig wrote:
> De-duplicate the code for updating queue limits by adding a store_limit
> method that allows having common code handle the actual queue limits
> update.
>
> Note that this is a pure refactoring patch and does not address the
> existing freeze vs limits lock order problem in the refactored code,
> which will be addressed next.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> block/blk-sysfs.c | 128 ++++++++++++++++++++++------------------------
> 1 file changed, 61 insertions(+), 67 deletions(-)
>
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 767598e719ab..8d69315e986d 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -24,6 +24,8 @@ struct queue_sysfs_entry {
> struct attribute attr;
> ssize_t (*show)(struct gendisk *disk, char *page);
> ssize_t (*store)(struct gendisk *disk, const char *page, size_t count);
> + int (*store_limit)(struct gendisk *disk, const char *page,
> + size_t count, struct queue_limits *lim);
> void (*load_module)(struct gendisk *disk, const char *page, size_t count);
> };
>
> @@ -153,13 +155,11 @@ QUEUE_SYSFS_SHOW_CONST(discard_zeroes_data, 0)
> QUEUE_SYSFS_SHOW_CONST(write_same_max, 0)
> QUEUE_SYSFS_SHOW_CONST(poll_delay, -1)
>
> -static ssize_t queue_max_discard_sectors_store(struct gendisk *disk,
> - const char *page, size_t count)
> +static int queue_max_discard_sectors_store(struct gendisk *disk,
> + const char *page, size_t count, struct queue_limits *lim)
> {
> unsigned long max_discard_bytes;
> - struct queue_limits lim;
> ssize_t ret;
> - int err;
>
> ret = queue_var_store(&max_discard_bytes, page, count);
> if (ret < 0)
> @@ -171,38 +171,28 @@ static ssize_t queue_max_discard_sectors_store(struct gendisk *disk,
> if ((max_discard_bytes >> SECTOR_SHIFT) > UINT_MAX)
> return -EINVAL;
>
> - lim = queue_limits_start_update(disk->queue);
> - lim.max_user_discard_sectors = max_discard_bytes >> SECTOR_SHIFT;
> - err = queue_limits_commit_update(disk->queue, &lim);
> - if (err)
> - return err;
> - return ret;
> + lim->max_user_discard_sectors = max_discard_bytes >> SECTOR_SHIFT;
> + return 0;
> }
>
> -static ssize_t
> -queue_max_sectors_store(struct gendisk *disk, const char *page, size_t count)
> +static int
> +queue_max_sectors_store(struct gendisk *disk, const char *page, size_t count,
> + struct queue_limits *lim)
> {
> unsigned long max_sectors_kb;
> - struct queue_limits lim;
> ssize_t ret;
> - int err;
>
> ret = queue_var_store(&max_sectors_kb, page, count);
> if (ret < 0)
> return ret;
>
> - lim = queue_limits_start_update(disk->queue);
> - lim.max_user_sectors = max_sectors_kb << 1;
> - err = queue_limits_commit_update(disk->queue, &lim);
> - if (err)
> - return err;
> - return ret;
> + lim->max_user_sectors = max_sectors_kb << 1;
> + return 0;
> }
>
> static ssize_t queue_feature_store(struct gendisk *disk, const char *page,
> - size_t count, blk_features_t feature)
> + size_t count, struct queue_limits *lim, blk_features_t feature)
> {
> - struct queue_limits lim;
> unsigned long val;
> ssize_t ret;
>
> @@ -210,15 +200,11 @@ static ssize_t queue_feature_store(struct gendisk *disk, const char *page,
> if (ret < 0)
> return ret;
>
> - lim = queue_limits_start_update(disk->queue);
> if (val)
> - lim.features |= feature;
> + lim->features |= feature;
> else
> - lim.features &= ~feature;
> - ret = queue_limits_commit_update(disk->queue, &lim);
> - if (ret)
> - return ret;
> - return count;
> + lim->features &= ~feature;
> + return 0;
> }
>
> #define QUEUE_SYSFS_FEATURE(_name, _feature) \
> @@ -227,10 +213,10 @@ static ssize_t queue_##_name##_show(struct gendisk *disk, char *page) \
> return sysfs_emit(page, "%u\n", \
> !!(disk->queue->limits.features & _feature)); \
> } \
> -static ssize_t queue_##_name##_store(struct gendisk *disk, \
> - const char *page, size_t count) \
> +static int queue_##_name##_store(struct gendisk *disk, \
> + const char *page, size_t count, struct queue_limits *lim) \
> { \
> - return queue_feature_store(disk, page, count, _feature); \
> + return queue_feature_store(disk, page, count, lim, _feature); \
> }
>
> QUEUE_SYSFS_FEATURE(rotational, BLK_FEAT_ROTATIONAL)
> @@ -266,10 +252,9 @@ static ssize_t queue_iostats_passthrough_show(struct gendisk *disk, char *page)
> return queue_var_show(!!blk_queue_passthrough_stat(disk->queue), page);
> }
>
> -static ssize_t queue_iostats_passthrough_store(struct gendisk *disk,
> - const char *page, size_t count)
> +static int queue_iostats_passthrough_store(struct gendisk *disk,
> + const char *page, size_t count, struct queue_limits *lim)
> {
> - struct queue_limits lim;
> unsigned long ios;
> ssize_t ret;
>
> @@ -277,18 +262,13 @@ static ssize_t queue_iostats_passthrough_store(struct gendisk *disk,
> if (ret < 0)
> return ret;
>
> - lim = queue_limits_start_update(disk->queue);
> if (ios)
> - lim.flags |= BLK_FLAG_IOSTATS_PASSTHROUGH;
> + lim->flags |= BLK_FLAG_IOSTATS_PASSTHROUGH;
> else
> - lim.flags &= ~BLK_FLAG_IOSTATS_PASSTHROUGH;
> -
> - ret = queue_limits_commit_update(disk->queue, &lim);
> - if (ret)
> - return ret;
> -
> - return count;
> + lim->flags &= ~BLK_FLAG_IOSTATS_PASSTHROUGH;
> + return 0;
> }
> +
> static ssize_t queue_nomerges_show(struct gendisk *disk, char *page)
> {
> return queue_var_show((blk_queue_nomerges(disk->queue) << 1) |
> @@ -391,12 +371,10 @@ static ssize_t queue_wc_show(struct gendisk *disk, char *page)
> return sysfs_emit(page, "write through\n");
> }
>
> -static ssize_t queue_wc_store(struct gendisk *disk, const char *page,
> - size_t count)
> +static int queue_wc_store(struct gendisk *disk, const char *page,
> + size_t count, struct queue_limits *lim)
> {
> - struct queue_limits lim;
> bool disable;
> - int err;
>
> if (!strncmp(page, "write back", 10)) {
> disable = false;
> @@ -407,15 +385,11 @@ static ssize_t queue_wc_store(struct gendisk *disk, const char *page,
> return -EINVAL;
> }
>
> - lim = queue_limits_start_update(disk->queue);
> if (disable)
> - lim.flags |= BLK_FLAG_WRITE_CACHE_DISABLED;
> + lim->flags |= BLK_FLAG_WRITE_CACHE_DISABLED;
> else
> - lim.flags &= ~BLK_FLAG_WRITE_CACHE_DISABLED;
> - err = queue_limits_commit_update(disk->queue, &lim);
> - if (err)
> - return err;
> - return count;
> + lim->flags &= ~BLK_FLAG_WRITE_CACHE_DISABLED;
> + return 0;
> }
>
> #define QUEUE_RO_ENTRY(_prefix, _name) \
> @@ -431,6 +405,13 @@ static struct queue_sysfs_entry _prefix##_entry = { \
> .store = _prefix##_store, \
> };
>
> +#define QUEUE_LIM_RW_ENTRY(_prefix, _name) \
> +static struct queue_sysfs_entry _prefix##_entry = { \
> + .attr = { .name = _name, .mode = 0644 }, \
> + .show = _prefix##_show, \
> + .store_limit = _prefix##_store, \
> +}
> +
> #define QUEUE_RW_LOAD_MODULE_ENTRY(_prefix, _name) \
> static struct queue_sysfs_entry _prefix##_entry = { \
> .attr = { .name = _name, .mode = 0644 }, \
> @@ -441,7 +422,7 @@ static struct queue_sysfs_entry _prefix##_entry = { \
>
> QUEUE_RW_ENTRY(queue_requests, "nr_requests");
> QUEUE_RW_ENTRY(queue_ra, "read_ahead_kb");
> -QUEUE_RW_ENTRY(queue_max_sectors, "max_sectors_kb");
> +QUEUE_LIM_RW_ENTRY(queue_max_sectors, "max_sectors_kb");
> QUEUE_RO_ENTRY(queue_max_hw_sectors, "max_hw_sectors_kb");
> QUEUE_RO_ENTRY(queue_max_segments, "max_segments");
> QUEUE_RO_ENTRY(queue_max_integrity_segments, "max_integrity_segments");
> @@ -457,7 +438,7 @@ QUEUE_RO_ENTRY(queue_io_opt, "optimal_io_size");
> QUEUE_RO_ENTRY(queue_max_discard_segments, "max_discard_segments");
> QUEUE_RO_ENTRY(queue_discard_granularity, "discard_granularity");
> QUEUE_RO_ENTRY(queue_max_hw_discard_sectors, "discard_max_hw_bytes");
> -QUEUE_RW_ENTRY(queue_max_discard_sectors, "discard_max_bytes");
> +QUEUE_LIM_RW_ENTRY(queue_max_discard_sectors, "discard_max_bytes");
> QUEUE_RO_ENTRY(queue_discard_zeroes_data, "discard_zeroes_data");
>
> QUEUE_RO_ENTRY(queue_atomic_write_max_sectors, "atomic_write_max_bytes");
> @@ -477,11 +458,11 @@ QUEUE_RO_ENTRY(queue_max_open_zones, "max_open_zones");
> QUEUE_RO_ENTRY(queue_max_active_zones, "max_active_zones");
>
> QUEUE_RW_ENTRY(queue_nomerges, "nomerges");
> -QUEUE_RW_ENTRY(queue_iostats_passthrough, "iostats_passthrough");
> +QUEUE_LIM_RW_ENTRY(queue_iostats_passthrough, "iostats_passthrough");
> QUEUE_RW_ENTRY(queue_rq_affinity, "rq_affinity");
> QUEUE_RW_ENTRY(queue_poll, "io_poll");
> QUEUE_RW_ENTRY(queue_poll_delay, "io_poll_delay");
> -QUEUE_RW_ENTRY(queue_wc, "write_cache");
> +QUEUE_LIM_RW_ENTRY(queue_wc, "write_cache");
> QUEUE_RO_ENTRY(queue_fua, "fua");
> QUEUE_RO_ENTRY(queue_dax, "dax");
> QUEUE_RW_ENTRY(queue_io_timeout, "io_timeout");
> @@ -494,10 +475,10 @@ static struct queue_sysfs_entry queue_hw_sector_size_entry = {
> .show = queue_logical_block_size_show,
> };
>
> -QUEUE_RW_ENTRY(queue_rotational, "rotational");
> -QUEUE_RW_ENTRY(queue_iostats, "iostats");
> -QUEUE_RW_ENTRY(queue_add_random, "add_random");
> -QUEUE_RW_ENTRY(queue_stable_writes, "stable_writes");
> +QUEUE_LIM_RW_ENTRY(queue_rotational, "rotational");
> +QUEUE_LIM_RW_ENTRY(queue_iostats, "iostats");
> +QUEUE_LIM_RW_ENTRY(queue_add_random, "add_random");
> +QUEUE_LIM_RW_ENTRY(queue_stable_writes, "stable_writes");
>
> #ifdef CONFIG_BLK_WBT
> static ssize_t queue_var_store64(s64 *var, const char *page)
> @@ -695,7 +676,7 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
> struct request_queue *q = disk->queue;
> ssize_t res;
>
> - if (!entry->store)
> + if (!entry->store_limit && !entry->store)
> return -EIO;
>
> /*
> @@ -706,11 +687,24 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
> if (entry->load_module)
> entry->load_module(disk, page, length);
>
> - blk_mq_freeze_queue(q);
> mutex_lock(&q->sysfs_lock);
> - res = entry->store(disk, page, length);
> - mutex_unlock(&q->sysfs_lock);
> + blk_mq_freeze_queue(q);
Order between freeze and ->sysfs_lock is changed, and it may cause new
lockdep warning because we may freeze queue first before acquiring
->sysfs_lock in del_gendisk().
thanks,
Ming
