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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
