[PATCH v3 0/2] mm: fix arithmetic for bdi min_ratio and max_ratio

[PATCH v3 0/2] mm: fix arithmetic for bdi min_ratio and max_ratio

[Jingbo Xu]

changes since v2:
- patch 2: drop div64_u64(), instead write it out mathematically
  (Matthew Wilcox)

changes since v1:
- split the previous v1 patch into two separate patches with
  corresponding "Fixes" tag (Andrew Morton)
- patch 2: remove "UL" suffix for the "100" constant

v1: https://lore.kernel.org/all/20231218031640.77983-1-jefflexu@linux.alibaba.com/
v2: https://lore.kernel.org/all/20231219024246.65654-1-jefflexu@linux.alibaba.com/

Jingbo Xu (2):
  mm: fix arithmetic for bdi min_ratio
  mm: fix arithmetic for max_prop_frac when setting max_ratio

 mm/page-writeback.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.19.1.6.gb485710b

[PATCH v3 1/2] mm: fix arithmetic for bdi min_ratio

[Jingbo Xu]

Since now bdi->min_ratio is part per million, fix the wrong arithmetic.
Otherwise it will fail with -EINVAL when setting a reasonable min_ratio,
as it tries to set min_ratio to (min_ratio * BDI_RATIO_SCALE) in
percentage unit, which exceeds 100% anyway.

    # cat /sys/class/bdi/253\:0/min_ratio
    0
    # cat /sys/class/bdi/253\:0/max_ratio
    100
    # echo 1 > /sys/class/bdi/253\:0/min_ratio
    -bash: echo: write error: Invalid argument

