| Message ID | 20240122173645.1686078-6-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/15] block: move max_{open,active}_zones to struct queue_limits | expand |
On Mon, Jan 22, 2024 at 06:36:35PM +0100, Christoph Hellwig wrote: > @@ -174,23 +174,23 @@ static ssize_t queue_discard_max_show(struct request_queue *q, char *page) > static ssize_t queue_discard_max_store(struct request_queue *q, > const char *page, size_t count) > { > - unsigned long max_discard; > - ssize_t ret = queue_var_store(&max_discard, page, count); > + unsigned long max_discard_bytes; > + ssize_t ret; > > + ret = queue_var_store(&max_discard_bytes, page, count); > if (ret < 0) > return ret; > > - if (max_discard & (q->limits.discard_granularity - 1)) > + if (max_discard_bytes & (q->limits.discard_granularity - 1)) > return -EINVAL; > > - max_discard >>= 9; > - if (max_discard > UINT_MAX) > + if ((max_discard_bytes >> SECTOR_SHIFT) > UINT_MAX) > return -EINVAL; > > - if (max_discard > q->limits.max_hw_discard_sectors) > - max_discard = q->limits.max_hw_discard_sectors; > - > - q->limits.max_discard_sectors = max_discard; > + q->limits.max_user_discard_sectors = max_discard_bytes >> SECTOR_SHIFT; > + q->limits.max_discard_sectors = > + min_not_zero(q->limits.max_hw_discard_sectors, > + q->limits.max_user_discard_sectors); Shouldn't writing 0 disable discards?
On Mon, Jan 22, 2024 at 11:27:15AM -0700, Keith Busch wrote: > > + q->limits.max_user_discard_sectors = max_discard_bytes >> SECTOR_SHIFT; > > + q->limits.max_discard_sectors = > > + min_not_zero(q->limits.max_hw_discard_sectors, > > + q->limits.max_user_discard_sectors); > > Shouldn't writing 0 disable discards? I mirror the max_user_sectors behavior here, where 0 disables the user limiting. But yes, that would be a behavior change.
On 1/22/24 18:36, Christoph Hellwig wrote: > Add a new max_user_discard_sectors limit that mirrors max_user_sectors > and stores the value that the user manually set. This now allows > updates of the max_hw_discard_sectors to not worry about the user > limit. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/blk-settings.c | 13 ++++++++++--- > block/blk-sysfs.c | 18 +++++++++--------- > include/linux/blkdev.h | 1 + > 3 files changed, 20 insertions(+), 12 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On Mon, Jan 22, 2024 at 07:38:57PM +0100, Christoph Hellwig wrote: > On Mon, Jan 22, 2024 at 11:27:15AM -0700, Keith Busch wrote: > > > + q->limits.max_user_discard_sectors = max_discard_bytes >> SECTOR_SHIFT; > > > + q->limits.max_discard_sectors = > > > + min_not_zero(q->limits.max_hw_discard_sectors, > > > + q->limits.max_user_discard_sectors); > > > > Shouldn't writing 0 disable discards? > > I mirror the max_user_sectors behavior here, where 0 disables the > user limiting. But yes, that would be a behavior change. But a user should be able to disable discards, right? Unless you really want to match max_user_sectors, I think you could default the user setting here to UINT_MAX and use "min" instead of "min_not_zero".
On Wed, Jan 24, 2024 at 08:44:16AM -0700, Keith Busch wrote: > > > > + min_not_zero(q->limits.max_hw_discard_sectors, > > > > + q->limits.max_user_discard_sectors); > > > > > > Shouldn't writing 0 disable discards? > > > > I mirror the max_user_sectors behavior here, where 0 disables the > > user limiting. But yes, that would be a behavior change. > > But a user should be able to disable discards, right? Unless you really > want to match max_user_sectors, I think you could default the user > setting here to UINT_MAX and use "min" instead of "min_not_zero". Not a fan of having different semantics for the different attributes, but given that we've had this or 9 years we better stick to it, so I'll change it.
diff --git a/block/blk-settings.c b/block/blk-settings.c index b6692143ccf034..f58da0810a00f2 100644 --- a/block/blk-settings.c +++ b/block/blk-settings.c @@ -47,6 +47,7 @@ void blk_set_default_limits(struct queue_limits *lim) lim->max_zone_append_sectors = 0; lim->max_discard_sectors = 0; lim->max_hw_discard_sectors = 0; + lim->max_user_discard_sectors = 0; lim->max_secure_erase_sectors = 0; lim->discard_granularity = 512; lim->discard_alignment = 0; @@ -167,7 +168,10 @@ int blk_validate_limits(struct queue_limits *lim) if (!lim->max_segments) lim->max_segments = BLK_MAX_SEGMENTS; - lim->max_discard_sectors = lim->max_hw_discard_sectors; + lim->max_discard_sectors = + min_not_zero(lim->max_hw_discard_sectors, + lim->max_user_discard_sectors); + if (!lim->max_discard_segments) lim->max_discard_segments = 1; @@ -328,8 +332,11 @@ EXPORT_SYMBOL(blk_queue_chunk_sectors); void blk_queue_max_discard_sectors(struct request_queue *q, unsigned int max_discard_sectors) { - q->limits.max_hw_discard_sectors = max_discard_sectors; - q->limits.max_discard_sectors = max_discard_sectors; + struct queue_limits *lim = &q->limits; + + lim->max_hw_discard_sectors = max_discard_sectors; + lim->max_discard_sectors = min_not_zero(max_discard_sectors, + lim->max_user_discard_sectors); } EXPORT_SYMBOL(blk_queue_max_discard_sectors); diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c index 26607f9825cb05..54e10604ddb1dd 100644 --- a/block/blk-sysfs.c +++ b/block/blk-sysfs.c @@ -174,23 +174,23 @@ static ssize_t queue_discard_max_show(struct request_queue *q, char *page) static ssize_t queue_discard_max_store(struct request_queue *q, const char *page, size_t count) { - unsigned long max_discard; - ssize_t ret = queue_var_store(&max_discard, page, count); + unsigned long max_discard_bytes; + ssize_t ret; + ret = queue_var_store(&max_discard_bytes, page, count); if (ret < 0) return ret; - if (max_discard & (q->limits.discard_granularity - 1)) + if (max_discard_bytes & (q->limits.discard_granularity - 1)) return -EINVAL; - max_discard >>= 9; - if (max_discard > UINT_MAX) + if ((max_discard_bytes >> SECTOR_SHIFT) > UINT_MAX) return -EINVAL; - if (max_discard > q->limits.max_hw_discard_sectors) - max_discard = q->limits.max_hw_discard_sectors; - - q->limits.max_discard_sectors = max_discard; + q->limits.max_user_discard_sectors = max_discard_bytes >> SECTOR_SHIFT; + q->limits.max_discard_sectors = + min_not_zero(q->limits.max_hw_discard_sectors, + q->limits.max_user_discard_sectors); return ret; } diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 5b5d3b238de1e7..700ec5055b668d 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -290,6 +290,7 @@ struct queue_limits { unsigned int io_opt; unsigned int max_discard_sectors; unsigned int max_hw_discard_sectors; + unsigned int max_user_discard_sectors; unsigned int max_secure_erase_sectors; unsigned int max_write_zeroes_sectors; unsigned int max_zone_append_sectors;
Add a new max_user_discard_sectors limit that mirrors max_user_sectors and stores the value that the user manually set. This now allows updates of the max_hw_discard_sectors to not worry about the user limit. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/blk-settings.c | 13 ++++++++++--- block/blk-sysfs.c | 18 +++++++++--------- include/linux/blkdev.h | 1 + 3 files changed, 20 insertions(+), 12 deletions(-)