On 2018/05/15 16:57, Gu, Jinxiang/顾 金香 wrote: > Hi, add a missed a comment. > >> -----Original Message----- >> From: Misono Tomohiro [mailto:misono.tomohiro@xxxxxxxxxxxxxx] >> Sent: Tuesday, May 15, 2018 3:04 PM >> To: Gu, Jinxiang/顾 金香 <gujx@xxxxxxxxxxxxxx>; linux-btrfs@xxxxxxxxxxxxxxx >> Subject: Re: [PATCH v4 1/3] btrfs: Add unprivileged ioctl which returns subvolume information >> >> On 2018/05/15 15:31, Gu, Jinxiang/顾 金香 wrote: >>> Hi, >>> >>>> -----Original Message----- >>>> From: linux-btrfs-owner@xxxxxxxxxxxxxxx >>>> [mailto:linux-btrfs-owner@xxxxxxxxxxxxxxx] On Behalf Of Tomohiro >>>> Misono >>>> Sent: Friday, May 11, 2018 3:26 PM >>>> To: linux-btrfs@xxxxxxxxxxxxxxx >>>> Subject: [PATCH v4 1/3] btrfs: Add unprivileged ioctl which returns >>>> subvolume information >>>> >>>> Add new unprivileged ioctl BTRFS_IOC_GET_SUBVOL_INFO which returns the information of subvolume containing this inode. >>>> (i.e. returns the information in ROOT_ITEM and ROOT_BACKREF.) >>>> >>>> Signed-off-by: Tomohiro Misono <misono.tomohiro@xxxxxxxxxxxxxx> >>>> --- >>>> fs/btrfs/ioctl.c | 129 +++++++++++++++++++++++++++++++++++++++++++++ >>>> include/uapi/linux/btrfs.h | 51 ++++++++++++++++++ >>>> 2 files changed, 180 insertions(+) >>>> >>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index >>>> 48e2ddff32bd..64b23e22852f 100644 >>>> --- a/fs/btrfs/ioctl.c >>>> +++ b/fs/btrfs/ioctl.c >>>> @@ -2242,6 +2242,133 @@ static noinline int btrfs_ioctl_ino_lookup(struct file *file, >>>> return ret; >>>> } >>>> >>>> +/* Get the subvolume information in BTRFS_ROOT_ITEM and >>>> +BTRFS_ROOT_BACKREF */ static noinline int btrfs_ioctl_get_subvol_info(struct file *file, >>>> + void __user *argp) >>>> +{ >>>> + struct btrfs_ioctl_get_subvol_info_args *subvol_info; >>>> + struct btrfs_root *root; >>>> + struct btrfs_path *path; >>>> + struct btrfs_key key; >>>> + >>>> + struct btrfs_root_item root_item; >>>> + struct btrfs_root_ref *rref; >>>> + struct extent_buffer *l; >>>> + int slot; >>>> + >>>> + unsigned long item_off; >>>> + unsigned long item_len; >>>> + >>>> + struct inode *inode; >>>> + int ret; >>>> + >>>> + path = btrfs_alloc_path(); >>>> + if (!path) >>>> + return -ENOMEM; >>>> + >>>> + subvol_info = kzalloc(sizeof(*subvol_info), GFP_KERNEL); >>>> + if (!subvol_info) { >>>> + btrfs_free_path(path); >>>> + return -ENOMEM; >>>> + } >>>> + inode = file_inode(file); >>>> + >>>> + root = BTRFS_I(inode)->root->fs_info->tree_root; >>>> + key.objectid = BTRFS_I(inode)->root->root_key.objectid; >>>> + key.type = BTRFS_ROOT_ITEM_KEY; >>>> + key.offset = 0; >>>> + ret = btrfs_search_slot(NULL, root, &key, path, 0, 0); >>>> + if (ret < 0) { >>>> + goto out; >>>> + } else if (ret > 0) { >>>> + u64 objectid = key.objectid; >>>> + >>>> + if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) { >>>> + ret = btrfs_next_leaf(root, path); >>>> + if (ret < 0) >>>> + return ret; >>> Should goto out; to free subvol_info and path. >> Thanks, will update both. >> > > Since btrfs_next_leaf may return 1 when nritems of next leaf is 0, > So, btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]); may goes wrong. > And I think it should add a judge before btrfs_item_key_to_cpu. Ok, I will update to handle all cases. [snip]>>>> + l = path->nodes[0]; >>>> + slot = path->slots[0]; >>>> + btrfs_item_key_to_cpu(l, &key, slot); >>>> + if (key.objectid == subvol_info->id && >>>> + key.type == BTRFS_ROOT_BACKREF_KEY){ >>>> + subvol_info->parent_id = key.offset; >>>> + >>>> + rref = btrfs_item_ptr(l, slot, struct btrfs_root_ref); >>>> + subvol_info->dirid = btrfs_root_ref_dirid(l, rref); >>>> + >>>> + item_off = btrfs_item_ptr_offset(l, slot) >>>> + + sizeof(struct btrfs_root_ref); >>>> + item_len = btrfs_item_size_nr(l, slot) >>>> + - sizeof(struct btrfs_root_ref); >>>> + read_extent_buffer(l, subvol_info->name, item_off, item_len); >>>> + } >>> If this if is not correct(ex. corrupt filesystem without backref), >>> should it return -ENOENT, or its be ok without parent_id, dirid and name. >>> Suggest to add logic of else. >> >> If backref does not exist (except top-level subvolume), it means filesystem corruption. >> So, I'd like to return -EUCLEAN here. On second thought, I notice if this ioctl is called after containing subvolume is deleted, the entry may not exist. So, -ENOENT is fine. Thanks, Tomohiro Misono >> >> Thanks, >> Tomohiro Misono >> >>> >>>> + >>>> + if (copy_to_user(argp, subvol_info, sizeof(*subvol_info))) >>>> + ret = -EFAULT; >>>> + >>>> +out: >>>> + kzfree(subvol_info); >>>> + btrfs_free_path(path); >>>> + return ret; >>>> +} >>>> + >>>> static noinline int btrfs_ioctl_snap_destroy(struct file *file, >>>> void __user *arg) >>>> { >>>> @@ -5374,6 +5501,8 @@ long btrfs_ioctl(struct file *file, unsigned int >>>> return btrfs_ioctl_get_features(file, argp); >>>> case BTRFS_IOC_SET_FEATURES: >>>> return btrfs_ioctl_set_features(file, argp); >>>> + case BTRFS_IOC_GET_SUBVOL_INFO: >>>> + return btrfs_ioctl_get_subvol_info(file, argp); >>>> } >>>> >>>> return -ENOTTY; >>>> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h >>>> index c8d99b9ca550..ed053852c71f 100644 >>>> --- a/include/uapi/linux/btrfs.h >>>> +++ b/include/uapi/linux/btrfs.h >>>> @@ -725,6 +725,55 @@ struct btrfs_ioctl_send_args { >>>> __u64 reserved[4]; /* in */ >>>> }; >>>> >>>> +struct btrfs_ioctl_get_subvol_info_args { >>>> + /* All filed is out */ >>>> + /* Id of this subvolume */ >>>> + __u64 id; >>>> + /* Name of this subvolume, used to get the real name at mount point */ >>>> + char name[BTRFS_VOL_NAME_MAX + 1]; >>>> + /* >>>> + * Id of the subvolume which contains this subvolume. >>>> + * Zero for top-level subvolume or deleted subvolume >>>> + */ >>>> + __u64 parent_id; >>>> + /* >>>> + * Inode number of the directory which contains this subvolume. >>>> + * Zero for top-level subvolume or deleted subvolume >>>> + */ >>>> + __u64 dirid; >>>> + >>>> + /* Latest transaction id of this subvolume */ >>>> + __u64 generation; >>>> + /* Flags of this subvolume */ >>>> + __u64 flags; >>>> + >>>> + /* uuid of this subvolume */ >>>> + __u8 uuid[BTRFS_UUID_SIZE]; >>>> + /* >>>> + * uuid of the subvolume of which this subvolume is a snapshot. >>>> + * All zero for non-snapshot subvolume >>>> + */ >>>> + __u8 parent_uuid[BTRFS_UUID_SIZE]; >>>> + /* >>>> + * uuid of the subvolume from which this subvolume is received. >>>> + * All zero for non-received subvolume >>>> + */ >>>> + __u8 received_uuid[BTRFS_UUID_SIZE]; >>>> + >>>> + /* Transaction id indicates when change/create/send/receive happens */ >>>> + __u64 ctransid; >>>> + __u64 otransid; >>>> + __u64 stransid; >>>> + __u64 rtransid; >>>> + /* Time corresponds to c/o/s/rtransid */ >>>> + struct btrfs_ioctl_timespec ctime; >>>> + struct btrfs_ioctl_timespec otime; >>>> + struct btrfs_ioctl_timespec stime; >>>> + struct btrfs_ioctl_timespec rtime; >>>> + >>>> + __u64 reserved[8]; >>>> +}; >>>> + >>>> /* Error codes as returned by the kernel */ enum btrfs_err_code { >>>> BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET = 1, @@ -843,5 +892,7 @@ enum btrfs_err_code { >>>> struct btrfs_ioctl_vol_args_v2) #define >>>> BTRFS_IOC_LOGICAL_INO_V2 _IOWR(BTRFS_IOCTL_MAGIC, 59, \ >>>> struct btrfs_ioctl_logical_ino_args) >>>> +#define BTRFS_IOC_GET_SUBVOL_INFO _IOR(BTRFS_IOCTL_MAGIC, 60, \ >>>> + struct btrfs_ioctl_get_subvol_info_args) >>>> >>>> #endif /* _UAPI_LINUX_BTRFS_H */ >>>> -- >>>> 2.14.3 >>>> >>> >>> Reviewed-by: Gu Jinxiang <gujx@xxxxxxxxxxxxxx> >>>> >>>> -- >>>> 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 >>>> >>> -- 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
