| Message ID | 1435528041-20878-2-git-send-email-dccitaliano@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 -- 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
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
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