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 <l...@cn.fujitsu.com> > --- > 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) <kreij...@inwind.it> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html