Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi <havasi@inf.u-szeged.hu>]

Jörn Engel wrote:

>>diff -Narup Mtd-orig/fs/jffs2/build.c mtd/fs/jffs2/build.c
>>--- Mtd-orig/fs/jffs2/build.c	2005-07-22 12:32:07.000000000 +0200
>>+++ mtd/fs/jffs2/build.c	2005-07-29 18:43:16.000000000 +0200
>>@@ -334,6 +334,9 @@ int jffs2_do_mount_fs(struct jffs2_sb_in
>> 		c->blocks[i].first_node = NULL;
>> 		c->blocks[i].last_node = NULL;
>> 		c->blocks[i].bad_count = 0;
>>+#ifdef CONFIG_JFFS2_SUMMARY
>>+		c->blocks[i].sum_collected = NULL;
>>+#endif
>>    
>>
>
>I guess we can start removing some of the #ifdef ugliness already.
>The extra field per eraseblock can be added unconditionally, as can
>this line.
>
>  
>
OK, we will eliminate all of these kind of #endifs.

>> 	D1(printk(KERN_DEBUG "Going to GC REF_PRISTINE node at 0x%08x\n", ref_offset(raw)));
>> 
>>@@ -513,8 +514,9 @@ static int jffs2_garbage_collect_pristin
>> 	/* Ask for a small amount of space (or the totlen if smaller) because we
>> 	   don't want to force wastage of the end of a block if splitting would
>> 	   work. */
>>-	ret = jffs2_reserve_space_gc(c, min_t(uint32_t, sizeof(struct jffs2_raw_inode) + JFFS2_MIN_DATA_LEN, 
>>-					      rawlen), &phys_ofs, &alloclen);
>>+	ret = jffs2_reserve_space_gc(c, min_t(uint32_t, sizeof(struct jffs2_raw_inode) +
>>+				JFFS2_MIN_DATA_LEN, rawlen), &phys_ofs, &alloclen, rawlen);
>>+				/* this is not optimal yet */
>>    
>>
>
>Can you add some more explanation to the comment?
>  
>
OK, we will.

The reason of "this is not optimal" is the following: for
jffs2_reserve_space_gc() we have to specify an argument called sumsize.
It is the summary size of the requested node. It needs it, because it is
necessary to fit not only the node but also its summary representation
into the erase block. In all other cases we know exactly the summary
size of all nodes except in this case, so we made an upper-estimation here.

>> 	if (ret)
>> 		return ret;
>> 
>>@@ -594,7 +596,10 @@ static int jffs2_garbage_collect_pristin
>> 	nraw->__totlen = rawlen;
>> 	nraw->next_phys = NULL;
>> 
>>-	ret = jffs2_flash_write(c, phys_ofs, rawlen, &retlen, (char *)node);
>>+	vecs[0].iov_base = (unsigned char *) node;
>>+	vecs[0].iov_len = rawlen;
>>+
>>+	ret = jffs2_flash_writev(c, vecs, 1, phys_ofs, &retlen, 0);
>>    
>>
>
>Why this change?  If it's required, it might make sense to split it
>out into a preparatory patch.  But my gut feeling is that dropping
>this part would be better.
>  
>
Jffs2_flash_write() simply write directly the data onto the flash, and
does not collect the summary information. Jffs2_flash_writev() does.

>>+#ifdef CONFIG_JFFS2_SUMMARY
>>+
>>+/* Called with alloc sem _and_ erase_completion_lock */
>>+static int jffs2_do_reserve_space_sum(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize)
>>+{
>>    
>>
>[...]
>  
>
>>+#else
>>+
>>+/* Called with alloc sem _and_ erase_completion_lock */
>>+static int jffs2_do_reserve_space_normal(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize)
>>+{
>>    
>>
>
>It doesn't take too much creativity to combine this and the above
>#ifdef into a single one.  That will still be ugly, but it's a step in
>the right direction.
>  
>
In the previous version it was combined, but we afraid of that it is
hard to see what exectly happen. But we can tranform it back.

Ferenc