[PATCH] Btrfs: do not set log for full commit when creating non-data block groups

[PATCH] Btrfs: do not set log for full commit when creating non-data block groups

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When creating a block group we don't need to set the log for full commit
if the new block group is not used for data. Logged items can only point
to logical addresses of data block groups (through file extent items) so
there is no need to for the next fsync to fallback to a transaction commit
if the new block group is for metadata.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/extent-tree.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 577878324799..588fbd1606fb 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -10112,7 +10112,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,
 	struct btrfs_block_group_cache *cache;
 	int ret;
 
-	btrfs_set_log_full_commit(fs_info, trans);
+	if (type & BTRFS_BLOCK_GROUP_DATA)
+		btrfs_set_log_full_commit(fs_info, trans);
 
 	cache = btrfs_create_block_group_cache(fs_info, chunk_offset, size);
 	if (!cache)
-- 
2.11.0

Re: [PATCH] Btrfs: do not set log for full commit when creating non-data block groups

[Qu Wenruo]

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



On 2018/11/8 下午9:17, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> When creating a block group we don't need to set the log for full commit
> if the new block group is not used for data. Logged items can only point
> to logical addresses of data block groups (through file extent items) so
> there is no need to for the next fsync to fallback to a transaction commit
> if the new block group is for metadata.

Is it possible for the log tree blocks to be allocated in that new block
group?

Thanks,
Qu

> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/extent-tree.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 577878324799..588fbd1606fb 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -10112,7 +10112,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,
>  	struct btrfs_block_group_cache *cache;
>  	int ret;
>  
> -	btrfs_set_log_full_commit(fs_info, trans);
> +	if (type & BTRFS_BLOCK_GROUP_DATA)
> +		btrfs_set_log_full_commit(fs_info, trans);
>  
>  	cache = btrfs_create_block_group_cache(fs_info, chunk_offset, size);
>  	if (!cache)
> 


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

Re: [PATCH] Btrfs: do not set log for full commit when creating non-data block groups

[Filipe Manana]

On Thu, Nov 8, 2018 at 2:35 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> On 2018/11/8 下午9:17, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > When creating a block group we don't need to set the log for full commit
> > if the new block group is not used for data. Logged items can only point
> > to logical addresses of data block groups (through file extent items) so
> > there is no need to for the next fsync to fallback to a transaction commit
> > if the new block group is for metadata.
>
> Is it possible for the log tree blocks to be allocated in that new block
> group?

Yes.

>
> Thanks,
> Qu
>
> >
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > ---
> >  fs/btrfs/extent-tree.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > index 577878324799..588fbd1606fb 100644
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -10112,7 +10112,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,
> >       struct btrfs_block_group_cache *cache;
> >       int ret;
> >
> > -     btrfs_set_log_full_commit(fs_info, trans);
> > +     if (type & BTRFS_BLOCK_GROUP_DATA)
> > +             btrfs_set_log_full_commit(fs_info, trans);
> >
> >       cache = btrfs_create_block_group_cache(fs_info, chunk_offset, size);
> >       if (!cache)
> >
>

Re: [PATCH] Btrfs: do not set log for full commit when creating non-data block groups

[Filipe Manana]

On Thu, Nov 8, 2018 at 2:37 PM Filipe Manana <fdmanana@kernel.org> wrote:
>
> On Thu, Nov 8, 2018 at 2:35 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> >
> >
> >
> > On 2018/11/8 下午9:17, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > When creating a block group we don't need to set the log for full commit
> > > if the new block group is not used for data. Logged items can only point
> > > to logical addresses of data block groups (through file extent items) so
> > > there is no need to for the next fsync to fallback to a transaction commit
> > > if the new block group is for metadata.
> >
> > Is it possible for the log tree blocks to be allocated in that new block
> > group?
>
> Yes.

Now I realize what might be your concern, and this would cause trouble.
Surprised this didn't trigger any problem and I had this (together
with other changes) running tests for some weeks already.

>
> >
> > Thanks,
> > Qu
> >
> > >
> > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > > ---
> > >  fs/btrfs/extent-tree.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > > index 577878324799..588fbd1606fb 100644
> > > --- a/fs/btrfs/extent-tree.c
> > > +++ b/fs/btrfs/extent-tree.c
> > > @@ -10112,7 +10112,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,
> > >       struct btrfs_block_group_cache *cache;
> > >       int ret;
> > >
> > > -     btrfs_set_log_full_commit(fs_info, trans);
> > > +     if (type & BTRFS_BLOCK_GROUP_DATA)
> > > +             btrfs_set_log_full_commit(fs_info, trans);
> > >
> > >       cache = btrfs_create_block_group_cache(fs_info, chunk_offset, size);
> > >       if (!cache)
> > >
> >

Re: [PATCH] Btrfs: do not set log for full commit when creating non-data block groups

[Qu Wenruo]

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



