Re: [RFC PATCH v3 07/14] mm: add bdi_set_max_bytes() function.

[Stefan Roesch <shr@devkernel.io>]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Mon, 24 Oct 2022 12:05:56 -0700 Stefan Roesch <shr@devkernel.io> wrote:
>
>> This introduces the bdi_set_max_bytes() function. The max_bytes function
>> does not store the max_bytes value. Instead it converts the max_bytes
>> value into the corresponding ratio value.
>>
>> ...
>>
>> --- a/include/linux/backing-dev.h
>> +++ b/include/linux/backing-dev.h
>> @@ -108,6 +108,7 @@ static inline unsigned long wb_stat_error(void)
>>  unsigned long long bdi_get_max_bytes(struct backing_dev_info *bdi);
>>  int bdi_set_min_ratio(struct backing_dev_info *bdi, unsigned int min_ratio);
>>  int bdi_set_max_ratio(struct backing_dev_info *bdi, unsigned int max_ratio);
>> +int bdi_set_max_bytes(struct backing_dev_info *bdi, unsigned long long max_bytes);
>>  int bdi_set_strict_limit(struct backing_dev_info *bdi, unsigned int strict_limit);
>>
>>  /*
>> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
>> index 8b8936603783..21d7c1880ea8 100644
>> --- a/mm/page-writeback.c
>> +++ b/mm/page-writeback.c
>> @@ -13,6 +13,7 @@
>>   */
>>
>>  #include <linux/kernel.h>
>> +#include <linux/math64.h>
>>  #include <linux/export.h>
>>  #include <linux/spinlock.h>
>>  #include <linux/fs.h>
>> @@ -650,6 +651,28 @@ void wb_domain_exit(struct wb_domain *dom)
>>   */
>>  static unsigned int bdi_min_ratio;
>>
>> +static int bdi_check_pages_limit(unsigned long pages)
>> +{
>> +	unsigned long max_dirty_pages = global_dirtyable_memory();
>> +
>> +	if (pages > max_dirty_pages / 2)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>
> Some code comments are needed here.  Explain what it does and why it
> does it.  The "/ 2" seems utterly arbitray - explain why this value was
> chosen?  Why is it better than "/ 3"?
>
>

I changed the check to
   (pages > max_dirty_pages)

>
>> +static unsigned long bdi_ratio_from_pages(unsigned long pages)
>> +{
>> +	unsigned long background_thresh;
>> +	unsigned long dirty_thresh;
>> +	unsigned long ratio;
>> +
>> +	global_dirty_limits(&background_thresh, &dirty_thresh);
>> +	ratio = div64_u64(pages * 100ULL * BDI_RATIO_SCALE, dirty_thresh);
>
> `unsigned long' is 32-bit on 32-bit machines, which makes this code a
> bit odd.  Should everything here be u64?

The function global_dirty_limits() uses unsigned long pointers for its
arguments. unsigned long looks like a better fit. Any thoughts?