From: Ming Lei <ming.lei@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Jens Axboe <axboe@kernel.dk>, Damien Le Moal <dlemoal@kernel.org>,
Nilay Shroff <nilay@linux.ibm.com>,
linux-block@vger.kernel.org, linux-nvme@lists.infradead.org,
nbd@other.debian.org, linux-scsi@vger.kernel.org,
usb-storage@lists.one-eyed-alien.net
Subject: Re: [PATCH 4/8] block: add a store_limit operations for sysfs entries
Date: Tue, 7 Jan 2025 15:25:52 +0800 [thread overview]
Message-ID: <Z3zXANbFk6GBZg_z@fedora> (raw)
In-Reply-To: <20250107063120.1011593-5-hch@lst.de>
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@lst.de>
> ---
> 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
next prev parent reply other threads:[~2025-01-07 7:26 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-07 6:30 fix queue freeze and limit locking order Christoph Hellwig
2025-01-07 6:30 ` [PATCH 1/8] block: fix docs for freezing of queue limits updates Christoph Hellwig
2025-01-07 7:01 ` Nilay Shroff
2025-01-07 18:05 ` Nilay Shroff
2025-01-07 10:03 ` Damien Le Moal
2025-01-09 11:19 ` John Garry
2025-01-09 11:24 ` Nilay Shroff
2025-01-09 11:26 ` John Garry
2025-01-07 6:30 ` [PATCH 2/8] block: add a queue_limits_commit_update_frozen helper Christoph Hellwig
2025-01-07 10:05 ` Damien Le Moal
2025-01-07 17:58 ` Nilay Shroff
2025-01-07 6:30 ` [PATCH 3/8] block: don't update BLK_FEAT_POLL in __blk_mq_update_nr_hw_queues Christoph Hellwig
2025-01-07 6:57 ` Nilay Shroff
2025-01-07 8:21 ` Christoph Hellwig
2025-01-07 9:23 ` Nilay Shroff
2025-01-07 13:51 ` Christoph Hellwig
2025-01-07 17:55 ` Nilay Shroff
2025-01-07 6:30 ` [PATCH 4/8] block: add a store_limit operations for sysfs entries Christoph Hellwig
2025-01-07 7:25 ` Ming Lei [this message]
2025-01-07 7:51 ` Nilay Shroff
2025-01-07 8:26 ` Christoph Hellwig
2025-01-07 10:25 ` Ming Lei
2025-01-07 13:49 ` Christoph Hellwig
2025-01-07 8:23 ` Christoph Hellwig
2025-01-07 10:05 ` Damien Le Moal
2025-01-07 17:58 ` Nilay Shroff
2025-01-07 6:30 ` [PATCH 5/8] block: fix queue freeze vs limits lock order in sysfs store methods Christoph Hellwig
2025-01-07 17:59 ` Nilay Shroff
2025-01-07 6:30 ` [PATCH 6/8] nvme: fix queue freeze vs limits lock order Christoph Hellwig
2025-01-07 6:58 ` Nilay Shroff
2025-01-07 8:22 ` Christoph Hellwig
2025-01-07 8:45 ` Nilay Shroff
2025-01-07 8:58 ` Christoph Hellwig
2025-01-07 9:29 ` Nilay Shroff
2025-01-07 9:58 ` Damien Le Moal
2025-01-07 18:00 ` Nilay Shroff
2025-01-07 6:30 ` [PATCH 7/8] nbd: " Christoph Hellwig
2025-01-07 10:00 ` Damien Le Moal
2025-01-07 18:01 ` Nilay Shroff
2025-01-07 6:30 ` [PATCH 8/8] usb-storage:fix " Christoph Hellwig
2025-01-07 10:00 ` Damien Le Moal
2025-01-07 18:01 ` Nilay Shroff
2025-01-08 2:21 ` fix queue freeze and limit locking order Ming Lei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Z3zXANbFk6GBZg_z@fedora \
--to=ming.lei@redhat.com \
--cc=axboe@kernel.dk \
--cc=dlemoal@kernel.org \
--cc=hch@lst.de \
--cc=linux-block@vger.kernel.org \
--cc=linux-nvme@lists.infradead.org \
--cc=linux-scsi@vger.kernel.org \
--cc=nbd@other.debian.org \
--cc=nilay@linux.ibm.com \
--cc=usb-storage@lists.one-eyed-alien.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox