Re: [PATCH v2 2/4] btrfs: mark device addition as mnt_want_write_file

[Filipe Manana <fdmanana@kernel.org>]

On Wed, Mar 16, 2022 at 10:22:38PM +0900, Naohiro Aota wrote:
> btrfs_init_new_device() calls btrfs_relocate_sys_chunk() which incurs
> file-system internal writing. That writing can cause a deadlock with
> FS freezing like as described in like as described in commit
> 26559780b953 ("btrfs: zoned: mark relocation as writing").
> 
> Mark the device addition as mnt_want_write_file. This is also consistent
> with the removing device ioctl counterpart.
> 
> Cc: stable@vger.kernel.org # 4.9+
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>  fs/btrfs/ioctl.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 60c907b14547..a6982a1fde65 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3474,8 +3474,10 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
>  	return ret;
>  }
>  
> -static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg)
> +static long btrfs_ioctl_add_dev(struct file *file, void __user *arg)
>  {
> +	struct inode *inode = file_inode(file);
> +	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>  	struct btrfs_ioctl_vol_args *vol_args;
>  	bool restore_op = false;
>  	int ret;
> @@ -3488,6 +3490,10 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg)
>  		return -EINVAL;
>  	}
>  
> +	ret = mnt_want_write_file(file);
> +	if (ret)
> +		return ret;

So, this now breaks all test cases that exercise device seeding, and I clearly
forgot about seeding when I asked about why not use mnt_want_write_file()
instead of a bare call to sb_start_write():

$ ./check btrfs/161 btrfs/162 btrfs/163 btrfs/164 btrfs/248
FSTYP         -- btrfs
PLATFORM      -- Linux/x86_64 debian9 5.17.0-rc8-btrfs-next-114 #2 SMP PREEMPT Wed Mar 16 14:10:07 WET 2022
MKFS_OPTIONS  -- /dev/sdc
MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1

