[Btrfs-devel] A few questions about error handling

[Philip Martin]

Hello,

I started looking at the code, choosing mkdir in inode.c more or less
at random, and I have a few questions.  The first bit of code

        mutex_lock(&root->fs_info->fs_mutex);
        trans = btrfs_start_transaction(root, 1);
        btrfs_set_trans_block_group(trans, dir);
        if (IS_ERR(trans)) {
                err = PTR_ERR(trans);
                goto out_unlock;
        }

Why is the IS_ERR call not immediately after the
btrfs_start_transaction call?  Calling btrfs_set_trans_block_group if
trans is an error looks a bit odd, and btrfs_set_trans_block_group
itself cannot change an non-error trans into an error.

Next

        err = btrfs_find_free_objectid(trans, root, dir->i_ino, &objectid);
        if (err) {
                err = -ENOSPC;
                goto out_unlock;
        }

Why does the error code go to out_unlock rather than out_fail?  It
bypasses the end_transaction call, doesn't that mean the transaction
sort of "leaks"?

Next I looked at start_transaction:

        struct btrfs_trans_handle *h =
                kmem_cache_alloc(btrfs_trans_handle_cachep, GFP_NOFS);
        int ret;

        mutex_lock(&root->fs_info->trans_mutex);
        ret = join_transaction(root);
        BUG_ON(ret);

I think the kmem_cache_alloc call can fail, but there is no error
checking.  Should this return -ENOMEM?  On the other hand
join_transaction can only ever return 0 so the BUG_ON is never going
to trigger, perhaps join_transaction should really be a void return?

I realise that in new code the error paths can sometimes be a bit
rough-and-ready, particularly paths that don't trigger very often.  Is
that the case here, or am I simply misunderstanding the code?  Are you
interested in patches for such things?