* [PATCH v3 0/2] fix bug in btrfs_init_new_device()
@ 2017-09-26 8:47 Anand Jain
2017-09-26 8:47 ` [PATCH v3 1/2] btrfs: undo writable when sprouting fails Anand Jain
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Anand Jain @ 2017-09-26 8:47 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v3 1/2] btrfs: undo writable when sprouting fails
2017-09-26 8:47 [PATCH v3 0/2] fix bug in btrfs_init_new_device() Anand Jain
@ 2017-09-26 8:47 ` Anand Jain
2017-09-26 8:47 ` [PATCH v3 2/2] btrfs: fix BUG_ON in btrfs_init_new_device() Anand Jain
2017-09-26 12:14 ` [PATCH v3 0/2] fix bug " Nikolay Borisov
2 siblings, 0 replies; 8+ messages in thread
From: Anand Jain @ 2017-09-26 8:47 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3 2/2] btrfs: fix BUG_ON in btrfs_init_new_device()
2017-09-26 8:47 [PATCH v3 0/2] fix bug in btrfs_init_new_device() Anand Jain
2017-09-26 8:47 ` [PATCH v3 1/2] btrfs: undo writable when sprouting fails Anand Jain
@ 2017-09-26 8:47 ` Anand Jain
2017-09-26 12:14 ` [PATCH v3 0/2] fix bug " Nikolay Borisov
2 siblings, 0 replies; 8+ messages in thread
From: Anand Jain @ 2017-09-26 8:47 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v3 0/2] fix bug in btrfs_init_new_device()
2017-09-26 8:47 [PATCH v3 0/2] fix bug in btrfs_init_new_device() Anand Jain
2017-09-26 8:47 ` [PATCH v3 1/2] btrfs: undo writable when sprouting fails Anand Jain
2017-09-26 8:47 ` [PATCH v3 2/2] btrfs: fix BUG_ON in btrfs_init_new_device() Anand Jain
@ 2017-09-26 12:14 ` Nikolay Borisov
2017-09-27 14:22 ` David Sterba
2 siblings, 1 reply; 8+ messages in thread
From: Nikolay Borisov @ 2017-09-26 12:14 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
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(-)
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 0/2] fix bug in btrfs_init_new_device()
2017-09-26 12:14 ` [PATCH v3 0/2] fix bug " Nikolay Borisov
@ 2017-09-27 14:22 ` David Sterba
2017-09-27 14:26 ` Nikolay Borisov
0 siblings, 1 reply; 8+ messages in thread
From: David Sterba @ 2017-09-27 14:22 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: Anand Jain, linux-btrfs
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 0/2] fix bug in btrfs_init_new_device()
2017-09-27 14:22 ` David Sterba
@ 2017-09-27 14:26 ` Nikolay Borisov
2017-09-28 6:55 ` Anand Jain
0 siblings, 1 reply; 8+ messages in thread
From: Nikolay Borisov @ 2017-09-27 14:26 UTC (permalink / raw)
To: dsterba, Anand Jain, linux-btrfs
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.
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 0/2] fix bug in btrfs_init_new_device()
2017-09-27 14:26 ` Nikolay Borisov
@ 2017-09-28 6:55 ` Anand Jain
0 siblings, 0 replies; 8+ messages in thread
From: Anand Jain @ 2017-09-28 6:55 UTC (permalink / raw)
To: Nikolay Borisov, dsterba, linux-btrfs
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 0/2] fix bug in btrfs_init_new_device()
@ 2017-09-26 8:41 Anand Jain
0 siblings, 0 replies; 8+ messages in thread
From: Anand Jain @ 2017-09-26 8:41 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-09-28 6:56 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-26 8:47 [PATCH v3 0/2] fix bug in btrfs_init_new_device() Anand Jain
2017-09-26 8:47 ` [PATCH v3 1/2] btrfs: undo writable when sprouting fails Anand Jain
2017-09-26 8:47 ` [PATCH v3 2/2] btrfs: fix BUG_ON in btrfs_init_new_device() Anand Jain
2017-09-26 12:14 ` [PATCH v3 0/2] fix bug " Nikolay Borisov
2017-09-27 14:22 ` David Sterba
2017-09-27 14:26 ` Nikolay Borisov
2017-09-28 6:55 ` Anand Jain
-- strict thread matches above, loose matches on Subject: below --
2017-09-26 8:41 Anand Jain
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).