[PATCH] btrfs: take the error path out if btrfs_attach_transaction() fails

[PATCH] btrfs: take the error path out if btrfs_attach_transaction() fails

[Anand Jain]

btrfs_init_new_device() calls btrfs_attach_transaction() to
commit sys chunks, however take the error path out if it fails.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index fad3b10a1f81..b526d13a74da 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2494,7 +2494,8 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		if (IS_ERR(trans)) {
 			if (PTR_ERR(trans) == -ENOENT)
 				return 0;
-			return PTR_ERR(trans);
+			ret = PTR_ERR(trans);
+			goto error_sysfs;
 		}
 		ret = btrfs_commit_transaction(trans);
 	}
-- 
2.13.1

Re: [PATCH] btrfs: take the error path out if btrfs_attach_transaction() fails

[Qu Wenruo]

On 2017年09月27日 17:50, Anand Jain wrote:
> btrfs_init_new_device() calls btrfs_attach_transaction() to
> commit sys chunks, however take the error path out if it fails.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>

Thanks,
Qu

> ---
>   fs/btrfs/volumes.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index fad3b10a1f81..b526d13a74da 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2494,7 +2494,8 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>   		if (IS_ERR(trans)) {
>   			if (PTR_ERR(trans) == -ENOENT)
>   				return 0;
> -			return PTR_ERR(trans);
> +			ret = PTR_ERR(trans);
> +			goto error_sysfs;
>   		}
>   		ret = btrfs_commit_transaction(trans);
>   	}
> 

Re: [PATCH] btrfs: take the error path out if btrfs_attach_transaction() fails

[David Sterba]

On Wed, Sep 27, 2017 at 05:50:52PM +0800, Anand Jain wrote:
> btrfs_init_new_device() calls btrfs_attach_transaction() to
> commit sys chunks, however take the error path out if it fails.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index fad3b10a1f81..b526d13a74da 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2494,7 +2494,8 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>  		if (IS_ERR(trans)) {
>  			if (PTR_ERR(trans) == -ENOENT)
>  				return 0;
> -			return PTR_ERR(trans);
> +			ret = PTR_ERR(trans);
> +			goto error_sysfs;

The label is introduced by another patch, please resend the whole
patchset, I've seen several iterations and feedback from various people
and I'm not sure I'm looking at the latest version.

Regarding error handling in btrfs_init_new_device, the seeding device
makes it hard to read. This patch would lead to a double unlock of
uuid_mutex and sb::s_umount, because the label error_sysfs will continue
to do the cleanups, that were already partially done in the containing
'if (seeding_dev)' block where the test fails.

I'd suggest to first get rid of the in-place returns and add necessary
labels or separate exit sequences and then address the new error
handling.

Re: [PATCH] btrfs: take the error path out if btrfs_attach_transaction() fails

[Anand Jain]

On 09/27/2017 10:17 PM, David Sterba wrote:
> On Wed, Sep 27, 2017 at 05:50:52PM +0800, Anand Jain wrote:
>> btrfs_init_new_device() calls btrfs_attach_transaction() to
>> commit sys chunks, however take the error path out if it fails.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index fad3b10a1f81..b526d13a74da 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -2494,7 +2494,8 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>>   		if (IS_ERR(trans)) {
>>   			if (PTR_ERR(trans) == -ENOENT)
>>   				return 0;
>> -			return PTR_ERR(trans);
>> +			ret = PTR_ERR(trans);
>> +			goto error_sysfs;
> 
> The label is introduced by another patch, please resend the whole
> patchset, I've seen several iterations and feedback from various people
> and I'm not sure I'm looking at the latest version.

  Pls consider V4, in the ML.

> Regarding error handling in btrfs_init_new_device, the seeding device
> makes it hard to read. This patch would lead to a double unlock of
> uuid_mutex and sb::s_umount, because the label error_sysfs will continue
> to do the cleanups, that were already partially done in the containing
> 'if (seeding_dev)' block where the test fails.

  Fixed this in
    [PATCH v4 3/3] btrfs: error out if btrfs_attach_transaction() fails

> I'd suggest to first get rid of the in-place returns and add necessary
> labels or separate exit sequences and then address the new error
> handling.

   As it goes deeper there are quite a number of things which aren't
   un-done during fail error return .. adding one more to the list
   is sb->super_copy updates. With this current design on this function
   its kind of too difficult to undo and error return. As
   btrfs_init_new_device() is shared between normal device add and
   sprout device add. I am mulling on completely removing seed part
   to outside of the btrfs_init_new_device(). such as ..
         prepare sprout.
         ret = btrfs_init_new_device() which is without the seed part
         if(ret) undo_prepare_sprout
         else finish sprouting.
    Also with this I think we would find few duplicate code sections
    between btrfs_init_new_device() and replace device which will be
    a nice cleanup as a whole. This is a long term plan, for now
    I think v4 is good.

Thanks, Anand


> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] btrfs: take the error path out if btrfs_attach_transaction() fails

[David Sterba]

On Thu, Sep 28, 2017 at 03:13:06PM +0800, Anand Jain wrote:
> > I'd suggest to first get rid of the in-place returns and add necessary
> > labels or separate exit sequences and then address the new error
> > handling.
> 
>    As it goes deeper there are quite a number of things which aren't
>    un-done during fail error return .. adding one more to the list
>    is sb->super_copy updates. With this current design on this function
>    its kind of too difficult to undo and error return. As
>    btrfs_init_new_device() is shared between normal device add and
>    sprout device add. I am mulling on completely removing seed part
>    to outside of the btrfs_init_new_device(). such as ..
>          prepare sprout.
>          ret = btrfs_init_new_device() which is without the seed part
>          if(ret) undo_prepare_sprout
>          else finish sprouting.

Yeah, this would make the code better structured.

>     Also with this I think we would find few duplicate code sections
>     between btrfs_init_new_device() and replace device which will be
>     a nice cleanup as a whole. This is a long term plan, for now
>     I think v4 is good.

Ok, thanks.