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

[Jeff Mahoney <jeffm@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 4231 bytes --]

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

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 841 bytes --]