XATTR issues on JFFS2

Hello,

In recent days, I'm interrested in XATTR support in JFFS2.

I have started a discussion about it on MTD-ML.
http://lists.infradead.org/pipermail/linux-mtd/2005-August/013415.html

By ML-archive, an implementation use hidden-file to store XATTR was rejected finally. Thus, I adopted different approach. This implementation added two new node types JFFS2_NODETYPE_XATTR and JFFS2_NODETYPE_XREF for XATTR representation.

If your comment can be gotten, I'm so glad.

# I think it's not appropriate to discuss JFFS2 at SELinux-ML. # Please comment it on MTD-ML, if possible.

Thanks,

*---- Example of use ------------

In my desktop PC, "fs_use_xattr jffs2 system_u:object_r:fs_t;" was configured. :)

[root@saba ~]# dd if=jffs2.xattr.img of=/dev/mtdblock0 8192+0 records in
8192+0 records out
[root@saba ~]# mount -t jffs2 /dev/mtdblock0 /mnt/0 [root@saba ~]# ls -lZ /mnt/0

drwxr-xr-x  root     root     system_u:object_r:bin_t          bin/
drwxr-xr-x  root     root     system_u:object_r:mnt_t          dev/
drwxr-xr-x  root     root     system_u:object_r:etc_t          etc/

(quoted diff) +0 -2

-rw-r--r--  root     root     system_u:object_r:mnt_t          hoge
-rwxr-xr-x  root     root     system_u:object_r:mnt_t          init*

drwxr-xr-x  root     root     system_u:object_r:lib_t          lib/
drwxr-xr-x  root     root     system_u:object_r:mnt_t          loopfs/
drwxr-xr-x  root     root     system_u:object_r:mnt_t          proc/
lrwxrwxrwx  root     root     system_u:object_r:bin_t          sbin
drwxr-xr-x  root     root     system_u:object_r:mnt_t          sys/
drwxr-xr-x  root     root     system_u:object_r:mnt_t          sysroot/

[root@saba ~]#

Thanks for working on this functionality. From a very brief look at the patch, one question I had was whether you could be using the generic xattr methods introduced by James Morris' earlier xattr code consolidation patches that are now in the mainline kernel. See the generic_* functions in fs/xattr.c and how they are used by the fs code (e.g. look at the ext3 xattr implementation). Per recent discussions on linux-fsdevel, using those helpers is the preferred approach for the future. You can then just set sb->s_xattr for the specific handlers in jffs2, e.g. see the ext3 code for an example.

Might be worth mentioning this work on acl-devel as well, although you don't yet support ACLs or other xattrs. But I'm sure Andreas Gruenbacher would be interested, as he did the original xattr and ACL development work for ext[23], and he may be able to give feedback on your approach to various aspects like the attribute caching.

The original reason not to use generic XATTR methods is that {get|set|list}_xattr methods are called with XATTR-name which prefix is omitted.
e.g, sys_getxattr("security.selinux", ...)

      -> generic_getxattr("security.selinux", ...)
        -> jffs2_security_getxattr("selinux", ...)
                                    ~~~~~~~<- "security." was omitted.

In my implementation, the full-name is used to identify XATTR-entry.

But I noticed that we can use a combination of integer index number (such as EXT2_XATTR_INDEX_SECURITY) and omitted XATTR-name for the replacement of full-XATTR-name, and can use generic_xxxx().

The attached patch provids XATTR support in JFFS2. We can apply it to mtd-snapshot-20050822.tar.bz2 tree.

I hope your worthwhile comments for improvement.
# By coincidence, two patches for similar functionality are posted in same day :)

Which is quite nice. Would you two mind reading the others patch and stating what is bad at that one and why your's is better? If there is a clear winner in this batter, we should merge that one.

I'm happier with inventing new node types for xattrs instead of re-using existing nodes, I think. And I'd also like to make sure we can share xattr data on the medium where appropriate. In an selinux system, you'll end up with a _multitude_ of xattrs used for selinux, many of which will have precisely the same contents. Each should be stored only once on the flash, like hard links. The ext2 implementation manages this.

* Dividing the global xattr_sem semaphore into more small lock.

Today, I think about appropriate locking in JFFS2's XATTR. I hope to get some comments about dividing locking.

Quoted 1 line

* Dividing the global xattr_sem semaphore into more small lock.

xattr_sem is a global read/write semaphore stored in jffs2_sb_info to protect any XATTR structure.
In common case, dividing lock/semaphore has good effects for scalability toward the number of CPUs. But JFFS2 is frequently used for small and embedded region in my understanding.

If global semaphore is divided into many per-entry locks, we may be able to execute setxattr() concurrently. But each locking/un-locking cost may also exceed the scalability effect.
Since I don't have much experience in this region, I hope any professional's worthwhile comments.

Now, I think one global read/write semaphore is enough for 1-2 CPUs, if assumption of the XATTR processing with wrinting does not happen so much is permitted. In almost case, we can execute get/listxattr cuncurrently with one lock/un-locking, I think.

This attached patch enables XATTR support in JFFS2. (take-2) # The contribution has slowed due to the trouble related to GC :-(

The my first patch posted in Aug 23 is revised at the points as follows.

- get/setxattr are implemented as a method of generic XATTR handler.
- new methods for "user.*" and "trusted.*" prefix were added.

- Time to retrieve the duplicate XATTR-cache shortened, when setxattr(). In first implementation, we must scan all XATTR-cache to find the duplicate
one, and it's so expensive. Now, we cau use the hash-list of XATTR-cache which is indexed by own contents.

- Now, Garbage Collector can deal JFFS2_NODETYPE_XATTR and JFFS2_NODETYPE_XREF.
It seems to me that GC can mark unconditionally any nodes without inode as obsolete. But new JFFS2_NODETYPE_XATTR/XREF should be an exception. GC must not mark them as obsolete, although their "next_in_ino" is NULL.

