diff mbox

[1/7] btrfs: add ability to query/change feature bits online

Message ID 20130910043007.438505775@suse.com
State New, archived
Headers show

Commit Message

Jeff Mahoney Sept. 10, 2013, 4:24 a.m. UTC
There are some feature bits that require no offline setup and can
be enabled online. I've only reviewed extended irefs, but there will
probably be more.

We introduce three new ioctls:
- BTRFS_IOC_GET_SUPPORTED_FEATURES: query the kernel for supported features.
- BTRFS_IOC_GET_FEATURES: query the kernel for enabled features on a per-fs
  basis, as well as querying for which features are changeable with mounted.
- BTRFS_IOC_SET_FEATURES: change features on a per-fs basis.

We introduce two new masks per feature set (_SAFE_SET and _SAFE_CLEAR) that
allow us to define which features are safe to change at runtime.

The failure modes for BTRFS_IOC_SET_FEATURES are as follows:
- Enabling a completely unsupported feature: warns and returns -ENOTSUPP
- Enabling a feature that can only be done offline: warns and returns -EPERM

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 fs/btrfs/ctree.h           |    9 ++
 fs/btrfs/ioctl.c           |  145 +++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/btrfs.h |   12 +++
 3 files changed, 166 insertions(+)



--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Sterba Sept. 16, 2013, 5:26 p.m. UTC | #1
On Tue, Sep 10, 2013 at 12:24:09AM -0400, Jeff Mahoney wrote:
> +static int btrfs_ioctl_set_features(struct file *file, void __user *arg)
> +{
> +	struct btrfs_root *root = BTRFS_I(file_inode(file))->root;
> +	struct btrfs_super_block *super_block = root->fs_info->super_copy;
> +	struct btrfs_ioctl_feature_flags flags[2];
> +	struct btrfs_trans_handle *trans;
> +	u64 newflags;
> +	int ret;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (copy_from_user(flags, arg, sizeof(flags)))
> +		return -EFAULT;
> +
> +	/* Nothing to do */
> +	if (!flags[0].compat_flags && !flags[0].compat_ro_flags &&
> +	    !flags[0].incompat_flags)
> +		return 0;
> +
> +	ret = check_feature(root, flags[0].compat_flags,
> +			    flags[1].compat_flags, COMPAT);
> +	if (ret)
> +		return ret;
> +
> +	ret = check_feature(root, flags[0].compat_ro_flags,
> +			    flags[1].compat_ro_flags, COMPAT_RO);
> +	if (ret)
> +		return ret;
> +
> +	ret = check_feature(root, flags[0].incompat_flags,
> +			    flags[1].incompat_flags, INCOMPAT);
> +	if (ret)
> +		return ret;
> +
> +	trans = btrfs_start_transaction(root, 1);

It's ok to use 0 here, it's a superblock change and not related the the
metadata (similar to changing the label).

> +	if (IS_ERR(trans))
> +		return PTR_ERR(trans);
> +
> +	spin_lock(&root->fs_info->super_lock);
> +	newflags = btrfs_super_compat_flags(super_block);
> +	newflags |= flags[0].compat_flags & flags[1].compat_flags;
> +	newflags &= ~(flags[0].compat_flags & ~flags[1].compat_flags);
> +	btrfs_set_super_compat_flags(super_block, newflags);
> +
> +	newflags = btrfs_super_compat_ro_flags(super_block);
> +	newflags |= flags[0].compat_ro_flags & flags[1].compat_ro_flags;
> +	newflags &= ~(flags[0].compat_ro_flags & ~flags[1].compat_ro_flags);
> +	btrfs_set_super_compat_ro_flags(super_block, newflags);
> +
> +	newflags = btrfs_super_incompat_flags(super_block);
> +	newflags |= flags[0].incompat_flags & flags[1].incompat_flags;
> +	newflags &= ~(flags[0].incompat_flags & ~flags[1].incompat_flags);
> +	btrfs_set_super_incompat_flags(super_block, newflags);
> +	spin_unlock(&root->fs_info->super_lock);
> +
> +	return btrfs_end_transaction(trans, root);

I think it's better to use btrfs_commit_transaction. The ioctl is about
to do a permanent change, any crash between ioctl and full commit will
revert to previous state of the features. This is not IMO desirable from
administrators POV.

(Sidenote, IOC_SET_FSLABEL needs the same update.)

> +}
> +
> --- a/include/uapi/linux/btrfs.h	2013-09-09 17:49:10.808003254 -0400
> +++ b/include/uapi/linux/btrfs.h	2013-09-09 17:49:12.483979430 -0400
> @@ -184,6 +184,12 @@ struct btrfs_ioctl_fs_info_args {
>  	__u64 reserved[124];			/* pad to 1k */
>  };
>  
> +struct btrfs_ioctl_feature_flags {
> +	__u64 compat_flags;
> +	__u64 compat_ro_flags;
> +	__u64 incompat_flags;

I wonder if it makes sense to add spare bytes here, but does not seem
necessary.

> +};
> @@ -579,4 +585,10 @@ static inline char *btrfs_err_str(enum b
> +#define BTRFS_IOC_GET_FEATURES _IOR(BTRFS_IOCTL_MAGIC, 54, \
> +				   struct btrfs_ioctl_feature_flags)
> +#define BTRFS_IOC_SET_FEATURES _IOW(BTRFS_IOCTL_MAGIC, 54, \
> +				   struct btrfs_ioctl_feature_flags[2])
> +#define BTRFS_IOC_GET_SUPPORTED_FEATURES _IOR(BTRFS_IOCTL_MAGIC, 54, \
> +				   struct btrfs_ioctl_feature_flags[3])

