btrfs: fix BUG trying to resume balance without resume flag

PATCH Superseded Anand Jain 3 participants 5 messages Apr 30, 2018 – May 17, 2018 Btrfs file system

Share & tools

Anand Jain

author

2018-04-30 09:46 UTC #0 lore raw

PATCH v2 1/1 btrfs: fix BUG trying to resume balance without resume flag

We set the BTRFS_BALANCE_RESUME flag in the btrfs_recover_balance(), which is not called during the remount. So when resuming from the paused balance we hit BUG.

kernel: kernel BUG at fs/btrfs/volumes.c:3890!
::
kernel: balance_kthread+0x51/0x60 [btrfs]
kernel: kthread+0x111/0x130
::
kernel: RIP: btrfs_balance+0x12e1/0x1570 [btrfs] RSP: ffffba7d0090bde8

Reproducer:
On a mounted BTRFS.

  btrfs balance start --full-balance /btrfs
  btrfs balance pause /btrfs
  mount -o remount,ro /dev/sdb /btrfs
  mount -o remount,rw /dev/sdb /btrfs

To fix this set the BTRFS_BALANCE_RESUME flag in btrfs_resume_balance_async() instead of btrfs_recover_balance().

Signed-off-by Anand Jain <[email protected]>

---
v1->v2: btrfs_resume_balance_async() can be called only from remount or mount, we don't need to hold fs_info->balance_lock.

Strictly speaking we should rather keep the balance at the paused state unless it is resumed by the user again, that means neither mount nor remount-rw should resume the balance automatically, former case needs writing balance status to the disk. Which needs compatibility verification. So for now just avoid BUG.

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

fs/btrfs/volumes.c +2 -1

index 3e6983a169c4..64bcaf25908b 100644
@@ -4115,6 +4115,8 @@ int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info)
 		return 0;
 	}
 
+	fs_info->balance_ctl->b_flags |= BTRFS_BALANCE_RESUME;
+
 	tsk = kthread_run(balance_kthread, fs_info, "btrfs-balance");
 	return PTR_ERR_OR_ZERO(tsk);
 }

@@ -4156,7 +4158,6 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
 
 	bctl->fs_info = fs_info;
 	bctl->flags = btrfs_balance_flags(leaf, item);
-	bctl->flags |= BTRFS_BALANCE_RESUME;
 
 	btrfs_balance_data(leaf, item, &disk_bargs);
 	btrfs_disk_balance_args_to_cpu(&bctl->data, &disk_bargs);

Signature · 1 line

2.7.0

kbuild test robot

build bot

2018-04-30 14:12 UTC #1 lore raw

Thank you for the patch! Yet something to improve:

Hi Anand,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on btrfs/next]
[also build test ERROR on v4.17-rc3 next-20180430]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Anand-Jain/btrfs-fix-BUG-trying-to-resume-balance-without-resume-flag/20180430-192532
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mason/linux-btrfs.git next
config: i386-randconfig-a1-201817 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386

All errors (new ones prefixed by >>):

fs/btrfs/volumes.c: In function 'btrfs_resume_balance_async':

>> fs/btrfs/volumes.c:3971:22: error: 'struct btrfs_balance_control' has no member named 'b_flags'
     fs_info->balance_ctl->b_flags |= BTRFS_BALANCE_RESUME;
                         ^

vim +3971 fs/btrfs/volumes.c

3954
3955 int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info)
3956 {
3957 struct task_struct *tsk;
3958
3959 spin_lock(&fs_info->balance_lock);
3960 if (!fs_info->balance_ctl) {
3961 spin_unlock(&fs_info->balance_lock);
3962 return 0;
3963 }
3964 spin_unlock(&fs_info->balance_lock);
3965
3966 if (btrfs_test_opt(fs_info, SKIP_BALANCE)) {
3967 btrfs_info(fs_info, "force skipping balance");
3968 return 0;
3969 }
3970

> 3971		fs_info->balance_ctl->b_flags |= BTRFS_BALANCE_RESUME;

3972
3973 tsk = kthread_run(balance_kthread, fs_info, "btrfs-balance");
3974 return PTR_ERR_OR_ZERO(tsk);
3975 }
3976

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

