Btrfs devel - Nov 2010 - R: Re: [PATCH 5/5] btrfs: Add ioctl to set snapshot readonly/writable

Hi Li,
>----Messaggio originale----
>Da: lizf@cn.fujitsu.com
>Data: 30/11/2010 8.03
>A: <kreijack@libero.it>
>Cc: <linux-btrfs@vger.kernel.org>
>Ogg: Re: [PATCH 5/5] btrfs: Add ioctl to set snapshot readonly/writable
>
>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.
Of course I never mind about  _CREATE_ASYNC to be set in
btrfs_root_item-
>flags. 
_CREATE_ASYNC is not a snapshot properties but a way of creating a subvolume.
But other flags may make sense to live in btrfs_root_item->flags.
 So I am suggesting to develop a more general 
interface for future improvement. These pair of functions (*_set/get) should 
be use to 
set the subvolume/snapshot properties. And the RDONLY is one of them.

In the detail to set an attributue an user should be:

- get the subvolume flags (ioctl(fd, BTRFS_IOC_SNAP_GETFLAGS....) )
- compute the new flags ( flags |= BTRFS_FLAGS_XXXX or flags &= 
~BTRFS_FLAG_XXXX)
- set the subvolume flags (ioctl(fd, BTRFS_IOC_SNAP_SGETFLAGS....) )
>
>- It seems to me there''s no user requirement for getflags ioctl to
>return _RDONLY/_WRITABLE flags of a tree root?
And how an user/admin can understand that a snapshot/subvolume is readonly ? 
How 
>
>- By suggesting BTRFS_SUBVOL_RDONLY, does it impliy not only snapshot
>but also a subvolume can be made readonly?
IIRC a snapshot is a subvolume already filled from the beginning. Why
doesn''t
share 
the capability of make a subvolume RO ?

Finally I have another suggestion: make sense to check that the file 
descriptor is referring 
to the root of a subvolume instead of a the tree. I highlight that because the 
other ioctls
suffer  the same problem and confused the user sometime. For example a lot of
people tough that was possible to snapshot a directory, because the ioctl 
doesn''t return
any error. But instead of the directory the snapshot was of the full 
subvolume.

Goffredo

[...]

--
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

Goffredo Baroncelli <kreijack@libero.it> wrote:
> Hi Li,
> 
>> ----Messaggio originale----
>> Da: lizf@cn.fujitsu.com
>> Data: 30/11/2010 8.03
>> A: <kreijack@libero.it>
>> Cc: <linux-btrfs@vger.kernel.org>
>> Ogg: Re: [PATCH 5/5] btrfs: Add ioctl to set snapshot readonly/writable
>>
>> 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.
> 
> Of course I never mind about  _CREATE_ASYNC to be set in btrfs_root_item-
>> flags. 
> _CREATE_ASYNC is not a snapshot properties but a way of creating a
subvolume.
> But other flags may make sense to live in btrfs_root_item->flags.
>  So I am suggesting to develop a more general 
> interface for future improvement. These pair of functions (*_set/get)
should
> be use to 
> set the subvolume/snapshot properties. And the RDONLY is one of them.
> 
> In the detail to set an attributue an user should be:
> 
> - get the subvolume flags (ioctl(fd, BTRFS_IOC_SNAP_GETFLAGS....) )
> - compute the new flags ( flags |= BTRFS_FLAGS_XXXX or flags &= 
> ~BTRFS_FLAG_XXXX)
> - set the subvolume flags (ioctl(fd, BTRFS_IOC_SNAP_SGETFLAGS....) )
> 
Yep, this style can be seen in some other places.
>> - It seems to me there''s no user requirement for getflags
ioctl to
>> return _RDONLY/_WRITABLE flags of a tree root?
> 
> And how an user/admin can understand that a snapshot/subvolume is readonly
?
> How 
> 
>> - By suggesting BTRFS_SUBVOL_RDONLY, does it impliy not only snapshot
>> but also a subvolume can be made readonly?
> 
> IIRC a snapshot is a subvolume already filled from the beginning. Why
doesn''t
> share 
> the capability of make a subvolume RO ?
> 
I''m not against it, but make sure we''re on the same page.
> Finally I have another suggestion: make sense to check that the file 
> descriptor is referring 
> to the root of a subvolume instead of a the tree. I highlight that because
the
Will fix.
> other ioctls
> suffer  the same problem and confused the user sometime. For example a lot
of
> people tough that was possible to snapshot a directory, because the ioctl 
> doesn''t return
> any error. But instead of the directory the snapshot was of the full 
> subvolume.
> 
Sounds like a bug to me.
--
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