R: Re: [PATCH v2 5/5] Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctl
>Messaggio originale >Da: l...@cn.fujitsu.com >Data: 10/12/2010 8.12 >A: >Cc: >Ogg: Re: [PATCH v2 5/5] Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctl > >Goffredo Baroncelli wrote: >> 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); [...] >>> + /* 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; >> > >They are not equivalent. The former checks if the flags and the >root both have readonly bit set or cleared. Right, my fault >> 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. >> > >Yep, that's because if we don't need to flip it, we've already exited early. > >Note, there's only one flag. Yes, it is true. However I strongly suggest to avoid that. If someone adds another flag this may miss... Obviously if we get rid of the root->readonly we solve at the root the problem :) >> Moreover I have another question: why internally the flags is >> BTRFS_ROOT_SNAP_RDONLY, instead in user space the flag is BTRFS_SUBVOL_RDONLY >> ? >> > >That's my carelessness.. > >> 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. >> > >I'd prefer not to mix flags for root_item flags and vol ioctl flags. > >And CREATE_RDONLY and CREATE_ASYNC can also be valid for subvolumes too. >Support for async subvolume creation is already there, just lack of an >user interface. So I've changed _CREATE_SNAP_ASYNC to _CREATE_ASYNC >in the patch I sent just now. I fully gree >-- >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 > -- 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
Re: [PATCH v2 5/5] Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctl
On Fri, Dec 10, 2010 at 03:12:41PM +0800, Li Zefan wrote: > >> + /* 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; > > They are not equivalent. The former checks if the flags and the > root both have readonly bit set or cleared. This is an excellent reason to add what you just replied (above) as a comment into the code ahead of that line, so others don't make the same mistake in interpreting what it does: /* nothing to do */ /* check if flags and root both have readonly bit set or cleared */ if (!!(flags & BTRFS_SUBVOL_RDONLY) == root->readonly) goto out_unlock; "Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." - Brian W. Kernighan -- | Ian! D. Allen - idal...@idallen.ca - Ottawa, Ontario, Canada | Home Page: http://idallen.com/ Contact Improv: http://contactimprov.ca/ | College professor (Free/Libre GNU+Linux) at: http://teaching.idallen.com/ | Defend digital freedom: http://eff.org/ and have fun: http://fools.ca/ -- 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
Re: [PATCH v2 5/5] Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctl
Goffredo Baroncelli wrote:
> 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
>> ---
>> 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.
>
I was considering using the same structure for all subvolume ioctls,
but here seems it's overkill..
> [..]
>> +
>> +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;
>
They are not equivalent. The former checks if the flags and the
root both have readonly bit set or cleared.
> 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.
>
Yep, that's because if we don't need to flip it, we've already exited early.
Note, there's only one flag.
> Moreover I have another question: why internally the flags is
> BTRFS_ROOT_SNAP_RDONLY, instead in user space the flag is BTRFS_SUBVOL_RDONLY
> ?
>
That's my carelessness..
> 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.
>
I'd prefer not to mix flags for root_item flags and vol ioctl flags.
And CREATE_RDONLY and CREATE_ASYNC can also be valid for subvolumes too.
Support for async subvolume creation is already there, just lack of an
user interface. So I've changed _CREATE_SNAP_ASYNC to _CREATE_ASYNC
in the patch I sent just now.
--
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
Re: [PATCH v2 5/5] Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctl
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
> ---
> 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)
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
[PATCH v2 5/5] Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctl
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
---
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:
return ret;
}
+static noinline int btrfs_ioctl_subvol_getflags(struct file *file,
+ void __user *arg)
+{
+ struct inode *inode = fdentry(file)->d_inode;
+ struct btrfs_root *root = BTRFS_I(inode)->root;
+ 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));
+ if (IS_ERR(vol_args))
+ return PTR_ERR(vol_args);
+
+ down_read(&root->fs_info->subvol_sem);
+ if (root->readonly)
+ flags |= BTRFS_SUBVOL_RDONLY;
+ up_read(&root->fs_info->subvol_sem);
+
+ if (copy_to_user(arg + offsetof(struct btrfs_ioctl_vol_args_v2, flags),
+&flags, sizeof(flags)))
+ ret = -EFAULT;
+
+ kfree(vol_args);
+ return ret;
+}
+
+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));
+ 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;
+
+ 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;
+
+ trans = btrfs_start_transaction(root, 1);
+ if (IS_ERR(trans)) {
+ ret = PTR_ERR(trans);
+ goto out_unlock;
+ }
+
+ ret = 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);
+ return ret;
+}
+
/*
* helper to check if the subvolume references other subvolumes
*/
@@ -2283,6 +2371,10 @@ long btrfs_ioctl(struct file *file, unsigned int
return btrfs_ioctl_snap_create(file, argp, 1);
case BTRFS_IOC_SNAP_DESTROY:
return btrfs_ioctl_snap_destroy(file, argp);
+ case BTRFS_IOC_SUBVOL_GETFLAGS:
+ return btrfs_ioctl_subvol_getflags(file, argp);
+ case BTRFS_IOC_SUBVOL_SETFLAGS:
+ return btrfs_ioctl_subvol_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 192fa99..19f7259 100644
--- a/fs/btrfs/ioctl.h
+++ b/fs/btrfs/ioctl.h
@@ -194,4 +194,8 @@ 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_SUBVOL_GETFLAGS _IOW(BTRFS_IOCTL_MAGIC, 25, \
+ struct btrfs_ioctl_vol_args_v2)
+#define BTRFS_IOC_SUBVOL_SETFLAGS _IOW(BTRFS_IOCTL_MAGIC, 26, \
+ 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 majord...@vger.kernel.org
More majordomo info a
