All of lore.kernel.org
 help / color / mirror / Atom feed
* [V2] btrfs: drop inode reference count on error path
@ 2019-04-18 11:06 Pan Bian
  2019-04-18 12:50 ` Nikolay Borisov
  2019-05-02 14:32 ` David Sterba
  0 siblings, 2 replies; 6+ messages in thread
From: Pan Bian @ 2019-04-18 11:06 UTC (permalink / raw)
  To: Chris Mason, Josef Bacik, David Sterba
  Cc: linux-btrfs, linux-kernel, Pan Bian

The reference count of inode is incremented by ihold. It should be
dropped if not used. However, the reference count is not dropped if
error occurs during updating the inode or deleting orphan items. This
patch fixes the bug.

Signed-off-by: Pan Bian <bianpan2016@163.com>
---
V2: move ihold just before device_initialize to make code clearer
---
 fs/btrfs/inode.c | 54 +++++++++++++++++++++++++-----------------------------
 1 file changed, 25 insertions(+), 29 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 82fdda8..d6630df 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6579,7 +6579,7 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	u64 index;
 	int err;
-	int drop_inode = 0;
+	int log_mode;
 
 	/* do not allow sys_link's with other subvols of the same device */
 	if (root->root_key.objectid != BTRFS_I(inode)->root->root_key.objectid)
@@ -6616,41 +6616,37 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
 	err = btrfs_add_nondir(trans, BTRFS_I(dir), dentry, BTRFS_I(inode),
 			1, index);
 
-	if (err) {
-		drop_inode = 1;
-	} else {
-		struct dentry *parent = dentry->d_parent;
-		int ret;
+	if (err)
+		goto err_link;
 
-		err = btrfs_update_inode(trans, root, inode);
+	err = btrfs_update_inode(trans, root, inode);
+	if (err)
+		goto err_link;
+	if (inode->i_nlink == 1) {
+		/*
+		 * If new hard link count is 1, it's a file created
+		 * with open(2) O_TMPFILE flag.
+		 */
+		err = btrfs_orphan_del(trans, BTRFS_I(inode));
 		if (err)
-			goto fail;
-		if (inode->i_nlink == 1) {
-			/*
-			 * If new hard link count is 1, it's a file created
-			 * with open(2) O_TMPFILE flag.
-			 */
-			err = btrfs_orphan_del(trans, BTRFS_I(inode));
-			if (err)
-				goto fail;
-		}
-		BTRFS_I(inode)->last_link_trans = trans->transid;
-		d_instantiate(dentry, inode);
-		ret = btrfs_log_new_name(trans, BTRFS_I(inode), NULL, parent,
-					 true, NULL);
-		if (ret == BTRFS_NEED_TRANS_COMMIT) {
-			err = btrfs_commit_transaction(trans);
-			trans = NULL;
-		}
+			goto err_link;
+	}
+	BTRFS_I(inode)->last_link_trans = trans->transid;
+	ihold(inode);
+	d_instantiate(dentry, inode);
+	log_mode = btrfs_log_new_name(trans, BTRFS_I(inode), NULL,
+			dentry->d_parent, true, NULL);
+	if (log_mode == BTRFS_NEED_TRANS_COMMIT) {
+		err = btrfs_commit_transaction(trans);
+		trans = NULL;
 	}
 
+err_link:
+	if (err)
+		inode_dec_link_count(inode);
 fail:
 	if (trans)
 		btrfs_end_transaction(trans);
-	if (drop_inode) {
-		inode_dec_link_count(inode);
-		iput(inode);
-	}
 	btrfs_btree_balance_dirty(fs_info);
 	return err;
 }
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [V2] btrfs: drop inode reference count on error path
  2019-04-18 11:06 [V2] btrfs: drop inode reference count on error path Pan Bian
