* [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.