On 2018/11/8 下午10:48, Filipe Manana wrote:
> On Thu, Nov 8, 2018 at 2:37 PM Filipe Manana <fdmanana@kernel.org> wrote:
>>
>> On Thu, Nov 8, 2018 at 2:35 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>>
>>>
>>>
>>> On 2018/11/8 下午9:17, fdmanana@kernel.org wrote:
>>>> From: Filipe Manana <fdmanana@suse.com>
>>>>
>>>> When creating a block group we don't need to set the log for full commit
>>>> if the new block group is not used for data. Logged items can only point
>>>> to logical addresses of data block groups (through file extent items) so
>>>> there is no need to for the next fsync to fallback to a transaction commit
>>>> if the new block group is for metadata.
>>>
>>> Is it possible for the log tree blocks to be allocated in that new block
>>> group?
>>
>> Yes.
> 
> Now I realize what might be your concern, and this would cause trouble.
> Surprised this didn't trigger any problem and I had this (together
> with other changes) running tests for some weeks already.

Maybe it's related metadata chunk pre-allocation so it will be super
hard to hit in normal case, or some extent allocation policy preventing
us from allocating tree block of newly created bg.

Thanks,
Qu

> 
>>
>>>
>>> Thanks,
>>> Qu
>>>
>>>>
>>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>>> ---
>>>>  fs/btrfs/extent-tree.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>> index 577878324799..588fbd1606fb 100644
>>>> --- a/fs/btrfs/extent-tree.c
>>>> +++ b/fs/btrfs/extent-tree.c
>>>> @@ -10112,7 +10112,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,
>>>>       struct btrfs_block_group_cache *cache;
>>>>       int ret;
>>>>
>>>> -     btrfs_set_log_full_commit(fs_info, trans);
>>>> +     if (type & BTRFS_BLOCK_GROUP_DATA)
>>>> +             btrfs_set_log_full_commit(fs_info, trans);
>>>>
>>>>       cache = btrfs_create_block_group_cache(fs_info, chunk_offset, size);
>>>>       if (!cache)
>>>>
>>>


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

Re: [PATCH] Btrfs: do not set log for full commit when creating non-data block groups

[Filipe Manana]

On Fri, Nov 9, 2018 at 12:27 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> On 2018/11/8 下午10:48, Filipe Manana wrote:
> > On Thu, Nov 8, 2018 at 2:37 PM Filipe Manana <fdmanana@kernel.org> wrote:
> >>
> >> On Thu, Nov 8, 2018 at 2:35 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> >>>
> >>>
> >>>
> >>> On 2018/11/8 下午9:17, fdmanana@kernel.org wrote:
> >>>> From: Filipe Manana <fdmanana@suse.com>
> >>>>
> >>>> When creating a block group we don't need to set the log for full commit
> >>>> if the new block group is not used for data. Logged items can only point
> >>>> to logical addresses of data block groups (through file extent items) so
> >>>> there is no need to for the next fsync to fallback to a transaction commit
> >>>> if the new block group is for metadata.
> >>>
> >>> Is it possible for the log tree blocks to be allocated in that new block
> >>> group?
> >>
> >> Yes.
> >
> > Now I realize what might be your concern, and this would cause trouble.
> > Surprised this didn't trigger any problem and I had this (together
> > with other changes) running tests for some weeks already.
>
> Maybe it's related metadata chunk pre-allocation so it will be super
> hard to hit in normal case, or some extent allocation policy preventing
> us from allocating tree block of newly created bg.

No, I don't think we have such kind of policy, would have noticed it
over the years if we did have one.
Metadata chunk allocation just happens much less frequently for
workloads the tests exercised.

>
> Thanks,
> Qu
>
> >
> >>
> >>>
> >>> Thanks,
> >>> Qu
> >>>
> >>>>
> >>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> >>>> ---
> >>>>  fs/btrfs/extent-tree.c | 3 ++-
> >>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> >>>> index 577878324799..588fbd1606fb 100644
> >>>> --- a/fs/btrfs/extent-tree.c
> >>>> +++ b/fs/btrfs/extent-tree.c
> >>>> @@ -10112,7 +10112,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,
> >>>>       struct btrfs_block_group_cache *cache;
> >>>>       int ret;
> >>>>
> >>>> -     btrfs_set_log_full_commit(fs_info, trans);
> >>>> +     if (type & BTRFS_BLOCK_GROUP_DATA)
> >>>> +             btrfs_set_log_full_commit(fs_info, trans);
> >>>>
> >>>>       cache = btrfs_create_block_group_cache(fs_info, chunk_offset, size);
> >>>>       if (!cache)
> >>>>
> >>>
>

Re: [PATCH] Btrfs: do not set log for full commit when creating non-data block groups

[Qu Wenruo]

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