@ 2019-04-18 12:50 ` Nikolay Borisov
  2019-04-18 14:07   ` Josef Bacik
  2019-05-02 14:32 ` David Sterba
  1 sibling, 1 reply; 6+ messages in thread
From: Nikolay Borisov @ 2019-04-18 12:50 UTC (permalink / raw)
  To: Pan Bian, Chris Mason, Josef Bacik, David Sterba
  Cc: linux-btrfs, linux-kernel



On 18.04.19 г. 14:06 ч., Pan Bian wrote:
> The reference count of inode is incremented by ihold. It should be
> dropped if not used. However, the reference count is not dropped if
> error occurs during updating the inode or deleting orphan items. This
> patch fixes the bug.
> 
> Signed-off-by: Pan Bian <bianpan2016@163.com>
> ---
> V2: move ihold just before device_initialize to make code clearer
> ---
>  fs/btrfs/inode.c | 54 +++++++++++++++++++++++++-----------------------------
>  1 file changed, 25 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 82fdda8..d6630df 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6579,7 +6579,7 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>  	u64 index;
>  	int err;
> -	int drop_inode = 0;
> +	int log_mode;
>  
>  	/* do not allow sys_link's with other subvols of the same device */
>  	if (root->root_key.objectid != BTRFS_I(inode)->root->root_key.objectid)
> @@ -6616,41 +6616,37 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
>  	err = btrfs_add_nondir(trans, BTRFS_I(dir), dentry, BTRFS_I(inode),
>  			1, index);
>  
> -	if (err) {
> -		drop_inode = 1;
> -	} else {
> -		struct dentry *parent = dentry->d_parent;
> -		int ret;
> +	if (err)
> +		goto err_link;
>  
> -		err = btrfs_update_inode(trans, root, inode);
> +	err = btrfs_update_inode(trans, root, inode);
> +	if (err)
> +		goto err_link;
> +	if (inode->i_nlink == 1) {
> +		/*
> +		 * If new hard link count is 1, it's a file created
> +		 * with open(2) O_TMPFILE flag.
> +		 */
> +		err = btrfs_orphan_del(trans, BTRFS_I(inode));
>  		if (err)
> -			goto fail;
> -		if (inode->i_nlink == 1) {
> -			/*
> -			 * If new hard link count is 1, it's a file created
> -			 * with open(2) O_TMPFILE flag.
> -			 */
> -			err = btrfs_orphan_del(trans, BTRFS_I(inode));
> -			if (err)
> -				goto fail;
> -		}
> -		BTRFS_I(inode)->last_link_trans = trans->transid;
> -		d_instantiate(dentry, inode);
> -		ret = btrfs_log_new_name(trans, BTRFS_I(inode), NULL, parent,
> -					 true, NULL);
> -		if (ret == BTRFS_NEED_TRANS_COMMIT) {
> -			err = btrfs_commit_transaction(trans);
> -			trans = NULL;
> -		}
> +			goto err_link;
> +	}
> +	BTRFS_I(inode)->last_link_trans = trans->transid;
> +	ihold(inode);
> +	d_instantiate(dentry, inode);
> +	log_mode = btrfs_log_new_name(trans, BTRFS_I(inode), NULL,
> +			dentry->d_parent, true, NULL);
> +	if (log_mode == BTRFS_NEED_TRANS_COMMIT) {
> +		err = btrfs_commit_transaction(trans);
> +		trans = NULL;
>  	}
>  
> +err_link:
> +	if (err)
> +		inode_dec_link_count(inode);

Any particular reason why you moved this before ending the transaction?
It potentially has an effect during tyransaction commit since doing
inode_dec_link_count does mark_inode_dirty which moves the inode on the
dirty list. Have you explicitly thought about that ? Note, I'm not
saying it's wrong but I want the rationale for the code move.

>  fail:
>  	if (trans)
>  		btrfs_end_transaction(trans);
> -	if (drop_inode) {
> -		inode_dec_link_count(inode);
> -		iput(inode);
> -	}
>  	btrfs_btree_balance_dirty(fs_info);
>  	return err;
>  }
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [V2] btrfs: drop inode reference count on error path
  2019-04-18 12:50 ` Nikolay Borisov
@ 2019-04-18 14:07   ` Josef Bacik
  2019-04-18 14:09     ` Nikolay Borisov
  0 siblings, 1 reply; 6+ messages in thread
