| Message ID | 20220721135006.3345302-2-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove duplicate code in btrfs_prune_dentries/find_next_inode | expand |
On Thu, Jul 21, 2022 at 04:50:04PM +0300, Nikolay Borisov wrote: > This function holds common code for searching the root's inode rb tree. > It will be used to reduce code duplication in future patches. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/ctree.h | 1 + > fs/btrfs/inode.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index 0ae7f6530da1..fc0a0ab01761 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -3311,6 +3311,7 @@ int btrfs_set_inode_index(struct btrfs_inode *dir, u64 *index); > int btrfs_unlink_inode(struct btrfs_trans_handle *trans, > struct btrfs_inode *dir, struct btrfs_inode *inode, > const char *name, int name_len); > +struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid); > int btrfs_add_link(struct btrfs_trans_handle *trans, > struct btrfs_inode *parent_inode, struct btrfs_inode *inode, > const char *name, int name_len, int add_backref, u64 index); > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 5fc831a8eba1..c11169ba28b2 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -4587,6 +4587,50 @@ static noinline int may_destroy_subvol(struct btrfs_root *root) > return ret; > } > > +/** > + * btrfs_find_inode - returns the rb_node pointing to the inode with an ino > + * equal or larger than @objectid Please use the simplified format that we have in btrfs. > + * > + * @root: root which is going to be searched for an inode > + * @objectid: ino being searched for, if no exact match can be found the > + * function returns the first largest inode > + * > + * Returns the rb_node pointing to the specified inode or returns NULL if no > + * match is found. > + * > + */ > +struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid) Const arguments for int types does not make sense. The root can be made const, compile tested, no complaints. > +{ > + struct rb_node *node = root->inode_tree.rb_node; > + struct rb_node *prev = NULL; > + struct btrfs_inode *entry; > + > + lockdep_assert_held(&root->inode_lock); > + > + while (node) { > + prev = node; > + entry = rb_entry(node, struct btrfs_inode, rb_node); > + > + if (objectid < btrfs_ino(entry)) > + node = node->rb_left; > + else if (objectid > btrfs_ino(entry)) > + node = node->rb_right; > + else > + break; > + } > + > + if (!node) { > + while (prev) { > + entry = rb_entry(prev, struct btrfs_inode, rb_node); > + if (objectid <= btrfs_ino(entry)) > + return prev; > + prev = rb_next(prev); > + } > + } > + > + return node; > +} > + > /* Delete all dentries for inodes belonging to the root */ > static void btrfs_prune_dentries(struct btrfs_root *root) > { > -- > 2.25.1
On Thu, Aug 04, 2022 at 05:28:24PM +0200, David Sterba wrote: > On Thu, Jul 21, 2022 at 04:50:04PM +0300, Nikolay Borisov wrote: > > This function holds common code for searching the root's inode rb tree. > > It will be used to reduce code duplication in future patches. > > > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > > --- > > fs/btrfs/ctree.h | 1 + > > fs/btrfs/inode.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 45 insertions(+) > > > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > > index 0ae7f6530da1..fc0a0ab01761 100644 > > --- a/fs/btrfs/ctree.h > > +++ b/fs/btrfs/ctree.h > > @@ -3311,6 +3311,7 @@ int btrfs_set_inode_index(struct btrfs_inode *dir, u64 *index); > > int btrfs_unlink_inode(struct btrfs_trans_handle *trans, > > struct btrfs_inode *dir, struct btrfs_inode *inode, > > const char *name, int name_len); > > +struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid); > > int btrfs_add_link(struct btrfs_trans_handle *trans, > > struct btrfs_inode *parent_inode, struct btrfs_inode *inode, > > const char *name, int name_len, int add_backref, u64 index); > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index 5fc831a8eba1..c11169ba28b2 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -4587,6 +4587,50 @@ static noinline int may_destroy_subvol(struct btrfs_root *root) > > return ret; > > } > > > > +/** > > + * btrfs_find_inode - returns the rb_node pointing to the inode with an ino > > + * equal or larger than @objectid > > Please use the simplified format that we have in btrfs. > > > + * > > + * @root: root which is going to be searched for an inode > > + * @objectid: ino being searched for, if no exact match can be found the > > + * function returns the first largest inode > > + * > > + * Returns the rb_node pointing to the specified inode or returns NULL if no > > + * match is found. > > + * > > + */ > > +struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid) > > Const arguments for int types does not make sense. It makes sense to me, as much as declaring local variables as const, and I don't recall you ever complain about local const variables before (I do it often, and I'm not the only one). Once I read the const part, I can tell for sure that nowhere in the function the value of the argument is changed. It happens often that large functions use an int argument as if it was a local variable and change its value later on, which makes reading the code often a bit more time consuming and often leads to mistakest too. > The root can be made > const, compile tested, no complaints. > > > +{ > > + struct rb_node *node = root->inode_tree.rb_node; > > + struct rb_node *prev = NULL; > > + struct btrfs_inode *entry; > > + > > + lockdep_assert_held(&root->inode_lock); > > + > > + while (node) { > > + prev = node; > > + entry = rb_entry(node, struct btrfs_inode, rb_node); > > + > > + if (objectid < btrfs_ino(entry)) > > + node = node->rb_left; > > + else if (objectid > btrfs_ino(entry)) > > + node = node->rb_right; > > + else > > + break; > > + } > > + > > + if (!node) { > > + while (prev) { > > + entry = rb_entry(prev, struct btrfs_inode, rb_node); > > + if (objectid <= btrfs_ino(entry)) > > + return prev; > > + prev = rb_next(prev); > > + } > > + } > > + > > + return node; > > +} > > + > > /* Delete all dentries for inodes belonging to the root */ > > static void btrfs_prune_dentries(struct btrfs_root *root) > > { > > -- > > 2.25.1
On Thu, Aug 04, 2022 at 04:52:21PM +0100, Filipe Manana wrote: > On Thu, Aug 04, 2022 at 05:28:24PM +0200, David Sterba wrote: > > On Thu, Jul 21, 2022 at 04:50:04PM +0300, Nikolay Borisov wrote: > > > > Please use the simplified format that we have in btrfs. > > > > > + * > > > + * @root: root which is going to be searched for an inode > > > + * @objectid: ino being searched for, if no exact match can be found the > > > + * function returns the first largest inode > > > + * > > > + * Returns the rb_node pointing to the specified inode or returns NULL if no > > > + * match is found. > > > + * > > > + */ > > > +struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid) > > > > Const arguments for int types does not make sense. > > It makes sense to me, as much as declaring local variables as const, and I don't > recall you ever complain about local const variables before (I do it often, and > I'm not the only one). The function parameters are supposed to be set by callers and what's in the prototype is contract. A const pointer says "callee will not change this, promise", but for integer types it does not make sense because it does not establish any guarantees to caller. Local variables completely live inside a function and adding the const there can in some cases optimize the code so that compiler does not need to read the memory repeatedly. We've been adding it to the known constant values like sectorsize, or when there's a feature bit or other status information that's clearly unchanged during the function. > Once I read the const part, I can tell for sure that nowhere in the function the > value of the argument is changed. > It happens often that large functions use an int argument as if it was a local > variable and change its value later on, which makes reading the code often a bit > more time consuming and often leads to mistakest too. I'd rather make this an exception than a rule, to avoid mistakes and for clarity that a long function takes certain input, but not for short functions.
On Thu, Aug 04, 2022 at 06:08:06PM +0200, David Sterba wrote: > On Thu, Aug 04, 2022 at 04:52:21PM +0100, Filipe Manana wrote: > > On Thu, Aug 04, 2022 at 05:28:24PM +0200, David Sterba wrote: > > > On Thu, Jul 21, 2022 at 04:50:04PM +0300, Nikolay Borisov wrote: > > > > > > Please use the simplified format that we have in btrfs. > > > > > > > + * > > > > + * @root: root which is going to be searched for an inode > > > > + * @objectid: ino being searched for, if no exact match can be found the > > > > + * function returns the first largest inode > > > > + * > > > > + * Returns the rb_node pointing to the specified inode or returns NULL if no > > > > + * match is found. > > > > + * > > > > + */ > > > > +struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid) > > > > > > Const arguments for int types does not make sense. > > > > It makes sense to me, as much as declaring local variables as const, and I don't > > recall you ever complain about local const variables before (I do it often, and > > I'm not the only one). > > The function parameters are supposed to be set by callers and what's in > the prototype is contract. A const pointer says "callee will not change > this, promise", but for integer types it does not make sense because it > does not establish any guarantees to caller. Sure, but my point was not about giving guarantees to the caller. It was all about readability for someone reading and changing code. > > Local variables completely live inside a function and adding the const > there can in some cases optimize the code so that compiler does not need > to read the memory repeatedly. We've been adding it to the known > constant values like sectorsize, or when there's a feature bit or other > status information that's clearly unchanged during the function. > > > Once I read the const part, I can tell for sure that nowhere in the function the > > value of the argument is changed. > > > It happens often that large functions use an int argument as if it was a local > > variable and change its value later on, which makes reading the code often a bit > > more time consuming and often leads to mistakest too. > > I'd rather make this an exception than a rule, to avoid mistakes and for > clarity that a long function takes certain input, but not for short > functions. Not sure if I understood that correctly. You mean it's ok to add the const qualifier only if the function is long?
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 0ae7f6530da1..fc0a0ab01761 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3311,6 +3311,7 @@ int btrfs_set_inode_index(struct btrfs_inode *dir, u64 *index); int btrfs_unlink_inode(struct btrfs_trans_handle *trans, struct btrfs_inode *dir, struct btrfs_inode *inode, const char *name, int name_len); +struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid); int btrfs_add_link(struct btrfs_trans_handle *trans, struct btrfs_inode *parent_inode, struct btrfs_inode *inode, const char *name, int name_len, int add_backref, u64 index); diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 5fc831a8eba1..c11169ba28b2 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -4587,6 +4587,50 @@ static noinline int may_destroy_subvol(struct btrfs_root *root) return ret; } +/** + * btrfs_find_inode - returns the rb_node pointing to the inode with an ino + * equal or larger than @objectid + * + * @root: root which is going to be searched for an inode + * @objectid: ino being searched for, if no exact match can be found the + * function returns the first largest inode + * + * Returns the rb_node pointing to the specified inode or returns NULL if no + * match is found. + * + */ +struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid) +{ + struct rb_node *node = root->inode_tree.rb_node; + struct rb_node *prev = NULL; + struct btrfs_inode *entry; + + lockdep_assert_held(&root->inode_lock); + + while (node) { + prev = node; + entry = rb_entry(node, struct btrfs_inode, rb_node); + + if (objectid < btrfs_ino(entry)) + node = node->rb_left; + else if (objectid > btrfs_ino(entry)) + node = node->rb_right; + else + break; + } + + if (!node) { + while (prev) { + entry = rb_entry(prev, struct btrfs_inode, rb_node); + if (objectid <= btrfs_ino(entry)) + return prev; + prev = rb_next(prev); + } + } + + return node; +} + /* Delete all dentries for inodes belonging to the root */ static void btrfs_prune_dentries(struct btrfs_root *root) {
This function holds common code for searching the root's inode rb tree. It will be used to reduce code duplication in future patches. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/ctree.h | 1 + fs/btrfs/inode.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+)