Re: [PATCH] btrfs: simplify error handling in btrfs_lookup_dentry

[Rohit Singh <rh0@fb.com>]

On Tue, Jul 12, 2022 at 11:36:00PM +0300, Nikolay Borisov wrote:
> 
> 
> On 12.07.22 г. 22:54 ч., Rohit Singh wrote:
> > On Mon, Jul 11, 2022 at 06:16:18PM +0300, Nikolay Borisov wrote:
> > > In btrfs_lookup_dentry releasing the reference of the sub_root and the
> > > running orphan cleanup should only happen if the dentry found actually
> > > represents a subvolume. This can only be true in the 'else' branch as
> > > otherwise either fixup_tree_root_location returned an ENOENT error, in
> > > which case sub_root wouldn't have been changed or if we got a different
> > > errno this means btrfs_get_fs_root couldn't have executed successfully
> > > again meaning sub_root will equal to root. So simplify all the branches
> > > by moving the code into the 'else'.
> > > 
> > > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > > ---
> > >   fs/btrfs/inode.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > > index 0b17335555e0..1dd073e96696 100644
> > > --- a/fs/btrfs/inode.c
> > > +++ b/fs/btrfs/inode.c
> > > @@ -5821,11 +5821,7 @@ struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry)
> > >   			inode = new_simple_dir(dir->i_sb, &location, sub_root);
> > >   	} else {
> > >   		inode = btrfs_iget(dir->i_sb, location.objectid, sub_root);
> > > -	}
> > > -	if (root != sub_root)
> > >   		btrfs_put_root(sub_root);
> > > -
> > > -	if (!IS_ERR(inode) && root != sub_root) {
> > 
> > It looks like the root != sub_root stuff looks correct.
> > 
> > Can't the btrfs inode have an error state on it though?
> 
> Yes it can, under low memory condition. So I guess the correct version would be:
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 0b17335555e0..44a2f38b2de0 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -5821,18 +5821,16 @@ struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry)
>                         inode = new_simple_dir(dir->i_sb, &location, sub_root);
>         } else {
>                 inode = btrfs_iget(dir->i_sb, location.objectid, sub_root);
> -       }
> -       if (root != sub_root)
>                 btrfs_put_root(sub_root);
> -
> -       if (!IS_ERR(inode) && root != sub_root) {
> -               down_read(&fs_info->cleanup_work_sem);
> -               if (!sb_rdonly(inode->i_sb))
> -                       ret = btrfs_orphan_cleanup(sub_root);
> -               up_read(&fs_info->cleanup_work_sem);
> -               if (ret) {
> -                       iput(inode);
> -                       inode = ERR_PTR(ret);
> +               if (!IS_ERR(inode)) {
> +                       down_read(&fs_info->cleanup_work_sem);
> +                       if (!sb_rdonly(inode->i_sb))
> +                               ret = btrfs_orphan_cleanup(sub_root);
> +                       up_read(&fs_info->cleanup_work_sem);
> +                       if (ret) {
> +                               iput(inode);
> +                               inode = ERR_PTR(ret);
> +                       }
>                 }
>         }
> 

Looks good!

Reviewed-by: Rohit Singh <rh0@fb.com>