----[ jffs2_garbage_collect_pass() in gc.c ]------------------------
249 if (!raw->next_in_ino) { <-- unconditionally mark it as
obsolete.
250 /* Inode-less node. Clean marker, snapshot or something like
that */
251 /* FIXME: If it's something that needs to be copied, including
something
252 we don't grok that has JFFS2_NODETYPE_RWCOMPAT_COPY, we
should do so */

  253      spin_unlock(&c->erase_completion_lock);
  254      jffs2_mark_node_obsolete(c, raw);
  255      up(&c->alloc_sem);
  256      goto eraseit_lock;

257 }
--------------------------------------------------------------------

I added a new variable 'owner' into jffs2_raw_node_ref to resolve this. It refers jffs2_xattr_cache or jffs2_xattr_ref object when raw-node reflect
XATTR-node. Thus, we can distinguish XATTR-node and others by whether 'owner'
is NULL or not.

- Consistency has improved.
check_xattr_ref_ilist() checks XATTRs which are binded to same inode. If multiple XATTR has same name exist, older one is marked as obsolete. In first implementation, there are possibilities to be created multiple XATTRs have same name in an inode, when system-crash happens durling setxattr()

diff -rpNU3 mtd-0829/fs/Kconfig mtd-0829.xattr/fs/Kconfig

fs/Kconfig +16 -0

@@ -64,6 +64,22 @@ config JFFS2_FS_WRITEBUFFER
 	    - NOR flash with transparent ECC
 	    - DataFlash
 
+config JFFS2_XATTR
+	bool "JFFS2 XATTR support (EXPERIMENTAL)"
+	depends on JFFS2_FS
+	default n
+	help
+	  This enables the XATTR support in JFFS2.
+	  
+	  It's neccesary for persistent security context in SELinux.
+	  Now, we can use the following kinds of prefix.
+	  
+	  - security.*
+	  - user.*
+	  - trust.*
+	  
+	  - system.posix_acl_{access|default} are in TODO list now.
+
 config JFFS2_COMPRESSION_OPTIONS
 	bool "Advanced compression options for JFFS2"
 	default n

diff -rpNU3 mtd-0829/fs/jffs2/Makefile.common mtd-0829.xattr/fs/jffs2/Makefile.common

fs/jffs2/Makefile.common +1 -0

@@ -12,6 +12,7 @@ jffs2-y	+= symlink.o build.o erase.o bac
 jffs2-y	+= super.o debug.o
 
 jffs2-$(CONFIG_JFFS2_FS_WRITEBUFFER)	+= wbuf.o
+jffs2-$(CONFIG_JFFS2_XATTR)	+= xattr.o
 jffs2-$(CONFIG_JFFS2_RUBIN)	+= compr_rubin.o
 jffs2-$(CONFIG_JFFS2_RTIME)	+= compr_rtime.o
 jffs2-$(CONFIG_JFFS2_ZLIB)	+= compr_zlib.o

diff -rpNU3 mtd-0829/fs/jffs2/build.c mtd-0829.xattr/fs/jffs2/build.c

fs/jffs2/build.c +2 -0

@@ -165,6 +165,7 @@ static int jffs2_build_filesystem(struct
 		ic->scan_dents = NULL;
 		cond_resched();
 	}
+	jffs2_build_xattr_caches(c);
 	c->flags &= ~JFFS2_SB_FLAG_BUILDING;
 
 	D1(printk(KERN_DEBUG "Pass 3 complete\n"));

@@ -184,6 +185,7 @@ exit:
 				jffs2_free_full_dirent(fd);
 			}
 		}
+		jffs2_clear_xattr_caches(c);
 	}
 
 	return ret;

diff -rpNU3 mtd-0829/fs/jffs2/dir.c mtd-0829.xattr/fs/jffs2/dir.c

fs/jffs2/dir.c +5 -0

@@ -57,7 +57,12 @@ struct inode_operations jffs2_dir_inode_
 	.rmdir =	jffs2_rmdir,
 	.mknod =	jffs2_mknod,
 	.rename =	jffs2_rename,
+	.permission =	jffs2_permission,
 	.setattr =	jffs2_setattr,
+	.setxattr =	jffs2_setxattr,
+	.getxattr =	jffs2_getxattr,
+	.listxattr =	jffs2_listxattr,
+	.removexattr =	jffs2_removexattr
 };
 
 /***********************************************************************/

diff -rpNU3 mtd-0829/fs/jffs2/file.c mtd-0829.xattr/fs/jffs2/file.c

fs/jffs2/file.c +6 -1

@@ -57,7 +57,12 @@ struct file_operations jffs2_file_operat
 
 struct inode_operations jffs2_file_inode_operations =
 {
-	.setattr =	jffs2_setattr
+	.permission =	jffs2_permission,
+	.setattr =	jffs2_setattr,
+	.setxattr =	jffs2_setxattr,
+	.getxattr =	jffs2_getxattr,
+	.listxattr =	jffs2_listxattr,
+	.removexattr =	jffs2_removexattr
 };
 
 struct address_space_operations jffs2_file_address_operations =

diff -rpNU3 mtd-0829/fs/jffs2/fs.c mtd-0829.xattr/fs/jffs2/fs.c

fs/jffs2/fs.c +1 -0

 	if ((ret = jffs2_do_mount_fs(c)))
 		goto out_inohash;
 

