| Message ID | 652ef8f5f0b46c2488a2f72bf34a83d9bc8357db.1724184314.git.loemra.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: clean up btrfs_iget, btrfs_iget_path usage | expand |
On Tue, Aug 20, 2024 at 01:13:18PM -0700, Leo Martins wrote: > Move the path allocation from inside btrfs_read_locked_inode > to btrfs_iget. This makes the code easier to reason about as it is > clear where the allocation occurs and who is in charge of freeing the > path. I have investigated all of the callers of btrfs_iget_path to make > sure that it is never called with a null path with the expectation > of a path allocation. All of the null calls seem to come from btrfs_iget > so it makes sense to do the allocation within btrfs_iget. You're missing your Signed-off-by. Also try to be less verbose in your title, "btrfs: move path allocation into btrfs_iget" or something like that, you want to avoid wrapping if you can. Fix those things and you can add Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
On Tue, Aug 20, 2024 at 01:13:18PM -0700, Leo Martins wrote: > Move the path allocation from inside btrfs_read_locked_inode > to btrfs_iget. This makes the code easier to reason about as it is > clear where the allocation occurs and who is in charge of freeing the > path. I have investigated all of the callers of btrfs_iget_path to make > sure that it is never called with a null path with the expectation > of a path allocation. All of the null calls seem to come from btrfs_iget > so it makes sense to do the allocation within btrfs_iget. > > --- > fs/btrfs/inode.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index a8ad540d6de2..f2959803f9d7 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -3790,10 +3790,9 @@ static int btrfs_init_file_extent_tree(struct btrfs_inode *inode) > * read an inode from the btree into the in-memory inode > */ > static int btrfs_read_locked_inode(struct inode *inode, > - struct btrfs_path *in_path) > + struct btrfs_path *path) > { > struct btrfs_fs_info *fs_info = inode_to_fs_info(inode); > - struct btrfs_path *path = in_path; > struct extent_buffer *leaf; > struct btrfs_inode_item *inode_item; > struct btrfs_root *root = BTRFS_I(inode)->root; > @@ -3813,20 +3812,13 @@ static int btrfs_read_locked_inode(struct inode *inode, > if (!ret) > filled = true; > > - if (!path) { > - path = btrfs_alloc_path(); > - if (!path) > - return -ENOMEM; > - } > + ASSERT(path); > > btrfs_get_inode_key(BTRFS_I(inode), &location); > > ret = btrfs_lookup_inode(NULL, root, path, &location, 0); > - if (ret) { > - if (path != in_path) > - btrfs_free_path(path); > + if (ret) > return ret; > - } > > leaf = path->nodes[0]; > > @@ -3960,8 +3952,6 @@ static int btrfs_read_locked_inode(struct inode *inode, > btrfs_ino(BTRFS_I(inode)), > btrfs_root_id(root), ret); > } > - if (path != in_path) > - btrfs_free_path(path); > > if (!maybe_acls) > cache_no_acl(inode); > @@ -5632,7 +5622,17 @@ struct inode *btrfs_iget_path(u64 ino, struct btrfs_root *root, > > struct inode *btrfs_iget(u64 ino, struct btrfs_root *root) > { > - return btrfs_iget_path(ino, root, NULL); > + struct btrfs_path *path; > + struct inode *inode; > + > + path = btrfs_alloc_path(); > + if (!path) > + return ERR_PTR(-ENOMEM); > + Actually now that I look at it, I don't want to do it this way. Sorry about that because I'm the one who suggested cleaning this all up, and I missed an important piece here. With btrfs_iget_path() we're doing the btrfs_iget_locked() first, which will find the inode in cache if it can, so the path allocation isn't necessary in that case. I missed this when I was looking at this, so I think it's better to move the path allocation out of btrfs_read_locked_inode(), but push it into btrfs_iget_path() instead. So btrfs_iget_path() will handle path == NULL the way that btrfs_read_locked_inode() currently does, allocating if it has to and freeing if it's necessary. The second patch is still good. Thanks, Josef
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index a8ad540d6de2..f2959803f9d7 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -3790,10 +3790,9 @@ static int btrfs_init_file_extent_tree(struct btrfs_inode *inode) * read an inode from the btree into the in-memory inode */ static int btrfs_read_locked_inode(struct inode *inode, - struct btrfs_path *in_path) + struct btrfs_path *path) { struct btrfs_fs_info *fs_info = inode_to_fs_info(inode); - struct btrfs_path *path = in_path; struct extent_buffer *leaf; struct btrfs_inode_item *inode_item; struct btrfs_root *root = BTRFS_I(inode)->root; @@ -3813,20 +3812,13 @@ static int btrfs_read_locked_inode(struct inode *inode, if (!ret) filled = true; - if (!path) { - path = btrfs_alloc_path(); - if (!path) - return -ENOMEM; - } + ASSERT(path); btrfs_get_inode_key(BTRFS_I(inode), &location); ret = btrfs_lookup_inode(NULL, root, path, &location, 0); - if (ret) { - if (path != in_path) - btrfs_free_path(path); + if (ret) return ret; - } leaf = path->nodes[0]; @@ -3960,8 +3952,6 @@ static int btrfs_read_locked_inode(struct inode *inode, btrfs_ino(BTRFS_I(inode)), btrfs_root_id(root), ret); } - if (path != in_path) - btrfs_free_path(path); if (!maybe_acls) cache_no_acl(inode); @@ -5632,7 +5622,17 @@ struct inode *btrfs_iget_path(u64 ino, struct btrfs_root *root, struct inode *btrfs_iget(u64 ino, struct btrfs_root *root) { - return btrfs_iget_path(ino, root, NULL); + struct btrfs_path *path; + struct inode *inode; + + path = btrfs_alloc_path(); + if (!path) + return ERR_PTR(-ENOMEM); + + inode = btrfs_iget_path(ino, root, path); + + btrfs_free_path(path); + return inode; } static struct inode *new_simple_dir(struct inode *dir,