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

[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>
---
 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>
---
 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 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

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

[Qu Wenruo]

On 2017年09月26日 16:41, Anand Jain wrote:
> 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>
> ---
>   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;

Still the same question.

---
	if (seeding_dev) {
		mutex_unlock(&uuid_mutex);
		up_write(&sb->s_umount);

		if (ret) /* transaction commit */
			return ret;

		ret = btrfs_relocate_sys_chunks(fs_info);
		if (ret < 0)
			btrfs_handle_fs_error(fs_info, ret,
				    "Failed to relocate sys chunks after device initialization. This 
can be fixed using the \"btrfs balance\" command.");
		trans = btrfs_attach_transaction(root);
		if (IS_ERR(trans)) {
			if (PTR_ERR(trans) == -ENOENT)
				return 0;
			return PTR_ERR(trans);
		}
---
In the above btrfs_attch_transaction() error handler, which just 
returned without going to error_trans tags, did we misseed the s_flag 
revert thing?

Thanks,
Qu

>   	btrfs_end_transaction(trans);
>   	rcu_string_free(device->name);
>   	btrfs_sysfs_rm_device_link(fs_info->fs_devices, device);
> 

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 1/2] btrfs: undo writable when sprouting fails

[David Sterba]

On Tue, Sep 26, 2017 at 08:57:47PM +0800, Qu Wenruo wrote:
> 
> 
> On 2017年09月26日 16:41, Anand Jain wrote:
> > 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>
> > ---
> >   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;
> 
> Still the same question.
> 
> ---
> 	if (seeding_dev) {
> 		mutex_unlock(&uuid_mutex);
> 		up_write(&sb->s_umount);
> 
> 		if (ret) /* transaction commit */
> 			return ret;
> 
> 		ret = btrfs_relocate_sys_chunks(fs_info);
> 		if (ret < 0)
> 			btrfs_handle_fs_error(fs_info, ret,
> 				    "Failed to relocate sys chunks after device initialization. This 
> can be fixed using the \"btrfs balance\" command.");
> 		trans = btrfs_attach_transaction(root);
> 		if (IS_ERR(trans)) {
> 			if (PTR_ERR(trans) == -ENOENT)
> 				return 0;
> 			return PTR_ERR(trans);
> 		}
> ---
> In the above btrfs_attch_transaction() error handler, which just 
> returned without going to error_trans tags, did we misseed the s_flag 
> revert thing?

I think so. Also I think "sb->s_flags |= MS_RDONLY;" can be moved to the
'if' after the error: label, where we already check the 'seeding_dev'
condition.

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

[Anand Jain]

On 09/27/2017 01:49 AM, David Sterba wrote:
> On Tue, Sep 26, 2017 at 08:57:47PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2017年09月26日 16:41, Anand Jain wrote:
>>> 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>
>>> ---
>>>    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;
>>
>> Still the same question.
>>
>> ---
>> 	if (seeding_dev) {
>> 		mutex_unlock(&uuid_mutex);
>> 		up_write(&sb->s_umount);
>>
>> 		if (ret) /* transaction commit */
>> 			return ret;
>>
>> 		ret = btrfs_relocate_sys_chunks(fs_info);
>> 		if (ret < 0)
>> 			btrfs_handle_fs_error(fs_info, ret,
>> 				    "Failed to relocate sys chunks after device initialization. This
>> can be fixed using the \"btrfs balance\" command.");
>> 		trans = btrfs_attach_transaction(root);
>> 		if (IS_ERR(trans)) {
>> 			if (PTR_ERR(trans) == -ENOENT)
>> 				return 0;
>> 			return PTR_ERR(trans);
>> 		}
>> ---
>> In the above btrfs_attch_transaction() error handler, which just
>> returned without going to error_trans tags, did we misseed the s_flag
>> revert thing?
> 
> I think so. Also I think "sb->s_flags |= MS_RDONLY;" can be moved to the
> 'if' after the error: label, where we already check the 'seeding_dev'
> condition.

Qu, David,

  Thanks. I have fixed that in a new patch as its wasn't about fixing 
the BUG_ON().

  Further, there is another bug that, in the original code we failed to 
undo the btrfs_prepare_sprout() part. I am attempting to fix that, its 
bit complicated. For now, fix for Qu found bug is sent to the ML.

-Anand