On 10/02/2022 16:51, Goldwyn Rodrigues wrote:
> If a read-write root mount is remounted as read-only, the subvolume
> is also set to read-only.
Errrr... Isn't that exactly what I want?
If I have a btrfs filesystem with hundreds of subvols, some of which may
be mounted into various places in my filesystem, I would expect that if
I remount the main mountpoint as RO, that all the subvols become RO as
well. I actually don't mind if the behaviour went further and remounting
ANY of the mount points as RO would make them all RO.
My mental model is that mounting a subvol somewhere is rather like a
bind mount. And a bind mount goes RO if the underlying fs goes RO -
doesn't it?
Or am I just confused about what this patch is discussing?
Graham
>
> Use a rw_mounts counter to check the number of read-write mounts, and change
> superblock to read-only only in case there are no read-write subvol mounts.
>
> Since sb->s_flags can change while calling legacy_reconfigure(), use
> sb->s_flags instead of fc->sb_flags to re-evaluate sb->s_flags in
> reconfigure_super().
>
> Test case:
> dd if=/dev/zero of=btrfsfs seek=240 count=0 bs=1M
> mkfs.btrfs btrfsfs
> mount btrfsfs /mnt
> btrfs subvol create /mnt/sv
> mount -o remount,ro /mnt
> mount -osubvol=/sv btrfsfs /mnt/sv
> findmnt # /mnt is RO, /mnt/sv RW
> mount -o remount,ro /mnt
> findmnt # /mnt is RO, /mnt/sv RO as well
> umount /mnt{/sv,}
> rm btrfsfs
>
> Reported-by: Fabian Vogt <fvogt@xxxxxxxx>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@xxxxxxxx>
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index a2991971c6b5..2bb6869f15af 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1060,6 +1060,9 @@ struct btrfs_fs_info {
> spinlock_t zone_active_bgs_lock;
> struct list_head zone_active_bgs;
>
> + /* Count of subvol mounts read-write */
> + int rw_mounts;
> +
> #ifdef CONFIG_BTRFS_FS_REF_VERIFY
> spinlock_t ref_verify_lock;
> struct rb_root block_tree;
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 33cfc9e27451..32941e11e551 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1835,6 +1835,11 @@ static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
> /* mount_subvol() will free subvol_name and mnt_root */
> root = mount_subvol(subvol_name, subvol_objectid, mnt_root);
>
> + if (!IS_ERR(root) && !(flags & SB_RDONLY)) {
> + struct btrfs_fs_info *fs_info = btrfs_sb(mnt_root->mnt_sb);
> + fs_info->rw_mounts++;
> + }
> +
> out:
> return root;
> }
> @@ -1958,6 +1963,9 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
> goto out;
>
> if (*flags & SB_RDONLY) {
> +
> + if (--fs_info->rw_mounts > 0)
> + goto out;
> /*
> * this also happens on 'umount -rf' or on shutdown, when
> * the filesystem is busy.
> @@ -2057,6 +2065,8 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
> if (ret)
> goto restore;
>
> + fs_info->rw_mounts++;
> +
> btrfs_clear_sb_rdonly(sb);
>
> set_bit(BTRFS_FS_OPEN, &fs_info->flags);
> diff --git a/fs/super.c b/fs/super.c
> index f1d4a193602d..fd528f76f14b 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -913,7 +913,8 @@ int reconfigure_super(struct fs_context *fc)
> }
>
> WRITE_ONCE(sb->s_flags, ((sb->s_flags & ~fc->sb_flags_mask) |
> - (fc->sb_flags & fc->sb_flags_mask)));
> + (sb->s_flags & fc->sb_flags_mask)));
> +
> /* Needs to be ordered wrt mnt_is_readonly() */
> smp_wmb();
> sb->s_readonly_remount = 0;
>
