> +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

_______________________________________________
Btrfs-devel mailing list
[email protected]
http://oss.oracle.com/mailman/listinfo/btrfs-devel

Reply via email to