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