[PATCH] btrfs-progs: check: change commit condition in fixup_extent_refs()

[PATCH] btrfs-progs: check: change commit condition in fixup_extent_refs()

[Sidong Yang]

This patch fixes potential bugs in fixup_extent_refs(). If
btrfs_start_transaction() fails in some way and returns error ptr, It
goes to out logic. But old code checkes whether it is null and it calls
commit. This patch solves the problem with change the condition to
checks if it is error by IS_ERR().

Issue: #409

Signed-off-by: Sidong Yang <realwakka@gmail.com>
---
 check/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/check/main.c b/check/main.c
index 235a9bab..ca858e07 100644
--- a/check/main.c
+++ b/check/main.c
@@ -7735,7 +7735,7 @@ static int fixup_extent_refs(struct cache_tree *extent_cache,
 			goto out;
 	}
 out:
-	if (trans) {
+	if (!IS_ERR(trans)) {
 		int err = btrfs_commit_transaction(trans, gfs_info->extent_root);
 
 		if (!ret)
-- 
2.25.1

Re: [PATCH] btrfs-progs: check: change commit condition in fixup_extent_refs()

[Nikolay Borisov]

On 3.11.21 г. 17:15, Sidong Yang wrote:
> This patch fixes potential bugs in fixup_extent_refs(). If
> btrfs_start_transaction() fails in some way and returns error ptr, It
> goes to out logic. But old code checkes whether it is null and it calls
> commit. This patch solves the problem with change the condition to
> checks if it is error by IS_ERR().
> 
> Issue: #409
> 
> Signed-off-by: Sidong Yang <realwakka@gmail.com>
> ---
>  check/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/check/main.c b/check/main.c
> index 235a9bab..ca858e07 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -7735,7 +7735,7 @@ static int fixup_extent_refs(struct cache_tree *extent_cache,
>  			goto out;
>  	}
>  out:
> -	if (trans) {
> +	if (!IS_ERR(trans)) {

Actually I think we want to commit the transaction only if ret is not
error, otherwise we run the risk of committing partial changes to the fs.

>  		int err = btrfs_commit_transaction(trans, gfs_info->extent_root);
>  
>  		if (!ret)
> 

Re: [PATCH] btrfs-progs: check: change commit condition in fixup_extent_refs()

[Sidong Yang]

On Wed, Nov 03, 2021 at 05:25:38PM +0200, Nikolay Borisov wrote:
> 
> 
> On 3.11.21 г. 17:15, Sidong Yang wrote:
> > This patch fixes potential bugs in fixup_extent_refs(). If
> > btrfs_start_transaction() fails in some way and returns error ptr, It
> > goes to out logic. But old code checkes whether it is null and it calls
> > commit. This patch solves the problem with change the condition to
> > checks if it is error by IS_ERR().
> > 
> > Issue: #409
> > 
> > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > ---
> >  check/main.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/check/main.c b/check/main.c
> > index 235a9bab..ca858e07 100644
> > --- a/check/main.c
> > +++ b/check/main.c
> > @@ -7735,7 +7735,7 @@ static int fixup_extent_refs(struct cache_tree *extent_cache,
> >  			goto out;
> >  	}
> >  out:
> > -	if (trans) {
> > +	if (!IS_ERR(trans)) {
> 
> Actually I think we want to commit the transaction only if ret is not
> error, otherwise we run the risk of committing partial changes to the fs.

I agree. If ret is error, committing should not be happen. But I 
think it needs to check trans. 

I wonder that if ret is error but trans is okay, trans needs to be
aborted by calling btrfs_abort_transaction()?
> 
> >  		int err = btrfs_commit_transaction(trans, gfs_info->extent_root);
> >  
> >  		if (!ret)
> > 

Re: [PATCH] btrfs-progs: check: change commit condition in fixup_extent_refs()

[Nikolay Borisov]

On 4.11.21 г. 1:52, Sidong Yang wrote:
> On Wed, Nov 03, 2021 at 05:25:38PM +0200, Nikolay Borisov wrote:
>>
>>
>> On 3.11.21 г. 17:15, Sidong Yang wrote:
>>> This patch fixes potential bugs in fixup_extent_refs(). If
>>> btrfs_start_transaction() fails in some way and returns error ptr, It
>>> goes to out logic. But old code checkes whether it is null and it calls
>>> commit. This patch solves the problem with change the condition to
>>> checks if it is error by IS_ERR().
>>>
>>> Issue: #409
>>>
>>> Signed-off-by: Sidong Yang <realwakka@gmail.com>
>>> ---
>>>  check/main.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/check/main.c b/check/main.c
>>> index 235a9bab..ca858e07 100644
>>> --- a/check/main.c
>>> +++ b/check/main.c
>>> @@ -7735,7 +7735,7 @@ static int fixup_extent_refs(struct cache_tree *extent_cache,
>>>  			goto out;
>>>  	}
>>>  out:
>>> -	if (trans) {
>>> +	if (!IS_ERR(trans)) {
>>
>> Actually I think we want to commit the transaction only if ret is not
>> error, otherwise we run the risk of committing partial changes to the fs.
> 
> I agree. If ret is error, committing should not be happen. But I 
> think it needs to check trans. 
> 
> I wonder that if ret is error but trans is okay, trans needs to be
> aborted by calling btrfs_abort_transaction()?

This is the general way errors are handled in kernel. Currently abort
trans in progs simply sets transaction_aborted = error and
commit_transaction checks if this is set and returns an EROFS. Every
failure site in this function should ideally have a
btrfs_abort_transaction, because in the future we probably want to add
code which would yield the exact call site where a transaction abort
occurred.



>>
>>>  		int err = btrfs_commit_transaction(trans, gfs_info->extent_root);
>>>  
>>>  		if (!ret)
>>>
> 

Re: [PATCH] btrfs-progs: check: change commit condition in fixup_extent_refs()

[Sidong Yang]

On Thu, Nov 04, 2021 at 09:50:33AM +0200, Nikolay Borisov wrote:
> 
> 
> On 4.11.21 г. 1:52, Sidong Yang wrote:
> > On Wed, Nov 03, 2021 at 05:25:38PM +0200, Nikolay Borisov wrote:
> >>
> >>
> >> On 3.11.21 г. 17:15, Sidong Yang wrote:
> >>> This patch fixes potential bugs in fixup_extent_refs(). If
> >>> btrfs_start_transaction() fails in some way and returns error ptr, It
> >>> goes to out logic. But old code checkes whether it is null and it calls
> >>> commit. This patch solves the problem with change the condition to
> >>> checks if it is error by IS_ERR().
> >>>
> >>> Issue: #409
> >>>
> >>> Signed-off-by: Sidong Yang <realwakka@gmail.com>
> >>> ---
> >>>  check/main.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/check/main.c b/check/main.c
> >>> index 235a9bab..ca858e07 100644
> >>> --- a/check/main.c
> >>> +++ b/check/main.c
> >>> @@ -7735,7 +7735,7 @@ static int fixup_extent_refs(struct cache_tree *extent_cache,
> >>>  			goto out;
> >>>  	}
> >>>  out:
> >>> -	if (trans) {
> >>> +	if (!IS_ERR(trans)) {
> >>
> >> Actually I think we want to commit the transaction only if ret is not
> >> error, otherwise we run the risk of committing partial changes to the fs.
> > 
> > I agree. If ret is error, committing should not be happen. But I 
> > think it needs to check trans. 
> > 
> > I wonder that if ret is error but trans is okay, trans needs to be
> > aborted by calling btrfs_abort_transaction()?
> 
> This is the general way errors are handled in kernel. Currently abort
> trans in progs simply sets transaction_aborted = error and
> commit_transaction checks if this is set and returns an EROFS. Every
> failure site in this function should ideally have a
> btrfs_abort_transaction, because in the future we probably want to add
> code which would yield the exact call site where a transaction abort
> occurred.

Okay, Thanks for detailed description. I understood that we don't need
to call btrfs_abort_transaction() in the function.
I would write new version for this patch.

Thanks again for helping me.

Thanks,
Sidong

> 
> 
> 
> >>
> >>>  		int err = btrfs_commit_transaction(trans, gfs_info->extent_root);
> >>>  
> >>>  		if (!ret)
> >>>
> >