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

Jörn Engel
2005-08-15T11:53:52Z

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