* [PATCH] btrfs: simplify error handling in btrfs_lookup_dentry @ 2022-07-11 15:16 Nikolay Borisov 2022-07-12 19:54 ` Rohit Singh 2022-07-13 14:03 ` David Sterba 0 siblings, 2 replies; 6+ messages in thread From: Nikolay Borisov @ 2022-07-11 15:16 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov 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) { down_read(&fs_info->cleanup_work_sem); if (!sb_rdonly(inode->i_sb)) ret = btrfs_orphan_cleanup(sub_root); -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: simplify error handling in btrfs_lookup_dentry 2022-07-11 15:16 [PATCH] btrfs: simplify error handling in btrfs_lookup_dentry Nikolay Borisov @ 2022-07-12 19:54 ` Rohit Singh 2022-07-12 20:36 ` Nikolay Borisov 2022-07-13 14:03 ` David Sterba 1 sibling, 1 reply; 6+ messages in thread From: Rohit Singh @ 2022-07-12 19:54 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-btrfs 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: simplify error handling in btrfs_lookup_dentry 2022-07-12 19:54 ` Rohit Singh @ 2022-07-12 20:36 ` Nikolay Borisov 2022-07-12 21:13 ` Rohit Singh 2022-07-12 21:26 ` Sweet Tea Dorminy 0 siblings, 2 replies; 6+ messages in thread From: Nikolay Borisov @ 2022-07-12 20:36 UTC (permalink / raw) To: Rohit Singh; +Cc: linux-btrfs 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); + } } } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: simplify error handling in btrfs_lookup_dentry 2022-07-12 20:36 ` Nikolay Borisov @ 2022-07-12 21:13 ` Rohit Singh 2022-07-12 21:26 ` Sweet Tea Dorminy 1 sibling, 0 replies; 6+ messages in thread From: Rohit Singh @ 2022-07-12 21:13 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-btrfs 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: simplify error handling in btrfs_lookup_dentry 2022-07-12 20:36 ` Nikolay Borisov 2022-07-12 21:13 ` Rohit Singh @ 2022-07-12 21:26 ` Sweet Tea Dorminy 1 sibling, 0 replies; 6+ messages in thread From: Sweet Tea Dorminy @ 2022-07-12 21:26 UTC (permalink / raw) To: Nikolay Borisov, Rohit Singh; +Cc: linux-btrfs > 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); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: simplify error handling in btrfs_lookup_dentry 2022-07-11 15:16 [PATCH] btrfs: simplify error handling in btrfs_lookup_dentry Nikolay Borisov 2022-07-12 19:54 ` Rohit Singh @ 2022-07-13 14:03 ` David Sterba 1 sibling, 0 replies; 6+ messages in thread From: David Sterba @ 2022-07-13 14:03 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-btrfs 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-13 14:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-11 15:16 [PATCH] btrfs: simplify error handling in btrfs_lookup_dentry Nikolay Borisov 2022-07-12 19:54 ` Rohit Singh 2022-07-12 20:36 ` Nikolay Borisov 2022-07-12 21:13 ` Rohit Singh 2022-07-12 21:26 ` Sweet Tea Dorminy 2022-07-13 14:03 ` David Sterba
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.