[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <474DDBBB.5050506@namesys.com>
Date: Thu, 29 Nov 2007 00:20:59 +0300
From: Edward Shishkin <edward@...esys.com>
To: Christoph Hellwig <hch@....de>
CC: Andrew Morton <akpm@...ux-foundation.org>,
Reiserfs-list <reiserfs-devel@...r.kernel.org>,
bfields@...i.umich.edu, Neil Brown <neilb@...e.de>,
Linux kernel mailing list <linux-kernel@...r.kernel.org>,
Edward Shishkin <edward.shishkin@...il.com>
Subject: Re: [patch 2/2] reiser4: new export ops
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.
View attachment "reiser4-new-export-ops.patch" of type "text/x-patch" (7630 bytes)
Powered by blists - more mailing lists