Hi Li,
On Thursday, 09 December, 2010, Li Zefan wrote:
> This allows us to set a snapshot or a subvolume readonly or writable
> on the fly.
>
> Usage:
>
> Set BTRFS_SUBVOL_RDONLY of btrfs_ioctl_vol_arg_v2->flags, and then
> call ioctl(BTRFS_IOCTL_SUBVOL_SETFLAGS);
>
> Changelog for v2:
> - Add _GETFLAGS ioctl.
> - Check if the passed fd is the root of a subvolume.
> - Change the name from _SNAP_SETFLAGS to _SUBVOL_SETFLAGS.
>
> Signed-off-by: Li Zefan <lizf@xxxxxxxxxxxxxx>
> ---
> fs/btrfs/ioctl.c | 92
++++++++++++++++++++++++++++++++++++++++++++++++++++++
> fs/btrfs/ioctl.h | 4 ++
> 2 files changed, 96 insertions(+), 0 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index db2b231..29304c7 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1010,6 +1010,94 @@ out:
[...]
> + struct btrfs_ioctl_vol_args_v2 *vol_args;
> + int ret = 0;
> + u64 flags = 0;
> +
> + if (inode->i_ino != BTRFS_FIRST_FREE_OBJECTID)
> + return -EINVAL;
> +
> + vol_args = memdup_user(arg, sizeof(*vol_args));
Would be better to avoid a dynamic allocation for a so small struct ?
And as more general comment: what is the reason to pass a so complex struct
only for setting/getting the flags ?
Would be it better to pass directly a u64 variable.
[..]
> +
> +static noinline int btrfs_ioctl_subvol_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;
> + u64 root_flags;
> + u64 flags;
> + int ret = 0;
> +
> + if (root->fs_info->sb->s_flags & MS_RDONLY)
> + return -EROFS;
> +
> + if (inode->i_ino != BTRFS_FIRST_FREE_OBJECTID)
> + return -EINVAL;
> +
> + vol_args = memdup_user(arg, sizeof(*vol_args));
Same as before.
> + if (IS_ERR(vol_args))
> + return PTR_ERR(vol_args);
> + flags = vol_args->flags;
> +
> + if (flags & ~BTRFS_SUBVOL_RDONLY) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + down_write(&root->fs_info->subvol_sem);
> +
> + /* nothing to do */
> + if (!!(flags & BTRFS_SUBVOL_RDONLY) == root->readonly)
> + goto out_unlock;
This is only an aesthetic comment: I prefer a simpler style like
if ((flags & BTRFS_SUBVOL_RDONLY) && root->readonly)
goto out_unlock;
But I know that every body has its style :-)
> +
> + root_flags = btrfs_root_flags(&root->root_item);
> + if (flags & BTRFS_SUBVOL_RDONLY)
> + btrfs_set_root_flags(&root->root_item,
> + root_flags | BTRFS_ROOT_SNAP_RDONLY);
> + else
> + btrfs_set_root_flags(&root->root_item,
> + root_flags & ~BTRFS_ROOT_SNAP_RDONLY);
> + root->readonly = !root->readonly;
I double checked this line. But if I read the code correctly I think that the
line above is wrong: the field "root->readonly" is flipped regardless the
value of the flags passed by the user.
Moreover I have another question: why internally the flags is
BTRFS_ROOT_SNAP_RDONLY, instead in user space the flag is BTRFS_SUBVOL_RDONLY
?
I suggest to
- rename BTRFS_SUBVOL_RDONLY in BTRFS_SUBVOL_CREATE_SNAP_RDONLY (like
BTRFS_SUBVOL_CREATE_SNAP_ASYNC)
- rename BTRFS_ROOT_SNAP_RDONLY in BTRFS_ROOT_FLAGS_RDONLY and use both in
userspace and in kernel space this flag. I suggested to remove SNAP because
the flag make sense also for a "standard" subvolume.
Gegards
G.Baroncelli
--
gpg key@ keyserver.linux.it: Goffredo Baroncelli (ghigo) <kreijack@xxxxxxxxx>
Key fingerprint = 4769 7E51 5293 D36C 814E C054 BF04 F161 3DC5 0512
--
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