@@ -537,6 +542,7 @@ int jffs2_do_fill_super(struct super_blo
 		vfree(c->blocks);
 	else
 		kfree(c->blocks);
+	jffs2_clear_xattr_caches(c);
  out_inohash:
 	kfree(c->inocache_list);
  out_wbuf:

diff -rpNU3 mtd-0829/fs/jffs2/gc.c mtd-0829.xattr/fs/jffs2/gc.c

fs/jffs2/gc.c +6 -1

@@ -251,7 +251,12 @@ int jffs2_garbage_collect_pass(struct jf
 		/* FIXME: If it's something that needs to be copied, including something
 		   we don't grok that has JFFS2_NODETYPE_RWCOMPAT_COPY, we should do so */
 		spin_unlock(&c->erase_completion_lock);
-		jffs2_mark_node_obsolete(c, raw);
+		if (raw->owner) {
+			/* JFFS2_NODETYPE_XATTR or JFFS2_NODETYPE_XREF */
+			ret = jffs2_garbage_collect_xattr(c, raw);
+		} else {
+			jffs2_mark_node_obsolete(c, raw);
+		}
 		up(&c->alloc_sem);
 		goto eraseit_lock;
 	}

diff -rpNU3 mtd-0829/fs/jffs2/malloc.c mtd-0829.xattr/fs/jffs2/malloc.c

fs/jffs2/malloc.c +28 -2

@@ -26,6 +26,10 @@ static kmem_cache_t *tmp_dnode_info_slab
 static kmem_cache_t *raw_node_ref_slab;
 static kmem_cache_t *node_frag_slab;
 static kmem_cache_t *inode_cache_slab;
+#ifdef CONFIG_JFFS2_XATTR
+static kmem_cache_t *xattr_cache_slab;
+static kmem_cache_t *xattr_ref_slab;
+#endif
 
 int __init jffs2_create_slab_caches(void)
 {

@@ -68,8 +72,24 @@ int __init jffs2_create_slab_caches(void
 	inode_cache_slab = kmem_cache_create("jffs2_inode_cache",
 					     sizeof(struct jffs2_inode_cache),
 					     0, 0, NULL, NULL);
-	if (inode_cache_slab)
-		return 0;
+	if (!inode_cache_slab)
+		goto err;
+
+#ifdef CONFIG_JFFS2_XATTR
+	xattr_cache_slab = kmem_cache_create("jffs2_xattr_cache",
+					     sizeof(struct jffs2_xattr_cache),
+					     0, 0, NULL, NULL);
+	if (!xattr_cache_slab)
+		goto err;
+
+	xattr_ref_slab = kmem_cache_create("jffs2_xattr_ref",
+					   sizeof(struct jffs2_xattr_ref),
+					   0, 0, NULL, NULL);
+	if (!xattr_ref_slab)
+		goto err;
+#endif
+
+	return 0;
  err:
 	jffs2_destroy_slab_caches();
 	return -ENOMEM;

@@ -91,6 +111,12 @@ void jffs2_destroy_slab_caches(void)
 		kmem_cache_destroy(node_frag_slab);
 	if(inode_cache_slab)
 		kmem_cache_destroy(inode_cache_slab);
+#ifdef CONFIG_JFFS2_XATTR
+	if(xattr_cache_slab)
+		kmem_cache_destroy(xattr_cache_slab);
+	if(xattr_ref_slab)
+		kmem_cache_destroy(xattr_ref_slab);
+#endif
 }
 
 struct jffs2_full_dirent *jffs2_alloc_full_dirent(int namesize)

fs/jffs2/malloc.c +11 -0

+	JFFS2_DBG_MEMALLOC("%p\n", ref);
+	return ref;
+}
+
+void jffs2_free_xattr_ref(struct jffs2_xattr_ref *ref)
+{
+	JFFS2_DBG_MEMALLOC("%p\n", ref);
+	kmem_cache_free(xattr_ref_slab, ref);
+}
+
+#endif

diff -rpNU3 mtd-0829/fs/jffs2/nodelist.c mtd-0829.xattr/fs/jffs2/nodelist.c

fs/jffs2/nodelist.c +1 -0

@@ -934,6 +934,7 @@ void jffs2_free_ino_caches(struct jffs2_
 		this = c->inocache_list[i];
 		while (this) {
 			next = this->next;
+			jffs2_xattr_free_inode(c, this);
 			jffs2_free_inode_cache(this);
 			this = next;
 		}

diff -rpNU3 mtd-0829/fs/jffs2/nodelist.h mtd-0829.xattr/fs/jffs2/nodelist.h

fs/jffs2/nodelist.h +11 -0

 };
 
         /* flash_offset & 3 always has to be zero, because nodes are

@@ -111,6 +113,10 @@ struct jffs2_inode_cache {
 	uint32_t ino;
 	int nlink;
 	int state;
+#ifdef CONFIG_JFFS2_XATTR
+	struct list_head ilist;
+	uint8_t ilist_checked;
+#endif
 };
 
 /* Inode states for 'state' above. We need the 'GC' state to prevent

@@ -367,6 +373,13 @@ void jffs2_free_node_frag(struct jffs2_n
 struct jffs2_inode_cache *jffs2_alloc_inode_cache(void);
 void jffs2_free_inode_cache(struct jffs2_inode_cache *);
 
+#ifdef CONFIG_JFFS2_XATTR
+struct jffs2_xattr_cache *jffs2_alloc_xattr_cache(void);
+void jffs2_free_xattr_cache(struct jffs2_xattr_cache *);
+struct jffs2_xattr_ref *jffs2_alloc_xattr_ref(void);
+void jffs2_free_xattr_ref(struct jffs2_xattr_ref *);
+#endif /* CONFIG_JFFS2_XATTR */
+
 /* gc.c */
 int jffs2_garbage_collect_pass(struct jffs2_sb_info *c);
 

diff -rpNU3 mtd-0829/fs/jffs2/readinode.c mtd-0829.xattr/fs/jffs2/readinode.c

fs/jffs2/readinode.c +1 -0

@@ -902,6 +902,7 @@ int jffs2_do_read_inode(struct jffs2_sb_
 		f->inocache->ino = f->inocache->nlink = 1;
 		f->inocache->nodes = (struct jffs2_raw_node_ref *)f->inocache;
 		f->inocache->state = INO_STATE_READING;
+		initxattr_jffs2_inode_cache(f->inocache);
 		jffs2_add_ino_cache(c, f->inocache);
 	}
 	if (!f->inocache) {

diff -rpNU3 mtd-0829/fs/jffs2/scan.c mtd-0829.xattr/fs/jffs2/scan.c

fs/jffs2/scan.c +24 -1

 #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)

fs/jffs2/scan.c +11 -0

 		case JFFS2_NODETYPE_CLEANMARKER:
 			D1(printk(KERN_DEBUG "CLEANMARKER node found at 0x%08x\n", ofs));
 			if (je32_to_cpu(node->totlen) != c->cleanmarker_size) {

@@ -665,6 +704,7 @@ static struct jffs2_inode_cache *jffs2_s
 
 	ic->ino = ino;
 	ic->nodes = (void *)ic;
+	initxattr_jffs2_inode_cache(ic);
 	jffs2_add_ino_cache(c, ic);
 	if (ino == 1)
 		ic->nlink = 1;

@@ -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);

fs/jffs2/scan.c +14 -0

+	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)));

