[PATCH] btrfs: limit the number of asynchronous delalloc pages to reasonable value

[PATCH] btrfs: limit the number of asynchronous delalloc pages to reasonable value

[Wang Xiaoguang]

The current limit of number of asynchronous delalloc pages is (10 * SZ_1M).
For 4K page, the total ram bytes would be 40G, very big value, I think in
most cases, this limit will not work, here I set limit of the number of
asynchronous delalloc pages to SZ_1M(4GB ram bytes).

Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
---
 fs/btrfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 8e3a5a2..3a910f6 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1158,7 +1158,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 	unsigned long nr_pages;
 	u64 cur_end;
-	int limit = 10 * SZ_1M;
+	int limit = SZ_1M;
 
 	clear_extent_bit(&BTRFS_I(inode)->io_tree, start, end, EXTENT_LOCKED,
 			 1, 0, NULL, GFP_NOFS);
-- 
2.5.0

Re: [PATCH] btrfs: limit the number of asynchronous delalloc pages to reasonable value

[David Sterba]

On Tue, Nov 08, 2016 at 05:30:58PM +0800, Wang Xiaoguang wrote:
> The current limit of number of asynchronous delalloc pages is (10 * SZ_1M).
> For 4K page, the total ram bytes would be 40G, very big value, I think in
> most cases, this limit will not work, here I set limit of the number of
> asynchronous delalloc pages to SZ_1M(4GB ram bytes).
> 
> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
> ---
>  fs/btrfs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 8e3a5a2..3a910f6 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1158,7 +1158,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
>  	unsigned long nr_pages;
>  	u64 cur_end;
> -	int limit = 10 * SZ_1M;
> +	int limit = SZ_1M;

This looks like mismatch in units, the limit seems to be "10MiB" but
it's used to compare async_delalloc_pages, which is in pages. I'm not
sure what's the expected value for limit, as 10MiB is too small.

Re: [PATCH] btrfs: limit the number of asynchronous delalloc pages to reasonable value

[Wang Xiaoguang]

hello,

On 11/22/2016 12:39 AM, David Sterba wrote:
> On Tue, Nov 08, 2016 at 05:30:58PM +0800, Wang Xiaoguang wrote:
>> The current limit of number of asynchronous delalloc pages is (10 * SZ_1M).
>> For 4K page, the total ram bytes would be 40G, very big value, I think in
>> most cases, this limit will not work, here I set limit of the number of
>> asynchronous delalloc pages to SZ_1M(4GB ram bytes).
>>
>> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>> ---
>>   fs/btrfs/inode.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 8e3a5a2..3a910f6 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -1158,7 +1158,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>>   	struct btrfs_root *root = BTRFS_I(inode)->root;
>>   	unsigned long nr_pages;
>>   	u64 cur_end;
>> -	int limit = 10 * SZ_1M;
>> +	int limit = SZ_1M;
> This looks like mismatch in units, the limit seems to be "10MiB" but
> it's used to compare async_delalloc_pages, which is in pages. I'm not
> sure what's the expected value for limit, as 10MiB is too small.
There are such codes in cow_file_range_async()
     nr_pages = (cur_end - start + PAGE_SIZE) >> PAGE_SHIFT;
     atomic_add(nr_pages, &root->fs_info->async_delalloc_pages);

So here the real limit is 10 * SZ_1M * 4096, 40GB memory, I think this
value is too big, so modify it to 4GB.

Regards,
Xiaoguang Wang
>
>

Re: [PATCH] btrfs: limit the number of asynchronous delalloc pages to reasonable value

[David Sterba]

On Tue, Nov 22, 2016 at 05:20:10PM +0800, Wang Xiaoguang wrote:
> hello,
> 
> On 11/22/2016 12:39 AM, David Sterba wrote:
> > On Tue, Nov 08, 2016 at 05:30:58PM +0800, Wang Xiaoguang wrote:
> >> The current limit of number of asynchronous delalloc pages is (10 * SZ_1M).
> >> For 4K page, the total ram bytes would be 40G, very big value, I think in
> >> most cases, this limit will not work, here I set limit of the number of
> >> asynchronous delalloc pages to SZ_1M(4GB ram bytes).
> >>
> >> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
> >> ---
> >>   fs/btrfs/inode.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> >> index 8e3a5a2..3a910f6 100644
> >> --- a/fs/btrfs/inode.c
> >> +++ b/fs/btrfs/inode.c
> >> @@ -1158,7 +1158,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
> >>   	struct btrfs_root *root = BTRFS_I(inode)->root;
> >>   	unsigned long nr_pages;
> >>   	u64 cur_end;
> >> -	int limit = 10 * SZ_1M;
> >> +	int limit = SZ_1M;
> > This looks like mismatch in units, the limit seems to be "10MiB" but
> > it's used to compare async_delalloc_pages, which is in pages. I'm not
> > sure what's the expected value for limit, as 10MiB is too small.
> There are such codes in cow_file_range_async()
>      nr_pages = (cur_end - start + PAGE_SIZE) >> PAGE_SHIFT;
>      atomic_add(nr_pages, &root->fs_info->async_delalloc_pages);
> 
> So here the real limit is 10 * SZ_1M * 4096, 40GB memory, I think this
> value is too big, so modify it to 4GB.

