[PATCH] Btrfs: remove duplicated btrfs_release_path in btrfs_unlink_subvol

[PATCH] Btrfs: remove duplicated btrfs_release_path in btrfs_unlink_subvol

[Liu Bo]

The btrfs_release_path() is just useless as path is only used in error handling.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 fs/btrfs/inode.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index eba61bcb9bb3..5680e9c462a3 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4148,7 +4148,6 @@ static int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
 		btrfs_release_path(path);
 		index = key.offset;
 	}
-	btrfs_release_path(path);
 
 	ret = btrfs_delete_delayed_dir_index(trans, fs_info, BTRFS_I(dir), index);
 	if (ret) {
-- 
1.8.3.1

Re: [PATCH] Btrfs: remove duplicated btrfs_release_path in btrfs_unlink_subvol

[David Sterba]

On Tue, Aug 14, 2018 at 10:47:09AM +0800, Liu Bo wrote:
> The btrfs_release_path() is just useless as path is only used in error handling.

Where is it duplicated? And I don't think it's useless, while the
changelog does not explain why and it's not obvious from the context. If
the path is locked, then releasing it right after it's not needed makes
sense.  There are several potentially heavyweight operations between the
release and final free.

Re: [PATCH] Btrfs: remove duplicated btrfs_release_path in btrfs_unlink_subvol

[Liu Bo]

On Tue, Aug 14, 2018 at 12:46:00PM +0200, David Sterba wrote:
> On Tue, Aug 14, 2018 at 10:47:09AM +0800, Liu Bo wrote:
> > The btrfs_release_path() is just useless as path is only used in error handling.
> 
> Where is it duplicated? And I don't think it's useless, while the
> changelog does not explain why and it's not obvious from the context. If
> the path is locked, then releasing it right after it's not needed makes
> sense.  There are several potentially heavyweight operations between the
> release and final free.

I see, the diff context is a little bit misleading, the logic in
btrfs_unlink_subvol() is like,

{
	...
	btrfs_delete_one_dir_name(path);
	btrfs_release_path(path);

	ret = btrfs_del_root_ref(); <<<< path is not used here.
	if (ret < 0) {
	   	btrfs_search_dir_index_item(path);
		btrfs_release_path(path);
	}
	btrfs_release_path(path);   <<<< path has been released anyway.
	...
}

thanks,
-liubo

Re: [PATCH] Btrfs: remove duplicated btrfs_release_path in btrfs_unlink_subvol

[David Sterba]

On Wed, Aug 15, 2018 at 10:52:56AM +0800, Liu Bo wrote:
> On Tue, Aug 14, 2018 at 12:46:00PM +0200, David Sterba wrote:
> > On Tue, Aug 14, 2018 at 10:47:09AM +0800, Liu Bo wrote:
> > > The btrfs_release_path() is just useless as path is only used in error handling.
> > 
> > Where is it duplicated? And I don't think it's useless, while the
> > changelog does not explain why and it's not obvious from the context. If
> > the path is locked, then releasing it right after it's not needed makes
> > sense.  There are several potentially heavyweight operations between the
> > release and final free.
> 
> I see, the diff context is a little bit misleading, the logic in
> btrfs_unlink_subvol() is like,

Well, you based you patch on old code, the duplicate btrfs_release_path
has been removed in 5b7d687ad5913a56b6a8788435d7a53990b4176d and was
in the devel branches since like 2 weeks.

The patch removes the first release while it would could have been the
other one to save some cycles when the branch with
btrfs_search_dir_index_item is not taken.

Re: [PATCH] Btrfs: remove duplicated btrfs_release_path in btrfs_unlink_subvol

[Liu Bo]

On Wed, Aug 15, 2018 at 4:48 AM, David Sterba <dsterba@suse.cz> wrote:
> On Wed, Aug 15, 2018 at 10:52:56AM +0800, Liu Bo wrote:
>> On Tue, Aug 14, 2018 at 12:46:00PM +0200, David Sterba wrote:
>> > On Tue, Aug 14, 2018 at 10:47:09AM +0800, Liu Bo wrote:
>> > > The btrfs_release_path() is just useless as path is only used in error handling.
>> >
>> > Where is it duplicated? And I don't think it's useless, while the
>> > changelog does not explain why and it's not obvious from the context. If
>> > the path is locked, then releasing it right after it's not needed makes
>> > sense.  There are several potentially heavyweight operations between the
>> > release and final free.
>>
>> I see, the diff context is a little bit misleading, the logic in
>> btrfs_unlink_subvol() is like,
>
> Well, you based you patch on old code, the duplicate btrfs_release_path
> has been removed in 5b7d687ad5913a56b6a8788435d7a53990b4176d and was
> in the devel branches since like 2 weeks.
>
> The patch removes the first release while it would could have been the
> other one to save some cycles when the branch with
> btrfs_search_dir_index_item is not taken.

Ah I see, sorry for the noise.

thanks,
liubo