[PATCH 0/2] btrfs_rename bugfixes.

[PATCH 0/2] btrfs_rename bugfixes.

[Davide C. C. Italiano]

From: Davide Italiano <dccitaliano@gmail.com>

These two patches fix some small problems in btrfs_rename() I noticed
while I was implementing RENAME_EXCHANGE.

Davide Italiano (2):
  [btrfs] btrfs_rename: abort transaction in case of error.
  [btrfs] btrfs_rename(): don't ignore btrfs_end_transaction() return

 fs/btrfs/inode.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.4.3

[PATCH 1/2] [btrfs] btrfs_rename: abort transaction in case of error.

[Davide C. C. Italiano]

From: Davide Italiano <dccitaliano@gmail.com>

btrfs_insert_inode_ref() may fail and we want to make sure
the transaction is aborted before calling btrfs_end_transaction(),
as it already happens everywhere else in this function in case
of error.

Signed-off-by: Davide Italiano <dccitaliano@gmail.com>
---
 fs/btrfs/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 8bb0136..59c475c 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9114,8 +9114,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 					     new_dentry->d_name.len,
 					     old_ino,
 					     btrfs_ino(new_dir), index);
-		if (ret)
+		if (ret) {
+			btrfs_abort_transaction(trans, root, ret);
 			goto out_fail;
+		}
+
 		/*
 		 * this is an ugly little race, but the rename is required
 		 * to make sure that if we crash, the inode is either at the
-- 
2.4.3

[PATCH 2/2] [btrfs] btrfs_rename(): don't ignore btrfs_end_transaction() return

[Davide C. C. Italiano]

From: Davide Italiano <dccitaliano@gmail.com>

btrfs_end_transaction() can return an error -- this happens, e.g.
if it tries to commit and the transaction was aborted in the meanhwile.
Swallowing the error is wrong, so explicitly return it.

Signed-off-by: Davide Italiano <dccitaliano@gmail.com>
---
 fs/btrfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 59c475c..7764132 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9199,7 +9199,7 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		btrfs_end_log_trans(root);
 	}
 out_fail:
-	btrfs_end_transaction(trans, root);
+	ret = btrfs_end_transaction(trans, root);
 out_notrans:
 	if (old_ino == BTRFS_FIRST_FREE_OBJECTID)
 		up_read(&root->fs_info->subvol_sem);
-- 
2.4.3

Re: [PATCH 1/2] [btrfs] btrfs_rename: abort transaction in case of error.

[Filipe David Manana]

On Sun, Jun 28, 2015 at 10:47 PM, Davide C. C. Italiano
<dccitaliano@gmail.com> wrote:
> From: Davide Italiano <dccitaliano@gmail.com>
>
> btrfs_insert_inode_ref() may fail and we want to make sure
> the transaction is aborted before calling btrfs_end_transaction(),
> as it already happens everywhere else in this function in case
> of error.
>
> Signed-off-by: Davide Italiano <dccitaliano@gmail.com>
> ---
>  fs/btrfs/inode.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 8bb0136..59c475c 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -9114,8 +9114,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>                                              new_dentry->d_name.len,
>                                              old_ino,
>                                              btrfs_ino(new_dir), index);
> -               if (ret)
> +               if (ret) {
> +                       btrfs_abort_transaction(trans, root, ret);
>                         goto out_fail;
> +               }
> +

Hi,

I don't think we need a transaction abortion here. The reason it's not
being done is likely because at that point the trees are in a
consistent state (i.e. we haven't touched any of them yet) and not
because it was forgotten. So an abortion there is
unnecessary/excessive.

thanks

>                 /*
>                  * this is an ugly little race, but the rename is required
>                  * to make sure that if we crash, the inode is either at the
> --
> 2.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 2/2] [btrfs] btrfs_rename(): don't ignore btrfs_end_transaction() return

[Filipe David Manana]

On Sun, Jun 28, 2015 at 10:47 PM, Davide C. C. Italiano
<dccitaliano@gmail.com> wrote:
> From: Davide Italiano <dccitaliano@gmail.com>
>
> btrfs_end_transaction() can return an error -- this happens, e.g.
> if it tries to commit and the transaction was aborted in the meanhwile.
> Swallowing the error is wrong, so explicitly return it.
>
> Signed-off-by: Davide Italiano <dccitaliano@gmail.com>
> ---
>  fs/btrfs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 59c475c..7764132 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -9199,7 +9199,7 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>                 btrfs_end_log_trans(root);
>         }
>  out_fail:
> -       btrfs_end_transaction(trans, root);
> +       ret = btrfs_end_transaction(trans, root);

Hi,

Good intention but it's now swallowing errors from earlier places in
the code that jump to the out_fail label. For e.g. if the call to
btrfs_set_inode_index() fails, we jump to out_fail and we lose the
error value that it returned (btrfs_end_transaction() returns 0 for
e.g.), so userspace thinks everything succeed when it didn't.
Correct fix is to set ret to the return value of
btrfs_end_transaction() only if ret is currently zero.

thanks

>  out_notrans:
>         if (old_ino == BTRFS_FIRST_FREE_OBJECTID)
>                 up_read(&root->fs_info->subvol_sem);
> --
> 2.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

[PATCH 2/2] [btrfs] btrfs_rename(): don't ignore btrfs_end_transaction() return

[Davide C. C. Italiano]

From: Davide Italiano <dccitaliano@gmail.com>

btrfs_end_transaction() can return an error -- this happens, e.g.
if it tries to commit and the transaction was aborted in the meanhwile.
Swallowing the error is wrong, so explicitly return it.

Signed-off-by: Davide Italiano <dccitaliano@gmail.com>
---
 fs/btrfs/inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 59c475c..61b26be 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9199,7 +9199,8 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		btrfs_end_log_trans(root);
 	}
 out_fail:
-	btrfs_end_transaction(trans, root);
+	if (!ret)
+		ret = btrfs_end_transaction(trans, root);
 out_notrans:
 	if (old_ino == BTRFS_FIRST_FREE_OBJECTID)
 		up_read(&root->fs_info->subvol_sem);
-- 
2.4.3

Re: [PATCH 1/2] [btrfs] btrfs_rename: abort transaction in case of error.

[Davide Italiano]

On Mon, Jun 29, 2015 at 4:59 AM, Filipe David Manana <fdmanana@gmail.com> wrote:
> On Sun, Jun 28, 2015 at 10:47 PM, Davide C. C. Italiano
> <dccitaliano@gmail.com> wrote:
>> From: Davide Italiano <dccitaliano@gmail.com>
>>
>> btrfs_insert_inode_ref() may fail and we want to make sure
>> the transaction is aborted before calling btrfs_end_transaction(),
>> as it already happens everywhere else in this function in case
>> of error.
>>
>> Signed-off-by: Davide Italiano <dccitaliano@gmail.com>
>> ---
>>  fs/btrfs/inode.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 8bb0136..59c475c 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -9114,8 +9114,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>                                              new_dentry->d_name.len,
>>                                              old_ino,
>>                                              btrfs_ino(new_dir), index);
>> -               if (ret)
>> +               if (ret) {
>> +                       btrfs_abort_transaction(trans, root, ret);
>>                         goto out_fail;
>> +               }
>> +
>
> Hi,
>
> I don't think we need a transaction abortion here. The reason it's not
> being done is likely because at that point the trees are in a
> consistent state (i.e. we haven't touched any of them yet) and not
> because it was forgotten. So an abortion there is
> unnecessary/excessive.
>
> thanks
>

Thank you for the comment -- I updated the other patch and I have
mixed feeling about this one.
I can either withdrawn the review or provide a new patch where I add a
comment to clarify why this is not needed, for the future.
Which one do you like better?

--
Davide

Re: [PATCH 2/2] [btrfs] btrfs_rename(): don't ignore btrfs_end_transaction() return

[Filipe David Manana]

On Tue, Jun 30, 2015 at 5:15 AM, Davide C. C. Italiano
<dccitaliano@gmail.com> wrote:
> From: Davide Italiano <dccitaliano@gmail.com>
>
> btrfs_end_transaction() can return an error -- this happens, e.g.
> if it tries to commit and the transaction was aborted in the meanhwile.
> Swallowing the error is wrong, so explicitly return it.
>
> Signed-off-by: Davide Italiano <dccitaliano@gmail.com>
> ---
>  fs/btrfs/inode.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 59c475c..61b26be 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -9199,7 +9199,8 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>                 btrfs_end_log_trans(root);
>         }
>  out_fail:
> -       btrfs_end_transaction(trans, root);
> +       if (!ret)
> +               ret = btrfs_end_transaction(trans, root);

So it an error happened before, we still want to call
btrfs_end_transaction(), otherwise the transaction's refcount never
drops to 0 and its resources are never freed (memory).

Something like this:

if (ret)
    btrfs_end_transaction(trans, root);
else
    ret = btrfs_end_transaction(trans, root);

or

int ret2;

ret2 = btrfs_end_transaction(trans, root);
if (!ret)
    ret = ret2;

Also don't forget to target your patches with V<number> and describe
and what changed between patch versions (see
https://btrfs.wiki.kernel.org/index.php/Writing_patch_for_btrfs for
instructions about how to do it).

thanks

>  out_notrans:
>         if (old_ino == BTRFS_FIRST_FREE_OBJECTID)
>                 up_read(&root->fs_info->subvol_sem);
> --
> 2.4.3
>



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 1/2] [btrfs] btrfs_rename: abort transaction in case of error.

[Filipe David Manana]

On Tue, Jun 30, 2015 at 5:17 AM, Davide Italiano <dccitaliano@gmail.com> wrote:
> On Mon, Jun 29, 2015 at 4:59 AM, Filipe David Manana <fdmanana@gmail.com> wrote:
>> On Sun, Jun 28, 2015 at 10:47 PM, Davide C. C. Italiano
>> <dccitaliano@gmail.com> wrote:
>>> From: Davide Italiano <dccitaliano@gmail.com>
>>>
>>> btrfs_insert_inode_ref() may fail and we want to make sure
>>> the transaction is aborted before calling btrfs_end_transaction(),
>>> as it already happens everywhere else in this function in case
>>> of error.
>>>
>>> Signed-off-by: Davide Italiano <dccitaliano@gmail.com>
>>> ---
>>>  fs/btrfs/inode.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 8bb0136..59c475c 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -9114,8 +9114,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>                                              new_dentry->d_name.len,
>>>                                              old_ino,
>>>                                              btrfs_ino(new_dir), index);
>>> -               if (ret)
>>> +               if (ret) {
>>> +                       btrfs_abort_transaction(trans, root, ret);
>>>                         goto out_fail;
>>> +               }
>>> +
>>
>> Hi,
>>
>> I don't think we need a transaction abortion here. The reason it's not
>> being done is likely because at that point the trees are in a
>> consistent state (i.e. we haven't touched any of them yet) and not
>> because it was forgotten. So an abortion there is
>> unnecessary/excessive.
>>
>> thanks
>>
>
> Thank you for the comment -- I updated the other patch and I have
> mixed feeling about this one.
> I can either withdrawn the review or provide a new patch where I add a
> comment to clarify why this is not needed, for the future.
> Which one do you like better?

Hi,

I don't think it's needed. We do this pattern in many places and it's
quite obvious if one reads the code flow.

thanks

>
> --
> Davide



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."