diff mbox series

[3/3] btrfs: Add parameter to force devices behave as single-dev ones

Message ID 20230803154453.1488248-4-gpiccoli@igalia.com (mailing list archive)
State New, archived
Headers show
Series Supporting same fsid mounting through a compat_ro feature | expand

Commit Message

Guilherme G. Piccoli Aug. 3, 2023, 3:43 p.m. UTC
Devices with the single-dev feature enabled in their superblock are
allowed to be mounted regardless of their fsid being already present
in the system - the goal of such feature is to have the device in a
single mode with no advanced features, like RAID; it is a compat_ro
feature present since kernel v6.5.

The thing is that such feature comes in the form of a superblock flag,
so devices that doesn't have it set, can't use the feature of course.
The Steam Deck console aims to have block-based updates in its
RO rootfs, and given its A/B partition nature, both block devices are
required to be the same for their hash to match, so it's not possible
to compare two images if one has this feature set in the superblock,
while the other has not. So if we end-up having two old images, we
couldn't make use of the single-dev feature to mount both at same time,
or if we set the flag in one of them to enable the feature, we break
the block-based hash comparison.

We propose here a module parameter approach to allow forcing any given
path (to a device holding a btrfs filesystem) behaving as a single-dev
device. That would useful for cases like the Steam Deck one, or for
debug purposes. If the filesystem already has the compat_ro flag set
in its superblock, the parameter is no-op.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 fs/btrfs/disk-io.c |  2 +-
 fs/btrfs/ioctl.c   |  6 +++---
 fs/btrfs/super.c   |  5 +++++
 fs/btrfs/super.h   |  2 ++
 fs/btrfs/volumes.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
 fs/btrfs/volumes.h |  2 ++
 6 files changed, 55 insertions(+), 7 deletions(-)

Comments

Josef Bacik Aug. 17, 2023, 3:44 p.m. UTC | #1
On Thu, Aug 03, 2023 at 12:43:41PM -0300, Guilherme G. Piccoli wrote:
> Devices with the single-dev feature enabled in their superblock are
> allowed to be mounted regardless of their fsid being already present
> in the system - the goal of such feature is to have the device in a
> single mode with no advanced features, like RAID; it is a compat_ro
> feature present since kernel v6.5.
> 
> The thing is that such feature comes in the form of a superblock flag,
> so devices that doesn't have it set, can't use the feature of course.
> The Steam Deck console aims to have block-based updates in its
> RO rootfs, and given its A/B partition nature, both block devices are
> required to be the same for their hash to match, so it's not possible
> to compare two images if one has this feature set in the superblock,
> while the other has not. So if we end-up having two old images, we
> couldn't make use of the single-dev feature to mount both at same time,
> or if we set the flag in one of them to enable the feature, we break
> the block-based hash comparison.
> 
> We propose here a module parameter approach to allow forcing any given
> path (to a device holding a btrfs filesystem) behaving as a single-dev
> device. That would useful for cases like the Steam Deck one, or for
> debug purposes. If the filesystem already has the compat_ro flag set
> in its superblock, the parameter is no-op.
> 

Now this one I'm not a fan of.  For old file systems you can simply btrfstune
them to have your new flag.  Is there a reason why that wouldn't be an option?

If it is indeed required, which is a huge if, I'd rather this be accomplished a
mount option.  I have a strong dislike for new mount options, but I think that's
a cleaner way to accomplish this than a module option.  Thanks,

Josef
Guilherme G. Piccoli Aug. 20, 2023, 6:16 p.m. UTC | #2
On 17/08/2023 12:44, Josef Bacik wrote:
> [...]
> Now this one I'm not a fan of.  For old file systems you can simply btrfstune
> them to have your new flag.  Is there a reason why that wouldn't be an option?
> 
> If it is indeed required, which is a huge if, I'd rather this be accomplished a
> mount option.  I have a strong dislike for new mount options, but I think that's
> a cleaner way to accomplish this than a module option.  Thanks,
> 

Thanks Josef, for your feedback. I'm also not a fan of this module
parameter, and agree that a mount option would be more interesting if
that's indeed a requirement.

But I've discussed internally and it seems we can live without this one,
I'll drop it for next version.

Cheers,


Guilherme
diff mbox series