The ioctl number 54 clashes with the OOB dedup merged into 3.12, 55 is
for the in-bound dedup and Anand has claimed 56. I've allocated 57 for
you and updated th wiki page:

https://btrfs.wiki.kernel.org/index.php/Project_ideas#Development_notes.2C_please_read

Stefan's trick to reuse the same number is really neat.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Mahoney Sept. 16, 2013, 6:13 p.m. UTC | #2
On 9/16/13 1:26 PM, David Sterba wrote:
> On Tue, Sep 10, 2013 at 12:24:09AM -0400, Jeff Mahoney wrote:
>> +static int btrfs_ioctl_set_features(struct file *file, void __user *arg)
>> +{
>> +	struct btrfs_root *root = BTRFS_I(file_inode(file))->root;
>> +	struct btrfs_super_block *super_block = root->fs_info->super_copy;
>> +	struct btrfs_ioctl_feature_flags flags[2];
>> +	struct btrfs_trans_handle *trans;
>> +	u64 newflags;
>> +	int ret;
>> +
>> +	if (!capable(CAP_SYS_ADMIN))
>> +		return -EPERM;
>> +
>> +	if (copy_from_user(flags, arg, sizeof(flags)))
>> +		return -EFAULT;
>> +
>> +	/* Nothing to do */
>> +	if (!flags[0].compat_flags && !flags[0].compat_ro_flags &&
>> +	    !flags[0].incompat_flags)
>> +		return 0;
>> +
>> +	ret = check_feature(root, flags[0].compat_flags,
>> +			    flags[1].compat_flags, COMPAT);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = check_feature(root, flags[0].compat_ro_flags,
>> +			    flags[1].compat_ro_flags, COMPAT_RO);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = check_feature(root, flags[0].incompat_flags,
>> +			    flags[1].incompat_flags, INCOMPAT);
>> +	if (ret)
>> +		return ret;
>> +
>> +	trans = btrfs_start_transaction(root, 1);
> 
> It's ok to use 0 here, it's a superblock change and not related the the
> metadata (similar to changing the label).
> 
>> +	if (IS_ERR(trans))
>> +		return PTR_ERR(trans);
>> +
>> +	spin_lock(&root->fs_info->super_lock);
>> +	newflags = btrfs_super_compat_flags(super_block);
>> +	newflags |= flags[0].compat_flags & flags[1].compat_flags;
>> +	newflags &= ~(flags[0].compat_flags & ~flags[1].compat_flags);
>> +	btrfs_set_super_compat_flags(super_block, newflags);
>> +
>> +	newflags = btrfs_super_compat_ro_flags(super_block);
>> +	newflags |= flags[0].compat_ro_flags & flags[1].compat_ro_flags;
>> +	newflags &= ~(flags[0].compat_ro_flags & ~flags[1].compat_ro_flags);
>> +	btrfs_set_super_compat_ro_flags(super_block, newflags);
>> +
>> +	newflags = btrfs_super_incompat_flags(super_block);
>> +	newflags |= flags[0].incompat_flags & flags[1].incompat_flags;
>> +	newflags &= ~(flags[0].incompat_flags & ~flags[1].incompat_flags);
>> +	btrfs_set_super_incompat_flags(super_block, newflags);
>> +	spin_unlock(&root->fs_info->super_lock);
>> +
>> +	return btrfs_end_transaction(trans, root);
> 
> I think it's better to use btrfs_commit_transaction. The ioctl is about
> to do a permanent change, any crash between ioctl and full commit will
> revert to previous state of the features. This is not IMO desirable from
> administrators POV.
> 
> (Sidenote, IOC_SET_FSLABEL needs the same update.)

Done, and I have a patch to fix the setlabel case as well.

>> +}
>> +
>> --- a/include/uapi/linux/btrfs.h	2013-09-09 17:49:10.808003254 -0400
>> +++ b/include/uapi/linux/btrfs.h	2013-09-09 17:49:12.483979430 -0400
>> @@ -184,6 +184,12 @@ struct btrfs_ioctl_fs_info_args {
>>  	__u64 reserved[124];			/* pad to 1k */
>>  };
>>  
>> +struct btrfs_ioctl_feature_flags {
>> +	__u64 compat_flags;
>> +	__u64 compat_ro_flags;
>> +	__u64 incompat_flags;
> 
> I wonder if it makes sense to add spare bytes here, but does not seem
> necessary.

We could double the size to allow for expansion, but I think it'd
probably make more sense to add another ioctl later if we end up getting
up to 64 features in a field.

>> +};
>> @@ -579,4 +585,10 @@ static inline char *btrfs_err_str(enum b
>> +#define BTRFS_IOC_GET_FEATURES _IOR(BTRFS_IOCTL_MAGIC, 54, \
>> +				   struct btrfs_ioctl_feature_flags)
>> +#define BTRFS_IOC_SET_FEATURES _IOW(BTRFS_IOCTL_MAGIC, 54, \
>> +				   struct btrfs_ioctl_feature_flags[2])
>> +#define BTRFS_IOC_GET_SUPPORTED_FEATURES _IOR(BTRFS_IOCTL_MAGIC, 54, \
>> +				   struct btrfs_ioctl_feature_flags[3])
> 
> The ioctl number 54 clashes with the OOB dedup merged into 3.12, 55 is
> for the in-bound dedup and Anand has claimed 56. I've allocated 57 for
> you and updated th wiki page:
> 
> https://btrfs.wiki.kernel.org/index.php/Project_ideas#Development_notes.2C_please_read
> 
> Stefan's trick to reuse the same number is really neat.