fs/jffs2/scan.c +33 -0

+
+	raw = jffs2_alloc_raw_node_ref();
+	if (!raw)
+		return -ENOMEM;
+
+	xc = jffs2_alloc_xattr_cache();
+	if (!xc) {
+		jffs2_free_raw_node_ref(raw);
+		return -ENOMEM;
+	}
+	initxattr_jffs2_xattr_cache(xc);
+
+	xc->xid = je32_to_cpu(rx->xid);
+	if (xc->xid > c->highest_xid)
+		c->highest_xid = xc->xid;
+	xc->xtype = xtype;
+	xc->node = raw;
+	xc->name_len = nlen;
+	xc->value_len = vlen;
+	xc->data_crc = crc;
+
+	raw->__totlen = PAD(je32_to_cpu(rx->totlen));
+	raw->flash_offset = ofs | REF_PRISTINE;
+	raw->next_phys = NULL;
+	raw->next_in_ino = NULL;
+	if (!jeb->first_node)
+		jeb->first_node = raw;
+	if (jeb->last_node)
+		jeb->last_node->next_phys = raw;
+	jeb->last_node = raw;
+	USED_SPACE(PAD(je32_to_cpu(rx->totlen)));
+
+	jffs2_attach_xattr_cache(c, xc);

fs/jffs2/scan.c +34 -0

+		if (new->seqno > old->seqno) {
+			jffs2_mark_node_obsolete(c, old->node);
+			old->seqno = new->seqno;
+			old->node = new->node;
+		} else {
+			jffs2_mark_node_obsolete(c, new->node);
+		}
+		return 1; /* 'new' should be released. */
+	}
+	return 0;
+}
+
+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;
+	struct jffs2_xattr_ref *ref, *pos, *pos2;
+	uint32_t crc;
+
+	crc = crc32(0, rr, sizeof(*rr)-4);
+	if (crc != je32_to_cpu(rr->node_crc)) {
+		printk(KERN_NOTICE "jffs2_scan_xref_node(): Node CRC failed"
+		       " on node at 0x%08x: Read 0x%08x, calculated 0x%08x\n",
+		       ofs, je32_to_cpu(rr->node_crc), crc);
+		DIRTY_SPACE(PAD(je32_to_cpu(rr->totlen)));
+		return 0;
+	}
+
+	ref = jffs2_alloc_xattr_ref();
+	if (!ref)
+		return -ENOMEM;
+	initxattr_jffs2_xattr_ref(ref);
+
+	raw = jffs2_alloc_raw_node_ref();

I prefer a longer name. jffs2_fs_xattr.h would be fine. jffs2_xattr.h would also, the "_fs" is more redundant that "attr".

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.

Review is just partial. It's late enough already and I need to get some sleep.

