On Wed, Sep 27, 2017 at 11:48:59AM +0300, Nikolay Borisov wrote:
> On 26.09.2017 20:39, David Sterba wrote:
> > On Tue, Sep 26, 2017 at 05:27:41PM +0300, Nikolay Borisov wrote:
> >> btrfs_update_root can fail for any number of reasons, however the only error
> >> handling we do is to revert the modified flags, yet we do not abort the
> >> transaction but proceed to commit it.
> >
> > AFAICS btrfs_update_root aborts the transaction itself, so it's not
> > needed in the caller.
> >
> >> Fix this by explicitly checking if the
> >> update root routine has failed and abort the transaction.
> >>
> >> Fixes: 0caa102da827 ("Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctls")
> >> Signed-off-by: Nikolay Borisov <nborisov@xxxxxxxx>
> >> ---
> >> fs/btrfs/ioctl.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> >> index d6715c2bcdc4..09fcd51f0e8c 100644
> >> --- a/fs/btrfs/ioctl.c
> >> +++ b/fs/btrfs/ioctl.c
> >> @@ -1842,8 +1842,11 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
> >>
> >> ret = btrfs_update_root(trans, fs_info->tree_root,
> >> &root->root_key, &root->root_item);
> >> + if (ret < 0)
> >> + btrfs_abort_transaction(trans, ret);
> >>
> >> btrfs_commit_transaction(trans);
> >
> > I think the error from commit_transaction should be returned as well if
> > we get here.
> >
> > So:
> >
> > ret = btrfs_update_root();
> >
> > if (ret < 0) {
> > end_transaction();
> > goto out_reset;
> > }
> >
> > ret = btrfs_commit_transaction();
> >
> > out_reset:
> > /* and then as it is */
> > if (ret)
> > btrfs_set_root_flags(...);
> >
> > etc.
>
> I think even this is not needed, since btrfs_commit_transaction actually
> handles aborted transaction by calling btrfs_end_transaction and
> returning the error with which the trans was aborted. So I will just
> drop this patch
>From API point of view, I'd like to see some pattern, where we don't
call commit after transaction abort IF we have enough information from
the context. Ie. the abort and commit would be in the same function.
Commit has to deal with an aborted transaction, that's right, but kind
of an implementation detail and last resort resolution.
--
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
