[PATCH v3 2/2] btrfs: fix BUG_ON in btrfs_init_new_device()

[PATCH v3 2/2] btrfs: fix BUG_ON in btrfs_init_new_device()

[Anand Jain]

Instead of BUG_ON return error to the caller. And handle the fail
condition by calling the abort transaction and going through the
error path.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9d64700cc9b6..4cb575fbf643 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2399,7 +2399,10 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	if (seeding_dev) {
 		sb->s_flags &= ~MS_RDONLY;
 		ret = btrfs_prepare_sprout(fs_info);
-		BUG_ON(ret); /* -ENOMEM */
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
+			goto error_trans;
+		}
 	}
 
 	device->fs_devices = fs_info->fs_devices;
@@ -2445,14 +2448,14 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		mutex_unlock(&fs_info->chunk_mutex);
 		if (ret) {
 			btrfs_abort_transaction(trans, ret);
-			goto error_trans;
+			goto error_sysfs;
 		}
 	}
 
 	ret = btrfs_add_device(trans, fs_info, device);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
-		goto error_trans;
+		goto error_sysfs;
 	}
 
 	if (seeding_dev) {
@@ -2461,7 +2464,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		ret = btrfs_finish_sprout(trans, fs_info);
 		if (ret) {
 			btrfs_abort_transaction(trans, ret);
-			goto error_trans;
+			goto error_sysfs;
 		}
 
 		/* Sprouting would change fsid of the mounted root,
@@ -2500,12 +2503,13 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	update_dev_time(device_path);
 	return ret;
 
+error_sysfs:
+	btrfs_sysfs_rm_device_link(fs_info->fs_devices, device);
 error_trans:
 	if (seeding_dev)
 		sb->s_flags |= MS_RDONLY;
 	btrfs_end_transaction(trans);
 	rcu_string_free(device->name);
-	btrfs_sysfs_rm_device_link(fs_info->fs_devices, device);
 	kfree(device);
 error:
 	blkdev_put(bdev, FMODE_EXCL);
-- 
2.13.1

[PATCH v3 0/2] fix bug in btrfs_init_new_device()

[Anand Jain]

1/2 fixes a bug which failed to reset writable when sprouting failed
2/2 fixes BUG_ON in btrfs_init_new_device()

Anand Jain (2):
  btrfs: undo writable when sprouting fails
  btrfs: fix BUG_ON in btrfs_init_new_device()

 fs/btrfs/volumes.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

-- 
2.13.1

[PATCH v3 1/2] btrfs: undo writable when sprouting fails

[Anand Jain]

When new device is being added to seed FS, seed FS is marked writable,
but when we fail to bring in the new device, we missed to undo the
writable part. This patch fixes it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3: none
v2: add commit log

 fs/btrfs/volumes.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0e8f16c305df..9d64700cc9b6 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2501,6 +2501,8 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	return ret;
 
 error_trans:
+	if (seeding_dev)
+		sb->s_flags |= MS_RDONLY;
 	btrfs_end_transaction(trans);
 	rcu_string_free(device->name);
 	btrfs_sysfs_rm_device_link(fs_info->fs_devices, device);
-- 
2.13.1

[PATCH v3 2/2] btrfs: fix BUG_ON in btrfs_init_new_device()

[Anand Jain]

Instead of BUG_ON return error to the caller. And handle the fail
condition by calling the abort transaction and going through the
error path.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: do not consolidate btrfs_abort_transaction()
v3: meld 2/3 and 3/3 from v2

 fs/btrfs/volumes.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9d64700cc9b6..4cb575fbf643 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2399,7 +2399,10 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	if (seeding_dev) {
 		sb->s_flags &= ~MS_RDONLY;
 		ret = btrfs_prepare_sprout(fs_info);
-		BUG_ON(ret); /* -ENOMEM */
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
+			goto error_trans;
+		}
 	}
 
 	device->fs_devices = fs_info->fs_devices;
@@ -2445,14 +2448,14 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		mutex_unlock(&fs_info->chunk_mutex);
 		if (ret) {
 			btrfs_abort_transaction(trans, ret);
-			goto error_trans;
+			goto error_sysfs;
 		}
 	}
 
 	ret = btrfs_add_device(trans, fs_info, device);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
