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

[Jörn Engel]

On Mon, 15 August 2005 15:07:31 +0400, Artem B. Bityuckiy wrote:
> 
> Ok, to keep you busy with some technical stuff also, here is a remark.
> 
> >+		if (je32_to_cpu(sm->magic) == JFFS2_SUM_MAGIC ) {
> >+
> >+			if(je32_to_cpu(sm->erase_size) == c->sector_size) {
> >+				err = 
> >jffs2_sum_scan_sumnode(c,jeb,je32_to_cpu(sm->offset),&pseudo_random);
> >+
> >+				if (err) {
> >+					kfree(sm);
> >+					return err;
> >+				}
> >+			}
> >+			printk(KERN_WARNING "FS erase_block_size != JFFS2 
> erase_block_size => skipping summary information\n");
> >+		}
> 
> if(je32_to_cpu(sm->erase_size) == c->sector_size) { ...
> 
> AFAIU, you're protecting against the situation when the size of the 
> JFFS2 virtual eraseblock is changed.
> 
> But why is the eraseblock size may change? If it becomes less, this is a 
> problem since some nodes that cross the boundary will be ignored. If the 
> size of the JFFS2 virtual eraseblock becomes larger, it is also a 
> problem - due to present bad blocks, some eraseblocks with valid data 
> will be ignored (remember, if there is a bad eraseblock in the JFFS2 
> virtual eraseblock, the whole virtual eraseblock is regarded as bad).
> 
> Thus, please, don't solve this problem this way. You're trying to solve 
> it privately for your summaries, while you have to solve it for *JFFS2 
> in general* (if it exists at all). I suppose it exists in your case 
> since you add one more field to struct eraseblock...

Agreed.  Best place to put the check would be in
jffs2_do_fill_super(), about here:

	c->sector_size = c->mtd->erasesize; 
	blocks = c->flash_size / c->sector_size;
	if (!(c->mtd->flags & MTD_NO_VIRTBLOCKS)) {
		while ((blocks * sizeof (struct jffs2_eraseblock))
				> (128 * 1024)) {
			blocks >>= 1;
			c->sector_size <<= 1;
		}	
	}

Imo, this block growing is an ugly hack.  It defeats the ability to
grow/shrink your partition on the fly.  Worse, it makes
growing/shrinking fail only rarely, while it usually just works.

Anyway.  Once the size of erase blocks changes, we're basically
fucked - even without summary.  So we want that check unconditionally.
Of course, we don't have any nodes containing the erase_size yet.
Hmm.

David, my best idea right now would be to change the erase marker and
add a field to it.  Doesn't really make me happy, though.  Do you have
a better idea?

And while we're at it, why not create a new function for this code.
Something like this:

int jffs2_check_erase_size(struct jffs2_sb_info *c)
{
	size_t blocks;

	c->sector_size = c->mtd->erasesize; 
	blocks = c->flash_size / c->sector_size;
	if (!(c->mtd->flags & MTD_NO_VIRTBLOCKS)) {
		while ((blocks * sizeof (struct jffs2_eraseblock))
				> (128 * 1024)) {
			blocks >>= 1;
			c->sector_size <<= 1;
		}	
	}

	if (/* magic check about changed erase_size fails */)
		return -EIO;
	return 0;
}

Jörn

-- 
He who knows others is wise.
He who knows himself is enlightened.
-- Lao Tsu