Christoph Hellwig wrote:

>This code looks a little confusing to me..
>
>  
>
>>  */
>> static char *decode_inode(struct super_block *s, char *addr,
>> 			  reiser4_object_on_wire * obj)
>>@@ -41,7 +42,8 @@
>> 	fplug = file_plugin_by_disk_id(reiser4_get_tree(s), (d16 *) addr);
>> 	if (fplug != NULL) {
>> 		addr += sizeof(d16);
>>-		obj->plugin = fplug;
>>+		if (obj)
>>+			obj->plugin = fplug;
>>    
>>
>
>You are adding quite a few of those if (obj) clauses.  I can't see a
>reason for that - care to explain?  The new aops
>

new _export_ ops

> should not disallow for
>any functionality that has been there before.
>
>  
>

Actually they don't disallow existing functionality, but
require a new one: earlier we performed decoding in
_each_ iteration (decode_fh), and now you want it to be
done separately (fh_to_dentry, fh_to_parent). So we need
something like "empty" iteration, which only moves to the
next position.

Every object in reiser4, that can be serialized, has
special non-zero "wire" methods. I guess that "empty"
iteration should be performed via wire->read() method
which is responsible for extracting on-wire object.
Can not promise to avoid "if" here: I think it is not
a good reason to add a new plugin method for such
empty skip.

The attached patch avoids other two ifs.

>>static struct dentry *reiser4_decode_fh(struct super_block *super, __u32 *fh,
>>+					int len, int fhtype, int parent)
>> {
>> 	reiser4_context *ctx;
>> 	reiser4_object_on_wire object;
>> 	char *addr;
>> 
>> 	ctx = reiser4_init_context(super);
>> 	if (IS_ERR(ctx))
>>@@ -80,25 +77,19 @@
>> 	assert("vs-1482",
>> 	       fhtype == FH_WITH_PARENT || fhtype == FH_WITHOUT_PARENT);
>> 
>>	addr = (char *)fh;
>>
>>	object_on_wire_init(&object);
>>+
>>+	if (parent)
>>+		/* skip first onwire object */
>>+		addr = decode_inode(super, addr, NULL);
>>	if (!IS_ERR(addr)) {
>>+		addr = decode_inode(super, addr, &object);
>>		if (!IS_ERR(addr)) {
>>			struct dentry *d;
>>
>>+			d = reiser4_get_dentry(super, &object);
>>    
>>
>
>I'd suggest to directly poke into the place where the parent handle
>is stored.  XFS used a similar construct to the decode_inode helper,
>but with the new aops it's faster and easier to read if you just have
>a helper on how many bytes to skip.  Did you take a look at how the
>various other filesystem handle the export ops?
>
>  
>

We don't have a number of u32s to poke, like other filesystems do;
packing/extracting serialized objects in reiser4 is more complex:
first extract object's plugin, which knows how to extract further, etc..

>>--- linux-2.6.23-mm1/fs/reiser4/dscale.c.orig
>>+++ linux-2.6.23-mm1/fs/reiser4/dscale.c
>>@@ -126,6 +126,24 @@
>>    
>>
>
>How are the changes to all these other files related to the export
>operations changes?
>
>  
>
So we have to define an "if" branch for wire_read_common()
(plugin/file_plugin_common.c). The best place to do it is kassign.c
(as we actually pack/extract key components). Plus a small change in
dscale.c where a packing taxonomy is implemented. Have I missed
something?

Thanks,
Edward.