diff mbox series

[1/2] btrfs: fix per-subvolume RO/RW flags with new mount API

Message ID 530cb7c961043767d15eac6c86d6cfd9a5d19ac5.1730249396.git.wqu@suse.com (mailing list archive)
State New
Headers show
Series btrfs: fixes for the new fd-based API | expand

Commit Message

Qu Wenruo Oct. 30, 2024, 12:55 a.m. UTC
[BUG]
With util-linux 2.40.2, the mount is already utilizing the new mount
API. e.g:

 # strace  mount -o subvol=subv1,ro /dev/test/scratch1 /mnt/test/
 ...
 fsconfig(3, FSCONFIG_SET_STRING, "source", "/dev/mapper/test-scratch1", 0) = 0
 fsconfig(3, FSCONFIG_SET_STRING, "subvol", "subv1", 0) = 0
 fsconfig(3, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
 fsconfig(3, FSCONFIG_CMD_CREATE, NULL, NULL, 0) = 0
 fsmount(3, FSMOUNT_CLOEXEC, 0)          = 4
 mount_setattr(4, "", AT_EMPTY_PATH, {attr_set=MOUNT_ATTR_RDONLY, attr_clr=0, propagation=0 /* MS_??? */, userns_fd=0}, 32) = 0
 move_mount(4, "", AT_FDCWD, "/mnt/test", MOVE_MOUNT_F_EMPTY_PATH) = 0

But this leads to a new problem, that per-subvolume RO/RW mount no
longer works, if the initial mount is RO:

 # mount -o subvol=subv1,ro /dev/test/scratch1 /mnt/test
 # mount -o rw,subvol=subv2 /dev/test/scratch1  /mnt/scratch
 # mount | grep mnt
 /dev/mapper/test-scratch1 on /mnt/test type btrfs (ro,relatime,discard=async,space_cache=v2,subvolid=256,subvol=/subv1)
 /dev/mapper/test-scratch1 on /mnt/scratch type btrfs (ro,relatime,discard=async,space_cache=v2,subvolid=257,subvol=/subv2)
 # touch /mnt/scratch/foobar
 touch: cannot touch '/mnt/scratch/foobar': Read-only file system

[CAUSE]
We have the remount "hack" to handle the RO->RW change, but if the mount
is using the new mount API, we do not do the hack, and rely on the mount
tool NOT to set the ro flag.

But that's not how the mount tool is doing for the new API:

 fsconfig(3, FSCONFIG_SET_STRING, "source", "/dev/mapper/test-scratch1", 0) = 0
 fsconfig(3, FSCONFIG_SET_STRING, "subvol", "subv1", 0) = 0
 fsconfig(3, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0 <<<< Setting RO flag for super block
 fsconfig(3, FSCONFIG_CMD_CREATE, NULL, NULL, 0) = 0
 fsmount(3, FSMOUNT_CLOEXEC, 0)          = 4
 mount_setattr(4, "", AT_EMPTY_PATH, {attr_set=MOUNT_ATTR_RDONLY, attr_clr=0, propagation=0 /* MS_??? */, userns_fd=0}, 32) = 0
 move_mount(4, "", AT_FDCWD, "/mnt/test", MOVE_MOUNT_F_EMPTY_PATH) = 0

This means we will set the super block RO at the first mount.

Later RW mount will not try to reconfigure the fs to RW because the
mount tool is already using the new API.

This totally breaks the per-subvolume RO/RW mount behavior.

[FIX]
Do not skip the reconfiguration even using the new API.
The old comments are just expecting the mount tool to properly skip RO
flag set even we specify "ro", which is not the reality.

And remove the comments pushing all the responsibility to mount command,
replace them with ones matching the reality instead.

Fixes: f044b318675f ("btrfs: handle the ro->rw transition for mounting different subvolumes")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/super.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)
diff mbox series

Patch

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 6cc9291c4552..d77cce8d633e 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1977,25 +1977,11 @@  static int btrfs_get_tree_super(struct fs_context *fc)
  *     fsconfig(FSCONFIG_SET_FLAG, "ro"). This option is seen by the filesystem
  *     in fc->sb_flags.
  *
- * This disambiguation has rather positive consequences.  Mounting a subvolume
- * ro will not also turn the superblock ro. Only the mount for the subvolume
- * will become ro.
+ * But in reality, even the newer util-linux mount command, which is already
+ * utilizing the new mount API, is still setting fsconfig(FSCONFIG_SET_FLAG, "ro")
+ * no matter if it's a btrfs or not, setting the whole super block RO.
  *
- * So, if the superblock creation request comes from the new mount API the
- * caller must have explicitly done:
- *
- *      fsconfig(FSCONFIG_SET_FLAG, "ro")
- *      fsmount/mount_setattr(MOUNT_ATTR_RDONLY)
- *
- * IOW, at some point the caller must have explicitly turned the whole
- * superblock ro and we shouldn't just undo it like we did for the old mount
- * API. In any case, it lets us avoid the hack in the new mount API.
- *
- * Consequently, the remounting hack must only be used for requests originating
- * from the old mount API and should be marked for full deprecation so it can be
- * turned off in a couple of years.
- *
- * The new mount API has no reason to support this hack.
+ * So here we always needs the remount hack to support per-subvolume RO/RW flags.
  */
 static struct vfsmount *btrfs_reconfigure_for_mount(struct fs_context *fc)
 {
@@ -2017,7 +2003,7 @@  static struct vfsmount *btrfs_reconfigure_for_mount(struct fs_context *fc)
 	if (IS_ERR(mnt))
 		return mnt;
 
-	if (!fc->oldapi || !ro2rw)
+	if (!ro2rw)
 		return mnt;
 
 	/* We need to convert to rw, call reconfigure. */