From: Josef Bacik @ 2019-04-18 14:07 UTC (permalink / raw)
  To: Nikolay Borisov
  Cc: Pan Bian, Chris Mason, Josef Bacik, David Sterba, linux-btrfs,
	linux-kernel

On Thu, Apr 18, 2019 at 03:50:00PM +0300, Nikolay Borisov wrote:
> 
> 
> On 18.04.19 г. 14:06 ч., Pan Bian wrote:
> > The reference count of inode is incremented by ihold. It should be
> > dropped if not used. However, the reference count is not dropped if
> > error occurs during updating the inode or deleting orphan items. This
> > patch fixes the bug.
> > 
> > Signed-off-by: Pan Bian <bianpan2016@163.com>
> > ---
> > V2: move ihold just before device_initialize to make code clearer
> > ---
> >  fs/btrfs/inode.c | 54 +++++++++++++++++++++++++-----------------------------
> >  1 file changed, 25 insertions(+), 29 deletions(-)
> > 
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 82fdda8..d6630df 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -6579,7 +6579,7 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
> >  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> >  	u64 index;
> >  	int err;
> > -	int drop_inode = 0;
> > +	int log_mode;
> >  
> >  	/* do not allow sys_link's with other subvols of the same device */
> >  	if (root->root_key.objectid != BTRFS_I(inode)->root->root_key.objectid)
> > @@ -6616,41 +6616,37 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
> >  	err = btrfs_add_nondir(trans, BTRFS_I(dir), dentry, BTRFS_I(inode),
> >  			1, index);
> >  
> > -	if (err) {
> > -		drop_inode = 1;
> > -	} else {
> > -		struct dentry *parent = dentry->d_parent;
> > -		int ret;
> > +	if (err)
> > +		goto err_link;
> >  
> > -		err = btrfs_update_inode(trans, root, inode);
> > +	err = btrfs_update_inode(trans, root, inode);
> > +	if (err)
> > +		goto err_link;
> > +	if (inode->i_nlink == 1) {
> > +		/*
> > +		 * If new hard link count is 1, it's a file created
> > +		 * with open(2) O_TMPFILE flag.
> > +		 */
> > +		err = btrfs_orphan_del(trans, BTRFS_I(inode));
> >  		if (err)
> > -			goto fail;
> > -		if (inode->i_nlink == 1) {
> > -			/*
> > -			 * If new hard link count is 1, it's a file created
> > -			 * with open(2) O_TMPFILE flag.
> > -			 */
> > -			err = btrfs_orphan_del(trans, BTRFS_I(inode));
> > -			if (err)
> > -				goto fail;
> > -		}
> > -		BTRFS_I(inode)->last_link_trans = trans->transid;
> > -		d_instantiate(dentry, inode);
> > -		ret = btrfs_log_new_name(trans, BTRFS_I(inode), NULL, parent,
> > -					 true, NULL);
> > -		if (ret == BTRFS_NEED_TRANS_COMMIT) {
> > -			err = btrfs_commit_transaction(trans);
> > -			trans = NULL;
> > -		}
> > +			goto err_link;
> > +	}
> > +	BTRFS_I(inode)->last_link_trans = trans->transid;
> > +	ihold(inode);

Where is the iput for this ihold?

Josef

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [V2] btrfs: drop inode reference count on error path
  2019-04-18 14:07   ` Josef Bacik
@ 2019-04-18 14:09     ` Nikolay Borisov
  2019-04-18 14:11       ` Josef Bacik
  0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Borisov @ 2019-04-18 14:09 UTC (permalink / raw)
  To: Josef Bacik
  Cc: Pan Bian, Chris Mason, David Sterba, linux-btrfs, linux-kernel



On 18.04.19 г. 17:07 ч., Josef Bacik wrote:
> On Thu, Apr 18, 2019 at 03:50:00PM +0300, Nikolay Borisov wrote:
>>
>>
>> On 18.04.19 г. 14:06 ч., Pan Bian wrote:
>>> The reference count of inode is incremented by ihold. It should be
>>> dropped if not used. However, the reference count is not dropped if
>>> error occurs during updating the inode or deleting orphan items. This
>>> patch fixes the bug.
>>>
>>> Signed-off-by: Pan Bian <bianpan2016@163.com>
>>> ---
>>> V2: move ihold just before device_initialize to make code clearer
>>> ---
>>>  fs/btrfs/inode.c | 54 +++++++++++++++++++++++++-----------------------------
>>>  1 file changed, 25 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 82fdda8..d6630df 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -6579,7 +6579,7 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
>>>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>>>  	u64 index;
>>>  	int err;
>>> -	int drop_inode = 0;
>>> +	int log_mode;
>>>  
>>>  	/* do not allow sys_link's with other subvols of the same device */
>>>  	if (root->root_key.objectid != BTRFS_I(inode)->root->root_key.objectid)
>>> @@ -6616,41 +6616,37 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
>>>  	err = btrfs_add_nondir(trans, BTRFS_I(dir), dentry, BTRFS_I(inode),
>>>  			1, index);
>>>  
>>> -	if (err) {
>>> -		drop_inode = 1;
>>> -	} else {
>>> -		struct dentry *parent = dentry->d_parent;
>>> -		int ret;
>>> +	if (err)
>>> +		goto err_link;
>>>  
>>> -		err = btrfs_update_inode(trans, root, inode);
>>> +	err = btrfs_update_inode(trans, root, inode);
>>> +	if (err)
>>> +		goto err_link;
>>> +	if (inode->i_nlink == 1) {
>>> +		/*
>>> +		 * If new hard link count is 1, it's a file created
>>> +		 * with open(2) O_TMPFILE flag.
>>> +		 */
>>> +		err = btrfs_orphan_del(trans, BTRFS_I(inode));
>>>  		if (err)
>>> -			goto fail;
>>> -		if (inode->i_nlink == 1) {
>>> -			/*
>>> -			 * If new hard link count is 1, it's a file created
>>> -			 * with open(2) O_TMPFILE flag.
>>> -			 */
>>> -			err = btrfs_orphan_del(trans, BTRFS_I(inode));
>>> -			if (err)
>>> -				goto fail;
>>> -		}
>>> -		BTRFS_I(inode)->last_link_trans = trans->transid;
>>> -		d_instantiate(dentry, inode);
>>> -		ret = btrfs_log_new_name(trans, BTRFS_I(inode), NULL, parent,
>>> -					 true, NULL);
>>> -		if (ret == BTRFS_NEED_TRANS_COMMIT) {
>>> -			err = btrfs_commit_transaction(trans);
>>> -			trans = NULL;
>>> -		}
>>> +			goto err_link;
>>> +	}
>>> +	BTRFS_I(inode)->last_link_trans = trans->transid;
>>> +	ihold(inode);
> 
> Where is the iput for this ihold?

This ihold is sort of "given" to the d_instantiate. I.e the iput happens
when the respective dentry is unhashed/removed.

> 
> Josef
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [V2] btrfs: drop inode reference count on error path
  2019-04-18 14:09     ` Nikolay Borisov
@ 2019-04-18 14:11       ` Josef Bacik
  0 siblings, 0 replies; 6+ messages in thread
From: Josef Bacik @ 2019-04-18 14:11 UTC (permalink / raw)
  To: Nikolay Borisov
  Cc: Josef Bacik, Pan Bian, Chris Mason, David Sterba, linux-btrfs,
	linux-kernel

On Thu, Apr 18, 2019 at 05:09:44PM +0300, Nikolay Borisov wrote:
> 
> 
> On 18.04.19 г. 17:07 ч., Josef Bacik wrote:
> > On Thu, Apr 18, 2019 at 03:50:00PM +0300, Nikolay Borisov wrote:
> >>
> >>
> >> On 18.04.19 г. 14:06 ч., Pan Bian wrote:
> >>> The reference count of inode is incremented by ihold. It should be
> >>> dropped if not used. However, the reference count is not dropped if
> >>> error occurs during updating the inode or deleting orphan items. This
> >>> patch fixes the bug.
> >>>
> >>> Signed-off-by: Pan Bian <bianpan2016@163.com>
> >>> ---
> >>> V2: move ihold just before device_initialize to make code clearer
> >>> ---
> >>>  fs/btrfs/inode.c | 54 +++++++++++++++++++++++++-----------------------------
> >>>  1 file changed, 25 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> >>> index 82fdda8..d6630df 100644
> >>> --- a/fs/btrfs/inode.c
> >>> +++ b/fs/btrfs/inode.c
> >>> @@ -6579,7 +6579,7 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
> >>>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> >>>  	u64 index;
> >>>  	int err;
> >>> -	int drop_inode = 0;
> >>> +	int log_mode;
> >>>  
> >>>  	/* do not allow sys_link's with other subvols of the same device */
> >>>  	if (root->root_key.objectid != BTRFS_I(inode)->root->root_key.objectid)
> >>> @@ -6616,41 +6616,37 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
> >>>  	err = btrfs_add_nondir(trans, BTRFS_I(dir), dentry, BTRFS_I(inode),
> >>>  			1, index);
> >>>  
> >>> -	if (err) {
> >>> -		drop_inode = 1;
> >>> -	} else {
> >>> -		struct dentry *parent = dentry->d_parent;
> >>> -		int ret;
> >>> +	if (err)
> >>> +		goto err_link;
> >>>  
> >>> -		err = btrfs_update_inode(trans, root, inode);
> >>> +	err = btrfs_update_inode(trans, root, inode);
> >>> +	if (err)
> >>> +		goto err_link;
> >>> +	if (inode->i_nlink == 1) {
> >>> +		/*
> >>> +		 * If new hard link count is 1, it's a file created
> >>> +		 * with open(2) O_TMPFILE flag.
> >>> +		 */
> >>> +		err = btrfs_orphan_del(trans, BTRFS_I(inode));
> >>>  		if (err)
> >>> -			goto fail;
> >>> -		if (inode->i_nlink == 1) {
> >>> -			/*
> >>> -			 * If new hard link count is 1, it's a file created
> >>> -			 * with open(2) O_TMPFILE flag.
> >>> -			 */
> >>> -			err = btrfs_orphan_del(trans, BTRFS_I(inode));
> >>> -			if (err)
> >>> -				goto fail;
> >>> -		}
> >>> -		BTRFS_I(inode)->last_link_trans = trans->transid;
> >>> -		d_instantiate(dentry, inode);
> >>> -		ret = btrfs_log_new_name(trans, BTRFS_I(inode), NULL, parent,
> >>> -					 true, NULL);
> >>> -		if (ret == BTRFS_NEED_TRANS_COMMIT) {
> >>> -			err = btrfs_commit_transaction(trans);
> >>> -			trans = NULL;
> >>> -		}
> >>> +			goto err_link;
> >>> +	}
> >>> +	BTRFS_I(inode)->last_link_trans = trans->transid;
> >>> +	ihold(inode);
> > 
> > Where is the iput for this ihold?
> 
> This ihold is sort of "given" to the d_instantiate. I.e the iput happens
> when the respective dentry is unhashed/removed.

Ah that's what I was missing, thanks,

Josef

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [V2] btrfs: drop inode reference count on error path
  2019-04-18 11:06 [V2] btrfs: drop inode reference count on error path Pan Bian
  2019-04-18 12:50 ` Nikolay Borisov
@ 2019-05-02 14:32 ` David Sterba
  1 sibling, 0 replies; 6+ messages in thread
From: David Sterba @ 2019-05-02 14:32 UTC (permalink / raw)
  To: Pan Bian
  Cc: Chris Mason, Josef Bacik, David Sterba, linux-btrfs, linux-kernel

On Thu, Apr 18, 2019 at 07:06:16PM +0800, Pan Bian wrote:
> The reference count of inode is incremented by ihold. It should be
> dropped if not used. However, the reference count is not dropped if
> error occurs during updating the inode or deleting orphan items. This
> patch fixes the bug.
> 
> Signed-off-by: Pan Bian <bianpan2016@163.com>
> ---
> V2: move ihold just before device_initialize to make code clearer

There's nothing like device_initialize, what does this refer to?

> ---
>  fs/btrfs/inode.c | 54 +++++++++++++++++++++++++-----------------------------
>  1 file changed, 25 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 82fdda8..d6630df 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6579,7 +6579,7 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>  	u64 index;
>  	int err;
> -	int drop_inode = 0;
> +	int log_mode;
>  
>  	/* do not allow sys_link's with other subvols of the same device */
>  	if (root->root_key.objectid != BTRFS_I(inode)->root->root_key.objectid)
> @@ -6616,41 +6616,37 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
>  	err = btrfs_add_nondir(trans, BTRFS_I(dir), dentry, BTRFS_I(inode),
>  			1, index);
>  
> -	if (err) {
> -		drop_inode = 1;
> -	} else {
> -		struct dentry *parent = dentry->d_parent;
> -		int ret;
> +	if (err)
> +		goto err_link;
>  
> -		err = btrfs_update_inode(trans, root, inode);
> +	err = btrfs_update_inode(trans, root, inode);
> +	if (err)
> +		goto err_link;
> +	if (inode->i_nlink == 1) {
> +		/*
> +		 * If new hard link count is 1, it's a file created
> +		 * with open(2) O_TMPFILE flag.
> +		 */
> +		err = btrfs_orphan_del(trans, BTRFS_I(inode));
>  		if (err)
> -			goto fail;
> -		if (inode->i_nlink == 1) {
> -			/*
> -			 * If new hard link count is 1, it's a file created
> -			 * with open(2) O_TMPFILE flag.
> -			 */
> -			err = btrfs_orphan_del(trans, BTRFS_I(inode));
> -			if (err)
> -				goto fail;
> -		}
> -		BTRFS_I(inode)->last_link_trans = trans->transid;
> -		d_instantiate(dentry, inode);
> -		ret = btrfs_log_new_name(trans, BTRFS_I(inode), NULL, parent,
> -					 true, NULL);
> -		if (ret == BTRFS_NEED_TRANS_COMMIT) {
> -			err = btrfs_commit_transaction(trans);
> -			trans = NULL;
> -		}
> +			goto err_link;
> +	}
> +	BTRFS_I(inode)->last_link_trans = trans->transid;
> +	ihold(inode);
> +	d_instantiate(dentry, inode);

So this ihold pairs with d_instantiate, and there's another ihold in the
function, before call to btrfs_add_nondir. Isn't this leaking the
references? In normal case it's 2x ihold, in error case 1x.

6645         /* There are several dir indexes for this inode, clear the cache. */                                                                                                                                               
6646         BTRFS_I(inode)->dir_index = 0ULL;                                                                                                                                                                                  
6647         inc_nlink(inode);                                                                                                                                                                                                  
6648         inode_inc_iversion(inode);                                                                                                                                                                                         
6649         inode->i_ctime = current_time(inode);                                                                                                                                                                              
6650         ihold(inode);                                                                                                                                                                                                      
6651         set_bit(BTRFS_INODE_COPY_EVERYTHING, &BTRFS_I(inode)->runtime_flags);

> +	log_mode = btrfs_log_new_name(trans, BTRFS_I(inode), NULL,
> +			dentry->d_parent, true, NULL);
> +	if (log_mode == BTRFS_NEED_TRANS_COMMIT) {
> +		err = btrfs_commit_transaction(trans);
> +		trans = NULL;
>  	}
>  
> +err_link:
> +	if (err)
> +		inode_dec_link_count(inode);
>  fail:
>  	if (trans)
>  		btrfs_end_transaction(trans);
> -	if (drop_inode) {
> -		inode_dec_link_count(inode);
> -		iput(inode);

Ie. this iput does not have any replacement in the new code.

> -	}
>  	btrfs_btree_balance_dirty(fs_info);
>  	return err;
>  }
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-05-02 14:31 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-18 11:06 [V2] btrfs: drop inode reference count on error path Pan Bian
2019-04-18 12:50 ` Nikolay Borisov
2019-04-18 14:07   ` Josef Bacik
2019-04-18 14:09     ` Nikolay Borisov
2019-04-18 14:11       ` Josef Bacik
2019-05-02 14:32 ` 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.