[Btrfs-devel] [PATCH 3/4] the main xattr support code

[Zach Brown]

> +ssize_t btrfs_xattr_get(struct inode *inode, int name_index,
> +			const char *attr_name, void *buffer, size_t size)
> +{
> +	struct btrfs_dir_item *di;
> +	struct btrfs_root *root = BTRFS_I(inode)->root;
> +	struct btrfs_path *path;
> +	struct xattr_handler *handler = btrfs_xattr_handler(name_index);
> +	int ret = 0;
> +	char *data_ptr, *name;
> +
> +	if (!handler)
> +		return -EOPNOTSUPP;
> +
> +	name = get_name(attr_name, name_index);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	mutex_lock(&root->fs_info->fs_mutex);
> +	/* lookup the xattr by name */
> +	path = btrfs_alloc_path();
> +	BUG_ON(!path);

BUG_ON() for an alloc failure?  It looks like it'd be pretty easy to get
the state unrolling right as this function exits.

The allocation should probably be done outside the fs_mutex so that we
don't hold it while trying to find memory.  It looks like it's done in
that order in a few other functions.

> +out:
> +	kfree(name);
> +	btrfs_free_path(path);
> +	mutex_unlock(&root->fs_info->fs_mutex);

Trivial, but unlock before freeing to reduce the mutex hold time :).

> +int btrfs_xattr_set(struct inode *inode, int name_index,
> +		    const char *attr_name, const void *value, size_t size,
> +		    int flags)
> +{

The previous comments apply here, too.  And in a few more places, it
looks like :).

> +	path = btrfs_alloc_path();
> +	path->reada = 2;
> +	BUG_ON(!path);

*cough*.  :)

- z