On 03/07/2018 01:40 AM, Misono, Tomohiro wrote:
> On 2018/03/07 5:29, Goffredo Baroncelli wrote:
>> On 03/06/2018 09:30 AM, Misono, Tomohiro wrote:
>>> Add new unprivileged ioctl (BTRFS_IOC_GET_SUBVOL_INFO) which searches
>>> and returns only subvolume related item (ROOT_ITEM/ROOT_BACKREF/ROOT_REF)
>>> from root tree. The arguments of this ioctl are the same as treesearch
>>> ioctl and can be used like treesearch ioctl.
>> Is it a pro ? The treesearch ioctl is tightly coupled to the btrfs internal 
>> structure, this means that if we would change the btrfs internal structure, 
>> we have to update all the clients of this api. For the treesearch it is an 
>> acceptable compromise between flexibility and speed of developing. But for a 
>> more specialized API, I think that it would make sense to provide a less 
>> coupled api to the internal structure.
> Thanks for the comments.
> The reason I choose the same api is just to minimize the code change in 
> btrfs-progs.
> For tree search part, it works just switching the ioctl number from 
> to BTRFS_IOC_GET_SUBVOL_INFO in list_subvol_search()[1].
> [1] https://marc.info/?l=linux-btrfs&m=152032537911218&w=2

I suggest to avoid this approach. An API is forever; we already have a 
"root-only" one which is quite unfriendly and error prone; I think that we 
should put all the energies to make a better one. 

I think that the major weaknesses of this api are:
- it exports the the data in "le" format  (see struct btrfs_root_item as 
- it requires the user to increase the key for the next ioctl call. This could 
be doable in the kernel space before returning
- this ioctl exports both the ROOT_BACKREF and ROOT_ITEM info. Why not make two 
separate (simplers) ioctl(s) ?

>> Below some comments

>>> +   if ((key->objectid == BTRFS_FS_TREE_OBJECTID ||
>>> +           (key->objectid >= BTRFS_FIRST_FREE_OBJECTID &&
>>> +            key->objectid <= BTRFS_LAST_FREE_OBJECTID)) &&
>>> +       key->type >= BTRFS_ROOT_ITEM_KEY &&
>>> +       key->type <= BTRFS_ROOT_BACKREF_KEY)
>> Why returning all the range [BTRFS_ROOT_ITEM_KEY...BTRFS_ROOT_BACKREF_KEY]. 
>> It would be sufficient to return only
>>   +      (key->type == BTRFS_ROOT_ITEM_KEY ||
>>   +       key->type == BTRFS_ROOT_BACKREF_KEY))
> Sorry, it is a mistake, I mean "key->type <= BTRFS_ROOTREF_KEY".
> Although btrfs-progs only uses BTRFS_ROOT_BACKREF_KEY, I notice libbtrfs 
> uses BTRFS_ROOT_REF_KEY instead. So, I think it is better to return both
> ROOT_BACKREF_KEY and ROOT_REF_KEY. I will fix this in v2.

I was referring to the '>=' and '<=' instead of '=='. If another type is added 
in the middle, it would be returned. I find it a bit error prone.

gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5
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

Reply via email to