From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:40545 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751730AbdI0ItC (ORCPT ); Wed, 27 Sep 2017 04:49:02 -0400 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 402D8AAC3 for ; Wed, 27 Sep 2017 08:49:01 +0000 (UTC) Subject: Re: [PATCH 1/2] btrfs: Abort transaction if btrfs_update_root fails in btrfs_ioctl_subvol_setflags To: dsterba@suse.cz, linux-btrfs@vger.kernel.org, fdmanana@suse.com References: <1506436062-23825-1-git-send-email-nborisov@suse.com> <20170926173939.GV31640@twin.jikos.cz> From: Nikolay Borisov Message-ID: <6c7f1d0c-d61e-8cab-7f90-01c76e10cd01@suse.com> Date: Wed, 27 Sep 2017 11:48:59 +0300 MIME-Version: 1.0 In-Reply-To: <20170926173939.GV31640@twin.jikos.cz> Content-Type: text/plain; charset=utf-8 Sender: linux-btrfs-owner@vger.kernel.org List-ID: 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 > > > >> + >> out_reset: >> if (ret) >> btrfs_set_root_flags(&root->root_item, root_flags); >> -- >> 2.7.4 >> >> -- >> 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