Patch

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 455fa4949c98..8df1defa1ede 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2378,7 +2378,7 @@  int btrfs_validate_super(struct btrfs_fs_info *fs_info,
 	 * in the disk. That's the reason we're required here to compare the
 	 * fsid with the metadata_uuid for such devices.
 	 */
-	if (btrfs_fs_compat_ro(fs_info, SINGLE_DEV))
+	if (fs_info->fs_devices->single_dev)
 		fsid = fs_info->fs_devices->metadata_uuid;
 	else
 		fsid = fs_info->fs_devices->fsid;
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 56703d87def9..4fc63e802b08 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2678,7 +2678,7 @@  static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user *arg)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	if (btrfs_fs_compat_ro(fs_info, SINGLE_DEV)) {
+	if (fs_info->fs_devices->single_dev) {
 		btrfs_err(fs_info,
 			  "device removal is unsupported on SINGLE_DEV devices\n");
 		return -EINVAL;
@@ -2750,7 +2750,7 @@  static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	if (btrfs_fs_compat_ro(fs_info, SINGLE_DEV)) {
+	if (fs_info->fs_devices->single_dev) {
 		btrfs_err(fs_info,
 			  "device removal is unsupported on SINGLE_DEV devices\n");
 		return -EINVAL;
@@ -3280,7 +3280,7 @@  static long btrfs_ioctl_dev_replace(struct btrfs_fs_info *fs_info,
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	if (btrfs_fs_compat_ro(fs_info, SINGLE_DEV)) {
+	if (fs_info->fs_devices->single_dev) {
 		btrfs_err(fs_info,
 			  "device removal is unsupported on SINGLE_DEV devices\n");
 		return -EINVAL;
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index ee87189b1ccd..3cfc9c63360f 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -62,6 +62,11 @@ 
 #define CREATE_TRACE_POINTS
 #include <trace/events/btrfs.h>
 
+char *force_single_dev;
+module_param(force_single_dev, charp, 0444);
+MODULE_PARM_DESC(force_single_dev,
+	"User list of devices to force acting as single-dev (comma separated)");
+
 static const struct super_operations btrfs_super_ops;
 
 /*
diff --git a/fs/btrfs/super.h b/fs/btrfs/super.h
index 8dbb909b364f..c855127600c8 100644
--- a/fs/btrfs/super.h
+++ b/fs/btrfs/super.h
@@ -3,6 +3,8 @@ 
 #ifndef BTRFS_SUPER_H
 #define BTRFS_SUPER_H
 
+extern char *force_single_dev;
+
 int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			unsigned long new_flags);
 int btrfs_sync_fs(struct super_block *sb, int wait);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 433a490f2de8..06c5bad77bdf 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -12,6 +12,7 @@ 
 #include <linux/uuid.h>
 #include <linux/list_sort.h>
 #include <linux/namei.h>
+#include <linux/string.h>
 #include "misc.h"
 #include "ctree.h"
 #include "extent_map.h"
@@ -865,6 +866,7 @@  static noinline struct btrfs_device *device_list_add(const char *path,
 			return ERR_CAST(fs_devices);
 
 		fs_devices->fsid_change = fsid_change_in_progress;
+		fs_devices->single_dev = single_dev;
 
 		mutex_lock(&fs_devices->device_list_mutex);
 		list_add(&fs_devices->fs_list, &fs_uuids);
@@ -1399,6 +1401,45 @@  int btrfs_forget_devices(dev_t devt)
 	return ret;
 }
 
+/*
+ * SINGLE_DEV is a compat_ro feature, but we also have the force_single_dev
+ * module parameter in order to allow forcing a device to behave as single-dev,
+ * so old filesystems could also get mounted in a same-fsid mounting way.
+ */
+
+static bool is_single_dev(const char *path, struct btrfs_super_block *sb)
+{
+
+	if (btrfs_super_compat_ro_flags(sb) & BTRFS_FEATURE_COMPAT_RO_SINGLE_DEV)
+		return true;
+
+	if (force_single_dev) {
+		char *p, *skip_devs, *orig;
+
+		skip_devs = kstrdup(force_single_dev, GFP_KERNEL);
+		if (!skip_devs) {
+			pr_err("BTRFS: couldn't parse force_single_dev parameter\n");
+			return false;
+		}
+
+		orig = skip_devs;
+		while ((p = strsep(&skip_devs, ",")) != NULL) {
+			if (!*p)
+				continue;
+
+			if (!strcmp(p, path)) {
+				pr_info(
+			"BTRFS: forcing device %s to be single-dev\n", path);
+				kfree(orig);
+				return true;
+			}
+		}
+		kfree(orig);
+	}
+
+	return false;
+}
+
 /*
  * Look for a btrfs signature on a device. This may be called out of the mount path
  * and we are not allowed to call set_blocksize during the scan. The superblock
@@ -1451,9 +1492,7 @@  struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
 		goto error_bdev_put;
 	}
 
-	single_dev = btrfs_super_compat_ro_flags(disk_super) &
-			BTRFS_FEATURE_COMPAT_RO_SINGLE_DEV;
-
+	single_dev = is_single_dev(path, disk_super);
 	if (!mounting && single_dev) {
 		pr_info("BTRFS: skipped non-mount scan on SINGLE_DEV device %s\n",
 			path);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index b9856c801567..57a3969f101c 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -293,6 +293,8 @@  struct btrfs_fs_devices {
 	 */
 	u8 metadata_uuid[BTRFS_FSID_SIZE];
 
+	bool single_dev;
+
 	struct list_head fs_list;
 
 	/*