Goffredo Baroncelli wrote:
> Hi Li,
>
> On Monday, 29 November, 2010, Li Zefan wrote:
>> This allows us to set a snapshot readonly or writable on the fly.
>>
>> Usage:
>>
>> Set BTRFS_SNAPSHOT_RDONLY/WRITABLE of btrfs_ioctl_vol_arg_v2->flags,
>> and then call ioctl(BTRFS_IOCTL_SNAP_SETFLAGS);
>
> I really appreciate your work, but I have some doubt about this interface. In
> particolar:
It's the interface that I would like to be discussed. Thanks!
> - how get the flags of a subvolume ? I suggest to implement a pair of ioctls:
> - subvolume_setflags -> get the flags
> - subvolume_getflags -> set the flags
> These ioctls would be more generic (there are a lot of flags which may be
> interested to put in the "root" of a subvolume: think about
> compress/nocompress, (no)datasum...)
> - For the reason abowe, I suggest to replace SNAPSHOT with SUBVOLUME
> - Finally, with a pair of get/set_flags functions we can avoid the use of the
> flags BTRFS_SNAPSHOT_WRITABLE.
>
There are some reasons that I created this interface:
- set/getflags should set/get root flags which reflect in struct
btrfs_root_item->flags.
- btrfs_root_item->flags was not used at all before this patch, so
(no)compress and (no)datasum is not reflect in ->flags.
- _CREATE_ASYNC flag is to create snapshot asynchronously, so it's not
a flag of tree root.
- It seems to me there's no user requirement for getflags ioctl to
return _RDONLY/_WRITABLE flags of a tree root?
- By suggesting BTRFS_SUBVOL_RDONLY, does it impliy not only snapshot
but also a subvolume can be made readonly?
>
>> Signed-off-by: Li Zefan <lizf@xxxxxxxxxxxxxx>
>> ---
>> fs/btrfs/ioctl.c | 88
> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>> fs/btrfs/ioctl.h | 3 ++
>> 2 files changed, 87 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 7f9c571..34d8683 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -939,6 +939,24 @@ out:
>> return ret;
>> }
>>
>> +static int snap_check_flags(u64 flags, bool create)
>> +{
>> + u64 valid_flags = BTRFS_SNAPSHOT_RDONLY | BTRFS_SNAPSHOT_WRITABLE;
>> +
>> + if (create)
>> + valid_flags |= BTRFS_SNAPSHOT_CREATE_ASYNC;
>> +
>> + if (flags & valid_flags)
>> + return -EINVAL;
>> +
>> + /* readonly and writable are mutually exclusive */
>> + if ((flags & BTRFS_SNAPSHOT_RDONLY) &&
>> + (flags & BTRFS_SNAPSHOT_WRITABLE))
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> static noinline int btrfs_ioctl_snap_create(struct file *file,
>> void __user *arg, int subvol,
>> bool v2)
>> @@ -957,11 +975,9 @@ static noinline int btrfs_ioctl_snap_create(struct file
> *file,
>> if (IS_ERR(vol_args_v2))
>> return PTR_ERR(vol_args_v2);
>>
>> - if (vol_args_v2->flags &
>> - ~(BTRFS_SNAPSHOT_CREATE_ASYNC | BTRFS_SNAPSHOT_RDONLY)) {
>> - ret = -EINVAL;
>> + ret = snap_check_flags(vol_args_v2->flags, true);
>> + if (ret)
>> goto out;
>> - }
>>
>> name = vol_args_v2->name;
>> fd = vol_args_v2->fd;
>> @@ -995,6 +1011,65 @@ out:
>> return ret;
>> }
>>
>> +static noinline int btrfs_ioctl_snap_setflags(struct file *file,
>> + void __user *arg)
>> +{
>> + struct inode *inode = fdentry(file)->d_inode;
>> + struct btrfs_root *root = BTRFS_I(inode)->root;
>> + struct btrfs_trans_handle *trans;
>> + struct btrfs_ioctl_vol_args_v2 *vol_args_v2;
>> + u64 root_flags;
>> + u64 flags;
>> + int err;
>> +
>> + if (root->fs_info->sb->s_flags & MS_RDONLY)
>> + return -EROFS;
>> +
>> + vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
>> + if (IS_ERR(vol_args_v2))
>> + return PTR_ERR(vol_args_v2);
>> + flags = vol_args_v2->flags;
>> +
>> + err = snap_check_flags(flags, false);
>> + if (err)
>> + goto out;
>> +
>> + down_write(&root->fs_info->subvol_sem);
>> +
>> + /* nothing to do */
>> + if ((BTRFS_SNAPSHOT_RDONLY && root->readonly) ||
>> + (BTRFS_SNAPSHOT_WRITABLE && !root->readonly))
>> + goto out_unlock;
>> +
>> + root_flags = btrfs_root_flags(&root->root_item);
>> + if (flags & BTRFS_SNAPSHOT_RDONLY) {
>> + btrfs_set_root_flags(&root->root_item,
>> + root_flags | BTRFS_ROOT_SNAP_RDONLY);
>> + root->readonly = true;
>> + }
>> + if (flags & BTRFS_SNAPSHOT_WRITABLE) {
>> + btrfs_set_root_flags(&root->root_item,
>> + root_flags & ~BTRFS_ROOT_SNAP_RDONLY);
>> + root->readonly = false;
>> + }
>> +
>> + trans = btrfs_start_transaction(root, 1);
>> + if (IS_ERR(trans)) {
>> + err = PTR_ERR(trans);
>> + goto out_unlock;
>> + }
>> +
>> + err = btrfs_update_root(trans, root,
>> + &root->root_key, &root->root_item);
>> +
>> + btrfs_commit_transaction(trans, root);
>> +out_unlock:
>> + up_write(&root->fs_info->subvol_sem);
>> +out:
>> + kfree(vol_args_v2);
>> + return err;
>> +}
>> +
>> /*
>> * helper to check if the subvolume references other subvolumes
>> */
>> @@ -1503,6 +1578,9 @@ static int btrfs_ioctl_defrag(struct file *file, void
> __user *argp)
>> struct btrfs_ioctl_defrag_range_args *range;
>> int ret;
>>
>> + if (root->readonly)
>> + return -EROFS;
>> +
>> ret = mnt_want_write(file->f_path.mnt);
>> if (ret)
>> return ret;
>> @@ -2266,6 +2344,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>> return btrfs_ioctl_snap_create(file, argp, 1, 0);
>> case BTRFS_IOC_SNAP_DESTROY:
>> return btrfs_ioctl_snap_destroy(file, argp);
>> + case BTRFS_IOC_SNAP_SETFLAGS:
>> + return btrfs_ioctl_snap_setflags(file, argp);
>> case BTRFS_IOC_DEFAULT_SUBVOL:
>> return btrfs_ioctl_default_subvol(file, argp);
>> case BTRFS_IOC_DEFRAG:
>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
>> index ff15fb2..559fd27 100644
>> --- a/fs/btrfs/ioctl.h
>> +++ b/fs/btrfs/ioctl.h
>> @@ -32,6 +32,7 @@ struct btrfs_ioctl_vol_args {
>>
>> #define BTRFS_SNAPSHOT_CREATE_ASYNC (1ULL << 0)
>> #define BTRFS_SNAPSHOT_RDONLY (1ULL << 1)
>> +#define BTRFS_SNAPSHOT_WRITABLE (1ULL << 2)
>>
>> #define BTRFS_SNAPSHOT_NAME_MAX 4039
>> struct btrfs_ioctl_vol_args_v2 {
>> @@ -194,4 +195,6 @@ struct btrfs_ioctl_space_args {
>> #define BTRFS_IOC_WAIT_SYNC _IOW(BTRFS_IOCTL_MAGIC, 22, __u64)
>> #define BTRFS_IOC_SNAP_CREATE_V2 _IOW(BTRFS_IOCTL_MAGIC, 23, \
>> struct btrfs_ioctl_vol_args_v2)
>> +#define BTRFS_IOC_SNAP_SETFLAGS _IOW(BTRFS_IOCTL_MAGIC, 24, \
>> + struct btrfs_ioctl_vol_args_v2)
>> #endif
>> --
>> 1.6.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