On 2018/11/9 下午6:21, Filipe Manana wrote:
> On Fri, Nov 9, 2018 at 12:27 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> On 2018/11/8 下午10:48, Filipe Manana wrote:
>>> On Thu, Nov 8, 2018 at 2:37 PM Filipe Manana <fdmanana@kernel.org> wrote:
>>>>
>>>> On Thu, Nov 8, 2018 at 2:35 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2018/11/8 下午9:17, fdmanana@kernel.org wrote:
>>>>>> From: Filipe Manana <fdmanana@suse.com>
>>>>>>
>>>>>> When creating a block group we don't need to set the log for full commit
>>>>>> if the new block group is not used for data. Logged items can only point
>>>>>> to logical addresses of data block groups (through file extent items) so
>>>>>> there is no need to for the next fsync to fallback to a transaction commit
>>>>>> if the new block group is for metadata.
>>>>>
>>>>> Is it possible for the log tree blocks to be allocated in that new block
>>>>> group?
>>>>
>>>> Yes.
>>>
>>> Now I realize what might be your concern, and this would cause trouble.
>>> Surprised this didn't trigger any problem and I had this (together
>>> with other changes) running tests for some weeks already.
>>
>> Maybe it's related metadata chunk pre-allocation so it will be super
>> hard to hit in normal case, or some extent allocation policy preventing
>> us from allocating tree block of newly created bg.
> 
> No, I don't think we have such kind of policy, would have noticed it
> over the years if we did have one.
> Metadata chunk allocation just happens much less frequently for
> workloads the tests exercised.

Then smaller fs along with 4K nodesize would help, but I'm still not
sure if we could hit such case.

Thanks,
Qu

> 
>>
>> Thanks,
>> Qu
>>
>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Qu
>>>>>
>>>>>>
>>>>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>>>>> ---
>>>>>>  fs/btrfs/extent-tree.c | 3 ++-
>>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>>>> index 577878324799..588fbd1606fb 100644
>>>>>> --- a/fs/btrfs/extent-tree.c
>>>>>> +++ b/fs/btrfs/extent-tree.c
>>>>>> @@ -10112,7 +10112,8 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,
>>>>>>       struct btrfs_block_group_cache *cache;
>>>>>>       int ret;
>>>>>>
>>>>>> -     btrfs_set_log_full_commit(fs_info, trans);
>>>>>> +     if (type & BTRFS_BLOCK_GROUP_DATA)
>>>>>> +             btrfs_set_log_full_commit(fs_info, trans);
>>>>>>
>>>>>>       cache = btrfs_create_block_group_cache(fs_info, chunk_offset, size);
>>>>>>       if (!cache)
>>>>>>
>>>>>
>>


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

Re: [PATCH] Btrfs: do not set log for full commit when creating non-data block groups

[David Sterba]

On Thu, Nov 08, 2018 at 02:48:29PM +0000, Filipe Manana wrote:
> On Thu, Nov 8, 2018 at 2:37 PM Filipe Manana <fdmanana@kernel.org> wrote:
> >
> > On Thu, Nov 8, 2018 at 2:35 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> > >
> > >
> > >
> > > On 2018/11/8 下午9:17, fdmanana@kernel.org wrote:
> > > > From: Filipe Manana <fdmanana@suse.com>
> > > >
> > > > When creating a block group we don't need to set the log for full commit
> > > > if the new block group is not used for data. Logged items can only point
> > > > to logical addresses of data block groups (through file extent items) so
> > > > there is no need to for the next fsync to fallback to a transaction commit
> > > > if the new block group is for metadata.
> > >
> > > Is it possible for the log tree blocks to be allocated in that new block
> > > group?
> >
> > Yes.
> 
> Now I realize what might be your concern, and this would cause trouble.

Is this patch ok for for-next or does it need more work? Thanks.

Re: [PATCH] Btrfs: do not set log for full commit when creating non-data block groups

[Filipe Manana]

On Tue, Nov 13, 2018 at 2:31 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Thu, Nov 08, 2018 at 02:48:29PM +0000, Filipe Manana wrote:
> > On Thu, Nov 8, 2018 at 2:37 PM Filipe Manana <fdmanana@kernel.org> wrote:
> > >
> > > On Thu, Nov 8, 2018 at 2:35 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> > > >
> > > >
> > > >
> > > > On 2018/11/8 下午9:17, fdmanana@kernel.org wrote:
> > > > > From: Filipe Manana <fdmanana@suse.com>
> > > > >
> > > > > When creating a block group we don't need to set the log for full commit
> > > > > if the new block group is not used for data. Logged items can only point
> > > > > to logical addresses of data block groups (through file extent items) so
> > > > > there is no need to for the next fsync to fallback to a transaction commit
> > > > > if the new block group is for metadata.
> > > >
> > > > Is it possible for the log tree blocks to be allocated in that new block
> > > > group?
> > >
> > > Yes.
> >
> > Now I realize what might be your concern, and this would cause trouble.
>
> Is this patch ok for for-next or does it need more work? Thanks.

Nop, it's no good (despite not triggering problems initially), due to
Qu's first question.
So just drop it and forget it.
Thanks.