From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:45969 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753057AbdI0OBm (ORCPT ); Wed, 27 Sep 2017 10:01:42 -0400 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 6BB59ABFB for ; Wed, 27 Sep 2017 14:01:41 +0000 (UTC) Date: Wed, 27 Sep 2017 16:00:08 +0200 From: David Sterba To: Nikolay Borisov Cc: dsterba@suse.cz, linux-btrfs@vger.kernel.org, fdmanana@suse.com Subject: Re: [PATCH 1/2] btrfs: Abort transaction if btrfs_update_root fails in btrfs_ioctl_subvol_setflags Message-ID: <20170927140007.GH31640@twin.jikos.cz> Reply-To: dsterba@suse.cz References: <1506436062-23825-1-git-send-email-nborisov@suse.com> <20170926173939.GV31640@twin.jikos.cz> <6c7f1d0c-d61e-8cab-7f90-01c76e10cd01@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <6c7f1d0c-d61e-8cab-7f90-01c76e10cd01@suse.com> Sender: linux-btrfs-owner@vger.kernel.org List-ID: 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 > >> --- > >> 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.