JFFS2 wbuf non-contiguous write bug

[Jörn Engel]

On Thu, 29 June 2006 16:00:05 +0400, Alexander Belyakov wrote:
> 
> "Non-contiguous write" bug appears in jffs2_flash_writev() function
> (JFFS2 mounted on Sibley). It seems the reason is "wbuf fixup" code
> removed from wbuf.c with this commit
> 
> "[MTD] Merge STMicro NOR_ECC code with Intel Sibley code"
> http://kernel.org/git/?p=linux/kernel/git/dwmw2/mtd-2.6.git;a=commit;h=c8b229de2b05c2b3e8d282ce260935a88ac030ca
> 
> The following lines were removed there
> 
> 		if (((c->wbuf_ofs % c->sector_size) == 0) && !c->wbuf_len) {
> 			c->wbuf_ofs = PAGE_DIV(to);
> 			c->wbuf_len = PAGE_MOD(to);
> 			memset(c->wbuf,0xff,c->wbuf_pagesize);
> 		}
> 
> Getting that code back fixes the problem.

How can that be?  The removed code was:
-	if (jffs2_nor_ecc(c)) {
-		if (((c->wbuf_ofs % c->sector_size) == 0) && !c->wbuf_len) {
-			c->wbuf_ofs = PAGE_DIV(to);
-			c->wbuf_len = PAGE_MOD(to);
-			memset(c->wbuf,0xff,c->wbuf_pagesize);
-		}
-	}

And jffs2_nor_ecc(c) would have been false for Sibley (true for
STMicro, though).  So if we have a bug for Sibley and adding this code
solves the problem _now_, how could it have solved the problem
_before_?

I'm not opposed to adding this code back, but would like to get a
better picture of things first.

> One may notice that there is another "new eraseblock" check left in
> jffs2_flash_writev()
> 
> 	if (SECTOR_ADDR(to) != SECTOR_ADDR(c->wbuf_ofs)) { ...
> 
> But it is not enough since SECTOR_ADDR(to) and
> SECTOR_ADDR(c->wbuf_ofs) could be the same here even in case of moving
> to a new eraseblock.

Hmm.  If that check is supposed to be
	if (we moved to a new eraseblock)
then the actual check is wrong.  What is it supposed to mean, though?

> Am I missing something or "wbuf fixup" code should be indeed restored?

You must be missing something, but I don't know what.  Maybe Josh has
some insight into this problem - he did a thorough review and asked
many probing questions about this particular part of the patch.

Jörn

-- 
Data expands to fill the space available for storage.
-- Parkinson's Law