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 BTRFS_IOC_TREE_SEARCH > 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 example); - 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. BR G.Baroncelli -- 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