kbuild test robot

build bot

2018-04-30 14:12 UTC #2 lore raw

In reply to Anand Jain

Thank you for the patch! Yet something to improve:

Hi Anand,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on btrfs/next]
[also build test ERROR on v4.17-rc3 next-20180430]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Anand-Jain/btrfs-fix-BUG-trying-to-resume-balance-without-resume-flag/20180430-192532
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mason/linux-btrfs.git next
config: i386-randconfig-x000-201817 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386

All errors (new ones prefixed by >>):

fs//btrfs/volumes.c: In function 'btrfs_resume_balance_async':

>> fs//btrfs/volumes.c:3971:24: error: 'struct btrfs_balance_control' has no member named 'b_flags'; did you mean 'flags'?
     fs_info->balance_ctl->b_flags |= BTRFS_BALANCE_RESUME;
                           ^~~~~~~
                           flags

vim +3971 fs//btrfs/volumes.c

> 3971		fs_info->balance_ctl->b_flags |= BTRFS_BALANCE_RESUME;

3972
3973 tsk = kthread_run(balance_kthread, fs_info, "btrfs-balance");
3974 return PTR_ERR_OR_ZERO(tsk);
3975 }
3976

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

David Sterba

2018-05-16 14:37 UTC #3 lore raw

In reply to Anand Jain

Quoted 19 lines · Anand Jain

We set the BTRFS_BALANCE_RESUME flag in the btrfs_recover_balance(), which is not called during the remount. So when resuming from the paused balance we hit BUG.
kernel: kernel BUG at fs/btrfs/volumes.c:3890!
::
kernel: balance_kthread+0x51/0x60 [btrfs]
kernel: kthread+0x111/0x130
::
kernel: RIP: btrfs_balance+0x12e1/0x1570 [btrfs] RSP: ffffba7d0090bde8
Reproducer:
On a mounted BTRFS.
  btrfs balance start --full-balance /btrfs
  btrfs balance pause /btrfs
  mount -o remount,ro /dev/sdb /btrfs
  mount -o remount,rw /dev/sdb /btrfs

Quoted 7 lines

To fix this set the BTRFS_BALANCE_RESUME flag in btrfs_resume_balance_async() instead of btrfs_recover_balance().
Signed-off-by Anand Jain <[email protected]>
---
v1->v2: btrfs_resume_balance_async() can be called only from remount or mount, we don't need to hold fs_info->balance_lock.

For mount it's ok, there's nothing that can run in parallel, but remount ro->rw that resumes the balance can be run with the ioctl that checks balance status in parallel, at any time. As the fs_info::balance_ctl is set up, btrfs_ioctl_balance_progress will proceed and return the current status.

Quoted 14 lines

Strictly speaking we should rather keep the balance at the paused state unless it is resumed by the user again, that means neither mount nor remount-rw should resume the balance automatically, former case needs writing balance status to the disk. Which needs compatibility verification. So for now just avoid BUG.
 fs/btrfs/volumes.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fs/btrfs/volumes.c +0 -0
index 3e6983a169c4..64bcaf25908b 100644

fs/btrfs/volumes.c int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info) · line 4118

Quoted 4 lines

fs/btrfs/volumes.c +1 -0

@@ -4115,6 +4115,8 @@ int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info)
 		return 0;
 	}
 
+	fs_info->balance_ctl->b_flags |= BTRFS_BALANCE_RESUME;

Though there's no update operation on flags, I think it's still better to add the locks that set the resume status. And a comment why it's here.

fs/btrfs/volumes.c int btrfs_recover_balance(struct btrfs_fs_info *fs_info) · line 4159

Quoted 9 lines

fs/btrfs/volumes.c +1 -1

