| Message ID | 20220711151618.2518485-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: simplify error handling in btrfs_lookup_dentry | expand |
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?
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); + } } }
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>
> 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); > + } > } > } I was looking at this just now because Rohit was talking about it, and noticed you could potentially reduce indentation a hair if you felt like it: } 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) { + if (IS_ERR(inode)) + return inode; down_read(&fs_info->cleanup_work_sem); if (!sb_rdonly(inode->i_sb)) ret = btrfs_orphan_cleanup(sub_root);
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> There have been some suggestions how to improve the code, please resend so we all have the same version to look at, thanks.
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) { down_read(&fs_info->cleanup_work_sem); if (!sb_rdonly(inode->i_sb)) ret = btrfs_orphan_cleanup(sub_root);
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(-)