| Message ID | 20230707094616.108430-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] block: don't unconditionally set max_discard_sectors in blk_queue_max_discard_sectors | expand |
On 7/7/23 18:46, Christoph Hellwig wrote: > max_discard_sectors is split into a hardware and a tunable value, but > blk_queue_max_discard_sectors sets both unconditionally, thus dropping > any user stored value on a rescan. Fix blk_queue_max_discard_sectors to > only set max_discard_sectors if it either wasn't set, or the new hardware > limit is smaller than the previous user limit. > > Fixes: 0034af036554 ("block: make /sys/block/<dev>/queue/discard_max_bytes writeable") > Signed-off-by: Christoph Hellwig <hch@lst.de> Look OK to me. Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > --- > block/blk-settings.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/block/blk-settings.c b/block/blk-settings.c > index 0046b447268f91..978d2e1fd67a51 100644 > --- a/block/blk-settings.c > +++ b/block/blk-settings.c > @@ -179,7 +179,9 @@ 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; > + if (!q->limits.max_discard_sectors || > + q->limits.max_discard_sectors > max_discard_sectors) > + q->limits.max_discard_sectors = max_discard_sectors; > } > EXPORT_SYMBOL(blk_queue_max_discard_sectors); >
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
On Fri, Jul 07, 2023 at 11:46:13AM +0200, Christoph Hellwig wrote: > max_discard_sectors is split into a hardware and a tunable value, but > blk_queue_max_discard_sectors sets both unconditionally, thus dropping > any user stored value on a rescan. Fix blk_queue_max_discard_sectors to > only set max_discard_sectors if it either wasn't set, or the new hardware > limit is smaller than the previous user limit. > > Fixes: 0034af036554 ("block: make /sys/block/<dev>/queue/discard_max_bytes writeable") It is hard to say a fix, given discard_max_bytes can still be changed by kernel. I'd suggest to document this behavior in Documentation/ABI/stable/sysfs-block. > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/blk-settings.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/block/blk-settings.c b/block/blk-settings.c > index 0046b447268f91..978d2e1fd67a51 100644 > --- a/block/blk-settings.c > +++ b/block/blk-settings.c > @@ -179,7 +179,9 @@ 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; > + if (!q->limits.max_discard_sectors || > + q->limits.max_discard_sectors > max_discard_sectors) > + q->limits.max_discard_sectors = max_discard_sectors; > } Userspace may write 0 to discard_max_bytes, and this patch still can override user setting. Thanks, Ming
On Fri, Jul 07, 2023 at 11:46:13AM +0200, Christoph Hellwig wrote: > { > q->limits.max_hw_discard_sectors = max_discard_sectors; > - q->limits.max_discard_sectors = max_discard_sectors; > + if (!q->limits.max_discard_sectors || > + q->limits.max_discard_sectors > max_discard_sectors) > + q->limits.max_discard_sectors = max_discard_sectors; Could simplify to min_not_zero(). But this only allows you to make the limit smaller. If the user never set max_discard_sectors before, and a firmware update allows a larger max_hw_discard_sectors, the subsequent rescan won't use the new limit.
On Mon, Jul 10, 2023 at 06:42:36PM +0800, Ming Lei wrote: > Userspace may write 0 to discard_max_bytes, and this patch still can > override user setting. True. Maybe the right thing is to have a user_limit field, and just looks at the min of that and the hw limit everywhere. These hardware vs user limits are a pain, and we'll probably need some proper infrastructure for them :P
On Wed, Jul 12, 2023 at 06:23:10PM +0200, Christoph Hellwig wrote: > On Mon, Jul 10, 2023 at 06:42:36PM +0800, Ming Lei wrote: > > Userspace may write 0 to discard_max_bytes, and this patch still can > > override user setting. > > True. Maybe the right thing is to have a user_limit field, and just > looks at the min of that and the hw limit everywhere. These hardware > vs user limits are a pain, and we'll probably need some proper > infrastructure for them :P Yeah, I had to do something very similiar for the max_sectors limit too: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=c9c77418a98273fe96835c42666f7427b3883f48
diff --git a/block/blk-settings.c b/block/blk-settings.c index 0046b447268f91..978d2e1fd67a51 100644 --- a/block/blk-settings.c +++ b/block/blk-settings.c @@ -179,7 +179,9 @@ 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; + if (!q->limits.max_discard_sectors || + q->limits.max_discard_sectors > max_discard_sectors) + q->limits.max_discard_sectors = max_discard_sectors; } EXPORT_SYMBOL(blk_queue_max_discard_sectors);
max_discard_sectors is split into a hardware and a tunable value, but blk_queue_max_discard_sectors sets both unconditionally, thus dropping any user stored value on a rescan. Fix blk_queue_max_discard_sectors to only set max_discard_sectors if it either wasn't set, or the new hardware limit is smaller than the previous user limit. Fixes: 0034af036554 ("block: make /sys/block/<dev>/queue/discard_max_bytes writeable") Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/blk-settings.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)