| Message ID | 1508741926-27125-1-git-send-email-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <nborisov@suse.com> > --- > 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 -- 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, Oct 23, 2017 at 10:29:27AM -0600, Edmund Nadolski wrote: > > --- 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); This would merge 2 abort sites into one, btrfs_search_slot and btrfs_del_item, so it wouldn't be obvious which one failed just from the stack trace and line number. V4 is ok, as it only joins return values from btrfs_search_slot, where the positive value means "search slot was not able to find the key, but this is where you should insert it". This really translates to ENOENT so we're not losing any information. -- 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
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; }
btrfs_rm_dev_item calls several function under an activa transaction, however 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 <nborisov@suse.com> --- 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(-)