Yeah, but I don't see why 4GB would be a good value either.

Re: [PATCH] btrfs: limit the number of asynchronous delalloc pages to reasonable value

[Chris Mason]

On 11/22/2016 04:20 AM, Wang Xiaoguang wrote:
> hello,
>
> On 11/22/2016 12:39 AM, David Sterba wrote:
>> On Tue, Nov 08, 2016 at 05:30:58PM +0800, Wang Xiaoguang wrote:
>>> The current limit of number of asynchronous delalloc pages is (10 *
>>> SZ_1M).
>>> For 4K page, the total ram bytes would be 40G, very big value, I
>>> think in
>>> most cases, this limit will not work, here I set limit of the number of
>>> asynchronous delalloc pages to SZ_1M(4GB ram bytes).
>>>
>>> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>>> ---
>>>   fs/btrfs/inode.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 8e3a5a2..3a910f6 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -1158,7 +1158,7 @@ static int cow_file_range_async(struct inode
>>> *inode, struct page *locked_page,
>>>       struct btrfs_root *root = BTRFS_I(inode)->root;
>>>       unsigned long nr_pages;
>>>       u64 cur_end;
>>> -    int limit = 10 * SZ_1M;
>>> +    int limit = SZ_1M;
>> This looks like mismatch in units, the limit seems to be "10MiB" but
>> it's used to compare async_delalloc_pages, which is in pages. I'm not
>> sure what's the expected value for limit, as 10MiB is too small.
> There are such codes in cow_file_range_async()
>     nr_pages = (cur_end - start + PAGE_SIZE) >> PAGE_SHIFT;
>     atomic_add(nr_pages, &root->fs_info->async_delalloc_pages);
>
> So here the real limit is 10 * SZ_1M * 4096, 40GB memory, I think this
> value is too big, so modify it to 4GB.

I don't disagree that its too big, but if the rest of the 
dirty/writeback throttling infra in the kernel is enough, we can just 
delete this limit completely.

-chris

Re: [PATCH] btrfs: limit the number of asynchronous delalloc pages to reasonable value

[Chris Mason]

On 11/08/2016 04:30 AM, Wang Xiaoguang wrote:
> The current limit of number of asynchronous delalloc pages is (10 * SZ_1M).
> For 4K page, the total ram bytes would be 40G, very big value, I think in
> most cases, this limit will not work, here I set limit of the number of
> asynchronous delalloc pages to SZ_1M(4GB ram bytes).
>
> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
> ---
>  fs/btrfs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 8e3a5a2..3a910f6 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1158,7 +1158,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
>  	unsigned long nr_pages;
>  	u64 cur_end;
> -	int limit = 10 * SZ_1M;
> +	int limit = SZ_1M;
>
>  	clear_extent_bit(&BTRFS_I(inode)->io_tree, start, end, EXTENT_LOCKED,
>  			 1, 0, NULL, GFP_NOFS);
>

As Dave points out, I didn't use the right units for this, so even 
though we definitely waited on this limit while it was in development, 
that was probably a different bug.

Do you have a test case where the regular writeback throttling isn't 
enough to also throttle the async delalloc pages?  It might be better to 
just delete the limit entirely.

-chris

Re: [PATCH] btrfs: limit the number of asynchronous delalloc pages to reasonable value

[Wang Xiaoguang]

hello,

On 11/22/2016 01:59 AM, Chris Mason wrote:
> On 11/08/2016 04:30 AM, Wang Xiaoguang wrote:
>> The current limit of number of asynchronous delalloc pages is (10 * 
>> SZ_1M).
>> For 4K page, the total ram bytes would be 40G, very big value, I 
>> think in
>> most cases, this limit will not work, here I set limit of the number of
>> asynchronous delalloc pages to SZ_1M(4GB ram bytes).
>>
>> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>> ---
>>  fs/btrfs/inode.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 8e3a5a2..3a910f6 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -1158,7 +1158,7 @@ static int cow_file_range_async(struct inode 
>> *inode, struct page *locked_page,
>>      struct btrfs_root *root = BTRFS_I(inode)->root;
>>      unsigned long nr_pages;
>>      u64 cur_end;
>> -    int limit = 10 * SZ_1M;
>> +    int limit = SZ_1M;
>>
>>      clear_extent_bit(&BTRFS_I(inode)->io_tree, start, end, 
>> EXTENT_LOCKED,
>>               1, 0, NULL, GFP_NOFS);
>>
>
> As Dave points out, I didn't use the right units for this, so even 
> though we definitely waited on this limit while it was in development, 
> that was probably a different bug.
>
> Do you have a test case where the regular writeback throttling isn't 
> enough to also throttle the async delalloc pages?  It might be better 
> to just delete the limit entirely.
Sorry, I don't have one. The real limit is 10 * SZ_1M * 4096, 40GB 
memory, too big value,
and I don't see this limit actually takes effect in my test environment. 
I'm OK with removing
this limit :)

Regards,
Xiaoguang Wang
>
> -chris
>
>