+
 	tsk = kthread_run(balance_kthread, fs_info, "btrfs-balance");
 	return PTR_ERR_OR_ZERO(tsk);
 }

@@ -4156,7 +4158,6 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
 
 	bctl->fs_info = fs_info;
 	bctl->flags = btrfs_balance_flags(leaf, item);
-	bctl->flags |= BTRFS_BALANCE_RESUME;

Also, it does not hurt to leave this here, as it logically matches what the function does: we found the balance item, thus we set the status accordingly.

Please update and resend, I'd like to push this to 4.17-rc as it's a crash fix with a reproducer. Thanks.

Anand Jain

author

2018-05-17 07:14 UTC #4 lore raw

Quoted 28 lines · David Sterba

Quoted 19 lines · Anand Jain
We set the BTRFS_BALANCE_RESUME flag in the btrfs_recover_balance(), which is not called during the remount. So when resuming from the paused balance we hit BUG.
kernel: kernel BUG at fs/btrfs/volumes.c:3890!
::
kernel: balance_kthread+0x51/0x60 [btrfs]
kernel: kthread+0x111/0x130
::
kernel: RIP: btrfs_balance+0x12e1/0x1570 [btrfs] RSP: ffffba7d0090bde8
Reproducer:
On a mounted BTRFS.
   btrfs balance start --full-balance /btrfs
   btrfs balance pause /btrfs
   mount -o remount,ro /dev/sdb /btrfs
   mount -o remount,rw /dev/sdb /btrfs
Quoted 7 lines
To fix this set the BTRFS_BALANCE_RESUME flag in btrfs_resume_balance_async() instead of btrfs_recover_balance().
Signed-off-by Anand Jain <[email protected]>
---
v1->v2: btrfs_resume_balance_async() can be called only from remount or mount, we don't need to hold fs_info->balance_lock.

Quoted 5 lines

For mount it's ok, there's nothing that can run in parallel, but remount ro->rw that resumes the balance can be run with the ioctl that checks balance status in parallel, at any time. As the fs_info::balance_ctl is set up, btrfs_ioctl_balance_progress will proceed and return the current status.

You are right.

Quoted 17 lines

Quoted 17 lines
Strictly speaking we should rather keep the balance at the paused state unless it is resumed by the user again, that means neither mount nor remount-rw should resume the balance automatically, former case needs writing balance status to the disk. Which needs compatibility verification. So for now just avoid BUG.
  fs/btrfs/volumes.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
fs/btrfs/volumes.c +0 -0
index 3e6983a169c4..64bcaf25908b 100644
@@ -4115,6 +4115,8 @@ int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info)
  		return 0;
  	}
 

Quoted 5 lines

fs/btrfs/volumes.c int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info) · line 4118
Quoted 1 line
fs/btrfs/volumes.c +1 -0
+	fs_info->balance_ctl->b_flags |= BTRFS_BALANCE_RESUME;
Though there's no update operation on flags, I think it's still better to add the locks that set the resume status. And a comment why it's here.

ok.

Quoted 6 lines

fs/btrfs/volumes.c +1 -0

+
  	tsk = kthread_run(balance_kthread, fs_info, "btrfs-balance");
  	return PTR_ERR_OR_ZERO(tsk);
  }

@@ -4156,7 +4158,6 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)

Quoted 7 lines

fs/btrfs/volumes.c int btrfs_recover_balance(struct btrfs_fs_info *fs_info) · line 4159
Quoted 3 lines
fs/btrfs/volumes.c +0 -1
  	bctl->fs_info = fs_info;
  	bctl->flags = btrfs_balance_flags(leaf, item);
-	bctl->flags |= BTRFS_BALANCE_RESUME;
Also, it does not hurt to leave this here, as it logically matches what the function does: we found the balance item, thus we set the status accordingly.

ok.

Quoted 2 lines

Please update and resend, I'd like to push this to 4.17-rc as it's a crash fix with a reproducer. Thanks.

Sent v3 with the above changes.

Thanks, Anand

Quoted 5 lines

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