[PATCH] XATTR issues on JFFS2
Kaigai Kohei
kaigai at ak.jp.nec.com
Fri Sep 9 00:15:54 EDT 2005
Hello,Jörn.
Thanks for reviewing in spite of so long patch.
> Sounds good.
>
> It appears as if your patch or something based on it will win. So
> let's have a look...
>
> I notice that you make a couple of mistakes by copying them from
> existing jffs2 code. You are excused for that, no doubt. But I still
> don't want copies of old mistakes to go in, just because we have a bad
> example.
Is this using uint32_t, xattr scanning code and so on ?
> Review is just partial. It's late enough already and I need to get
> some sleep.
Please take a rest slowly on the weekend.
I'll modify my patch during this period. ;-)
>>--- mtd-0829/fs/jffs2/fs.c 2005-08-06 18:00:16.000000000 -0400
>>+++ mtd-0829.xattr/fs/jffs2/fs.c 2005-09-06 10:28:53.000000000 -0400
>>@@ -217,6 +217,8 @@ void jffs2_clear_inode (struct inode *in
>>
>> D1(printk(KERN_DEBUG "jffs2_clear_inode(): ino #%lu mode %o\n", inode->i_ino, inode->i_mode));
>>
>>+ if (f->inocache && !f->inocache->nlink)
>>+ jffs2_xattr_clear_inode(c, f->inocache);
>
>
> Move the test into jffs2_xattr_clear_inode().
OK, I'll modify it.
>>@@ -504,6 +506,9 @@ int jffs2_do_fill_super(struct super_blo
>> }
>> memset(c->inocache_list, 0, INOCACHE_HASHSIZE * sizeof(struct jffs2_inode_cache *));
>>
>>+ if ((ret = jffs2_init_xattr_caches(c)))
>>+ goto out_inohash;
>>+
>
>
> ret = jffs2_init_xattr_caches(c);
> if (ret)
> goto out_inohash;
>
> I agree with gcc, when it warns about assignments in conditions. What
> I don't agree with is the proposal to use double brackets.
OK, I'll modify this and similar codes at different location if exist.
>> struct jffs2_full_dirent *jffs2_alloc_full_dirent(int namesize)
>>@@ -169,6 +195,7 @@ struct jffs2_raw_node_ref *jffs2_alloc_r
>> struct jffs2_raw_node_ref *ret;
>> ret = kmem_cache_alloc(raw_node_ref_slab, GFP_KERNEL);
>> JFFS2_DBG_MEMALLOC("%p\n", ret);
>>+ memset(ret, 0, sizeof(*ret)); /* to guarantee owner==NULL */
>
>
> This could be optimized a little by using the constructor/destructor
> stuff in slabs. Not too important.
I'll prepare a tiny and obvious constructor for jffs2_raw_node_ref.
NULL is set in raw->owner by default is important, the method of setting
is not so, I think.
>>@@ -205,3 +232,35 @@ void jffs2_free_inode_cache(struct jffs2
>> JFFS2_DBG_MEMALLOC("%p\n", x);
>> kmem_cache_free(inode_cache_slab, x);
>> }
>>+
>>+#ifdef CONFIG_JFFS2_XATTR
>>+
>>+struct jffs2_xattr_cache *jffs2_alloc_xattr_cache(void)
>>+{
>>+ struct jffs2_xattr_cache *xc;
>
> ^^^^^^
> The name doesn't make sense. You're referring to a single object, not
> the complete cache. Quite confusing.
>
>
>>+ xc = kmem_cache_alloc(xattr_cache_slab, GFP_KERNEL);
>
> ^^^^^
> This is the cache. A slab is just part of the cache. So please
> rename the two.
For example, how about the following names ?
"jffs2_xattr_datum" as an alternative of jffs2_xattr_cache.
"xattr_datum_cache" as an alternative of xattr_cache_slab.
In this case, "xc" defined as jffs2_xattr_cache structure
should be renamed to "xd" defined as jffs2_xattr_datum.
>>+void jffs2_free_xattr_cache(struct jffs2_xattr_cache *xc)
>>+{
>>+ JFFS2_DBG_MEMALLOC("%p\n", xc);
>>+ kmem_cache_free(xattr_cache_slab, xc);
>>+}
>>+
>>+struct jffs2_xattr_ref *jffs2_alloc_xattr_ref(void)
>>+{
>>+ struct jffs2_xattr_ref *ref;
>
>
> This name makes sense.
>
>
>>+ ref = kmem_cache_alloc(xattr_ref_slab, GFP_KERNEL);
>
>
> s/slab/cache/
OK, I'll rename it.
>>diff -rpNU3 mtd-0829/fs/jffs2/nodelist.h mtd-0829.xattr/fs/jffs2/nodelist.h
>>--- mtd-0829/fs/jffs2/nodelist.h 2005-08-17 18:00:12.000000000 -0400
>>+++ mtd-0829.xattr/fs/jffs2/nodelist.h 2005-09-06 10:28:53.000000000 -0400
>>@@ -20,6 +20,7 @@
>> #include <linux/jffs2.h>
>> #include <linux/jffs2_fs_sb.h>
>> #include <linux/jffs2_fs_i.h>
>>+#include <linux/jffs2_fs_x.h>
>
>
> I p a l n.
>
> I prefer a longer name. jffs2_fs_xattr.h would be fine.
> jffs2_xattr.h would also, the "_fs" is more redundant that "attr".
OK, I'll rename "jffs2_fs_x.h" to "jffs2_xattr.h" and deploy it under
"fs/jffs2" as David says.
>> #ifdef __ECOS
>> #include "os-ecos.h"
>>@@ -80,6 +81,7 @@ struct jffs2_raw_node_ref
>> struct jffs2_raw_node_ref *next_phys;
>> uint32_t flash_offset;
>> uint32_t __totlen; /* This may die; use ref_totlen(c, jeb, ) below */
>>+ void *owner;
>
>
> This grows a node reference from 16/24 bytes to 20/32 bytes for 32/64
> bit architectures. I'm not entirely happy about that, but don't a
> better solution either.
Indeed, it's involved the growing of object size.
But we must discriminate XATTR node from other non-inode node
and indicate jffs2_xattr_cache/jffs2_xattr_ref object from this.
I didn't have a good idea more than this.
And, I forgot to enclose the declaration of "void *owner"
by #ifdef - #endif.
>>diff -rpNU3 mtd-0829/fs/jffs2/scan.c mtd-0829.xattr/fs/jffs2/scan.c
>>--- mtd-0829/fs/jffs2/scan.c 2005-07-20 18:00:12.000000000 -0400
>>+++ mtd-0829.xattr/fs/jffs2/scan.c 2005-09-06 10:28:53.000000000 -0400
>>@@ -57,6 +57,12 @@ static int jffs2_scan_inode_node(struct
>> struct jffs2_raw_inode *ri, uint32_t ofs);
>> static int jffs2_scan_dirent_node(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb,
>> struct jffs2_raw_dirent *rd, uint32_t ofs);
>>+#ifdef CONFIG_JFFS2_XATTR
>>+static int jffs2_scan_xattr_node(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb,
>>+ struct jffs2_raw_xattr *rx, uint32_t ofs);
>>+static int jffs2_scan_xref_node(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb,
>>+ struct jffs2_raw_xref *rr, uint32_t ofs);
>>+#endif
>
>
> You don't need the function definitions if you change the order inside
> the file.
OK, I'll change the deployment of above functions.
>> #define BLK_STATE_ALLFF 0
>> #define BLK_STATE_CLEAN 1
>>@@ -552,7 +558,40 @@ scan_more:
>> if (err) return err;
>> ofs += PAD(je32_to_cpu(node->totlen));
>> break;
>>-
>>+#ifdef CONFIG_JFFS2_XATTR
>>+ case JFFS2_NODETYPE_XATTR:
>>+ if (buf_ofs + buf_len < ofs + je32_to_cpu(node->totlen)) {
>>+ buf_len = min_t(uint32_t, buf_size, jeb->offset + c->sector_size - ofs);
>>+ D1(printk(KERN_DEBUG "Fewer than %d bytes (xattr node) left to end of buf."
>>+ " Reading 0x%x at 0x%08x\n",je32_to_cpu(node->totlen), buf_len, ofs));
>>+ err = jffs2_fill_scan_buf(c, buf, ofs, buf_len);
>>+ if (err)
>>+ return err;
>>+ buf_ofs = ofs;
>>+ node = (void *)buf;
>>+ }
>>+ err = jffs2_scan_xattr_node(c, jeb, (void *)node, ofs);
>>+ if (err)
>>+ return err;
>>+ ofs += PAD(je32_to_cpu(node->totlen));
>>+ break;
>>+ case JFFS2_NODETYPE_XREF:
>>+ if (buf_ofs + buf_len < ofs + je32_to_cpu(node->totlen)) {
>>+ buf_len = min_t(uint32_t, buf_size, jeb->offset + c->sector_size - ofs);
>>+ D1(printk(KERN_DEBUG "Fewer than %d bytes (xref node) left to end of buf."
>>+ " Reading 0x%x at 0x%08x\n",je32_to_cpu(node->totlen), buf_len, ofs));
>>+ err = jffs2_fill_scan_buf(c, buf, ofs, buf_len);
>>+ if (err)
>>+ return err;
>>+ buf_ofs = ofs;
>>+ node = (void *)buf;
>>+ }
>>+ err = jffs2_scan_xref_node(c, jeb, (void *)node, ofs);
>>+ if (err)
>>+ return err;
>>+ ofs += PAD(je32_to_cpu(node->totlen));
>>+ break;
>>+#endif
>
>
> By itself, this code is not too bad. But it add a little more crap to
> a function that is aching for a rewrite already. Not sure.
Hmm,,, I wrote the above part of codes with referring to existing code.
If the policy is shown, I'll follow it. How should I do ?
>>@@ -820,6 +860,190 @@ static int jffs2_scan_dirent_node(struct
>> return 0;
>> }
>>
>>+#ifdef CONFIG_JFFS2_XATTR
>>+static int jffs2_scan_xattr_node(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb,
>>+ struct jffs2_raw_xattr *rx, uint32_t ofs)
>>+{
>>+ struct jffs2_raw_node_ref *raw;
>>+ struct jffs2_xattr_cache *xc;
>>+ uint32_t crc;
>>+ int xtype, nlen, vlen;
>>+
>>+ crc = crc32(0, rx, sizeof(*rx)-8);
>>+ if (crc != je32_to_cpu(rx->node_crc)) {
>>+ printk(KERN_NOTICE "jffs2_scan_xattr_node(): Node CRC failed"
>>+ " on node at 0x%08x: Read 0x%08x, calculated 0x%08x\n",
>>+ ofs, je32_to_cpu(rx->node_crc), crc);
>>+ DIRTY_SPACE(PAD(je32_to_cpu(rx->totlen)));
>>+ return 0;
>>+ }
>>+
>>+ nlen = JFFS2_XATTR_NAMELEN(je32_to_cpu(rx->length));
>>+ vlen = JFFS2_XATTR_VALUELEN(je32_to_cpu(rx->length));
>
>
> Types don't match. nlen and vlen should become u32.
Is it enought for "int"? JFFS2_XATTR_NAMELEN has 12-bit length, and
JFFS2_XATTR_VALUELEN has 20-bit length.
> Btw, I agree with Linus. u32 is _much_ nicer than uint32_t, not to
> speak of the uint32_fast_t insanity.
Is is necessary to correct the part where other uint32_t used.
>>+ if (je32_to_cpu(rx->totlen) != sizeof(*rx)+PAD(nlen+1)+PAD(vlen)) {
>>+ printk(KERN_NOTICE "jffs2_scan_xattr_node(): length mimatch!"
>>+ " totlen=%d length=0x%08x nlen=%d vlen=%d on 0x%08x\n",
>>+ je32_to_cpu(rx->totlen), je32_to_cpu(rx->length), nlen, vlen, ofs);
>>+ DIRTY_SPACE(PAD(je32_to_cpu(rx->totlen)));
>>+ return 0;
>>+ }
>>+
>>+ crc = crc32(0, rx->data, PAD(nlen+1)+PAD(vlen));
>>+ if (crc != je32_to_cpu(rx->data_crc)) {
>>+ printk(KERN_NOTICE "jffs2_scan_xattr_node(): Data CRC failed"
>>+ " on node at 0x%08x: Read 0x%08x, calculated 0x%08x\n",
>>+ ofs, je32_to_cpu(rx->data_crc), crc);
>>+ DIRTY_SPACE(PAD(je32_to_cpu(rx->totlen)));
>>+ return 0;
>>+ }
>>+
>>+ xc = jffs2_find_xattr_cache_xid(c, je32_to_cpu(rx->xid));
>>+ if (xc) {
>>+ printk(KERN_NOTICE "jffs2_scan_xattr_node(): Duplicate XID Found"
>>+ " on node at 0x%08x XID=%d Later one ignored\n", ofs, xc->xid);
>>+ DIRTY_SPACE(PAD(je32_to_cpu(rx->totlen)));
>>+ return 0;
>>+ }
>
>
> How do you deal with xattr reference counting and lifetime?
When xc->xlist is empty, jffs2_xattr_cache type object will be released.
jffs2_xattr_ref type object is chained to xc->xlist.
list_empty(xc->xlist) means nobady refers this XATTR.
>>+ xtype = jffs2_xattr_prefix_to_xtype(rx->data, nlen);
>>+ if (JFFS2_XATTRTYPE_INVALID==xtype) {
>>+ printk(KERN_NOTICE "jffs2_scan_xattr_node(): Unsupported xattr type"
>>+ " on node at 0x%08x\n", ofs);
>>+ USED_SPACE(PAD(je32_to_cpu(rx->totlen)));
>>+ return 0;
>>+ }
>
>
> Why is it required that JFFS2 knows the type of the xattr?
Hmm,,, I added this member to discriminate the prefix of xattr
without strncmp(). But it might be a bit ugly design.
>>+static inline int jffs2_scan_xref_node_helper(struct jffs2_sb_info *c,
>>+ struct jffs2_xattr_ref *new, struct jffs2_xattr_ref *old)
>>+{
>>+ /* (uint32_t)new->xc == (uint32_t)old->xc has been made sure. */
>>+ if ((uint32_t)new->ic == (uint32_t)old->ic) {
>
>
> Why the casts? How can this be safe on 64bit machines?
This function is called before jffs2_build_xattr_cache() while FS-mounting.
In this implementation, xid (uint32 value) is stored in jffs2_xattr_ref->xc
before jffs2_build_xattr_cache(), and ino (uint32 value) is stored in
jffs2_xattr_ref->ic.
After calling jffs2_build_xattr_cache(), xc and ic is used as a pointer
which indicate to jffs2_xattr_cache/jffs2_inode_cache.
It might be bad manner, but is the following implementation preferable ?
struct jffs2_xattr_ref {
uint32_t seqno;
struct list_head ilist;
struct list_head xlist;
union {
struct jffs2_xattr_cache *xc;
uint32_t xid;
} x;
union {
struct jffs2_inode_cache *ic;
uint32_t ino;
} i;
struct jffs2_raw_node_ref *node;
};
>>+static int jffs2_scan_xref_node(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb,
>>+ struct jffs2_raw_xref *rr, uint32_t ofs)
>>+{
>>+ struct jffs2_raw_node_ref *raw;
- snip -
>>+ ref->seqno = je32_to_cpu(rr->seqno);
>>+ if (ref->seqno > c->highest_seqno)
>>+ c->highest_seqno = ref->seqno;
>>+ ref->ic = (void *)je32_to_cpu(rr->ino);
>>+ ref->xc = (void *)je32_to_cpu(rr->xid);
>
>
> WTF?!?
>
> You read a je32 from flash and use that as a pointer?!?
>
> Please tell me I'm wrong.
If we use above emample, we can write it as follows:
ref->i.ino = je32_to_cpu(rr->ino);
ref->x.xid = je32_to_cpu(rr->xid);
In jffs2_build_xattr_cache(), we find up the jffs2_inode_cache and
jffs2_xattr_cache by using ino/xid as a key. Then the correct pointers
are stored in ref->ic and ref->xc after jffs2_build_xattr_cache().
Is it the situation to use union ?
Thanks for attentive comments.
--
Linux Promotion Center, NEC
KaiGai Kohei <kaigai at ak.jp.nec.com>
More information about the linux-mtd
mailing list