Thanks, changed.

-Jeff
diff mbox

Patch

--- a/fs/btrfs/ctree.h	2013-09-09 17:49:10.808003254 -0400
+++ b/fs/btrfs/ctree.h	2013-09-10 00:07:58.415119201 -0400
@@ -512,7 +512,12 @@  struct btrfs_super_block {
 #define BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA	(1ULL << 8)
 
 #define BTRFS_FEATURE_COMPAT_SUPP		0ULL
+#define BTRFS_FEATURE_COMPAT_SAFE_SET		0ULL
+#define BTRFS_FEATURE_COMPAT_SAFE_CLEAR		0ULL
 #define BTRFS_FEATURE_COMPAT_RO_SUPP		0ULL
+#define BTRFS_FEATURE_COMPAT_RO_SAFE_SET	0ULL
+#define BTRFS_FEATURE_COMPAT_RO_SAFE_CLEAR	0ULL
+
 #define BTRFS_FEATURE_INCOMPAT_SUPP			\
 	(BTRFS_FEATURE_INCOMPAT_MIXED_BACKREF |		\
 	 BTRFS_FEATURE_INCOMPAT_DEFAULT_SUBVOL |	\
@@ -523,6 +528,10 @@  struct btrfs_super_block {
 	 BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF |		\
 	 BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA)
 
+#define BTRFS_FEATURE_INCOMPAT_SAFE_SET			\
+	(BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF)
+#define BTRFS_FEATURE_INCOMPAT_SAFE_CLEAR		0ULL
+
 /*
  * A leaf is full of items. offset and size tell us where to find
  * the item in the leaf (relative to the start of the data area)
--- a/fs/btrfs/ioctl.c	2013-09-09 17:49:10.808003254 -0400
+++ b/fs/btrfs/ioctl.c	2013-09-10 00:08:10.514945219 -0400
@@ -4098,6 +4098,145 @@  out_unlock:
 	return ret;
 }
 
+static int btrfs_ioctl_get_supported_features(struct file *file,
+					      void __user *arg)
+{
+	struct btrfs_ioctl_feature_flags features[3];
+
+	features[0].compat_flags = BTRFS_FEATURE_COMPAT_SUPP;
+	features[0].compat_ro_flags = BTRFS_FEATURE_COMPAT_RO_SUPP;
+	features[0].incompat_flags = BTRFS_FEATURE_INCOMPAT_SUPP;
+
+	features[1].compat_flags = BTRFS_FEATURE_COMPAT_SAFE_SET;
+	features[1].compat_ro_flags = BTRFS_FEATURE_COMPAT_RO_SAFE_SET;
+	features[1].incompat_flags = BTRFS_FEATURE_INCOMPAT_SAFE_SET;
+
+	features[2].compat_flags = BTRFS_FEATURE_COMPAT_SAFE_CLEAR;
+	features[2].compat_ro_flags = BTRFS_FEATURE_COMPAT_RO_SAFE_CLEAR;
+	features[2].incompat_flags = BTRFS_FEATURE_INCOMPAT_SAFE_CLEAR;
+
+	if (copy_to_user(arg, &features, sizeof(features)))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int btrfs_ioctl_get_features(struct file *file, void __user *arg)
+{
+	struct btrfs_root *root = BTRFS_I(file_inode(file))->root;
+	struct btrfs_super_block *super_block = root->fs_info->super_copy;
+	struct btrfs_ioctl_feature_flags features;
+
+	features.compat_flags = btrfs_super_compat_flags(super_block);
+	features.compat_ro_flags = btrfs_super_compat_ro_flags(super_block);
+	features.incompat_flags = btrfs_super_incompat_flags(super_block);
+
+	if (copy_to_user(arg, &features, sizeof(features)))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int check_feature_bits(struct btrfs_root *root, const char *type,
+			      u64 change_mask, u64 flags, u64 supported_flags,
+			      u64 safe_set, u64 safe_clear)
+{
+	u64 disallowed, unsupported;
+	u64 set_mask = flags & change_mask;
+	u64 clear_mask = ~flags & change_mask;
+
+	unsupported = set_mask & ~supported_flags;
+	if (unsupported) {
+		btrfs_warn(root->fs_info,
+			   "this kernel does not support %s bits 0x%llx",
+			   type, unsupported);
+		return -EOPNOTSUPP;
+	}
+
+	disallowed = set_mask & ~safe_set;
+	if (disallowed) {
+		btrfs_warn(root->fs_info,
+			   "can't set %s bits 0x%llx while mounted",
+			   type, disallowed);
+		return -EPERM;
+	}
+
+	disallowed = clear_mask & ~safe_clear;
+	if (disallowed) {
+		btrfs_warn(root->fs_info,
+			   "can't clear %s bits 0x%llx while mounted",
+			   type, disallowed);
+		return -EPERM;
+	}
+
+	return 0;
+}
+
+#define check_feature(root, change_mask, flags, mask_base)	\
+check_feature_bits(root, # mask_base, change_mask, flags,	\
+		   BTRFS_FEATURE_ ## mask_base ## _SUPP,	\
+		   BTRFS_FEATURE_ ## mask_base ## _SAFE_SET,	\
+		   BTRFS_FEATURE_ ## mask_base ## _SAFE_CLEAR)
+
+static int btrfs_ioctl_set_features(struct file *file, void __user *arg)
+{
+	struct btrfs_root *root = BTRFS_I(file_inode(file))->root;
+	struct btrfs_super_block *super_block = root->fs_info->super_copy;
+	struct btrfs_ioctl_feature_flags flags[2];
+	struct btrfs_trans_handle *trans;
+	u64 newflags;
+	int ret;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (copy_from_user(flags, arg, sizeof(flags)))
+		return -EFAULT;
+
+	/* Nothing to do */
+	if (!flags[0].compat_flags && !flags[0].compat_ro_flags &&
+	    !flags[0].incompat_flags)
+		return 0;
+
+	ret = check_feature(root, flags[0].compat_flags,
+			    flags[1].compat_flags, COMPAT);
+	if (ret)
+		return ret;
+
+	ret = check_feature(root, flags[0].compat_ro_flags,
+			    flags[1].compat_ro_flags, COMPAT_RO);
+	if (ret)
+		return ret;
+
+	ret = check_feature(root, flags[0].incompat_flags,
+			    flags[1].incompat_flags, INCOMPAT);
+	if (ret)
+		return ret;
+
+	trans = btrfs_start_transaction(root, 1);
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
+
+	spin_lock(&root->fs_info->super_lock);
+	newflags = btrfs_super_compat_flags(super_block);
+	newflags |= flags[0].compat_flags & flags[1].compat_flags;
+	newflags &= ~(flags[0].compat_flags & ~flags[1].compat_flags);
+	btrfs_set_super_compat_flags(super_block, newflags);
+
+	newflags = btrfs_super_compat_ro_flags(super_block);
+	newflags |= flags[0].compat_ro_flags & flags[1].compat_ro_flags;
+	newflags &= ~(flags[0].compat_ro_flags & ~flags[1].compat_ro_flags);
+	btrfs_set_super_compat_ro_flags(super_block, newflags);
+
+	newflags = btrfs_super_incompat_flags(super_block);
+	newflags |= flags[0].incompat_flags & flags[1].incompat_flags;
+	newflags &= ~(flags[0].incompat_flags & ~flags[1].incompat_flags);
+	btrfs_set_super_incompat_flags(super_block, newflags);
+	spin_unlock(&root->fs_info->super_lock);
+
+	return btrfs_end_transaction(trans, root);
+}
+
 long btrfs_ioctl(struct file *file, unsigned int
 		cmd, unsigned long arg)
 {
@@ -4208,6 +4347,12 @@  long btrfs_ioctl(struct file *file, unsi
 		return btrfs_ioctl_get_fslabel(file, argp);
 	case BTRFS_IOC_SET_FSLABEL:
 		return btrfs_ioctl_set_fslabel(file, argp);
+	case BTRFS_IOC_GET_SUPPORTED_FEATURES:
+		return btrfs_ioctl_get_supported_features(file, argp);
+	case BTRFS_IOC_GET_FEATURES:
+		return btrfs_ioctl_get_features(file, argp);
+	case BTRFS_IOC_SET_FEATURES:
+		return btrfs_ioctl_set_features(file, argp);
 	}
 
 	return -ENOTTY;
--- a/include/uapi/linux/btrfs.h	2013-09-09 17:49:10.808003254 -0400
+++ b/include/uapi/linux/btrfs.h	2013-09-09 17:49:12.483979430 -0400
@@ -184,6 +184,12 @@  struct btrfs_ioctl_fs_info_args {
 	__u64 reserved[124];			/* pad to 1k */
 };
 
+struct btrfs_ioctl_feature_flags {
+	__u64 compat_flags;
+	__u64 compat_ro_flags;
+	__u64 incompat_flags;
+};
+
 /* balance control ioctl modes */
 #define BTRFS_BALANCE_CTL_PAUSE		1
 #define BTRFS_BALANCE_CTL_CANCEL	2
@@ -579,4 +585,10 @@  static inline char *btrfs_err_str(enum b
 				      struct btrfs_ioctl_get_dev_stats)
 #define BTRFS_IOC_DEV_REPLACE _IOWR(BTRFS_IOCTL_MAGIC, 53, \
 				    struct btrfs_ioctl_dev_replace_args)
+#define BTRFS_IOC_GET_FEATURES _IOR(BTRFS_IOCTL_MAGIC, 54, \
+				   struct btrfs_ioctl_feature_flags)
+#define BTRFS_IOC_SET_FEATURES _IOW(BTRFS_IOCTL_MAGIC, 54, \
+				   struct btrfs_ioctl_feature_flags[2])
+#define BTRFS_IOC_GET_SUPPORTED_FEATURES _IOR(BTRFS_IOCTL_MAGIC, 54, \
+				   struct btrfs_ioctl_feature_flags[3])
 #endif /* _UAPI_LINUX_BTRFS_H */