From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Morton Subject: Re: [PATCH 01/35] Btrfs xattr code Date: Mon, 12 Jan 2009 20:59:04 -0800 Message-ID: <20090112205904.c9e11d85.akpm@linux-foundation.org> References: <1231387045-27838-1-git-send-email-chris.mason@oracle.com> <1231387045-27838-2-git-send-email-chris.mason@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: torvalds@linux-foundation.org, linux-fsdevel@vger.kernel.org To: Chris Mason Return-path: Received: from smtp1.linux-foundation.org ([140.211.169.13]:52305 "EHLO smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752064AbZAME7W (ORCPT ); Mon, 12 Jan 2009 23:59:22 -0500 In-Reply-To: <1231387045-27838-2-git-send-email-chris.mason@oracle.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, 7 Jan 2009 22:56:51 -0500 Chris Mason wrote: > Xattrs in btrfs are stored in the btree close to the inode item. They use an > indexing scheme very similar to directories. > > The max size of a btrfs xattr is limited by the size of a btree item, > or about 4k. A future version will support larger items. > > > ... > > +ssize_t __btrfs_getxattr(struct inode *inode, const char *name, > + void *buffer, size_t size) > +{ > + struct btrfs_dir_item *di; > + struct btrfs_root *root = BTRFS_I(inode)->root; > + struct btrfs_path *path; > + struct extent_buffer *leaf; > + int ret = 0; > + unsigned long data_ptr; > + > + path = btrfs_alloc_path(); > + if (!path) > + return -ENOMEM; > + > + /* lookup the xattr by name */ > + di = btrfs_lookup_xattr(NULL, root, path, inode->i_ino, name, > + strlen(name), 0); > + if (!di || IS_ERR(di)) { This seems quite bad. An error return from btrfs_lookup_xattr() can arise from (I assume) OOM, corrupted filesystem, I/O error, etc. Yet here we treat such errors as being the same as "not found". This seems grossly misleading to userspace and possible a security problem? (I'm kind of guessing here, and might be wrong and wasting everyone's time, because the semantics of the btrfs_lookup_xattr() and btrfs_getxattr() return values aren't documented). > + ret = -ENODATA; > + goto out; > + } > + > + leaf = path->nodes[0]; > + /* if size is 0, that means we want the size of the attr */ > + if (!size) { > + ret = btrfs_dir_data_len(leaf, di); > + goto out; > + } > + > + /* now get the data out of our dir_item */ > + if (btrfs_dir_data_len(leaf, di) > size) { OK, where did we hide the btrfs_dir_data_len() definition? > + ret = -ERANGE; > + goto out; > + } > + data_ptr = (unsigned long)((char *)(di + 1) + > + btrfs_dir_name_len(leaf, di)); > + read_extent_buffer(leaf, buffer, data_ptr, > + btrfs_dir_data_len(leaf, di)); Please document read_extent_buffer(). Please rename read_extent_buffer() to something more appropriate to a kernel-wide symbol. Please review all kernel-wide identifiers in btrfs for appropriateness. I'm scratching my head wondering about this `data_ptr' thing. Is it a disk offset? Is it really a pointer to kernel memory? According to this code it is indeed a kernel pointer, but it then gets stuffed into an unsigned long (wtf?) and then passed to the mysterious read_extent_buffer(). > + ret = btrfs_dir_data_len(leaf, di); > + > +out: > + btrfs_free_path(path); > + return ret; > +} > + > +int __btrfs_setxattr(struct inode *inode, const char *name, > + const void *value, size_t size, int flags) > +{ > + struct btrfs_dir_item *di; > + struct btrfs_root *root = BTRFS_I(inode)->root; > + struct btrfs_trans_handle *trans; > + struct btrfs_path *path; > + int ret = 0, mod = 0; > + > + path = btrfs_alloc_path(); > + if (!path) > + return -ENOMEM; > + > + trans = btrfs_start_transaction(root, 1); > + btrfs_set_trans_block_group(trans, inode); > + > + /* first lets see if we already have this xattr */ let's :) > + di = btrfs_lookup_xattr(trans, root, path, inode->i_ino, name, > + strlen(name), -1); > + if (IS_ERR(di)) { > + ret = PTR_ERR(di); > + goto out; > + } > + > + /* ok we already have this xattr, lets remove it */ > + if (di) { > + /* if we want create only exit */ > + if (flags & XATTR_CREATE) { > + ret = -EEXIST; > + goto out; > + } > + > + ret = btrfs_delete_one_dir_name(trans, root, path, di); > + if (ret) > + goto out; > + btrfs_release_path(root, path); > + > + /* if we don't have a value then we are removing the xattr */ > + if (!value) { > + mod = 1; > + goto out; > + } > + } else { > + btrfs_release_path(root, path); > + > + if (flags & XATTR_REPLACE) { > + /* we couldn't find the attr to replace */ > + ret = -ENODATA; > + goto out; > + } > + } > + > + /* ok we have to create a completely new xattr */ > + ret = btrfs_insert_xattr_item(trans, root, name, strlen(name), > + value, size, inode->i_ino); > + if (ret) > + goto out; > + mod = 1; > + > +out: > + if (mod) { > + inode->i_ctime = CURRENT_TIME; > + ret = btrfs_update_inode(trans, root, inode); > + } > + > + btrfs_end_transaction(trans, root); > + btrfs_free_path(path); > + return ret; > +} > + > +ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size) > +{ > + struct btrfs_key key, found_key; > + struct inode *inode = dentry->d_inode; > + struct btrfs_root *root = BTRFS_I(inode)->root; > + struct btrfs_path *path; > + struct btrfs_item *item; > + struct extent_buffer *leaf; > + struct btrfs_dir_item *di; > + int ret = 0, slot, advance; > + size_t total_size = 0, size_left = size; > + unsigned long name_ptr; > + size_t name_len; > + u32 nritems; > + > + /* > + * ok we want all objects associated with this id. > + * NOTE: we set key.offset = 0; because we want to start with the > + * first xattr that we find and walk forward > + */ > + key.objectid = inode->i_ino; > + btrfs_set_key_type(&key, BTRFS_XATTR_ITEM_KEY); > + key.offset = 0; > + > + path = btrfs_alloc_path(); > + if (!path) > + return -ENOMEM; > + path->reada = 2; It's snowing towels in here! path->reada = 2; gack! int direction = path->reada; ... if (direction < 0) { if (nr == 0) break; nr--; } else if (direction > 0) { nr++; if (nr >= nritems) break; } > + /* search for our xattrs */ > + ret = btrfs_search_slot(NULL, root, &key, path, 0, 0); > + if (ret < 0) > + goto err; > + ret = 0; this statement isn't needed. > + advance = 0; > + while (1) { > + leaf = path->nodes[0]; > + nritems = btrfs_header_nritems(leaf); > + slot = path->slots[0]; > + > + /* this is where we start walking through the path */ > + if (advance || slot >= nritems) { > + /* > + * if we've reached the last slot in this leaf we need > + * to go to the next leaf and reset everything > + */ > + if (slot >= nritems-1) { > + ret = btrfs_next_leaf(root, path); > + if (ret) > + break; > + leaf = path->nodes[0]; > + nritems = btrfs_header_nritems(leaf); > + slot = path->slots[0]; > + } else { > + /* > + * just walking through the slots on this leaf > + */ > + slot++; > + path->slots[0]++; > + } > + } > + advance = 1; > + > + item = btrfs_item_nr(leaf, slot); > + btrfs_item_key_to_cpu(leaf, &found_key, slot); > + > + /* check to make sure this item is what we want */ > + if (found_key.objectid != key.objectid) > + break; > + if (btrfs_key_type(&found_key) != BTRFS_XATTR_ITEM_KEY) > + break; > + > + di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item); > + > + name_len = btrfs_dir_name_len(leaf, di); > + total_size += name_len + 1; > + > + /* we are just looking for how big our buffer needs to be */ > + if (!size) > + continue; > + > + if (!buffer || (name_len + 1) > size_left) { > + ret = -ERANGE; > + goto err; > + } > + > + name_ptr = (unsigned long)(di + 1); > + read_extent_buffer(leaf, buffer, name_ptr, name_len); > + buffer[name_len] = '\0'; > + > + size_left -= name_len + 1; > + buffer += name_len + 1; > + } > + ret = total_size; > + > +err: > + btrfs_free_path(path); > + > + return ret; > +} > > ... >