| Message ID | 20210804184854.10696-2-mpdesouza@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Use btrfs_find_item whenever possible | expand |
On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote: > It's more natural do use objectid, type and offset, in this order, when > dealing with btrfs keys. I'm completely fine with this part. > > No functional changes. > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > --- > fs/btrfs/backref.c | 9 ++++----- > fs/btrfs/ctree.c | 2 +- > fs/btrfs/ctree.h | 2 +- > 3 files changed, 6 insertions(+), 7 deletions(-) > > diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c > index f735b8798ba1..9e92faaafa02 100644 > --- a/fs/btrfs/backref.c > +++ b/fs/btrfs/backref.c > @@ -1691,8 +1691,8 @@ char *btrfs_ref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path, > btrfs_tree_read_unlock(eb); > free_extent_buffer(eb); > } > - ret = btrfs_find_item(fs_root, path, parent, 0, > - BTRFS_INODE_REF_KEY, &found_key); > + ret = btrfs_find_item(fs_root, path, parent, BTRFS_INODE_REF_KEY, > + 0, &found_key); > if (ret > 0) > ret = -ENOENT; > if (ret) > @@ -2063,9 +2063,8 @@ static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root, > struct btrfs_key found_key; > > while (!ret) { > - ret = btrfs_find_item(fs_root, path, inum, > - parent ? parent + 1 : 0, BTRFS_INODE_REF_KEY, > - &found_key); > + ret = btrfs_find_item(fs_root, path, inum, BTRFS_INODE_REF_KEY, > + parent ? parent + 1 : 0, &found_key); > > if (ret < 0) > break; > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > index 84627cbd5b5b..c0002ec9c025 100644 > --- a/fs/btrfs/ctree.c > +++ b/fs/btrfs/ctree.c > @@ -1528,7 +1528,7 @@ setup_nodes_for_search(struct btrfs_trans_handle *trans, > } > > int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path, > - u64 iobjectid, u64 ioff, u8 key_type, > + u64 iobjectid, u8 key_type, u64 ioff, > struct btrfs_key *found_key) But the @found_key part makes me wonder. Is it really needed? The caller has @path and return value. If we return 0, we know it's an exact match, no need to grab the key. If we return 1, caller can easily grab the key using @path (if they really need). So can we also remove @found_key parameter, and add some comment on the function? Thanks, Qu > { > int ret; > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index a898257ad2b5..0a971e98f5f9 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -2858,7 +2858,7 @@ int btrfs_duplicate_item(struct btrfs_trans_handle *trans, > struct btrfs_path *path, > const struct btrfs_key *new_key); > int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path, > - u64 inum, u64 ioff, u8 key_type, struct btrfs_key *found_key); > + u64 inum, u8 key_type, u64 ioff, struct btrfs_key *found_key); > int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root, > const struct btrfs_key *key, struct btrfs_path *p, > int ins_len, int cow); >
On Thu, 2021-08-05 at 10:16 +0800, Qu Wenruo wrote: > > On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote: > > It's more natural do use objectid, type and offset, in this order, > > when > > dealing with btrfs keys. > > I'm completely fine with this part. > > > No functional changes. > > > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > > --- > > fs/btrfs/backref.c | 9 ++++----- > > fs/btrfs/ctree.c | 2 +- > > fs/btrfs/ctree.h | 2 +- > > 3 files changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c > > index f735b8798ba1..9e92faaafa02 100644 > > --- a/fs/btrfs/backref.c > > +++ b/fs/btrfs/backref.c > > @@ -1691,8 +1691,8 @@ char *btrfs_ref_to_path(struct btrfs_root > > *fs_root, struct btrfs_path *path, > > btrfs_tree_read_unlock(eb); > > free_extent_buffer(eb); > > } > > - ret = btrfs_find_item(fs_root, path, parent, 0, > > - BTRFS_INODE_REF_KEY, &found_key); > > + ret = btrfs_find_item(fs_root, path, parent, > > BTRFS_INODE_REF_KEY, > > + 0, &found_key); > > if (ret > 0) > > ret = -ENOENT; > > if (ret) > > @@ -2063,9 +2063,8 @@ static int iterate_inode_refs(u64 inum, > > struct btrfs_root *fs_root, > > struct btrfs_key found_key; > > > > while (!ret) { > > - ret = btrfs_find_item(fs_root, path, inum, > > - parent ? parent + 1 : 0, > > BTRFS_INODE_REF_KEY, > > - &found_key); > > + ret = btrfs_find_item(fs_root, path, inum, > > BTRFS_INODE_REF_KEY, > > + parent ? parent + 1 : 0, &found_key); > > > > if (ret < 0) > > break; > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > > index 84627cbd5b5b..c0002ec9c025 100644 > > --- a/fs/btrfs/ctree.c > > +++ b/fs/btrfs/ctree.c > > @@ -1528,7 +1528,7 @@ setup_nodes_for_search(struct > > btrfs_trans_handle *trans, > > } > > > > int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path > > *path, > > - u64 iobjectid, u64 ioff, u8 key_type, > > + u64 iobjectid, u8 key_type, u64 ioff, > > struct btrfs_key *found_key) > > But the @found_key part makes me wonder. > > Is it really needed? > > The caller has @path and return value. If we return 0, we know it's > an > exact match, no need to grab the key. > If we return 1, caller can easily grab the key using @path (if they > really need). > > So can we also remove @found_key parameter, and add some comment on > the > function? I believe that the function name is misleading. Maybe we can adjust it to something like btrfs_find_item_offset, since it validates if the found item has the same objectid and type of the searched key. This is very common for a lot of the callers, which expect to receive the same objectid and type, and each caller validate the offset as required. Maybe we can add a comment and change the function name to reflect all aspects of how it works. What do you think? Thanks, Marcos > > Thanks, > Qu > > > { > > int ret; > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > > index a898257ad2b5..0a971e98f5f9 100644 > > --- a/fs/btrfs/ctree.h > > +++ b/fs/btrfs/ctree.h > > @@ -2858,7 +2858,7 @@ int btrfs_duplicate_item(struct > > btrfs_trans_handle *trans, > > struct btrfs_path *path, > > const struct btrfs_key *new_key); > > int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path > > *path, > > - u64 inum, u64 ioff, u8 key_type, struct btrfs_key > > *found_key); > > + u64 inum, u8 key_type, u64 ioff, struct btrfs_key > > *found_key); > > int btrfs_search_slot(struct btrfs_trans_handle *trans, struct > > btrfs_root *root, > > const struct btrfs_key *key, struct btrfs_path > > *p, > > int ins_len, int cow); > >
On 2021/8/6 上午1:28, Marcos Paulo de Souza wrote: > On Thu, 2021-08-05 at 10:16 +0800, Qu Wenruo wrote: >> >> On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote: >>> It's more natural do use objectid, type and offset, in this order, >>> when >>> dealing with btrfs keys. >> >> I'm completely fine with this part. >> >>> No functional changes. >>> >>> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> >>> --- >>> fs/btrfs/backref.c | 9 ++++----- >>> fs/btrfs/ctree.c | 2 +- >>> fs/btrfs/ctree.h | 2 +- >>> 3 files changed, 6 insertions(+), 7 deletions(-) >>> >>> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c >>> index f735b8798ba1..9e92faaafa02 100644 >>> --- a/fs/btrfs/backref.c >>> +++ b/fs/btrfs/backref.c >>> @@ -1691,8 +1691,8 @@ char *btrfs_ref_to_path(struct btrfs_root >>> *fs_root, struct btrfs_path *path, >>> btrfs_tree_read_unlock(eb); >>> free_extent_buffer(eb); >>> } >>> - ret = btrfs_find_item(fs_root, path, parent, 0, >>> - BTRFS_INODE_REF_KEY, &found_key); >>> + ret = btrfs_find_item(fs_root, path, parent, >>> BTRFS_INODE_REF_KEY, >>> + 0, &found_key); >>> if (ret > 0) >>> ret = -ENOENT; >>> if (ret) >>> @@ -2063,9 +2063,8 @@ static int iterate_inode_refs(u64 inum, >>> struct btrfs_root *fs_root, >>> struct btrfs_key found_key; >>> >>> while (!ret) { >>> - ret = btrfs_find_item(fs_root, path, inum, >>> - parent ? parent + 1 : 0, >>> BTRFS_INODE_REF_KEY, >>> - &found_key); >>> + ret = btrfs_find_item(fs_root, path, inum, >>> BTRFS_INODE_REF_KEY, >>> + parent ? parent + 1 : 0, &found_key); >>> >>> if (ret < 0) >>> break; >>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c >>> index 84627cbd5b5b..c0002ec9c025 100644 >>> --- a/fs/btrfs/ctree.c >>> +++ b/fs/btrfs/ctree.c >>> @@ -1528,7 +1528,7 @@ setup_nodes_for_search(struct >>> btrfs_trans_handle *trans, >>> } >>> >>> int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path >>> *path, >>> - u64 iobjectid, u64 ioff, u8 key_type, >>> + u64 iobjectid, u8 key_type, u64 ioff, >>> struct btrfs_key *found_key) >> >> But the @found_key part makes me wonder. >> >> Is it really needed? >> >> The caller has @path and return value. If we return 0, we know it's >> an >> exact match, no need to grab the key. >> If we return 1, caller can easily grab the key using @path (if they >> really need). >> >> So can we also remove @found_key parameter, and add some comment on >> the >> function? > > I believe that the function name is misleading. Maybe we can adjust it > to something like btrfs_find_item_offset, since it validates if the > found item has the same objectid and type of the searched key. Then the parameter @offset makes no sense. Since we have exact key, it's really intuitional to think we're searching for a specific key, and under most cases we are. > > This is very common for a lot of the callers, which expect to receive > the same objectid and type, and each caller validate the offset as > required. Maybe we can add a comment and change the function name to > reflect all aspects of how it works. What do you think? For such case, the caller doesn't have the full key, but are looking for a key to meet certain key *range* requirement. I believe that needs a new interface. IMHO, we need a generic way to search for a key range (or doing iteration for a key range), and a subset of above interface to just search for a specific key (thus with much simpler interface). Thanks, Qu > > Thanks, > Marcos > >> >> Thanks, >> Qu >> >>> { >>> int ret; >>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h >>> index a898257ad2b5..0a971e98f5f9 100644 >>> --- a/fs/btrfs/ctree.h >>> +++ b/fs/btrfs/ctree.h >>> @@ -2858,7 +2858,7 @@ int btrfs_duplicate_item(struct >>> btrfs_trans_handle *trans, >>> struct btrfs_path *path, >>> const struct btrfs_key *new_key); >>> int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path >>> *path, >>> - u64 inum, u64 ioff, u8 key_type, struct btrfs_key >>> *found_key); >>> + u64 inum, u8 key_type, u64 ioff, struct btrfs_key >>> *found_key); >>> int btrfs_search_slot(struct btrfs_trans_handle *trans, struct >>> btrfs_root *root, >>> const struct btrfs_key *key, struct btrfs_path >>> *p, >>> int ins_len, int cow); >>> >
On Wed, Aug 04, 2021 at 03:48:48PM -0300, Marcos Paulo de Souza wrote: > It's more natural do use objectid, type and offset, in this order, when > dealing with btrfs keys. > > No functional changes. > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > --- > fs/btrfs/backref.c | 9 ++++----- > fs/btrfs/ctree.c | 2 +- > fs/btrfs/ctree.h | 2 +- > 3 files changed, 6 insertions(+), 7 deletions(-) > > diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c > index f735b8798ba1..9e92faaafa02 100644 > --- a/fs/btrfs/backref.c > +++ b/fs/btrfs/backref.c > @@ -1691,8 +1691,8 @@ char *btrfs_ref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path, > btrfs_tree_read_unlock(eb); > free_extent_buffer(eb); > } > - ret = btrfs_find_item(fs_root, path, parent, 0, > - BTRFS_INODE_REF_KEY, &found_key); > + ret = btrfs_find_item(fs_root, path, parent, BTRFS_INODE_REF_KEY, > + 0, &found_key); Have you considered using the found_key as both input and output parameter? As input it stores the objectid/key/offset parameters and with no errors it's set to the found key. All callers of the function already have a key and you add another variable just for the one that changes in the iterations (eg. offset).
diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c index f735b8798ba1..9e92faaafa02 100644 --- a/fs/btrfs/backref.c +++ b/fs/btrfs/backref.c @@ -1691,8 +1691,8 @@ char *btrfs_ref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path, btrfs_tree_read_unlock(eb); free_extent_buffer(eb); } - ret = btrfs_find_item(fs_root, path, parent, 0, - BTRFS_INODE_REF_KEY, &found_key); + ret = btrfs_find_item(fs_root, path, parent, BTRFS_INODE_REF_KEY, + 0, &found_key); if (ret > 0) ret = -ENOENT; if (ret) @@ -2063,9 +2063,8 @@ static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root, struct btrfs_key found_key; while (!ret) { - ret = btrfs_find_item(fs_root, path, inum, - parent ? parent + 1 : 0, BTRFS_INODE_REF_KEY, - &found_key); + ret = btrfs_find_item(fs_root, path, inum, BTRFS_INODE_REF_KEY, + parent ? parent + 1 : 0, &found_key); if (ret < 0) break; diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index 84627cbd5b5b..c0002ec9c025 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -1528,7 +1528,7 @@ setup_nodes_for_search(struct btrfs_trans_handle *trans, } int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path, - u64 iobjectid, u64 ioff, u8 key_type, + u64 iobjectid, u8 key_type, u64 ioff, struct btrfs_key *found_key) { int ret; diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index a898257ad2b5..0a971e98f5f9 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -2858,7 +2858,7 @@ int btrfs_duplicate_item(struct btrfs_trans_handle *trans, struct btrfs_path *path, const struct btrfs_key *new_key); int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path, - u64 inum, u64 ioff, u8 key_type, struct btrfs_key *found_key); + u64 inum, u8 key_type, u64 ioff, struct btrfs_key *found_key); int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root, const struct btrfs_key *key, struct btrfs_path *p, int ins_len, int cow);
It's more natural do use objectid, type and offset, in this order, when dealing with btrfs keys. No functional changes. Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> --- fs/btrfs/backref.c | 9 ++++----- fs/btrfs/ctree.c | 2 +- fs/btrfs/ctree.h | 2 +- 3 files changed, 6 insertions(+), 7 deletions(-)