* Re: [PATCH 1/5] btrfs: Make async snapshot ioctl more generic
2010-11-29 8:02 ` [PATCH 1/5] btrfs: Make async snapshot ioctl more generic Li Zefan
@ 2010-11-29 18:52 ` Goffredo Baroncelli
2010-11-30 1:13 ` Li Zefan
2010-11-29 19:28 ` Sage Weil
2010-12-07 19:04 ` Sage Weil
2 siblings, 1 reply; 7+ messages in thread
From: Goffredo Baroncelli @ 2010-11-29 18:52 UTC (permalink / raw)
To: linux-btrfs
Hi Li,
great work, but I have some suggestions:
On Monday, 29 November, 2010, Li Zefan wrote:
> So we don't have to add new structures as we create more ioctls
> for snapshots.
>
> Now to create async snapshot, set BTRFS_SNAPSHOT_CREATE_ASYNC bit of
> vol_arg_v2->flags, and then call ioctl(BTRFS_IOCT_SNAP_CREATE_V2).
>
> Note: this changes the async snapshot ioctl ABI, which was merged
> in .37 merge window, so we have to make this change into .37.
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
> fs/btrfs/ioctl.c | 34 +++++++++++++++++++++-------------
> fs/btrfs/ioctl.h | 12 ++++++++----
> 2 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 463d91b..d3f1a60 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -935,23 +935,31 @@ out:
>
> static noinline int btrfs_ioctl_snap_create(struct file *file,
> void __user *arg, int subvol,
> - int async)
> + bool v2)
This is a aesthetic suggestion: instead of passing a flag called v2 I suggest
to add two wrapper functions, which extract the parameters and passes all
available parameter to the "generic" function. Something like:
static inline btrfs_ioctl_snap_create_v1(struct file *file,
void __user *arg, int subvol)
{
vol_args = memdup_user(arg, sizeof(*vol_args));
if (IS_ERR(vol_args))
return PTR_ERR(vol_args);
name = vol_args->name;
fd = vol_args->fd;
vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
btrfs_ioctl_snap_create_generic(file, subvol, name, fd, 0, 0);
}
static inline btrfs_ioctl_snap_create_v2(struct file *file,
void __user *arg, int subvol,
)
{
vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
if (IS_ERR(vol_args_v2))
return PTR_ERR(vol_args_v2);
ret = snap_check_flags(vol_args_v2->flags, true);
if (ret)
goto out;
name = vol_args_v2->name;
fd = vol_args_v2->fd;
vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
async = true;
if (vol_args_v2->flags & BTRFS_SNAPSHOT_RDONLY)
readonly = true;
btrfs_ioctl_snap_create_generic(file, subvol, name, fd, async,
readonly);
}
and
case BTRFS_IOC_SNAP_CREATE:
return btrfs_ioctl_snap_create_v1(file, argp, 0);
case BTRFS_IOC_SNAP_CREATE_V2:
return btrfs_ioctl_snap_create_v2(file, argp, 0);
Frankly speaking, we could get rid of the subvol parameter adding another
wrapper function like "btrfs_ioctl_subvol_create( )", but this would be
another story :)
> {
> struct btrfs_ioctl_vol_args *vol_args = NULL;
> - struct btrfs_ioctl_async_vol_args *async_vol_args = NULL;
> + struct btrfs_ioctl_vol_args_v2 *vol_args_v2 = NULL;
> char *name;
> u64 fd;
> u64 transid = 0;
> + bool async = false;
> int ret;
>
> - if (async) {
> - async_vol_args = memdup_user(arg, sizeof(*async_vol_args));
> - if (IS_ERR(async_vol_args))
> - return PTR_ERR(async_vol_args);
> + if (v2) {
> + vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
> + if (IS_ERR(vol_args_v2))
> + return PTR_ERR(vol_args_v2);
>
> - name = async_vol_args->name;
> - fd = async_vol_args->fd;
> - async_vol_args->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
> + if (vol_args_v2->flags & ~BTRFS_SNAPSHOT_CREATE_ASYNC) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + name = vol_args_v2->name;
> + fd = vol_args_v2->fd;
> + vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
> + if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
> + async = true;
> } else {
> vol_args = memdup_user(arg, sizeof(*vol_args));
> if (IS_ERR(vol_args))
> @@ -966,13 +974,13 @@ static noinline int btrfs_ioctl_snap_create(struct
file *file,
>
> if (!ret && async) {
> if (copy_to_user(arg +
> - offsetof(struct btrfs_ioctl_async_vol_args,
> + offsetof(struct btrfs_ioctl_vol_args_v2,
> transid), &transid, sizeof(transid)))
> return -EFAULT;
> }
> -
> +out:
> kfree(vol_args);
> - kfree(async_vol_args);
> + kfree(vol_args_v2);
>
> return ret;
> }
> @@ -2235,7 +2243,7 @@ long btrfs_ioctl(struct file *file, unsigned int
> return btrfs_ioctl_getversion(file, argp);
> case BTRFS_IOC_SNAP_CREATE:
> return btrfs_ioctl_snap_create(file, argp, 0, 0);
> - case BTRFS_IOC_SNAP_CREATE_ASYNC:
> + case BTRFS_IOC_SNAP_CREATE_V2:
> return btrfs_ioctl_snap_create(file, argp, 0, 1);
> case BTRFS_IOC_SUBVOL_CREATE:
> return btrfs_ioctl_snap_create(file, argp, 1, 0);
> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
> index 17c99eb..bc70584 100644
> --- a/fs/btrfs/ioctl.h
> +++ b/fs/btrfs/ioctl.h
> @@ -30,10 +30,14 @@ struct btrfs_ioctl_vol_args {
> char name[BTRFS_PATH_NAME_MAX + 1];
> };
>
> -#define BTRFS_SNAPSHOT_NAME_MAX 4079
> -struct btrfs_ioctl_async_vol_args {
> +#define BTRFS_SNAPSHOT_CREATE_ASYNC (1ULL << 0)
> +
> +#define BTRFS_SNAPSHOT_NAME_MAX 4039
> +struct btrfs_ioctl_vol_args_v2 {
> __s64 fd;
> __u64 transid;
> + __u64 flags;
> + __u64 unused[4];
> char name[BTRFS_SNAPSHOT_NAME_MAX + 1];
> };
Why the unused fields ? What happens if you use a more recent btrfs-tools
which take advantage of these fields but the kernel is an old one ? At the
minimum please check the flags so
(flags^(BTRFS_SNAPSHOT_CREATE_ASYNC|BTRFS_SNAPSHOT_RDONLY)) == 0
and
unused[0..3] == 0
For future expansion I suggest to use a different ioctl. To me it seems a more
robust API.
>
> @@ -187,6 +191,6 @@ struct btrfs_ioctl_space_args {
> struct btrfs_ioctl_space_args)
> #define BTRFS_IOC_START_SYNC _IOR(BTRFS_IOCTL_MAGIC, 24, __u64)
> #define BTRFS_IOC_WAIT_SYNC _IOW(BTRFS_IOCTL_MAGIC, 22, __u64)
> -#define BTRFS_IOC_SNAP_CREATE_ASYNC _IOW(BTRFS_IOCTL_MAGIC, 23, \
> - struct btrfs_ioctl_async_vol_args)
> +#define BTRFS_IOC_SNAP_CREATE_V2 _IOW(BTRFS_IOCTL_MAGIC, 23, \
> + struct btrfs_ioctl_vol_args_v2)
I repeat: great work, please take my comments only a suggestion to improve
instead of an empty criticism.
Regards
G.Baroncelli
> #endif
> --
> 1.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
gpg key@ keyserver.linux.it: Goffredo Baroncelli (ghigo) <kreijack@inwind.it>
Key fingerprint = 4769 7E51 5293 D36C 814E C054 BF04 F161 3DC5 0512
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/5] btrfs: Make async snapshot ioctl more generic
2010-11-29 18:52 ` Goffredo Baroncelli
@ 2010-11-30 1:13 ` Li Zefan
0 siblings, 0 replies; 7+ messages in thread
From: Li Zefan @ 2010-11-30 1:13 UTC (permalink / raw)
To: kreijack; +Cc: linux-btrfs
Goffredo Baroncelli wrote:
> Hi Li,
>
> great work, but I have some suggestions:
>
> On Monday, 29 November, 2010, Li Zefan wrote:
>> So we don't have to add new structures as we create more ioctls
>> for snapshots.
>>
>> Now to create async snapshot, set BTRFS_SNAPSHOT_CREATE_ASYNC bit of
>> vol_arg_v2->flags, and then call ioctl(BTRFS_IOCT_SNAP_CREATE_V2).
>>
>> Note: this changes the async snapshot ioctl ABI, which was merged
>> in .37 merge window, so we have to make this change into .37.
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>> fs/btrfs/ioctl.c | 34 +++++++++++++++++++++-------------
>> fs/btrfs/ioctl.h | 12 ++++++++----
>> 2 files changed, 29 insertions(+), 17 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 463d91b..d3f1a60 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -935,23 +935,31 @@ out:
>>
>> static noinline int btrfs_ioctl_snap_create(struct file *file,
>> void __user *arg, int subvol,
>> - int async)
>> + bool v2)
>
> This is a aesthetic suggestion: instead of passing a flag called v2 I suggest
> to add two wrapper functions, which extract the parameters and passes all
> available parameter to the "generic" function. Something like:
>
Sure, as long as it won't result in code duplication and can
improve readability.
> static inline btrfs_ioctl_snap_create_v1(struct file *file,
> void __user *arg, int subvol)
> {
> vol_args = memdup_user(arg, sizeof(*vol_args));
> if (IS_ERR(vol_args))
> return PTR_ERR(vol_args);
> name = vol_args->name;
> fd = vol_args->fd;
> vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
>
> btrfs_ioctl_snap_create_generic(file, subvol, name, fd, 0, 0);
>
> }
>
> static inline btrfs_ioctl_snap_create_v2(struct file *file,
> void __user *arg, int subvol,
> )
> {
> vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
> if (IS_ERR(vol_args_v2))
> return PTR_ERR(vol_args_v2);
>
> ret = snap_check_flags(vol_args_v2->flags, true);
> if (ret)
> goto out;
>
> name = vol_args_v2->name;
> fd = vol_args_v2->fd;
> vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
> if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
> async = true;
> if (vol_args_v2->flags & BTRFS_SNAPSHOT_RDONLY)
> readonly = true;
>
> btrfs_ioctl_snap_create_generic(file, subvol, name, fd, async,
> readonly);
>
> }
>
> and
>
> case BTRFS_IOC_SNAP_CREATE:
> return btrfs_ioctl_snap_create_v1(file, argp, 0);
> case BTRFS_IOC_SNAP_CREATE_V2:
> return btrfs_ioctl_snap_create_v2(file, argp, 0);
>
>
> Frankly speaking, we could get rid of the subvol parameter adding another
> wrapper function like "btrfs_ioctl_subvol_create( )", but this would be
> another story :)
>
I thought about that too.
>> {
>> struct btrfs_ioctl_vol_args *vol_args = NULL;
>> - struct btrfs_ioctl_async_vol_args *async_vol_args = NULL;
>> + struct btrfs_ioctl_vol_args_v2 *vol_args_v2 = NULL;
>> char *name;
>> u64 fd;
>> u64 transid = 0;
>> + bool async = false;
>> int ret;
>>
>> - if (async) {
>> - async_vol_args = memdup_user(arg, sizeof(*async_vol_args));
>> - if (IS_ERR(async_vol_args))
>> - return PTR_ERR(async_vol_args);
>> + if (v2) {
>> + vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
>> + if (IS_ERR(vol_args_v2))
>> + return PTR_ERR(vol_args_v2);
>>
>> - name = async_vol_args->name;
>> - fd = async_vol_args->fd;
>> - async_vol_args->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
>> + if (vol_args_v2->flags & ~BTRFS_SNAPSHOT_CREATE_ASYNC) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + name = vol_args_v2->name;
>> + fd = vol_args_v2->fd;
>> + vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
>> + if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
>> + async = true;
>> } else {
>> vol_args = memdup_user(arg, sizeof(*vol_args));
>> if (IS_ERR(vol_args))
>> @@ -966,13 +974,13 @@ static noinline int btrfs_ioctl_snap_create(struct
> file *file,
>>
>> if (!ret && async) {
>> if (copy_to_user(arg +
>> - offsetof(struct btrfs_ioctl_async_vol_args,
>> + offsetof(struct btrfs_ioctl_vol_args_v2,
>> transid), &transid, sizeof(transid)))
>> return -EFAULT;
>> }
>> -
>> +out:
>> kfree(vol_args);
>> - kfree(async_vol_args);
>> + kfree(vol_args_v2);
>>
>> return ret;
>> }
>> @@ -2235,7 +2243,7 @@ long btrfs_ioctl(struct file *file, unsigned int
>> return btrfs_ioctl_getversion(file, argp);
>> case BTRFS_IOC_SNAP_CREATE:
>> return btrfs_ioctl_snap_create(file, argp, 0, 0);
>> - case BTRFS_IOC_SNAP_CREATE_ASYNC:
>> + case BTRFS_IOC_SNAP_CREATE_V2:
>> return btrfs_ioctl_snap_create(file, argp, 0, 1);
>> case BTRFS_IOC_SUBVOL_CREATE:
>> return btrfs_ioctl_snap_create(file, argp, 1, 0);
>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
>> index 17c99eb..bc70584 100644
>> --- a/fs/btrfs/ioctl.h
>> +++ b/fs/btrfs/ioctl.h
>> @@ -30,10 +30,14 @@ struct btrfs_ioctl_vol_args {
>> char name[BTRFS_PATH_NAME_MAX + 1];
>> };
>>
>> -#define BTRFS_SNAPSHOT_NAME_MAX 4079
>> -struct btrfs_ioctl_async_vol_args {
>> +#define BTRFS_SNAPSHOT_CREATE_ASYNC (1ULL << 0)
>> +
>> +#define BTRFS_SNAPSHOT_NAME_MAX 4039
>> +struct btrfs_ioctl_vol_args_v2 {
>> __s64 fd;
>> __u64 transid;
>> + __u64 flags;
>> + __u64 unused[4];
>> char name[BTRFS_SNAPSHOT_NAME_MAX + 1];
>> };
>
> Why the unused fields ? What happens if you use a more recent btrfs-tools
> which take advantage of these fields but the kernel is an old one ? At the
It's common that we reserve some place in an ABI for future expansion.
If later those unused bytes are used, it should make sure it won't
break old kernels.
> minimum please check the flags so
> (flags^(BTRFS_SNAPSHOT_CREATE_ASYNC|BTRFS_SNAPSHOT_RDONLY)) == 0
This is checked.
> and
> unused[0..3] == 0
No, we don't need to check this. I don't think other ioctls that have
reserved bytes check this.
>
> For future expansion I suggest to use a different ioctl. To me it seems a more
> robust API.
I'd like to avoid new ioctls if possible. If we had had some reserved
bytes in struct btrfs_ioctl_vol_args, we wouldn't need to create
a v2 ioctl.
>
>>
>> @@ -187,6 +191,6 @@ struct btrfs_ioctl_space_args {
>> struct btrfs_ioctl_space_args)
>> #define BTRFS_IOC_START_SYNC _IOR(BTRFS_IOCTL_MAGIC, 24, __u64)
>> #define BTRFS_IOC_WAIT_SYNC _IOW(BTRFS_IOCTL_MAGIC, 22, __u64)
>> -#define BTRFS_IOC_SNAP_CREATE_ASYNC _IOW(BTRFS_IOCTL_MAGIC, 23, \
>> - struct btrfs_ioctl_async_vol_args)
>> +#define BTRFS_IOC_SNAP_CREATE_V2 _IOW(BTRFS_IOCTL_MAGIC, 23, \
>> + struct btrfs_ioctl_vol_args_v2)
>
> I repeat: great work, please take my comments only a suggestion to improve
> instead of an empty criticism.
>
Thanks!
> Regards
> G.Baroncelli
>
>> #endif
>> --
>> 1.6.3
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/5] btrfs: Make async snapshot ioctl more generic
2010-11-29 8:02 ` [PATCH 1/5] btrfs: Make async snapshot ioctl more generic Li Zefan
2010-11-29 18:52 ` Goffredo Baroncelli
@ 2010-11-29 19:28 ` Sage Weil
2010-12-07 19:04 ` Sage Weil
2 siblings, 0 replies; 7+ messages in thread
From: Sage Weil @ 2010-11-29 19:28 UTC (permalink / raw)
To: Li Zefan; +Cc: linux-btrfs
Hi Li,
On Mon, 29 Nov 2010, Li Zefan wrote:
> So we don't have to add new structures as we create more ioctls
> for snapshots.
>
> Now to create async snapshot, set BTRFS_SNAPSHOT_CREATE_ASYNC bit of
> vol_arg_v2->flags, and then call ioctl(BTRFS_IOCT_SNAP_CREATE_V2).
>
> Note: this changes the async snapshot ioctl ABI, which was merged
> in .37 merge window, so we have to make this change into .37.
These changes all look good to me.
Thanks!
sage
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
> fs/btrfs/ioctl.c | 34 +++++++++++++++++++++-------------
> fs/btrfs/ioctl.h | 12 ++++++++----
> 2 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 463d91b..d3f1a60 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -935,23 +935,31 @@ out:
>
> static noinline int btrfs_ioctl_snap_create(struct file *file,
> void __user *arg, int subvol,
> - int async)
> + bool v2)
> {
> struct btrfs_ioctl_vol_args *vol_args = NULL;
> - struct btrfs_ioctl_async_vol_args *async_vol_args = NULL;
> + struct btrfs_ioctl_vol_args_v2 *vol_args_v2 = NULL;
> char *name;
> u64 fd;
> u64 transid = 0;
> + bool async = false;
> int ret;
>
> - if (async) {
> - async_vol_args = memdup_user(arg, sizeof(*async_vol_args));
> - if (IS_ERR(async_vol_args))
> - return PTR_ERR(async_vol_args);
> + if (v2) {
> + vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
> + if (IS_ERR(vol_args_v2))
> + return PTR_ERR(vol_args_v2);
>
> - name = async_vol_args->name;
> - fd = async_vol_args->fd;
> - async_vol_args->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
> + if (vol_args_v2->flags & ~BTRFS_SNAPSHOT_CREATE_ASYNC) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + name = vol_args_v2->name;
> + fd = vol_args_v2->fd;
> + vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
> + if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
> + async = true;
> } else {
> vol_args = memdup_user(arg, sizeof(*vol_args));
> if (IS_ERR(vol_args))
> @@ -966,13 +974,13 @@ static noinline int btrfs_ioctl_snap_create(struct file *file,
>
> if (!ret && async) {
> if (copy_to_user(arg +
> - offsetof(struct btrfs_ioctl_async_vol_args,
> + offsetof(struct btrfs_ioctl_vol_args_v2,
> transid), &transid, sizeof(transid)))
> return -EFAULT;
> }
> -
> +out:
> kfree(vol_args);
> - kfree(async_vol_args);
> + kfree(vol_args_v2);
>
> return ret;
> }
> @@ -2235,7 +2243,7 @@ long btrfs_ioctl(struct file *file, unsigned int
> return btrfs_ioctl_getversion(file, argp);
> case BTRFS_IOC_SNAP_CREATE:
> return btrfs_ioctl_snap_create(file, argp, 0, 0);
> - case BTRFS_IOC_SNAP_CREATE_ASYNC:
> + case BTRFS_IOC_SNAP_CREATE_V2:
> return btrfs_ioctl_snap_create(file, argp, 0, 1);
> case BTRFS_IOC_SUBVOL_CREATE:
> return btrfs_ioctl_snap_create(file, argp, 1, 0);
> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
> index 17c99eb..bc70584 100644
> --- a/fs/btrfs/ioctl.h
> +++ b/fs/btrfs/ioctl.h
> @@ -30,10 +30,14 @@ struct btrfs_ioctl_vol_args {
> char name[BTRFS_PATH_NAME_MAX + 1];
> };
>
> -#define BTRFS_SNAPSHOT_NAME_MAX 4079
> -struct btrfs_ioctl_async_vol_args {
> +#define BTRFS_SNAPSHOT_CREATE_ASYNC (1ULL << 0)
> +
> +#define BTRFS_SNAPSHOT_NAME_MAX 4039
> +struct btrfs_ioctl_vol_args_v2 {
> __s64 fd;
> __u64 transid;
> + __u64 flags;
> + __u64 unused[4];
> char name[BTRFS_SNAPSHOT_NAME_MAX + 1];
> };
>
> @@ -187,6 +191,6 @@ struct btrfs_ioctl_space_args {
> struct btrfs_ioctl_space_args)
> #define BTRFS_IOC_START_SYNC _IOR(BTRFS_IOCTL_MAGIC, 24, __u64)
> #define BTRFS_IOC_WAIT_SYNC _IOW(BTRFS_IOCTL_MAGIC, 22, __u64)
> -#define BTRFS_IOC_SNAP_CREATE_ASYNC _IOW(BTRFS_IOCTL_MAGIC, 23, \
> - struct btrfs_ioctl_async_vol_args)
> +#define BTRFS_IOC_SNAP_CREATE_V2 _IOW(BTRFS_IOCTL_MAGIC, 23, \
> + 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/5] btrfs: Make async snapshot ioctl more generic
2010-11-29 8:02 ` [PATCH 1/5] btrfs: Make async snapshot ioctl more generic Li Zefan
2010-11-29 18:52 ` Goffredo Baroncelli
2010-11-29 19:28 ` Sage Weil
@ 2010-12-07 19:04 ` Sage Weil
2010-12-08 1:09 ` Li Zefan
2 siblings, 1 reply; 7+ messages in thread
From: Sage Weil @ 2010-12-07 19:04 UTC (permalink / raw)
To: chris.mason; +Cc: Li Zefan, linux-btrfs
Hi Chris,
Is this ioctl change destined for 2.6.37? If it (or something similar)
looks good it should probably be merged (independent of the rest of the
series) this time around before we're stuck with the current version.
Thanks-
sage
On Mon, 29 Nov 2010, Li Zefan wrote:
> So we don't have to add new structures as we create more ioctls
> for snapshots.
>
> Now to create async snapshot, set BTRFS_SNAPSHOT_CREATE_ASYNC bit of
> vol_arg_v2->flags, and then call ioctl(BTRFS_IOCT_SNAP_CREATE_V2).
>
> Note: this changes the async snapshot ioctl ABI, which was merged
> in .37 merge window, so we have to make this change into .37.
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
> fs/btrfs/ioctl.c | 34 +++++++++++++++++++++-------------
> fs/btrfs/ioctl.h | 12 ++++++++----
> 2 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 463d91b..d3f1a60 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -935,23 +935,31 @@ out:
>
> static noinline int btrfs_ioctl_snap_create(struct file *file,
> void __user *arg, int subvol,
> - int async)
> + bool v2)
> {
> struct btrfs_ioctl_vol_args *vol_args = NULL;
> - struct btrfs_ioctl_async_vol_args *async_vol_args = NULL;
> + struct btrfs_ioctl_vol_args_v2 *vol_args_v2 = NULL;
> char *name;
> u64 fd;
> u64 transid = 0;
> + bool async = false;
> int ret;
>
> - if (async) {
> - async_vol_args = memdup_user(arg, sizeof(*async_vol_args));
> - if (IS_ERR(async_vol_args))
> - return PTR_ERR(async_vol_args);
> + if (v2) {
> + vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
> + if (IS_ERR(vol_args_v2))
> + return PTR_ERR(vol_args_v2);
>
> - name = async_vol_args->name;
> - fd = async_vol_args->fd;
> - async_vol_args->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
> + if (vol_args_v2->flags & ~BTRFS_SNAPSHOT_CREATE_ASYNC) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + name = vol_args_v2->name;
> + fd = vol_args_v2->fd;
> + vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
> + if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
> + async = true;
> } else {
> vol_args = memdup_user(arg, sizeof(*vol_args));
> if (IS_ERR(vol_args))
> @@ -966,13 +974,13 @@ static noinline int btrfs_ioctl_snap_create(struct file *file,
>
> if (!ret && async) {
> if (copy_to_user(arg +
> - offsetof(struct btrfs_ioctl_async_vol_args,
> + offsetof(struct btrfs_ioctl_vol_args_v2,
> transid), &transid, sizeof(transid)))
> return -EFAULT;
> }
> -
> +out:
> kfree(vol_args);
> - kfree(async_vol_args);
> + kfree(vol_args_v2);
>
> return ret;
> }
> @@ -2235,7 +2243,7 @@ long btrfs_ioctl(struct file *file, unsigned int
> return btrfs_ioctl_getversion(file, argp);
> case BTRFS_IOC_SNAP_CREATE:
> return btrfs_ioctl_snap_create(file, argp, 0, 0);
> - case BTRFS_IOC_SNAP_CREATE_ASYNC:
> + case BTRFS_IOC_SNAP_CREATE_V2:
> return btrfs_ioctl_snap_create(file, argp, 0, 1);
> case BTRFS_IOC_SUBVOL_CREATE:
> return btrfs_ioctl_snap_create(file, argp, 1, 0);
> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
> index 17c99eb..bc70584 100644
> --- a/fs/btrfs/ioctl.h
> +++ b/fs/btrfs/ioctl.h
> @@ -30,10 +30,14 @@ struct btrfs_ioctl_vol_args {
> char name[BTRFS_PATH_NAME_MAX + 1];
> };
>
> -#define BTRFS_SNAPSHOT_NAME_MAX 4079
> -struct btrfs_ioctl_async_vol_args {
> +#define BTRFS_SNAPSHOT_CREATE_ASYNC (1ULL << 0)
> +
> +#define BTRFS_SNAPSHOT_NAME_MAX 4039
> +struct btrfs_ioctl_vol_args_v2 {
> __s64 fd;
> __u64 transid;
> + __u64 flags;
> + __u64 unused[4];
> char name[BTRFS_SNAPSHOT_NAME_MAX + 1];
> };
>
> @@ -187,6 +191,6 @@ struct btrfs_ioctl_space_args {
> struct btrfs_ioctl_space_args)
> #define BTRFS_IOC_START_SYNC _IOR(BTRFS_IOCTL_MAGIC, 24, __u64)
> #define BTRFS_IOC_WAIT_SYNC _IOW(BTRFS_IOCTL_MAGIC, 22, __u64)
> -#define BTRFS_IOC_SNAP_CREATE_ASYNC _IOW(BTRFS_IOCTL_MAGIC, 23, \
> - struct btrfs_ioctl_async_vol_args)
> +#define BTRFS_IOC_SNAP_CREATE_V2 _IOW(BTRFS_IOCTL_MAGIC, 23, \
> + 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/5] btrfs: Make async snapshot ioctl more generic
2010-12-07 19:04 ` Sage Weil
@ 2010-12-08 1:09 ` Li Zefan
0 siblings, 0 replies; 7+ messages in thread
From: Li Zefan @ 2010-12-08 1:09 UTC (permalink / raw)
To: Sage Weil; +Cc: chris.mason, linux-btrfs
03:04, Sage Weil wrote:
> Hi Chris,
>
> Is this ioctl change destined for 2.6.37? If it (or something similar)
> looks good it should probably be merged (independent of the rest of the
> series) this time around before we're stuck with the current version.
>
I'm going to post a revised patchset today or tomorrow. Thanks!
btw, is there a typo?
#define BTRFS_IOC_SPACE_INFO _IOWR(BTRFS_IOCTL_MAGIC, 20, \
struct btrfs_ioctl_space_args)
#define BTRFS_IOC_START_SYNC _IOR(BTRFS_IOCTL_MAGIC, 24, __u64)
#define BTRFS_IOC_WAIT_SYNC _IOW(BTRFS_IOCTL_MAGIC, 22, __u64)
It seems we should have given 21 to START_SYNC ioctl.
> Thanks-
> sage
>
>
> On Mon, 29 Nov 2010, Li Zefan wrote:
>
>> So we don't have to add new structures as we create more ioctls
>> for snapshots.
>>
>> Now to create async snapshot, set BTRFS_SNAPSHOT_CREATE_ASYNC bit of
>> vol_arg_v2->flags, and then call ioctl(BTRFS_IOCT_SNAP_CREATE_V2).
>>
>> Note: this changes the async snapshot ioctl ABI, which was merged
>> in .37 merge window, so we have to make this change into .37.
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>> fs/btrfs/ioctl.c | 34 +++++++++++++++++++++-------------
>> fs/btrfs/ioctl.h | 12 ++++++++----
>> 2 files changed, 29 insertions(+), 17 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 463d91b..d3f1a60 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -935,23 +935,31 @@ out:
>>
>> static noinline int btrfs_ioctl_snap_create(struct file *file,
>> void __user *arg, int subvol,
>> - int async)
>> + bool v2)
>> {
>> struct btrfs_ioctl_vol_args *vol_args = NULL;
>> - struct btrfs_ioctl_async_vol_args *async_vol_args = NULL;
>> + struct btrfs_ioctl_vol_args_v2 *vol_args_v2 = NULL;
>> char *name;
>> u64 fd;
>> u64 transid = 0;
>> + bool async = false;
>> int ret;
>>
>> - if (async) {
>> - async_vol_args = memdup_user(arg, sizeof(*async_vol_args));
>> - if (IS_ERR(async_vol_args))
>> - return PTR_ERR(async_vol_args);
>> + if (v2) {
>> + vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
>> + if (IS_ERR(vol_args_v2))
>> + return PTR_ERR(vol_args_v2);
>>
>> - name = async_vol_args->name;
>> - fd = async_vol_args->fd;
>> - async_vol_args->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
>> + if (vol_args_v2->flags & ~BTRFS_SNAPSHOT_CREATE_ASYNC) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + name = vol_args_v2->name;
>> + fd = vol_args_v2->fd;
>> + vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
>> + if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
>> + async = true;
>> } else {
>> vol_args = memdup_user(arg, sizeof(*vol_args));
>> if (IS_ERR(vol_args))
>> @@ -966,13 +974,13 @@ static noinline int btrfs_ioctl_snap_create(struct file *file,
>>
>> if (!ret && async) {
>> if (copy_to_user(arg +
>> - offsetof(struct btrfs_ioctl_async_vol_args,
>> + offsetof(struct btrfs_ioctl_vol_args_v2,
>> transid), &transid, sizeof(transid)))
>> return -EFAULT;
>> }
>> -
>> +out:
>> kfree(vol_args);
>> - kfree(async_vol_args);
>> + kfree(vol_args_v2);
>>
>> return ret;
>> }
>> @@ -2235,7 +2243,7 @@ long btrfs_ioctl(struct file *file, unsigned int
>> return btrfs_ioctl_getversion(file, argp);
>> case BTRFS_IOC_SNAP_CREATE:
>> return btrfs_ioctl_snap_create(file, argp, 0, 0);
>> - case BTRFS_IOC_SNAP_CREATE_ASYNC:
>> + case BTRFS_IOC_SNAP_CREATE_V2:
>> return btrfs_ioctl_snap_create(file, argp, 0, 1);
>> case BTRFS_IOC_SUBVOL_CREATE:
>> return btrfs_ioctl_snap_create(file, argp, 1, 0);
>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
>> index 17c99eb..bc70584 100644
>> --- a/fs/btrfs/ioctl.h
>> +++ b/fs/btrfs/ioctl.h
>> @@ -30,10 +30,14 @@ struct btrfs_ioctl_vol_args {
>> char name[BTRFS_PATH_NAME_MAX + 1];
>> };
>>
>> -#define BTRFS_SNAPSHOT_NAME_MAX 4079
>> -struct btrfs_ioctl_async_vol_args {
>> +#define BTRFS_SNAPSHOT_CREATE_ASYNC (1ULL << 0)
>> +
>> +#define BTRFS_SNAPSHOT_NAME_MAX 4039
>> +struct btrfs_ioctl_vol_args_v2 {
>> __s64 fd;
>> __u64 transid;
>> + __u64 flags;
>> + __u64 unused[4];
>> char name[BTRFS_SNAPSHOT_NAME_MAX + 1];
>> };
>>
>> @@ -187,6 +191,6 @@ struct btrfs_ioctl_space_args {
>> struct btrfs_ioctl_space_args)
>> #define BTRFS_IOC_START_SYNC _IOR(BTRFS_IOCTL_MAGIC, 24, __u64)
>> #define BTRFS_IOC_WAIT_SYNC _IOW(BTRFS_IOCTL_MAGIC, 22, __u64)
>> -#define BTRFS_IOC_SNAP_CREATE_ASYNC _IOW(BTRFS_IOCTL_MAGIC, 23, \
>> - struct btrfs_ioctl_async_vol_args)
>> +#define BTRFS_IOC_SNAP_CREATE_V2 _IOW(BTRFS_IOCTL_MAGIC, 23, \
>> + struct btrfs_ioctl_vol_args_v2)
>> #endif
>> --
>> 1.6.3
>>
>> --
^ permalink raw reply [flat|nested] 7+ messages in thread