| Message ID | 20220210165142.7zfgotun5qdtx4rq@fiona (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix read-only superblock in case of subvol RO remount | expand |
On Thu, Feb 10, 2022 at 10:51:42AM -0600, Goldwyn Rodrigues wrote: > 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))); > + what? (a & ~b) | (a & b) == a, no?
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@suse.com> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > > 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; >
On 19:27 10/02, Matthew Wilcox wrote: > On Thu, Feb 10, 2022 at 10:51:42AM -0600, Goldwyn Rodrigues wrote: > > 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))); > > + > > what? > > (a & ~b) | (a & b) == a, no? Yup! I am re-enrolling into middle school to learn bit-arithmetic. Apparently, I don't need this change at all and can be done in btrfs code by changing the flags parameter. Will post a patch to btrfs ML with the updated patch.
On 19:54 10/02, Graham Cobb wrote: > 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? > If we want bind mount, we would use bind mount. subvolume mounts and bind mounts are different and should be treated as different features. > Or am I just confused about what this patch is discussing? Root can also be considered as a unique subvolume with a unique subvolume id and a unique name=/
On 10/02/2022 21:30, Goldwyn Rodrigues wrote: > On 19:54 10/02, Graham Cobb wrote: >> 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? >> > > If we want bind mount, we would use bind mount. subvolume mounts and bind > mounts are different and should be treated as different features. Yes that's a good point. However, I am still not convinced that this is a change in behaviour that is obvious enough to justify the risk of disruption to existing systems, admin scripts or system managers. > >> Or am I just confused about what this patch is discussing? > > Root can also be considered as a unique subvolume with a unique > subvolume id and a unique name=/ But with an important special property that is different from all other subvolumes: all other subvolumes are reachable from it.
On 10/02/2022 22:03, Graham Cobb wrote: > On 10/02/2022 21:30, Goldwyn Rodrigues wrote: >> On 19:54 10/02, Graham Cobb wrote: >>> 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? >>> >> >> If we want bind mount, we would use bind mount. subvolume mounts and bind >> mounts are different and should be treated as different features. > > Yes that's a good point. However, I am still not convinced that this is > a change in behaviour that is obvious enough to justify the risk of > disruption to existing systems, admin scripts or system managers. > >> >>> Or am I just confused about what this patch is discussing? >> >> Root can also be considered as a unique subvolume with a unique >> subvolume id and a unique name=/ > > But with an important special property that is different from all other > subvolumes: all other subvolumes are reachable from it. I should be a bit clearer. Imagine you create a filesystem and then create two subvolumes within it: a and a/b. You are suggesting that the result of remounting the top level of the filesystem as RO causes different effect on whether subvolume b goes RO depending on whether subvolume a has also been mounted somewhere?
On 22:09 10/02, Graham Cobb wrote: > On 10/02/2022 22:03, Graham Cobb wrote: > > On 10/02/2022 21:30, Goldwyn Rodrigues wrote: > >> On 19:54 10/02, Graham Cobb wrote: > >>> 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? > >>> > >> > >> If we want bind mount, we would use bind mount. subvolume mounts and bind > >> mounts are different and should be treated as different features. > > > > Yes that's a good point. However, I am still not convinced that this is > > a change in behaviour that is obvious enough to justify the risk of > > disruption to existing systems, admin scripts or system managers. > > > >> > >>> Or am I just confused about what this patch is discussing? > >> > >> Root can also be considered as a unique subvolume with a unique > >> subvolume id and a unique name=/ > > > > But with an important special property that is different from all other > > subvolumes: all other subvolumes are reachable from it. Not quite. You can mount a subvolume directly without mounting the root: mount -o subvol=<subvolname> <device> <mountpoint> > > I should be a bit clearer. Imagine you create a filesystem and then > create two subvolumes within it: a and a/b. You are suggesting that the > result of remounting the top level of the filesystem as RO causes > different effect on whether subvolume b goes RO depending on whether > subvolume a has also been mounted somewhere? I did not understand the question, too many "whether"s. :) Check the test case. If you mount a root subvolume (/) and a root's subvolume (/a or /a/b in your case). You remount root as ro, the subvolume a or a/b mounts also become read-only. On the other hand, if you mount a or a/b (RW) _after_ root is remounted RO, the subvolume mounts are RW.
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;
If a read-write root mount is remounted as read-only, the subvolume is also set to read-only. 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@suse.com> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>