fs/jffs2/fs.c void jffs2_clear_inode (struct inode *in · line 221

Quoted 8 lines

fs/jffs2/fs.c +2 -0

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

fs/jffs2/fs.c int jffs2_do_fill_super(struct super_blo · line 511

Quoted 7 lines

fs/jffs2/fs.c +3 -0

@@ -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.

fs/jffs2/fs.c struct jffs2_raw_node_ref *jffs2_alloc_r · line 198

Quoted 6 lines

fs/jffs2/fs.c +1 -0

 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.

fs/jffs2/fs.c void jffs2_free_inode_cache(struct jffs2 · line 240

Quoted 10 lines

fs/jffs2/fs.c +6 -0

@@ -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.

fs/jffs2/fs.c void jffs2_free_inode_cache(struct jffs2 · line 241

Quoted 1 line

fs/jffs2/fs.c +1 -0

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

fs/jffs2/fs.c void jffs2_free_inode_cache(struct jffs2 · line 250

Quoted 9 lines

fs/jffs2/fs.c +9 -0

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

fs/jffs2/fs.c void jffs2_free_inode_cache(struct jffs2 · line 251

Quoted 1 line

fs/jffs2/fs.c +1 -0

+	ref = kmem_cache_alloc(xattr_ref_slab, GFP_KERNEL);

s/slab/cache/

fs/jffs2/nodelist.h line 23

Quoted 8 lines

diff -rpNU3 mtd-0829/fs/jffs2/nodelist.h mtd-0829.xattr/fs/jffs2/nodelist.h

fs/jffs2/nodelist.h +1 -0

@@ -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".

Quoted 4 lines

fs/jffs2/nodelist.h +0 -0

 #ifdef __ECOS
 #include "os-ecos.h"

@@ -80,6 +81,7 @@ struct jffs2_raw_node_ref
 	struct jffs2_raw_node_ref *next_phys;

fs/jffs2/nodelist.h struct jffs2_raw_node_ref · line 84

Quoted 3 lines

fs/jffs2/nodelist.h +1 -0

 	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.

Quoted 9 lines

diff -rpNU3 mtd-0829/fs/jffs2/scan.c mtd-0829.xattr/fs/jffs2/scan.c

fs/jffs2/scan.c +2 -0

@@ -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,

fs/jffs2/scan.c static int jffs2_scan_inode_node(struct · line 65

Quoted 4 lines

fs/jffs2/scan.c +4 -0

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

Quoted 37 lines

fs/jffs2/scan.c +30 -1

 #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)

fs/jffs2/scan.c scan_more: · line 594

Quoted 4 lines

fs/jffs2/scan.c +4 -0

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

Quoted 19 lines

fs/jffs2/scan.c +15 -0

@@ -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)));

fs/jffs2/scan.c static int jffs2_scan_dirent_node(struct · line 882

Quoted 5 lines

fs/jffs2/scan.c +5 -0

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

Btw, I agree with Linus. u32 is _much_ nicer than uint32_t, not to speak of the uint32_fast_t insanity.

Quoted 19 lines

fs/jffs2/scan.c +19 -0

+	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) {

fs/jffs2/scan.c static int jffs2_scan_dirent_node(struct · line 906

Quoted 5 lines

fs/jffs2/scan.c +5 -0

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

fs/jffs2/scan.c static int jffs2_scan_dirent_node(struct · line 914

Quoted 7 lines

fs/jffs2/scan.c +7 -0

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

fs/jffs2/scan.c static int jffs2_scan_dirent_node(struct · line 957

Quoted 5 lines

fs/jffs2/scan.c +5 -0

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

fs/jffs2/scan.c static int jffs2_scan_dirent_node(struct · line 973

Quoted 4 lines

fs/jffs2/scan.c +4 -0

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

fs/jffs2/scan.c static int jffs2_scan_dirent_node(struct · line 1001

Quoted 5 lines

fs/jffs2/scan.c +5 -0

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

Thanks for reviewing in spite of so long patch.

Quoted 5 lines

Sounds good.

It appears as if your patch or something based on it will win. So let's have a look...

Quoted 4 lines

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 ?

Quoted 2 lines

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. ;-)

Quoted 10 lines

fs/jffs2/malloc.c struct jffs2_raw_node_ref *jffs2_alloc_r · line 198

Quoted 6 lines

struct jffs2_full_dirent *jffs2_alloc_full_dirent(int namesize)

fs/jffs2/malloc.c +1 -0

@@ -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.

Quoted 16 lines

fs/jffs2/malloc.c void jffs2_free_inode_cache(struct jffs2 · line 240

Quoted 10 lines

fs/jffs2/malloc.c +6 -0

@@ -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.

Quoted 5 lines

fs/jffs2/malloc.c void jffs2_free_inode_cache(struct jffs2 · line 241

Quoted 1 line

fs/jffs2/malloc.c +1 -0

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

Quoted 1 line

Quoted 1 line

diff -rpNU3 mtd-0829/fs/jffs2/nodelist.h

mtd-0829.xattr/fs/jffs2/nodelist.h

Quoted 2 lines

fs/jffs2/nodelist.h

Quoted 2 lines

fs/jffs2/nodelist.h +0 -0

10:28:53.000000000 -0400

Quoted 9 lines

fs/jffs2/nodelist.h line 23

Quoted 5 lines

fs/jffs2/nodelist.h +1 -0

@@ -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.

Quoted 2 lines

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.

Quoted 4 lines

Quoted 4 lines

fs/jffs2/nodelist.h +0 -0

 #ifdef __ECOS
 #include "os-ecos.h"

@@ -80,6 +81,7 @@ struct jffs2_raw_node_ref
 	struct jffs2_raw_node_ref *next_phys;

Quoted 8 lines

fs/jffs2/nodelist.h struct jffs2_raw_node_ref · line 84

Quoted 3 lines

fs/jffs2/nodelist.h +1 -0

 	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.

Quoted 2 lines

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 ?

Quoted 6 lines

fs/jffs2/nodelist.h struct jffs2_raw_node_ref · line 87

Quoted 3 lines

fs/jffs2/nodelist.h +3 -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.

Quoted 2 lines

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.

Quoted 3 lines

fs/jffs2/nodelist.h struct jffs2_raw_node_ref

Quoted 3 lines

fs/jffs2/nodelist.h +3 -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"

Quoted 1 line

fs/jffs2/nodelist.h

Quoted 1 line

fs/jffs2/nodelist.h +1 -0

+		       " on node at 0x%08x XID=%d Later one ignored\n", ofs, 

xc->xid);

Quoted 6 lines

fs/jffs2/nodelist.h

Quoted 3 lines

fs/jffs2/nodelist.h +3 -0

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

Quoted 3 lines

fs/jffs2/nodelist.h

Quoted 3 lines

fs/jffs2/nodelist.h +3 -0

+	xtype = jffs2_xattr_prefix_to_xtype(rx->data, nlen);
+	if (JFFS2_XATTRTYPE_INVALID==xtype) {
+		printk(KERN_NOTICE "jffs2_scan_xattr_node(): Unsupported 

xattr type"

Quoted 7 lines

fs/jffs2/nodelist.h

Quoted 4 lines

fs/jffs2/nodelist.h +4 -0

+		       " 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.

Quoted 2 lines

fs/jffs2/nodelist.h

Quoted 2 lines

fs/jffs2/nodelist.h +2 -0

+static inline int jffs2_scan_xref_node_helper(struct jffs2_sb_info *c,
+			struct jffs2_xattr_ref *new, struct jffs2_xattr_ref 

*old)

Quoted 6 lines

fs/jffs2/nodelist.h

Quoted 3 lines

fs/jffs2/nodelist.h +3 -0

+{
+	/* (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;
};

Quoted 1 line

Quoted 1 line

+static int jffs2_scan_xref_node(struct jffs2_sb_info *c, struct

jffs2_eraseblock *jeb,

Quoted 3 lines

fs/jffs2/nodelist.h

Quoted 3 lines

fs/jffs2/nodelist.h +3 -0

+				struct jffs2_raw_xref *rr, uint32_t ofs)
+{
+	struct jffs2_raw_node_ref *raw;

- snip -

Quoted 9 lines

fs/jffs2/nodelist.h

Quoted 5 lines

fs/jffs2/nodelist.h +5 -0

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

Quoted 3 lines

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 ?

Quoted 7 lines

Quoted 4 lines

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.

Can you rename it to xattr.h instead? The prefix has lost its meaning in fs/jffs2/.

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

#ifdef __ECOS
#include "os-ecos.h"

fs/jffs2/nodelist.h +0 -0

@@ -80,6 +81,7 @@ struct jffs2_raw_node_ref
 	struct jffs2_raw_node_ref *next_phys;

Quoted 8 lines

fs/jffs2/nodelist.h struct jffs2_raw_node_ref · line 84

Quoted 3 lines

fs/jffs2/nodelist.h +1 -0

 	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.

Quoted 7 lines

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.

True.

Most likely this idea is stupid, but let me have a try anyway. You could give your xattr/xref objects an inode number - either one per object or a special one shared for all xattr/xref objects. That would open a new bag of worms, but keep the size down.

Would it be possible to work with such an approach?

Quoted 5 lines

Quoted 2 lines

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 ?

In a perfect world, you should send a cleanup patch to break jffs2_scan_eraseblock() into several smaller functions. Have the xattr patch depend on the cleanup. I might still have an old patch that I could send you for inspiration.

Quoted 17 lines

Quoted 3 lines

fs/jffs2/scan.c static int jffs2_scan_dirent_node(struct

Quoted 3 lines

fs/jffs2/scan.c +3 -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"

Quoted 1 line

Review struct jffs2_raw_node_ref

Quoted 1 line

(quoted diff) +1 -0

+		       " on node at 0x%08x XID=%d Later one ignored\n", ofs,

xc->xid);

Quoted 6 lines

fs/jffs2/scan.c static int jffs2_scan_dirent_node(struct · line 879

Quoted 3 lines

fs/jffs2/scan.c +3 -0

+		DIRTY_SPACE(PAD(je32_to_cpu(rx->totlen)));
+		return 0;
+	}

How do you deal with xattr reference counting and lifetime?

Quoted 3 lines

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.

....at which time you also remove the xattr from medium, I assume. Ok.

Next question: How do you remove the xattr from medium? How is it garbage collected?

Quoted 14 lines

Quoted 3 lines

fs/jffs2/scan.c static int jffs2_scan_dirent_node(struct

Quoted 3 lines

fs/jffs2/scan.c +3 -0

+	xtype = jffs2_xattr_prefix_to_xtype(rx->data, nlen);
+	if (JFFS2_XATTRTYPE_INVALID==xtype) {
+		printk(KERN_NOTICE "jffs2_scan_xattr_node(): Unsupported

xattr type"

Quoted 7 lines

fs/jffs2/scan.c static int jffs2_scan_dirent_node(struct · line 914

Quoted 4 lines

fs/jffs2/scan.c +4 -0

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

Quoted 2 lines

Hmm,,, I added this member to discriminate the prefix of xattr without strncmp(). But it might be a bit ugly design.

Then why is it required that JFFS2 knows the prefix?

Quoted 20 lines

Quoted 1 line

Quoted 1 line

+static int jffs2_scan_xref_node(struct jffs2_sb_info *c, struct

jffs2_eraseblock *jeb,

Quoted 3 lines

fs/jffs2/scan.c static int jffs2_scan_dirent_node(struct · line 973

Quoted 3 lines

fs/jffs2/scan.c +3 -0

+				struct jffs2_raw_xref *rr, uint32_t ofs)
+{
+	struct jffs2_raw_node_ref *raw;

- snip -

Quoted 9 lines

fs/jffs2/scan.c static int jffs2_scan_dirent_node(struct · line 1001

Quoted 5 lines

fs/jffs2/scan.c +5 -0

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

Quoted 8 lines

Quoted 3 lines

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);

	ret->ino = je32_to_cpu(rr->ino);
	...

Quoted 5 lines

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 ?

Yes. Makes more sense now. Sorry about the tone.

Still, you can take it as an indication that your reuse-old-fields trick is not obvious to new readers. A comment explaining this would be useful. Maybe you already have it further down, in the part I didn't review yet.

Quoted 7 lines

Most likely this idea is stupid, but let me have a try anyway. You could give your xattr/xref objects an inode number - either one per object or a special one shared for all xattr/xref objects. That would open a new bag of worms, but keep the size down.

Would it be possible to work with such an approach?

It accompanies a so big remodeling, and hope not to do so, if possible.

I think it is important here to be able to distinguish three kinds of raw_node by a reasonable method (1. existing raw_node_ref of inode,
2. existing raw_node_ref without inode, 3. new raw_node_ref for XATTR), and to be able to refer from the third type of raw_node to the jffs2_xattr_(cache|ref) objects.

I noticed an idea to resolve it.
Any objects refered by the last next_in_ino are allocated by slab. If we can use kmem_ptr_validate() function, we can discriminate whether this pointer indicate xattr_cache/ref or not. Then, we can keep the size of jffs2_raw_node_ref.
(the member of 'owner' will go anyware.)

The issues of this method are as follows:
- If there is an adverse effect by existing of non-inode raw_node_ref whose next_in_ino is not NULL, I must resolve this.

- It's necessary to discuss in LKML, since this function is not EXPORT_SYMBOL()ed for kernel loadable modules currently.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

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 ?

Quoted 4 lines

In a perfect world, you should send a cleanup patch to break jffs2_scan_eraseblock() into several smaller functions. Have the xattr patch depend on the cleanup. I might still have an old patch that I could send you for inspiration.

I have developed this functionality based on mtd-snapshot-20050829. It means I should develop this based on mtd-snapshot-XXXX + your cleanup patches, isn't it?

Quoted 2 lines

Next question: How do you remove the xattr from medium? How is it garbage collected?

If my understanding is not wrong, an obsolete XATTR or XREF node is garbase collected as raw-node which does not have inode. (Becase next_in_ino of XATTR/XREF node is NULL.)

Quoted 1 line

Then why is it required that JFFS2 knows the prefix?

A permittion checking policy depends on XATTR's prefix. For example, only an user has CAP_SYS_ADMIN capability can read/write "trusted.*" xattr. In the "security.*" prefix case, this checking was done in LSM module. Therefore, filesystem must know XATTR's prefix.

Quoted 4 lines

Still, you can take it as an indication that your reuse-old-fields trick is not obvious to new readers. A comment explaining this would be useful. Maybe you already have it further down, in the part I didn't review yet.

Sorry for tricky coding without comments. I'll pay attention of such coding style.

Quoted 7 lines

It accompanies a so big remodeling, and hope not to do so, if possible.

I think it is important here to be able to distinguish three kinds of raw_node by a reasonable method (1. existing raw_node_ref of inode,
2. existing raw_node_ref without inode, 3. new raw_node_ref for XATTR), and to be able to refer from the third type of raw_node to the jffs2_xattr_(cache|ref) objects.

Yes. 1 and 2 are currently distinguished by next_in_ino being either either valid or NULL.

Quoted 6 lines

I noticed an idea to resolve it.
Any objects refered by the last next_in_ino are allocated by slab. If we can use kmem_ptr_validate() function, we can discriminate whether this pointer indicate xattr_cache/ref or not. Then, we can keep the size of jffs2_raw_node_ref.
(the member of 'owner' will go anyware.)

Sounds scary. But in principle, you are right. In case of either 1 or 3, we attach "something" to next_in_ino. Now we only have to decide whether "something" is an inode, or the new xattr-struff.

If you add an extra field to jffs2_inode_cache (which also is grossly misnamed, old mistake), you can do so. And since inodes are a lot less frequent than raw nodes, the memory consumption should have improved quite a bit.

Quoted 8 lines

Quoted 4 lines

In a perfect world, you should send a cleanup patch to break jffs2_scan_eraseblock() into several smaller functions. Have the xattr patch depend on the cleanup. I might still have an old patch that I could send you for inspiration.

I have developed this functionality based on mtd-snapshot-20050829. It means I should develop this based on mtd-snapshot-XXXX + your cleanup patches, isn't it?

No. You should develop two patches based on current cvs:
[PATCH 1/2] Cleanup of jffs2_scan_eraseblock() [PATCH 2/2] xattr for jffs2

And you may or may not use my previous attempt as a basis, that's your decision.

Quoted 7 lines

Quoted 1 line

Then why is it required that JFFS2 knows the prefix?

A permittion checking policy depends on XATTR's prefix. For example, only an user has CAP_SYS_ADMIN capability can read/write "trusted.*" xattr. In the "security.*" prefix case, this checking was done in LSM module. Therefore, filesystem must know XATTR's prefix.

This is just wrong. I agree that something inside the kernel must know whether users need specific capabilities to read/write the xattr. Sure. But why in hell does it have to be JFFS2?

How do other filesystems deal with this? ext[23] and reiserfs (v3) all have xattr, I believe.

Quoted 9 lines

Sounds scary. But in principle, you are right. In case of either 1 or 3, we attach "something" to next_in_ino. Now we only have to decide whether "something" is an inode, or the new xattr-struff.

If you add an extra field to jffs2_inode_cache (which also is grossly misnamed, old mistake), you can do so. And since inodes are a lot less frequent than raw nodes, the memory consumption should have improved quite a bit.

I prefer this kind of approach than other one. It's simple solution. I'm afraid of breaking something to be assumed implicitly by reserving some inode-numbers.

And, how about such an idea ?
Currently, the variable of 'state' in jffs2_inode_cache is defined as int type, but the range of this variable is between 0 and 6. In general case, 'int' has 32-bit width. It's so much for this field.

For example, we can redefine the 'state' as 16-bit width variable, and insert a new 16-bit width variable to discriminate own object. If acceptable, it can also keep the memory comsumption for jffs2_inode_cache.

In this plan, jffs2_inode_cahce and jffs2_xattr_datum will be defined as follows:

struct jffs2_inode_cache {
    struct jffs2_full_dirent *scan_dents;
    uint16_t class; /* =0 means jffs2_inode_cache */
    uint16_t state;
       :

struct jffs2_xattr_datum {
    void *dummy;    /* always NULL */
    uint16_t class; /* =1 means jffs2_xattr_datum */
       :

I see, jffs2_scan_eraseblock() is indeed a bit long. Please send your attempt patches at first. I'll try to cleanup it for the basis of XATTR patch.

Quoted 4 lines

This is just wrong. I agree that something inside the kernel must know whether users need specific capabilities to read/write the xattr. Sure. But why in hell does it have to be JFFS2?

Quoted 2 lines

How do other filesystems deal with this? ext[23] and reiserfs (v3) all have xattr, I believe.

Indeed, more upper layer might be able to manage this kind of decision making as you say. But it's transferred to the filesystem currently. All of them (ext2/3, reiser and so on) have own permission checking codes like 'fs/ext2/xattr_trusted.c', and I imitated those implementations. I don't know the reason correctly.

Quoted 11 lines

In this plan, jffs2_inode_cahce and jffs2_xattr_datum will be defined as follows:

struct jffs2_inode_cache {
   struct jffs2_full_dirent *scan_dents;
   uint16_t class; /* =0 means jffs2_inode_cache */
   uint16_t state;
      :

struct jffs2_xattr_datum {
   void *dummy;    /* always NULL */
   uint16_t class; /* =1 means jffs2_xattr_datum */
      :

You could even make the class a u8. For inodes, the remaining 8 bits would just be reserved space for proper alignment. If either of your xattr object could make use of the extra byte, it would be worth it. If not, just ignore my silly idea.

Quoted 3 lines

I see, jffs2_scan_eraseblock() is indeed a bit long. Please send your attempt patches at first. I'll try to cleanup it for the basis of XATTR patch.

It was 34 patches going over all of jffs2, actually. I guess it's easier to send the changed scan.c. The pseudo-random stuff was controversial, so I lost interest after a while.

Quoted 4 lines

Quoted 4 lines

This is just wrong. I agree that something inside the kernel must know whether users need specific capabilities to read/write the xattr. Sure. But why in hell does it have to be JFFS2?

Quoted 8 lines

Quoted 2 lines

How do other filesystems deal with this? ext[23] and reiserfs (v3) all have xattr, I believe.

Indeed, more upper layer might be able to manage this kind of decision making as you say. But it's transferred to the filesystem currently. All of them (ext2/3, reiser and so on) have own permission checking codes like 'fs/ext2/xattr_trusted.c', and I imitated those implementations. I don't know the reason correctly.

The code is not exactly easy to follow. But it appears as if they used that stuff for compression. If prefix is "trusted.", just use an e_name_index of 4 and drop the prefix. It can be reconstructed on reads.

I revised the previous xattr patch in jffs2 based on some suggestions.

[1] scanning xattr related node for cleaned-up scan.c. (jffs2_xattr_scan.c-cleanup.patch)
[2] enables xattr support in jffs2.

    (jffs2_xattr_take-3.patch)

(*) We can apply jffs2_xattr_take-3.patch for current CVS independently.

Quoted 7 lines · Kaigai Kohei

I revised the previous xattr patch in jffs2 based on some suggestions.

[1] scanning xattr related node for cleaned-up scan.c. (jffs2_xattr_scan.c-cleanup.patch)
[2] enables xattr support in jffs2. (jffs2_xattr_take-3.patch)
(*) We can apply jffs2_xattr_take-3.patch for current CVS independently.

Looks quite interesting. I'll take a closer look over the weekend.

Thanks!

Jörn

Quoted 7 lines · Kaigai Kohei

I revised the previous xattr patch in jffs2 based on some suggestions.

[1] scanning xattr related node for cleaned-up scan.c. (jffs2_xattr_scan.c-cleanup.patch)
[2] enables xattr support in jffs2. (jffs2_xattr_take-3.patch)
(*) We can apply jffs2_xattr_take-3.patch for current CVS independently.

Looks quite interesting. I'll take a closer look over the weekend.

Thanks!

Jörn

What status is the XATTR patch of JFFS2 now ??

I hope to merge it, if this patch has no problem.
Of course, I'll correct any inconvenient points which we found.

# I droped a Cc: for Ma, since his address seems to be invalid.

Lack of time. I still have your patch and some from Forrest lingering in my inbox. They all should receive a thorough line-by-line review - not necessarily by me, but by someone with enough knowledge.

If it helps you, your patches are already in a relatively good state. I cannot easily find problems with it.

Hi,

This attached patches are enables XATTR support on JFFS2. some new functionalities are added from take-3 patches.

# Sorry for slowing update.

[NOTICE]
We can test this patch with ONLY 2.6.14 kernel.
The previous versions of kernel do not support an new LSM hook.

[1/2] jffs2_xattr_take-4.patch
* CONFIG_JFFS2_XATTR was renamed to CONFIG_JFFS2_FS_XATTR The XATTR configuration switch are named CONFIG_<fsname>_FS_XATTR on another some filesystems which support XATTR.
I also follow this manner.

* CONFIG_JFFS2_FS_POSIX_ACL and JFFS2_FS_SECURITY are selectable. this follows another FS's manner.

* POSIX-ACL support was added.
This functionality is implemented in acl.c, acl.h.
I implemented the most part of this functionalities by referring the Ext2/3's implementation of POSIX-ACL.
If I took bad manner or misunderstanding for implementation, notice me please.

* jffs2_init_security() was added for inode initialization. At 2.6.14, new LSM hook(security_inode_init_security) was added. Because of this, any filesystems obtain security attribtues must call this this hook to set the security label for new inode. It's deployed in jffs2_create(), jffs2_symlink(), jffs2_mkdir() and jffs2_mknod().

* Some patch conflicts were fixed.
We can apply this patches for latest MTD-CVS without incident.

* An obvious bug was fixed.
In jffs2_build_xattr_subsystem(), NULL checking was omitted.

[2/2] mkfs.jffs2-xattr.patch
* --with-{xattr|selinux|posix-acl} options are added. --with-xattr enables to pack all xattr-entries into jffs2 image file. --with-selinux enables to pack security related xattr-entries which are named as 'security.*' into jffs2 image file. --with-posix-acl enables to pack ACL related xattr-entries which are names as 'system.posix_acl_access' or 'system.posix_acl_default'.

I hope to merge those functionalities into MTD's CVS tree. Thanks,