btrfs/161 1s ... [failed, exit status 1]- output mismatch (see /home/fdmanana/git/hub/xfstests/results//btrfs/161.out.bad)
    --- tests/btrfs/161.out	2020-06-10 19:29:03.822519250 +0100
    +++ /home/fdmanana/git/hub/xfstests/results//btrfs/161.out.bad	2022-03-16 15:48:10.835678228 +0000
    @@ -3,7 +3,3 @@
     0000000 abab abab abab abab abab abab abab abab
     *
     1000000
    --- sprout --
    -0000000 abab abab abab abab abab abab abab abab
    -*
    -1000000
    ...
    (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/161.out /home/fdmanana/git/hub/xfstests/results//btrfs/161.out.bad'  to see the entire diff)
btrfs/162 1s ... [failed, exit status 1]- output mismatch (see /home/fdmanana/git/hub/xfstests/results//btrfs/162.out.bad)
    --- tests/btrfs/162.out	2020-06-10 19:29:03.822519250 +0100
    +++ /home/fdmanana/git/hub/xfstests/results//btrfs/162.out.bad	2022-03-16 15:48:12.815741973 +0000
    @@ -3,7 +3,3 @@
     0000000 abab abab abab abab abab abab abab abab
     *
     1000000
    --- sprout --
    -0000000 abab abab abab abab abab abab abab abab
    -*
    -1000000
    ...
    (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/162.out /home/fdmanana/git/hub/xfstests/results//btrfs/162.out.bad'  to see the entire diff)
btrfs/163 2s ... [failed, exit status 1]- output mismatch (see /home/fdmanana/git/hub/xfstests/results//btrfs/163.out.bad)
    --- tests/btrfs/163.out	2020-11-05 15:55:23.585035140 +0000
    +++ /home/fdmanana/git/hub/xfstests/results//btrfs/163.out.bad	2022-03-16 15:48:15.215819215 +0000
    @@ -3,10 +3,3 @@
     0000000 abab abab abab abab abab abab abab abab
     *
     20000000
    --- sprout --
    -0000000 abab abab abab abab abab abab abab abab
    -*
    -20000000
    ...
    (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/163.out /home/fdmanana/git/hub/xfstests/results//btrfs/163.out.bad'  to see the entire diff)
btrfs/164 1s ... [failed, exit status 1]- output mismatch (see /home/fdmanana/git/hub/xfstests/results//btrfs/164.out.bad)
    --- tests/btrfs/164.out	2020-06-10 19:29:03.822519250 +0100
    +++ /home/fdmanana/git/hub/xfstests/results//btrfs/164.out.bad	2022-03-16 15:48:17.291886010 +0000
    @@ -3,7 +3,3 @@
     0000000 abab abab abab abab abab abab abab abab
     *
     1000000
    --- sprout --
    -0000000 abab abab abab abab abab abab abab abab
    -*
    -1000000
    ...
    (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/164.out /home/fdmanana/git/hub/xfstests/results//btrfs/164.out.bad'  to see the entire diff)
btrfs/248 1s ... - output mismatch (see /home/fdmanana/git/hub/xfstests/results//btrfs/248.out.bad)
    --- tests/btrfs/248.out	2021-10-26 11:04:03.879678608 +0100
    +++ /home/fdmanana/git/hub/xfstests/results//btrfs/248.out.bad	2022-03-16 15:48:19.363952657 +0000
    @@ -1,2 +1,9 @@
     QA output created by 248
    +ERROR: bad magic on superblock on /dev/sdd at 65536
    +mount: /home/fdmanana/btrfs-tests/scratch_1: wrong fs type, bad option, bad superblock on /dev/sdd, missing codepage or helper program, or other error.
    +cat: /sys/fs/btrfs//devinfo/2/fsid: No such file or directory
    +Usage: grep [OPTION]... PATTERNS [FILE]...
    +Try 'grep --help' for more information.
    +cat: /sys/fs/btrfs//devinfo/1/fsid: No such file or directory
    ...
    (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/248.out /home/fdmanana/git/hub/xfstests/results//btrfs/248.out.bad'  to see the entire diff)
Ran: btrfs/161 btrfs/162 btrfs/163 btrfs/164 btrfs/248
Failures: btrfs/161 btrfs/162 btrfs/163 btrfs/164 btrfs/248
Failed 5 of 5 tests

So device seeding introduces a special case. If we mount a seeding
filesystem, it's RO, so the mnt_want_write_file() fails.

Something like this deals with it and it makes the tests pass:

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index d7d9e1f39b87..4f347e363a8e 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3472,6 +3472,7 @@ static long btrfs_ioctl_add_dev(struct file *file, void __user *arg)
 {
        struct inode *inode = file_inode(file);
        struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
+       const bool seeding = fs_info->fs_devices->seeding;
        struct btrfs_ioctl_vol_args *vol_args;
        bool restore_op = false;
        int ret;
@@ -3484,9 +3485,13 @@ static long btrfs_ioctl_add_dev(struct file *file, void __user *arg)
                return -EINVAL;
        }
 
-       ret = mnt_want_write_file(file);
-       if (ret)
-               return ret;
+       if (seeding) {
+               sb_start_write(fs_info->sb);
+       } else {
+               ret = mnt_want_write_file(file);
+               if (ret)
+                       return ret;
+       }
 
        if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_DEV_ADD)) {
                if (!btrfs_exclop_start_try_lock(fs_info, BTRFS_EXCLOP_DEV_ADD))
@@ -3520,7 +3525,12 @@ static long btrfs_ioctl_add_dev(struct file *file, void __user *arg)
                btrfs_exclop_balance(fs_info, BTRFS_EXCLOP_BALANCE_PAUSED);
        else
                btrfs_exclop_finish(fs_info);
-       mnt_drop_write_file(file);
+
+       if (seeding)
+               sb_end_write(fs_info->sb);
+       else
+               mnt_drop_write_file(file);
+
        return ret;
 }

We are also changing the semantics as we no longer allow for adding a device
to a RO filesystem. So the lack of a mnt_want_write_file() was intentional
to deal with the seeding filesystem case. But calling mnt_want_write_file()
if we are not seeding, changes the semantics - I'm not sure if anyone relies
on the ability to add a device to a fs mounted RO, I'm not seeing if it's an
useful use case.

So either we do that special casing like in that diff, or we always do the
sb_start_write() / sb_end_write() - in any case please add a comment explaining
why we do it like that, why we can't use mnt_want_write_file().
 
Thanks.


> +
>  	if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_DEV_ADD)) {
>  		if (!btrfs_exclop_start_try_lock(fs_info, BTRFS_EXCLOP_DEV_ADD))
>  			return BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
> @@ -3520,6 +3526,7 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg)
>  		btrfs_exclop_balance(fs_info, BTRFS_EXCLOP_BALANCE_PAUSED);
>  	else
>  		btrfs_exclop_finish(fs_info);
> +	mnt_drop_write_file(file);
>  	return ret;
>  }
>  
> @@ -5443,7 +5450,7 @@ long btrfs_ioctl(struct file *file, unsigned int
>  	case BTRFS_IOC_RESIZE:
>  		return btrfs_ioctl_resize(file, argp);
>  	case BTRFS_IOC_ADD_DEV:
> -		return btrfs_ioctl_add_dev(fs_info, argp);
> +		return btrfs_ioctl_add_dev(file, argp);
>  	case BTRFS_IOC_RM_DEV:
>  		return btrfs_ioctl_rm_dev(file, argp);
>  	case BTRFS_IOC_RM_DEV_V2:
> -- 
> 2.35.1
>