On Mon, Jun 29, 2015 at 4:59 AM, Filipe David Manana <fdmanana@xxxxxxxxx> wrote:
> On Sun, Jun 28, 2015 at 10:47 PM, Davide C. C. Italiano
> <dccitaliano@xxxxxxxxx> wrote:
>> From: Davide Italiano <dccitaliano@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
