| Message ID | 20210913131729.37897-6-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement progs support for removing received uuid on RW vols | expand |
On 2021/9/13 下午9:17, Nikolay Borisov wrote: > It will be used to clear received data on RW snapshots that were > received. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > kernel-shared/ctree.h | 3 ++ > kernel-shared/uuid-tree.c | 82 +++++++++++++++++++++++++++++++++++++++ > 2 files changed, 85 insertions(+) > > diff --git a/kernel-shared/ctree.h b/kernel-shared/ctree.h > index 91a85796a678..158281a9fd67 100644 > --- a/kernel-shared/ctree.h > +++ b/kernel-shared/ctree.h > @@ -2860,6 +2860,9 @@ int btrfs_uuid_tree_add(struct btrfs_trans_handle *trans, u8 *uuid, u8 type, > u64 subvol_id_cpu); > > int btrfs_is_empty_uuid(u8 *uuid); > +int btrfs_uuid_tree_remove(struct btrfs_trans_handle *trans, u8 *uuid, u8 type, > + u64 subid); > + I have checked the callers in the incoming check patches, they are all starting a new trans on uuid tree, thus there is no need to start a trans out of this function. Let's just pass the subvolume root into the function, and start a new trans in side the function. So that we only need one parameter @root, everything else can be fetched from @root, and we can also modify the root item and uuid tree just in one go. Thanks, Qu > > static inline int is_fstree(u64 rootid) > { > diff --git a/kernel-shared/uuid-tree.c b/kernel-shared/uuid-tree.c > index 51a7b5d9ff5d..0f0fbf667dda 100644 > --- a/kernel-shared/uuid-tree.c > +++ b/kernel-shared/uuid-tree.c > @@ -120,3 +120,85 @@ int btrfs_is_empty_uuid(u8 *uuid) > } > return 1; > } > + > +int btrfs_uuid_tree_remove(struct btrfs_trans_handle *trans, u8 *uuid, u8 type, > + u64 subid) > +{ > + struct btrfs_fs_info *fs_info = trans->fs_info; > + struct btrfs_root *uuid_root = fs_info->uuid_root; > + int ret; > + struct btrfs_path *path = NULL; > + struct btrfs_key key; > + struct extent_buffer *eb; > + int slot; > + unsigned long offset; > + u32 item_size; > + unsigned long move_dst; > + unsigned long move_src; > + unsigned long move_len; > + > + if (!uuid_root) { > + ret = -EINVAL; > + goto out; > + } > + > + btrfs_uuid_to_key(uuid, &key); > + key.type = type; > + > + path = btrfs_alloc_path(); > + if (!path) { > + ret = -ENOMEM; > + goto out; > + } > + > + ret = btrfs_search_slot(trans, uuid_root, &key, path, -1, 1); > + if (ret < 0) { > + warning("error %d while searching for uuid item!", ret); > + goto out; > + } > + if (ret > 0) { > + ret = -ENOENT; > + goto out; > + } > + > + > + eb = path->nodes[0]; > + slot = path->slots[0]; > + offset = btrfs_item_ptr_offset(eb, slot); > + item_size = btrfs_item_size_nr(eb, slot); > + if (!IS_ALIGNED(item_size, sizeof(u64))) { > + warning("uuid item with illegal size %lu!", (unsigned long)item_size); > + ret = -ENOENT; > + goto out; > + } > + while (item_size) { > + __le64 read_subid; > + > + read_extent_buffer(eb, &read_subid, offset, sizeof(read_subid)); > + if (le64_to_cpu(read_subid) == subid) > + break; > + offset += sizeof(read_subid); > + item_size -= sizeof(read_subid); > + } > + > + if (!item_size) { > + ret = -ENOENT; > + goto out; > + } > + > + item_size = btrfs_item_size_nr(eb, slot); > + if (item_size == sizeof(subid)) { > + ret = btrfs_del_item(trans, uuid_root, path); > + goto out; > + } > + > + move_dst = offset; > + move_src = offset + sizeof(subid); > + move_len = item_size - (move_src - btrfs_item_ptr_offset(eb, slot)); > + memmove_extent_buffer(eb, move_dst, move_src, move_len); > + btrfs_truncate_item(path, item_size - sizeof(subid), 1); > + > +out: > + btrfs_free_path(path); > + return ret; > +} >
On 14.09.21 г. 4:07, Qu Wenruo wrote: > > > On 2021/9/13 下午9:17, Nikolay Borisov wrote: >> It will be used to clear received data on RW snapshots that were >> received. >> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com> >> --- >> kernel-shared/ctree.h | 3 ++ >> kernel-shared/uuid-tree.c | 82 +++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 85 insertions(+) >> >> diff --git a/kernel-shared/ctree.h b/kernel-shared/ctree.h >> index 91a85796a678..158281a9fd67 100644 >> --- a/kernel-shared/ctree.h >> +++ b/kernel-shared/ctree.h >> @@ -2860,6 +2860,9 @@ int btrfs_uuid_tree_add(struct >> btrfs_trans_handle *trans, u8 *uuid, u8 type, >> u64 subvol_id_cpu); >> >> int btrfs_is_empty_uuid(u8 *uuid); >> +int btrfs_uuid_tree_remove(struct btrfs_trans_handle *trans, u8 >> *uuid, u8 type, >> + u64 subid); >> + > > I have checked the callers in the incoming check patches, they are all > starting a new trans on uuid tree, thus there is no need to start a > trans out of this function. > > Let's just pass the subvolume root into the function, and start a new > trans in side the function. > > So that we only need one parameter @root, everything else can be fetched > from @root, and we can also modify the root item and uuid tree just in > one go. I disagree, this function is a direct copy from the kernel with the only adjustment that btrfs_uuid_to_key doesn't set key.type. If I change it in progs then this means it won't be unified with the kernel when this happens (if ever). I really rather keep shared functions as close as possible. > > Thanks, > Qu >> >> static inline int is_fstree(u64 rootid) >> { >> diff --git a/kernel-shared/uuid-tree.c b/kernel-shared/uuid-tree.c >> index 51a7b5d9ff5d..0f0fbf667dda 100644 >> --- a/kernel-shared/uuid-tree.c >> +++ b/kernel-shared/uuid-tree.c >> @@ -120,3 +120,85 @@ int btrfs_is_empty_uuid(u8 *uuid) >> } >> return 1; >> } >> + >> +int btrfs_uuid_tree_remove(struct btrfs_trans_handle *trans, u8 >> *uuid, u8 type, >> + u64 subid) >> +{ >> + struct btrfs_fs_info *fs_info = trans->fs_info; >> + struct btrfs_root *uuid_root = fs_info->uuid_root; >> + int ret; >> + struct btrfs_path *path = NULL; >> + struct btrfs_key key; >> + struct extent_buffer *eb; >> + int slot; >> + unsigned long offset; >> + u32 item_size; >> + unsigned long move_dst; >> + unsigned long move_src; >> + unsigned long move_len; >> + >> + if (!uuid_root) { >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + btrfs_uuid_to_key(uuid, &key); >> + key.type = type; >> + >> + path = btrfs_alloc_path(); >> + if (!path) { >> + ret = -ENOMEM; >> + goto out; >> + } >> + >> + ret = btrfs_search_slot(trans, uuid_root, &key, path, -1, 1); >> + if (ret < 0) { >> + warning("error %d while searching for uuid item!", ret); >> + goto out; >> + } >> + if (ret > 0) { >> + ret = -ENOENT; >> + goto out; >> + } >> + >> + >> + eb = path->nodes[0]; >> + slot = path->slots[0]; >> + offset = btrfs_item_ptr_offset(eb, slot); >> + item_size = btrfs_item_size_nr(eb, slot); >> + if (!IS_ALIGNED(item_size, sizeof(u64))) { >> + warning("uuid item with illegal size %lu!", (unsigned >> long)item_size); >> + ret = -ENOENT; >> + goto out; >> + } >> + while (item_size) { >> + __le64 read_subid; >> + >> + read_extent_buffer(eb, &read_subid, offset, sizeof(read_subid)); >> + if (le64_to_cpu(read_subid) == subid) >> + break; >> + offset += sizeof(read_subid); >> + item_size -= sizeof(read_subid); >> + } >> + >> + if (!item_size) { >> + ret = -ENOENT; >> + goto out; >> + } >> + >> + item_size = btrfs_item_size_nr(eb, slot); >> + if (item_size == sizeof(subid)) { >> + ret = btrfs_del_item(trans, uuid_root, path); >> + goto out; >> + } >> + >> + move_dst = offset; >> + move_src = offset + sizeof(subid); >> + move_len = item_size - (move_src - btrfs_item_ptr_offset(eb, slot)); >> + memmove_extent_buffer(eb, move_dst, move_src, move_len); >> + btrfs_truncate_item(path, item_size - sizeof(subid), 1); >> + >> +out: >> + btrfs_free_path(path); >> + return ret; >> +} >> >
On 2021/9/14 下午12:49, Nikolay Borisov wrote: > > > On 14.09.21 г. 4:07, Qu Wenruo wrote: >> >> >> On 2021/9/13 下午9:17, Nikolay Borisov wrote: >>> It will be used to clear received data on RW snapshots that were >>> received. >>> >>> Signed-off-by: Nikolay Borisov <nborisov@suse.com> >>> --- >>> kernel-shared/ctree.h | 3 ++ >>> kernel-shared/uuid-tree.c | 82 +++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 85 insertions(+) >>> >>> diff --git a/kernel-shared/ctree.h b/kernel-shared/ctree.h >>> index 91a85796a678..158281a9fd67 100644 >>> --- a/kernel-shared/ctree.h >>> +++ b/kernel-shared/ctree.h >>> @@ -2860,6 +2860,9 @@ int btrfs_uuid_tree_add(struct >>> btrfs_trans_handle *trans, u8 *uuid, u8 type, >>> u64 subvol_id_cpu); >>> >>> int btrfs_is_empty_uuid(u8 *uuid); >>> +int btrfs_uuid_tree_remove(struct btrfs_trans_handle *trans, u8 >>> *uuid, u8 type, >>> + u64 subid); >>> + >> >> I have checked the callers in the incoming check patches, they are all >> starting a new trans on uuid tree, thus there is no need to start a >> trans out of this function. >> >> Let's just pass the subvolume root into the function, and start a new >> trans in side the function. >> >> So that we only need one parameter @root, everything else can be fetched >> from @root, and we can also modify the root item and uuid tree just in >> one go. > > I disagree, this function is a direct copy from the kernel with the only > adjustment that btrfs_uuid_to_key doesn't set key.type. If I change it > in progs then this means it won't be unified with the kernel when this > happens (if ever). I really rather keep shared functions as close as > possible. OK, didn't notice it's really some shared function. (in kernel it's implemented in uuid-tree.c, not ctree.c). Then maybe it's better to introduce a btrfs-progs specific helper like btrfs_remove_recevied_uuid(), other than calling the function directly. In fact, both mode shares the same root item modification code, which is really a duplication. As in btrfs-progs we don't need the other call sites in kernels which is shared for subvolume removal. Thanks, Qu > >> >> Thanks, >> Qu >>> >>> static inline int is_fstree(u64 rootid) >>> { >>> diff --git a/kernel-shared/uuid-tree.c b/kernel-shared/uuid-tree.c >>> index 51a7b5d9ff5d..0f0fbf667dda 100644 >>> --- a/kernel-shared/uuid-tree.c >>> +++ b/kernel-shared/uuid-tree.c >>> @@ -120,3 +120,85 @@ int btrfs_is_empty_uuid(u8 *uuid) >>> } >>> return 1; >>> } >>> + >>> +int btrfs_uuid_tree_remove(struct btrfs_trans_handle *trans, u8 >>> *uuid, u8 type, >>> + u64 subid) >>> +{ >>> + struct btrfs_fs_info *fs_info = trans->fs_info; >>> + struct btrfs_root *uuid_root = fs_info->uuid_root; >>> + int ret; >>> + struct btrfs_path *path = NULL; >>> + struct btrfs_key key; >>> + struct extent_buffer *eb; >>> + int slot; >>> + unsigned long offset; >>> + u32 item_size; >>> + unsigned long move_dst; >>> + unsigned long move_src; >>> + unsigned long move_len; >>> + >>> + if (!uuid_root) { >>> + ret = -EINVAL; >>> + goto out; >>> + } >>> + >>> + btrfs_uuid_to_key(uuid, &key); >>> + key.type = type; >>> + >>> + path = btrfs_alloc_path(); >>> + if (!path) { >>> + ret = -ENOMEM; >>> + goto out; >>> + } >>> + >>> + ret = btrfs_search_slot(trans, uuid_root, &key, path, -1, 1); >>> + if (ret < 0) { >>> + warning("error %d while searching for uuid item!", ret); >>> + goto out; >>> + } >>> + if (ret > 0) { >>> + ret = -ENOENT; >>> + goto out; >>> + } >>> + >>> + >>> + eb = path->nodes[0]; >>> + slot = path->slots[0]; >>> + offset = btrfs_item_ptr_offset(eb, slot); >>> + item_size = btrfs_item_size_nr(eb, slot); >>> + if (!IS_ALIGNED(item_size, sizeof(u64))) { >>> + warning("uuid item with illegal size %lu!", (unsigned >>> long)item_size); >>> + ret = -ENOENT; >>> + goto out; >>> + } >>> + while (item_size) { >>> + __le64 read_subid; >>> + >>> + read_extent_buffer(eb, &read_subid, offset, sizeof(read_subid)); >>> + if (le64_to_cpu(read_subid) == subid) >>> + break; >>> + offset += sizeof(read_subid); >>> + item_size -= sizeof(read_subid); >>> + } >>> + >>> + if (!item_size) { >>> + ret = -ENOENT; >>> + goto out; >>> + } >>> + >>> + item_size = btrfs_item_size_nr(eb, slot); >>> + if (item_size == sizeof(subid)) { >>> + ret = btrfs_del_item(trans, uuid_root, path); >>> + goto out; >>> + } >>> + >>> + move_dst = offset; >>> + move_src = offset + sizeof(subid); >>> + move_len = item_size - (move_src - btrfs_item_ptr_offset(eb, slot)); >>> + memmove_extent_buffer(eb, move_dst, move_src, move_len); >>> + btrfs_truncate_item(path, item_size - sizeof(subid), 1); >>> + >>> +out: >>> + btrfs_free_path(path); >>> + return ret; >>> +} >>> >>
diff --git a/kernel-shared/ctree.h b/kernel-shared/ctree.h index 91a85796a678..158281a9fd67 100644 --- a/kernel-shared/ctree.h +++ b/kernel-shared/ctree.h @@ -2860,6 +2860,9 @@ int btrfs_uuid_tree_add(struct btrfs_trans_handle *trans, u8 *uuid, u8 type, u64 subvol_id_cpu); int btrfs_is_empty_uuid(u8 *uuid); +int btrfs_uuid_tree_remove(struct btrfs_trans_handle *trans, u8 *uuid, u8 type, + u64 subid); + static inline int is_fstree(u64 rootid) { diff --git a/kernel-shared/uuid-tree.c b/kernel-shared/uuid-tree.c index 51a7b5d9ff5d..0f0fbf667dda 100644 --- a/kernel-shared/uuid-tree.c +++ b/kernel-shared/uuid-tree.c @@ -120,3 +120,85 @@ int btrfs_is_empty_uuid(u8 *uuid) } return 1; } + +int btrfs_uuid_tree_remove(struct btrfs_trans_handle *trans, u8 *uuid, u8 type, + u64 subid) +{ + struct btrfs_fs_info *fs_info = trans->fs_info; + struct btrfs_root *uuid_root = fs_info->uuid_root; + int ret; + struct btrfs_path *path = NULL; + struct btrfs_key key; + struct extent_buffer *eb; + int slot; + unsigned long offset; + u32 item_size; + unsigned long move_dst; + unsigned long move_src; + unsigned long move_len; + + if (!uuid_root) { + ret = -EINVAL; + goto out; + } + + btrfs_uuid_to_key(uuid, &key); + key.type = type; + + path = btrfs_alloc_path(); + if (!path) { + ret = -ENOMEM; + goto out; + } + + ret = btrfs_search_slot(trans, uuid_root, &key, path, -1, 1); + if (ret < 0) { + warning("error %d while searching for uuid item!", ret); + goto out; + } + if (ret > 0) { + ret = -ENOENT; + goto out; + } + + + eb = path->nodes[0]; + slot = path->slots[0]; + offset = btrfs_item_ptr_offset(eb, slot); + item_size = btrfs_item_size_nr(eb, slot); + if (!IS_ALIGNED(item_size, sizeof(u64))) { + warning("uuid item with illegal size %lu!", (unsigned long)item_size); + ret = -ENOENT; + goto out; + } + while (item_size) { + __le64 read_subid; + + read_extent_buffer(eb, &read_subid, offset, sizeof(read_subid)); + if (le64_to_cpu(read_subid) == subid) + break; + offset += sizeof(read_subid); + item_size -= sizeof(read_subid); + } + + if (!item_size) { + ret = -ENOENT; + goto out; + } + + item_size = btrfs_item_size_nr(eb, slot); + if (item_size == sizeof(subid)) { + ret = btrfs_del_item(trans, uuid_root, path); + goto out; + } + + move_dst = offset; + move_src = offset + sizeof(subid); + move_len = item_size - (move_src - btrfs_item_ptr_offset(eb, slot)); + memmove_extent_buffer(eb, move_dst, move_src, move_len); + btrfs_truncate_item(path, item_size - sizeof(subid), 1); + +out: + btrfs_free_path(path); + return ret; +}
It will be used to clear received data on RW snapshots that were received. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- kernel-shared/ctree.h | 3 ++ kernel-shared/uuid-tree.c | 82 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+)