-		goto error_trans;
+		goto error_sysfs;
 	}
 
 	if (seeding_dev) {
@@ -2461,7 +2464,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 		ret = btrfs_finish_sprout(trans, fs_info);
 		if (ret) {
 			btrfs_abort_transaction(trans, ret);
-			goto error_trans;
+			goto error_sysfs;
 		}
 
 		/* Sprouting would change fsid of the mounted root,
@@ -2500,12 +2503,13 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	update_dev_time(device_path);
 	return ret;
 
+error_sysfs:
+	btrfs_sysfs_rm_device_link(fs_info->fs_devices, device);
 error_trans:
 	if (seeding_dev)
 		sb->s_flags |= MS_RDONLY;
 	btrfs_end_transaction(trans);
 	rcu_string_free(device->name);
-	btrfs_sysfs_rm_device_link(fs_info->fs_devices, device);
 	kfree(device);
 error:
 	blkdev_put(bdev, FMODE_EXCL);
-- 
2.13.1

Re: [PATCH v3 0/2] fix bug in btrfs_init_new_device()

[Nikolay Borisov]

On 26.09.2017 11:47, Anand Jain wrote:
> 1/2 fixes a bug which failed to reset writable when sprouting failed
> 2/2 fixes BUG_ON in btrfs_init_new_device()
> 
> Anand Jain (2):
>   btrfs: undo writable when sprouting fails
>   btrfs: fix BUG_ON in btrfs_init_new_device()

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> 
>  fs/btrfs/volumes.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 

Re: [PATCH v3 2/2] btrfs: fix BUG_ON in btrfs_init_new_device()

[Qu Wenruo]

On 2017年09月26日 16:41, Anand Jain wrote:
> Instead of BUG_ON return error to the caller. And handle the fail
> condition by calling the abort transaction and going through the
> error path.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

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

Thanks,
Qu

> ---
>   fs/btrfs/volumes.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 9d64700cc9b6..4cb575fbf643 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2399,7 +2399,10 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>   	if (seeding_dev) {
>   		sb->s_flags &= ~MS_RDONLY;
>   		ret = btrfs_prepare_sprout(fs_info);
> -		BUG_ON(ret); /* -ENOMEM */
> +		if (ret) {
> +			btrfs_abort_transaction(trans, ret);
> +			goto error_trans;
> +		}
>   	}
>   
>   	device->fs_devices = fs_info->fs_devices;
> @@ -2445,14 +2448,14 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>   		mutex_unlock(&fs_info->chunk_mutex);
>   		if (ret) {
>   			btrfs_abort_transaction(trans, ret);
> -			goto error_trans;
> +			goto error_sysfs;
>   		}
>   	}
>   
>   	ret = btrfs_add_device(trans, fs_info, device);
>   	if (ret) {
>   		btrfs_abort_transaction(trans, ret);
> -		goto error_trans;
> +		goto error_sysfs;
>   	}
>   
>   	if (seeding_dev) {
> @@ -2461,7 +2464,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>   		ret = btrfs_finish_sprout(trans, fs_info);
>   		if (ret) {
>   			btrfs_abort_transaction(trans, ret);
> -			goto error_trans;
> +			goto error_sysfs;
>   		}
>   
>   		/* Sprouting would change fsid of the mounted root,
> @@ -2500,12 +2503,13 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>   	update_dev_time(device_path);
>   	return ret;
>   
> +error_sysfs:
> +	btrfs_sysfs_rm_device_link(fs_info->fs_devices, device);
>   error_trans:
>   	if (seeding_dev)
>   		sb->s_flags |= MS_RDONLY;
>   	btrfs_end_transaction(trans);
>   	rcu_string_free(device->name);
> -	btrfs_sysfs_rm_device_link(fs_info->fs_devices, device);
>   	kfree(device);
>   error:
>   	blkdev_put(bdev, FMODE_EXCL);
> 

Re: [PATCH v3 0/2] fix bug in btrfs_init_new_device()

[David Sterba]

On Tue, Sep 26, 2017 at 03:14:27PM +0300, Nikolay Borisov wrote:
> 
> 
> On 26.09.2017 11:47, Anand Jain wrote:
> > 1/2 fixes a bug which failed to reset writable when sprouting failed
> > 2/2 fixes BUG_ON in btrfs_init_new_device()
> > 
> > Anand Jain (2):
> >   btrfs: undo writable when sprouting fails
> >   btrfs: fix BUG_ON in btrfs_init_new_device()
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Please note that this would lead to unexpected unlocks of uuid_mutex and
sb::s_umount:

2465         ret = btrfs_commit_transaction(trans);
2466
2467         if (seeding_dev) {
2468                 mutex_unlock(&uuid_mutex);
2469                 up_write(&sb->s_umount);

first unlocks

2470
2471                 if (ret) /* transaction commit */
2472                         return ret;
2473
2474                 ret = btrfs_relocate_sys_chunks(fs_info);
2475                 if (ret < 0)
2476                         btrfs_handle_fs_error(fs_info, ret,
2477                                     "Failed to relocate sys chunks after device initialization. This can be fixed using the \"btrfs balance\"
2478                 trans = btrfs_attach_transaction(root);
2479                 if (IS_ERR(trans)) {
2480                         if (PTR_ERR(trans) == -ENOENT)
2481                                 return 0;
2482                         return PTR_ERR(trans);
^^^^

this becomes goto 2494

2483                 }
2484                 ret = btrfs_commit_transaction(trans);
2485         }
2486
2487         /* Update ctime/mtime for libblkid */
2488         update_dev_time(device_path);
2489         return ret;
2490
2491 error_trans:
2492         btrfs_end_transaction(trans);
2493         rcu_string_free(device->name);
2494         btrfs_sysfs_rm_device_link(fs_info->fs_devices, device);

here

2495         kfree(device);
2496 error:
2497         blkdev_put(bdev, FMODE_EXCL);
2498         if (seeding_dev) {
2499                 mutex_unlock(&uuid_mutex);
2500                 up_write(&sb->s_umount);

and unlocking again

2501         }
2502         return ret;

So the in-place returns had a meaning but are quite confusing.

Re: [PATCH v3 0/2] fix bug in btrfs_init_new_device()

[Nikolay Borisov]

On 27.09.2017 17:22, David Sterba wrote:
> On Tue, Sep 26, 2017 at 03:14:27PM +0300, Nikolay Borisov wrote:
>>
>>
>> On 26.09.2017 11:47, Anand Jain wrote:
>>> 1/2 fixes a bug which failed to reset writable when sprouting failed
>>> 2/2 fixes BUG_ON in btrfs_init_new_device()
>>>
>>> Anand Jain (2):
>>>   btrfs: undo writable when sprouting fails
>>>   btrfs: fix BUG_ON in btrfs_init_new_device()
>>
>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
> Please note that this would lead to unexpected unlocks of uuid_mutex and
> sb::s_umount:
> 
> 2465         ret = btrfs_commit_transaction(trans);
> 2466
> 2467         if (seeding_dev) {
> 2468                 mutex_unlock(&uuid_mutex);
> 2469                 up_write(&sb->s_umount);
> 
> first unlocks
> 
> 2470
> 2471                 if (ret) /* transaction commit */
> 2472                         return ret;
> 2473
> 2474                 ret = btrfs_relocate_sys_chunks(fs_info);
> 2475                 if (ret < 0)
> 2476                         btrfs_handle_fs_error(fs_info, ret,
> 2477                                     "Failed to relocate sys chunks after device initialization. This can be fixed using the \"btrfs balance\"
> 2478                 trans = btrfs_attach_transaction(root);
> 2479                 if (IS_ERR(trans)) {
> 2480                         if (PTR_ERR(trans) == -ENOENT)
> 2481                                 return 0;
> 2482                         return PTR_ERR(trans);
> ^^^^
> 
> this becomes goto 2494
> 
> 2483                 }
> 2484                 ret = btrfs_commit_transaction(trans);
> 2485         }
> 2486
> 2487         /* Update ctime/mtime for libblkid */
> 2488         update_dev_time(device_path);
> 2489         return ret;
> 2490
> 2491 error_trans:
> 2492         btrfs_end_transaction(trans);
> 2493         rcu_string_free(device->name);
> 2494         btrfs_sysfs_rm_device_link(fs_info->fs_devices, device);
> 
> here
> 
> 2495         kfree(device);
> 2496 error:
> 2497         blkdev_put(bdev, FMODE_EXCL);
> 2498         if (seeding_dev) {
> 2499                 mutex_unlock(&uuid_mutex);
> 2500                 up_write(&sb->s_umount);
> 
> and unlocking again
> 
> 2501         }
> 2502         return ret;
> 
> So the in-place returns had a meaning but are quite confusing.

I concur and had missed that ;(. I saw that you suggested Anand to
overhaul the overall error handling in this function and I believe this
would be the best course of action.

> 

Re: [PATCH v3 0/2] fix bug in btrfs_init_new_device()

[Anand Jain]

On 09/27/2017 10:26 PM, Nikolay Borisov wrote:
> 
> 
> On 27.09.2017 17:22, David Sterba wrote:
>> On Tue, Sep 26, 2017 at 03:14:27PM +0300, Nikolay Borisov wrote:
>>>
>>>
>>> On 26.09.2017 11:47, Anand Jain wrote:
>>>> 1/2 fixes a bug which failed to reset writable when sprouting failed
>>>> 2/2 fixes BUG_ON in btrfs_init_new_device()
>>>>
>>>> Anand Jain (2):
>>>>    btrfs: undo writable when sprouting fails
>>>>    btrfs: fix BUG_ON in btrfs_init_new_device()
>>>
>>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>>
>> Please note that this would lead to unexpected unlocks of uuid_mutex and
>> sb::s_umount:
>>
>> 2465         ret = btrfs_commit_transaction(trans);
>> 2466
>> 2467         if (seeding_dev) {
>> 2468                 mutex_unlock(&uuid_mutex);
>> 2469                 up_write(&sb->s_umount);
>>
>> first unlocks
>>
>> 2470
>> 2471                 if (ret) /* transaction commit */
>> 2472                         return ret;
>> 2473
>> 2474                 ret = btrfs_relocate_sys_chunks(fs_info);
>> 2475                 if (ret < 0)
>> 2476                         btrfs_handle_fs_error(fs_info, ret,
>> 2477                                     "Failed to relocate sys chunks after device initialization. This can be fixed using the \"btrfs balance\"
>> 2478                 trans = btrfs_attach_transaction(root);
>> 2479                 if (IS_ERR(trans)) {
>> 2480                         if (PTR_ERR(trans) == -ENOENT)
>> 2481                                 return 0;
>> 2482                         return PTR_ERR(trans);
>> ^^^^
>>
>> this becomes goto 2494
>>
>> 2483                 }
>> 2484                 ret = btrfs_commit_transaction(trans);
>> 2485         }
>> 2486
>> 2487         /* Update ctime/mtime for libblkid */
>> 2488         update_dev_time(device_path);
>> 2489         return ret;
>> 2490
>> 2491 error_trans:
>> 2492         btrfs_end_transaction(trans);
>> 2493         rcu_string_free(device->name);
>> 2494         btrfs_sysfs_rm_device_link(fs_info->fs_devices, device);
>>
>> here
>>
>> 2495         kfree(device);
>> 2496 error:
>> 2497         blkdev_put(bdev, FMODE_EXCL);
>> 2498         if (seeding_dev) {
>> 2499                 mutex_unlock(&uuid_mutex);
>> 2500                 up_write(&sb->s_umount);
>>
>> and unlocking again
>>
>> 2501         }
>> 2502         return ret;
>>
>> So the in-place returns had a meaning but are quite confusing.

  Oops. I forgot about it though I noticed, will fix it.

> I concur and had missed that ;(. 

  No you didn't, the changes which David is talking about isn't in this 
set. It was independent patch: But, my fault, to confuse about the patch 
set.
    [PATCH] btrfs: take the error path out if btrfs_attach_transaction() 
fails

V4 is sent to clear this.

Thanks, Anand


> I saw that you suggested Anand to
> overhaul the overall error handling in this function and I believe this
> would be the best course of action.