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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.