From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:49409 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751280AbdJWQ3c (ORCPT ); Mon, 23 Oct 2017 12:29:32 -0400 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id EF85769259 for ; Mon, 23 Oct 2017 16:29:30 +0000 (UTC) Subject: Re: [PATCH v4] btrfs: Fix transaction abort during failure in btrfs_rm_dev_item To: Nikolay Borisov , dsterba@suse.cz References: <1508481013-16074-1-git-send-email-nborisov@suse.com> <1508741926-27125-1-git-send-email-nborisov@suse.com> Cc: linux-btrfs@vger.kernel.org From: Edmund Nadolski Message-ID: <7f2a3c59-818f-c7dd-de6a-7db8f9e8d818@suse.de> Date: Mon, 23 Oct 2017 10:29:27 -0600 MIME-Version: 1.0 In-Reply-To: <1508741926-27125-1-git-send-email-nborisov@suse.com> Content-Type: text/plain; charset=utf-8 Sender: linux-btrfs-owner@vger.kernel.org List-ID: On 10/23/2017 12:58 AM, Nikolay Borisov wrote: > btrfs_rm_dev_item calls several function under an activa transaction, however ^^ active > it fails to abort it if an error happens. Fix this by adding explicit > btrfs_abort_transaction/btrfs_end_transaction calls > > Signed-off-by: Nikolay Borisov > --- > V4: > * Reorder the code a bit to prevent duplication of btrfs_free_path > invocation. > > * Collapse the handling of btrfs_search_slot return value in a single if > branch rather than having it spread across 2 branches > > V3: > * The path needs to be freed before the the transaction is comitted otherwise > we will deadlock. > fs/btrfs/volumes.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 0e8f16c305df..8b139d203f8c 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -1765,20 +1765,24 @@ static int btrfs_rm_dev_item(struct btrfs_fs_info *fs_info, > key.offset = device->devid; > > ret = btrfs_search_slot(trans, root, &key, path, -1, 1); > - if (ret < 0) > - goto out; > - > - if (ret > 0) { > - ret = -ENOENT; > + if (ret) { > + if (ret > 0) > + ret = -ENOENT; > + btrfs_abort_transaction(trans, ret); > + btrfs_end_transaction(trans); > goto out; > } > > ret = btrfs_del_item(trans, root, path); > - if (ret) > - goto out; > + if (ret) { > + btrfs_abort_transaction(trans, ret); > + btrfs_end_transaction(trans); > + } > + > out: > btrfs_free_path(path); > - btrfs_commit_transaction(trans); > + if (!ret) > + ret = btrfs_commit_transaction(trans); > return ret; > } > > Perhaps slightly simpler (and the 'out:' label maybe goes away): ..... ret = btrfs_search_slot(trans, root, &key, path, -1, 1); if (ret > 0) ret = -ENOENT; else if (!ret) ret = btrfs_del_item(trans, root, path); if (ret) { btrfs_abort_transaction(trans, ret); btrfs_end_transaction(trans); } out: ..... Ed