[PATCH 1/2] btrfs: account for pinned bytes and bytes_may_use in should_alloc_chunk

[PATCH 1/2] btrfs: account for pinned bytes and bytes_may_use in should_alloc_chunk

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

In a heavy write scenario, we can end up with a large number of pinned
bytes.  This can translate into (very) premature ENOSPC because pinned
bytes must be accounted for when allowing a reservation but aren't
accounted for when deciding whether to create a new chunk.

This patch adds the accounting to should_alloc_chunk so that we can
create the chunk.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 fs/btrfs/extent-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index cb0b924..d027807 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4389,7 +4389,7 @@ static int should_alloc_chunk(struct btrfs_fs_info *fs_info,
 {
 	struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
 	u64 num_bytes = sinfo->total_bytes - sinfo->bytes_readonly;
-	u64 num_allocated = sinfo->bytes_used + sinfo->bytes_reserved;
+	u64 num_allocated = sinfo->bytes_used + sinfo->bytes_reserved + sinfo->bytes_pinned + sinfo->bytes_may_use;
 	u64 thresh;
 
 	if (force == CHUNK_ALLOC_FORCE)
-- 
1.8.5.6

[PATCH 2/2] btrfs: Simplify math in should_alloc chunk

[jeffm]

From: Nikolay Borisov <nborisov@suse.com>

Currently should_alloc_chunk uses ->total_bytes - ->bytes_readonly to
signify the total amount of bytes in this space info. However, given
Jeff's patch which adds bytes_pinned and bytes_may_use to the calculation
of num_allocated it becomes a lot more clear to just eliminate num_bytes
altogether and add the bytes_readonly to the amount of used space. That
way we don't change the results of the following statements. In the
process also start using btrfs_space_info_used.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 fs/btrfs/extent-tree.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index d027807..6111f79 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4388,8 +4388,7 @@ static int should_alloc_chunk(struct btrfs_fs_info *fs_info,
 			      struct btrfs_space_info *sinfo, int force)
 {
 	struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
-	u64 num_bytes = sinfo->total_bytes - sinfo->bytes_readonly;
-	u64 num_allocated = sinfo->bytes_used + sinfo->bytes_reserved + sinfo->bytes_pinned + sinfo->bytes_may_use;
+	u64 bytes_used = btrfs_space_info_used(sinfo, true);
 	u64 thresh;
 
 	if (force == CHUNK_ALLOC_FORCE)
@@ -4401,7 +4400,7 @@ static int should_alloc_chunk(struct btrfs_fs_info *fs_info,
 	 * global_rsv, it doesn't change except when the transaction commits.
 	 */
 	if (sinfo->flags & BTRFS_BLOCK_GROUP_METADATA)
-		num_allocated += calc_global_rsv_need_space(global_rsv);
+		bytes_used += calc_global_rsv_need_space(global_rsv);
 
 	/*
 	 * in limited mode, we want to have some free space up to
@@ -4411,11 +4410,11 @@ static int should_alloc_chunk(struct btrfs_fs_info *fs_info,
 		thresh = btrfs_super_total_bytes(fs_info->super_copy);
 		thresh = max_t(u64, SZ_64M, div_factor_fine(thresh, 1));
 
-		if (num_bytes - num_allocated < thresh)
+		if (sinfo->total_bytes - bytes_used < thresh)
 			return 1;
 	}
 
-	if (num_allocated + SZ_2M < div_factor(num_bytes, 8))
+	if (bytes_used + SZ_2M < div_factor(sinfo->total_bytes, 8))
 		return 0;
 	return 1;
 }
-- 
1.8.5.6

Re: [PATCH 1/2] btrfs: account for pinned bytes and bytes_may_use in should_alloc_chunk

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 1453 bytes --]

On 6/14/17 11:44 AM, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> In a heavy write scenario, we can end up with a large number of pinned
> bytes.  This can translate into (very) premature ENOSPC because pinned
> bytes must be accounted for when allowing a reservation but aren't
> accounted for when deciding whether to create a new chunk.
> 
> This patch adds the accounting to should_alloc_chunk so that we can
> create the chunk.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  fs/btrfs/extent-tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index cb0b924..d027807 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4389,7 +4389,7 @@ static int should_alloc_chunk(struct btrfs_fs_info *fs_info,
>  {
>  	struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
>  	u64 num_bytes = sinfo->total_bytes - sinfo->bytes_readonly;
> -	u64 num_allocated = sinfo->bytes_used + sinfo->bytes_reserved;
> +	u64 num_allocated = sinfo->bytes_used + sinfo->bytes_reserved + sinfo->bytes_pinned + sinfo->bytes_may_use;
>  	u64 thresh;
>  
>  	if (force == CHUNK_ALLOC_FORCE)
> 


Ignore this patch.  It certainly allocates chunks more aggressively, but
it means we end up with a ton of metadata chunks even when we don't have
much metadata.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] btrfs: account for pinned bytes and bytes_may_use in should_alloc_chunk

[Chris Mason]

On 06/21/2017 04:14 PM, Jeff Mahoney wrote:
> On 6/14/17 11:44 AM, jeffm@suse.com wrote:
>> From: Jeff Mahoney <jeffm@suse.com>
>>
>> In a heavy write scenario, we can end up with a large number of pinned
>> bytes.  This can translate into (very) premature ENOSPC because pinned
>> bytes must be accounted for when allowing a reservation but aren't
>> accounted for when deciding whether to create a new chunk.
>>
>> This patch adds the accounting to should_alloc_chunk so that we can
>> create the chunk.
>>
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>> ---
>>  fs/btrfs/extent-tree.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index cb0b924..d027807 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -4389,7 +4389,7 @@ static int should_alloc_chunk(struct btrfs_fs_info *fs_info,
>>  {
>>  	struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
>>  	u64 num_bytes = sinfo->total_bytes - sinfo->bytes_readonly;
>> -	u64 num_allocated = sinfo->bytes_used + sinfo->bytes_reserved;
>> +	u64 num_allocated = sinfo->bytes_used + sinfo->bytes_reserved + sinfo->bytes_pinned + sinfo->bytes_may_use;
>>  	u64 thresh;
>>
>>  	if (force == CHUNK_ALLOC_FORCE)
>>
>
>
> Ignore this patch.  It certainly allocates chunks more aggressively, but
> it means we end up with a ton of metadata chunks even when we don't have
> much metadata.
>

Josef and I pushed this needle back and forth a bunch of times in the 
early days.  I still think we can allocate a few more chunks than we do 
now...

-chris

Re: [PATCH 1/2] btrfs: account for pinned bytes and bytes_may_use in should_alloc_chunk

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 2415 bytes --]

On 6/21/17 4:31 PM, Chris Mason wrote:
> On 06/21/2017 04:14 PM, Jeff Mahoney wrote:
>> On 6/14/17 11:44 AM, jeffm@suse.com wrote:
>>> From: Jeff Mahoney <jeffm@suse.com>
>>>
>>> In a heavy write scenario, we can end up with a large number of pinned
>>> bytes.  This can translate into (very) premature ENOSPC because pinned
>>> bytes must be accounted for when allowing a reservation but aren't
>>> accounted for when deciding whether to create a new chunk.
>>>
>>> This patch adds the accounting to should_alloc_chunk so that we can
>>> create the chunk.
>>>
>>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>>> ---
>>>  fs/btrfs/extent-tree.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index cb0b924..d027807 100644
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -4389,7 +4389,7 @@ static int should_alloc_chunk(struct
>>> btrfs_fs_info *fs_info,
>>>  {
>>>      struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
>>>      u64 num_bytes = sinfo->total_bytes - sinfo->bytes_readonly;
>>> -    u64 num_allocated = sinfo->bytes_used + sinfo->bytes_reserved;
>>> +    u64 num_allocated = sinfo->bytes_used + sinfo->bytes_reserved +
>>> sinfo->bytes_pinned + sinfo->bytes_may_use;
>>>      u64 thresh;
>>>
>>>      if (force == CHUNK_ALLOC_FORCE)
>>>
>>
>>
>> Ignore this patch.  It certainly allocates chunks more aggressively, but
>> it means we end up with a ton of metadata chunks even when we don't have
>> much metadata.
>>
> 
> Josef and I pushed this needle back and forth a bunch of times in the
> early days.  I still think we can allocate a few more chunks than we do
> now...

I agree.  This patch was to fix an issue that we are seeing during
installation.  It'd stop with ENOSPC with >50GB completely unallocated.
The patch passed the test cases that were failing before but now it's
failing differently.  I was worried this pattern might be the end result:

Data,single: Size:4.00GiB, Used:3.32GiB
   /dev/vde        4.00GiB

Metadata,DUP: Size:20.00GiB, Used:204.12MiB
   /dev/vde       40.00GiB

System,DUP: Size:8.00MiB, Used:16.00KiB
   /dev/vde       16.00MiB

This is on a fresh file system with just "cp /usr /mnt" executed.

I'm looking into it a bit more now.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] btrfs: account for pinned bytes and bytes_may_use in should_alloc_chunk

[Chris Mason]

On 06/21/2017 05:08 PM, Jeff Mahoney wrote:
> On 6/21/17 4:31 PM, Chris Mason wrote:
>> On 06/21/2017 04:14 PM, Jeff Mahoney wrote:
>>> On 6/14/17 11:44 AM, jeffm@suse.com wrote:
>>>> From: Jeff Mahoney <jeffm@suse.com>
>>>>
>>>> In a heavy write scenario, we can end up with a large number of pinned
>>>> bytes.  This can translate into (very) premature ENOSPC because pinned
>>>> bytes must be accounted for when allowing a reservation but aren't
>>>> accounted for when deciding whether to create a new chunk.
>>>>
>>>> This patch adds the accounting to should_alloc_chunk so that we can
>>>> create the chunk.
>>>>
>>>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>>>> ---
>>>>  fs/btrfs/extent-tree.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>> index cb0b924..d027807 100644
>>>> --- a/fs/btrfs/extent-tree.c
>>>> +++ b/fs/btrfs/extent-tree.c
>>>> @@ -4389,7 +4389,7 @@ static int should_alloc_chunk(struct
>>>> btrfs_fs_info *fs_info,
>>>>  {
>>>>      struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
>>>>      u64 num_bytes = sinfo->total_bytes - sinfo->bytes_readonly;
>>>> -    u64 num_allocated = sinfo->bytes_used + sinfo->bytes_reserved;
>>>> +    u64 num_allocated = sinfo->bytes_used + sinfo->bytes_reserved +
>>>> sinfo->bytes_pinned + sinfo->bytes_may_use;
>>>>      u64 thresh;
>>>>
>>>>      if (force == CHUNK_ALLOC_FORCE)
>>>>
>>>
>>>
>>> Ignore this patch.  It certainly allocates chunks more aggressively, but
>>> it means we end up with a ton of metadata chunks even when we don't have
>>> much metadata.
>>>
>>
>> Josef and I pushed this needle back and forth a bunch of times in the
>> early days.  I still think we can allocate a few more chunks than we do
>> now...
>
> I agree.  This patch was to fix an issue that we are seeing during
> installation.  It'd stop with ENOSPC with >50GB completely unallocated.
> The patch passed the test cases that were failing before but now it's
> failing differently.  I was worried this pattern might be the end result:
>
> Data,single: Size:4.00GiB, Used:3.32GiB
>    /dev/vde        4.00GiB
>
> Metadata,DUP: Size:20.00GiB, Used:204.12MiB
>    /dev/vde       40.00GiB
>
> System,DUP: Size:8.00MiB, Used:16.00KiB
>    /dev/vde       16.00MiB
>
> This is on a fresh file system with just "cp /usr /mnt" executed.
>
> I'm looking into it a bit more now.

Does this failure still happen with Omar's ENOSPC fix (commit: 
70e7af244f24c94604ef6eca32ad297632018583)

-chris

Re: [PATCH 1/2] btrfs: account for pinned bytes and bytes_may_use in should_alloc_chunk

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 2885 bytes --]

On 6/21/17 5:15 PM, Chris Mason wrote:
> 
> 
> On 06/21/2017 05:08 PM, Jeff Mahoney wrote:
>> On 6/21/17 4:31 PM, Chris Mason wrote:
>>> On 06/21/2017 04:14 PM, Jeff Mahoney wrote:
>>>> On 6/14/17 11:44 AM, jeffm@suse.com wrote:
>>>>> From: Jeff Mahoney <jeffm@suse.com>
>>>>>
>>>>> In a heavy write scenario, we can end up with a large number of pinned
>>>>> bytes.  This can translate into (very) premature ENOSPC because pinned
>>>>> bytes must be accounted for when allowing a reservation but aren't
>>>>> accounted for when deciding whether to create a new chunk.
>>>>>
>>>>> This patch adds the accounting to should_alloc_chunk so that we can
>>>>> create the chunk.
>>>>>
>>>>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>>>>> ---
>>>>>  fs/btrfs/extent-tree.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>>> index cb0b924..d027807 100644
>>>>> --- a/fs/btrfs/extent-tree.c
>>>>> +++ b/fs/btrfs/extent-tree.c
>>>>> @@ -4389,7 +4389,7 @@ static int should_alloc_chunk(struct
>>>>> btrfs_fs_info *fs_info,
>>>>>  {
>>>>>      struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
>>>>>      u64 num_bytes = sinfo->total_bytes - sinfo->bytes_readonly;
>>>>> -    u64 num_allocated = sinfo->bytes_used + sinfo->bytes_reserved;
>>>>> +    u64 num_allocated = sinfo->bytes_used + sinfo->bytes_reserved +
>>>>> sinfo->bytes_pinned + sinfo->bytes_may_use;
>>>>>      u64 thresh;
>>>>>
>>>>>      if (force == CHUNK_ALLOC_FORCE)
>>>>>
>>>>
>>>>
>>>> Ignore this patch.  It certainly allocates chunks more aggressively,
>>>> but
>>>> it means we end up with a ton of metadata chunks even when we don't
>>>> have
>>>> much metadata.
>>>>
>>>
>>> Josef and I pushed this needle back and forth a bunch of times in the
>>> early days.  I still think we can allocate a few more chunks than we do
>>> now...
>>
>> I agree.  This patch was to fix an issue that we are seeing during
>> installation.  It'd stop with ENOSPC with >50GB completely unallocated.
>> The patch passed the test cases that were failing before but now it's
>> failing differently.  I was worried this pattern might be the end result:
>>
>> Data,single: Size:4.00GiB, Used:3.32GiB
>>    /dev/vde        4.00GiB
>>
>> Metadata,DUP: Size:20.00GiB, Used:204.12MiB
>>    /dev/vde       40.00GiB
>>
>> System,DUP: Size:8.00MiB, Used:16.00KiB
>>    /dev/vde       16.00MiB
>>
>> This is on a fresh file system with just "cp /usr /mnt" executed.
>>
>> I'm looking into it a bit more now.
> 
> Does this failure still happen with Omar's ENOSPC fix (commit:
> 70e7af244f24c94604ef6eca32ad297632018583)

Nope.  There aren't any warnings either with or without my patch.
Adding Omar's didn't make a difference.

-Jeff


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]