Fixes: 8021fb3232f2 ("mm: split off __bdi_set_min_ratio() function")
Reported-by: Joseph Qi <joseph.qi@linux.alibaba.com>
Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 mm/page-writeback.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index ee2fd6a6af40..2140382dd768 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -692,7 +692,6 @@ static int __bdi_set_min_ratio(struct backing_dev_info *bdi, unsigned int min_ra
 
 	if (min_ratio > 100 * BDI_RATIO_SCALE)
 		return -EINVAL;
-	min_ratio *= BDI_RATIO_SCALE;
 
 	spin_lock_bh(&bdi_lock);
 	if (min_ratio > bdi->max_ratio) {
-- 
2.19.1.6.gb485710b

[PATCH v3 2/2] mm: fix arithmetic for max_prop_frac when setting max_ratio

[Jingbo Xu]

Since now bdi->max_ratio is part per million, fix the wrong arithmetic
for max_prop_frac when setting max_ratio.  Otherwise the miscalculated
max_prop_frac will affect the incrementing of writeout completion count
when max_ratio is not 100%.

Fixes: efc3e6ad53ea ("mm: split off __bdi_set_max_ratio() function")
Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 mm/page-writeback.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 2140382dd768..05e5c425b3ff 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -728,7 +728,8 @@ static int __bdi_set_max_ratio(struct backing_dev_info *bdi, unsigned int max_ra
 		ret = -EINVAL;
 	} else {
 		bdi->max_ratio = max_ratio;
-		bdi->max_prop_frac = (FPROP_FRAC_BASE * max_ratio) / 100;
+		bdi->max_prop_frac = (FPROP_FRAC_BASE * max_ratio) /
+						(100 * BDI_RATIO_SCALE);
 	}
 	spin_unlock_bh(&bdi_lock);
 
-- 
2.19.1.6.gb485710b

Re: [PATCH v3 0/2] mm: fix arithmetic for bdi min_ratio and max_ratio

[Stefan Roesch]

It would be good if the cover letter describes what this fixes.

On Tue, Dec 19, 2023, at 6:25 AM, Jingbo Xu wrote:
> changes since v2:
> - patch 2: drop div64_u64(), instead write it out mathematically
>   (Matthew Wilcox)
>
> changes since v1:
> - split the previous v1 patch into two separate patches with
>   corresponding "Fixes" tag (Andrew Morton)
> - patch 2: remove "UL" suffix for the "100" constant
>
> v1: 
> https://lore.kernel.org/all/20231218031640.77983-1-jefflexu@linux.alibaba.com/
> v2: 
> https://lore.kernel.org/all/20231219024246.65654-1-jefflexu@linux.alibaba.com/
>
> Jingbo Xu (2):
>   mm: fix arithmetic for bdi min_ratio
>   mm: fix arithmetic for max_prop_frac when setting max_ratio
>
>  mm/page-writeback.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> -- 
> 2.19.1.6.gb485710b

Re: [PATCH v3 0/2] mm: fix arithmetic for bdi min_ratio and max_ratio

[Jingbo Xu]

On 12/20/23 1:56 AM, Stefan Roesch wrote:
> It would be good if the cover letter describes what this fixes.

Thanks. Would notice that next time :)

> 
> On Tue, Dec 19, 2023, at 6:25 AM, Jingbo Xu wrote:
>> changes since v2:
>> - patch 2: drop div64_u64(), instead write it out mathematically
>>   (Matthew Wilcox)
>>
>> changes since v1:
>> - split the previous v1 patch into two separate patches with
>>   corresponding "Fixes" tag (Andrew Morton)
>> - patch 2: remove "UL" suffix for the "100" constant
>>
>> v1: 
>> https://lore.kernel.org/all/20231218031640.77983-1-jefflexu@linux.alibaba.com/
>> v2: 
>> https://lore.kernel.org/all/20231219024246.65654-1-jefflexu@linux.alibaba.com/
>>
>> Jingbo Xu (2):
>>   mm: fix arithmetic for bdi min_ratio
>>   mm: fix arithmetic for max_prop_frac when setting max_ratio
>>
>>  mm/page-writeback.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> -- 
>> 2.19.1.6.gb485710b

-- 
Thanks,
Jingbo

Re: [PATCH v3 1/2] mm: fix arithmetic for bdi min_ratio

[Jan Kara]

On Tue 19-12-23 22:25:07, Jingbo Xu wrote:
> Since now bdi->min_ratio is part per million, fix the wrong arithmetic.
> Otherwise it will fail with -EINVAL when setting a reasonable min_ratio,
> as it tries to set min_ratio to (min_ratio * BDI_RATIO_SCALE) in
> percentage unit, which exceeds 100% anyway.
> 
>     # cat /sys/class/bdi/253\:0/min_ratio
>     0
>     # cat /sys/class/bdi/253\:0/max_ratio
>     100
>     # echo 1 > /sys/class/bdi/253\:0/min_ratio
>     -bash: echo: write error: Invalid argument
> 
> Fixes: 8021fb3232f2 ("mm: split off __bdi_set_min_ratio() function")
> Reported-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza


> ---
>  mm/page-writeback.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index ee2fd6a6af40..2140382dd768 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -692,7 +692,6 @@ static int __bdi_set_min_ratio(struct backing_dev_info *bdi, unsigned int min_ra
>  
>  	if (min_ratio > 100 * BDI_RATIO_SCALE)
>  		return -EINVAL;
> -	min_ratio *= BDI_RATIO_SCALE;
>  
>  	spin_lock_bh(&bdi_lock);
>  	if (min_ratio > bdi->max_ratio) {
> -- 
> 2.19.1.6.gb485710b
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 2/2] mm: fix arithmetic for max_prop_frac when setting max_ratio

[Jan Kara]

On Tue 19-12-23 22:25:08, Jingbo Xu wrote:
> Since now bdi->max_ratio is part per million, fix the wrong arithmetic
> for max_prop_frac when setting max_ratio.  Otherwise the miscalculated
> max_prop_frac will affect the incrementing of writeout completion count
> when max_ratio is not 100%.
> 
> Fixes: efc3e6ad53ea ("mm: split off __bdi_set_max_ratio() function")
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>

Good catch. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  mm/page-writeback.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 2140382dd768..05e5c425b3ff 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -728,7 +728,8 @@ static int __bdi_set_max_ratio(struct backing_dev_info *bdi, unsigned int max_ra
>  		ret = -EINVAL;
>  	} else {
>  		bdi->max_ratio = max_ratio;
> -		bdi->max_prop_frac = (FPROP_FRAC_BASE * max_ratio) / 100;
> +		bdi->max_prop_frac = (FPROP_FRAC_BASE * max_ratio) /
> +						(100 * BDI_RATIO_SCALE);
>  	}
>  	spin_unlock_bh(&bdi_lock);
>  
> -- 
> 2.19.1.6.gb485710b
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR