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?