Re: [PATCH 01/35] Btrfs xattr code

[Chris Mason <chris.mason@oracle.com>]

On Mon, 2009-01-12 at 20:59 -0800, Andrew Morton wrote:
> On Wed,  7 Jan 2009 22:56:51 -0500 Chris Mason
> <chris.mason@oracle.com> wrote:
> 

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

I had this strange dream that google airlines was bombing my house with
towels....

ctree.h:
/* struct btrfs_dir_item */
BTRFS_SETGET_FUNCS(dir_data_len, struct btrfs_dir_item, data_len, 16);
BTRFS_SETGET_FUNCS(dir_type, struct btrfs_dir_item, type, 8);
BTRFS_SETGET_FUNCS(dir_name_len, struct btrfs_dir_item, name_len, 16);
BTRFS_SETGET_FUNCS(dir_transid, struct btrfs_dir_item, transid, 64);

The macros that define the SETGET_FUNCS are the ugliest part of btrfs.
They are defined and documented inside struct-funcs.c.  In general they
provide access to btree blocks that may be smaller/bigger than a single
page and hide the kmap fun.

Because the kmap fun can be a little slow, performance sensitive loops
can use map_extent_buffer to kmap one of the pages in the extent_buffer
and do their work.

> > +		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().

Will document *extent_buffer*

> 
> Please rename read_extent_buffer() to something more appropriate to a
> kernel-wide symbol.

The long term plan was for the extent_buffers to be a generic interface,
so I gave them generic names.  Before I can finish that off I need to
fix them to properly work with blocksize != pagesize.  They have
supported multi-page btree blocks, but I turned that off for a while to
finish off other things.

If you'd rather see them renamed to btrfs_, I'll do so.

> 
> <spies map_private_extent_buffer as well>
> 
> 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().

The data_ptr is an unsigned long offset into the btree block.  If you
think of read_extent_buffer as a fancy memcpy wrapper, data_ptr is the
offset in the buffer to read from.

The code in struct-funcs.c hides that most of the time, casting the
unsigned long into a strict type so that most operations look like they
are happening against a real type.

So for: ret = btrfs_dir_data_len(leaf, di);

leaf is the extent_buffer which holds the btree block in ram, and di is
an offset from zero into that buffer where you can find a directory item
struct.  It has been cast into a struct btrfs_dir_item, but it is some
number between 0 and 4096.

(It won't actually be zero since that is the start of the header and no
items are stored in the header).

> 
> > +	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);
> 
> <wonders what the -1 does>
> 
> <goes to the btrfs_lookup_xattr() definition site>
> 

I'll replace the -1 with BTRFS_SEARCH_ITEM_DEL, but as Josef said this
tells the searching code to balance for item deletion.  Nothing is
actually deleted unless btrfs_del_item is called, but the tree is
prepared for deletion/insertion from the top down, so the initial search
needs to know what the code intends to do.

> 
> > +	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;
> 
> <gets interested in btrfs_path.reada>
> 
> <greps for a while>
> 

> 	path->reada = 2;

I'll fix this to BTRFS_SEARCH_FORCE_RA

There are three types of metadata ra done.  Some funcs need to walk
backwards from a high key to a low key, so that's backwards ra (-1)

The default is to just read blocks that are close to the target block.
The code above is forcing ra because it knows it is going to be walking
forward in the key space.

-chris