[PATCH] [JFFS2] Non-contiguous write bug fix (Sibley)

[PATCH] [JFFS2] Non-contiguous write bug fix (Sibley)

[Alexander Belyakov]

Yes, the bug still exists. The patch fixes JFFS2 non-contiguous write
error on Sibley.

Signed-off-by: Alexander Belyakov <alexander.belyakov@intel.com>
---
diff -uNr a/fs/jffs2/wbuf.c b/fs/jffs2/wbuf.c
--- a/fs/jffs2/wbuf.c	2007-03-23 22:52:51.000000000 +0300
+++ b/fs/jffs2/wbuf.c	2007-04-06 14:06:16.000000000 +0400
@@ -755,6 +755,13 @@
 		memset(c->wbuf,0xff,c->wbuf_pagesize);
 	}

+	/* Fixup the wbuf if we are moving to a new eraseblock */
+	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);
+	}
+
 	/*
 	 * Sanity checks on target address.  It's permitted to write
 	 * at PAD(c->wbuf_len+c->wbuf_ofs), and it's permitted to

Re: [PATCH] [JFFS2] Non-contiguous write bug fix (Sibley)

[David Woodhouse]

On Tue, 2007-04-17 at 15:14 +0400, Alexander Belyakov wrote:
> Yes, the bug still exists. The patch fixes JFFS2 non-contiguous write
> error on Sibley.

Sorry, I missed (or have forgotten) this. Can you explain in a little
more detail?

The following condition (SECTOR_ADDR(to) != SECTOR_ADDR(wbuf_ofs)) is
supposed to handle this, surely? In what circumstances is it
insufficient?

As far as I can tell, your new check allows us to write anywhere _after_
writing the first wbuf-load in a new eraseblock.

Hm, is this because the cleanmarker size in Sibley is also the wbuf
size? So you were hitting the BUG() after writing the cleanmarker to a
newly-erased block (hence setting wbuf_ofs to point there), but then
moving on to actually _write_ elsewhere?

If that's the case, perhaps the code which writes the cleanmarker
shouldn't be using jffs2_flash_writev() to write it -- perhaps it should
be bypassing the wbuf handling altogether?

-- 
dwmw2

Re: [PATCH] [JFFS2] Non-contiguous write bug fix (Sibley)

[Alexander Belyakov]

On 4/18/07, David Woodhouse <dwmw2@infradead.org> wrote:
> On Tue, 2007-04-17 at 15:14 +0400, Alexander Belyakov wrote:
> > Yes, the bug still exists. The patch fixes JFFS2 non-contiguous write
> > error on Sibley.
>
> Sorry, I missed (or have forgotten) this. Can you explain in a little
> more detail?

I'm hitting BUG() in jffs2_flash_writev() working with JFFS2 on
Sibley. The issue is 100% reproducible and seems to be critical for
everyone who uses the recent MTD source on Sibley.

The problem was noticed first time after MTD migration from cvs to
git. It seems several pieces of code were cleaned at that time
including wbuf fixup.

There was one unfinished discussion
http://lists.infradead.org/pipermail/linux-mtd/2006-June/016043.html

another corresponding thread is here
http://lists.infradead.org/pipermail/linux-mtd/2006-April/015340.html


> Hm, is this because the cleanmarker size in Sibley is also the wbuf
> size? So you were hitting the BUG() after writing the cleanmarker to a
> newly-erased block (hence setting wbuf_ofs to point there), but then
> moving on to actually _write_ elsewhere?
>
> If that's the case, perhaps the code which writes the cleanmarker
> shouldn't be using jffs2_flash_writev() to write it -- perhaps it should
> be bypassing the wbuf handling altogether?

The idea behind this patch is just to restore working piece of code
(wbuf fixup) now.

But you're probably right and there is another way to fix the problem.
It is worth to think about it.

Thanks,
Alexander

Re: [PATCH] [JFFS2] Non-contiguous write bug fix (Sibley)

[Alexander Belyakov]

On 4/18/07, David Woodhouse <dwmw2@infradead.org> wrote:
>
> Sorry, I missed (or have forgotten) this. Can you explain in a little
> more detail?
>
> The following condition (SECTOR_ADDR(to) != SECTOR_ADDR(wbuf_ofs)) is
> supposed to handle this, surely? In what circumstances is it
> insufficient?
>

David,

I just perform random length writes to JFFS2 and bug _always_ appears
at some point.

Currently it is possible to get 'wbuf_ofs' pointing to the beginning
of the eraseblock, meanwhile 'to' points one page ahead (cleanmarker
on Sibley) and 'wbuf_len' equals to zero. Both 'wbuf_ofs' and 'to'
belong to the same eraseblock and condition (SECTOR_ADDR(to) !=
SECTOR_ADDR(wbuf_ofs)) fails. So we hit BUG() in jffs2_flash_writev()
with non-contiguous write error.

>
> Hm, is this because the cleanmarker size in Sibley is also the wbuf
> size? So you were hitting the BUG() after writing the cleanmarker to a
> newly-erased block (hence setting wbuf_ofs to point there), but then
> moving on to actually _write_ elsewhere?
>
> If that's the case, perhaps the code which writes the cleanmarker
> shouldn't be using jffs2_flash_writev() to write it -- perhaps it should
> be bypassing the wbuf handling altogether?
>

Cleanmarker is written with jffs2_flash_direct_writev() and thus
doesn't affect wbuf.

David, do you have something in mind about fixing the bug without
applying the patch suggested?

Thanks,
Alexander

Re: [PATCH] [JFFS2] Non-contiguous write bug fix (Sibley)

[David Woodhouse]

On Wed, 2007-05-02 at 17:49 +0400, Alexander Belyakov wrote:
> Currently it is possible to get 'wbuf_ofs' pointing to the beginning
> of the eraseblock, meanwhile 'to' points one page ahead (cleanmarker
> on Sibley) and 'wbuf_len' equals to zero. Both 'wbuf_ofs' and 'to'
> belong to the same eraseblock and condition (SECTOR_ADDR(to) !=
> SECTOR_ADDR(wbuf_ofs)) fails. So we hit BUG() in jffs2_flash_writev()
> with non-contiguous write error.

Hm. But _how_ do we get into that state? If it isn't through writing the
cleanmarker (and it looks like you're right; it isn't), then _how_ is
c->wbuf_ofs ever setting set to point to the first page in the block?

> David, do you have something in mind about fixing the bug without
> applying the patch suggested?

I'm concerned by the patch, because I think it's "protecting" against a
situation which should never happen. I want to understand _why_ we get
into this state, not just apply the symptomatic fix.

Can you add something like a WARN_ON(!(c->wbuf_ofs % c->sector_size))
whereever we set c->wbuf_ofs, and see where it's happening?

-- 
dwmw2                       09 F9 11 02 9D 74 E3 5B D8 41 56 C5 63 56 88 C0

Re: [PATCH] [JFFS2] Non-contiguous write bug fix (Sibley)

[Alexander Belyakov]

On 5/2/07, David Woodhouse <dwmw2@infradead.org> wrote:
>
> Can you add something like a WARN_ON(!(c->wbuf_ofs % c->sector_size))
> whereever we set c->wbuf_ofs, and see where it's happening?
>

Done. It seems __jffs2_flush_wbuf() sets 'wbuf_ofs' to the first page in block.

BUG: at fs/jffs2/wbuf.c:639 __jffs2_flush_wbuf()
jffs2_flash_writev(): Non-contiguous write to 008c0400
kernel BUG at fs/jffs2/wbuf.c:790!

'wbuf_ofs' is set to the first page in block quite often in
__jffs2_flush_wbuf(), but this doesn't always lead to
jffs2_flash_writev() failure. If 'to' is pointing to the different
block, condition (SECTOR_ADDR(to) != SECTOR_ADDR(wbuf_ofs)) works fine
and we don't hit BUG() despite 'wbuf_ofs' points to the first page in
block.

Probably we should put additional 'wbuf_ofs' adjustment to the
__jffs2_flush_wbuf() checking if we're moving to the new erseblock.

Thanks,
Alexander

Re: [PATCH] [JFFS2] Non-contiguous write bug fix (Sibley)

[David Woodhouse]

On Wed, 2007-05-02 at 18:53 +0400, Alexander Belyakov wrote:
> 'wbuf_ofs' is set to the first page in block quite often in
> __jffs2_flush_wbuf(), but this doesn't always lead to
> jffs2_flash_writev() failure. If 'to' is pointing to the different
> block, condition (SECTOR_ADDR(to) != SECTOR_ADDR(wbuf_ofs)) works fine
> and we don't hit BUG() despite 'wbuf_ofs' points to the first page in
> block.
> 
> Probably we should put additional 'wbuf_ofs' adjustment to the
> __jffs2_flush_wbuf() checking if we're moving to the new erseblock. 

Ah, I understand. So this should fix it?

diff --git a/fs/jffs2/wbuf.c b/fs/jffs2/wbuf.c
index c556e85..91d1d0f 100644
--- a/fs/jffs2/wbuf.c
+++ b/fs/jffs2/wbuf.c
@@ -637,7 +637,10 @@ static int __jffs2_flush_wbuf(struct jffs2_sb_info *c, int pad)
 
 	memset(c->wbuf,0xff,c->wbuf_pagesize);
 	/* adjust write buffer offset, else we get a non contiguous write bug */
-	c->wbuf_ofs += c->wbuf_pagesize;
+	if (SECTOR_ADDR(c->wbuf_ofs) == SECTOR_ADDR(c->wbuf_ofs+c->wbuf_pagesize))
+		c->wbuf_ofs += c->wbuf_pagesize;
+	else
+		c->wbuf_ofs = 0xffffffff;
 	c->wbuf_len = 0;
 	return 0;
 }

-- 
dwmw2                       09 F9 11 02 9D 74 E3 5B D8 41 56 C5 63 56 88 C0

Re: [PATCH] [JFFS2] Non-contiguous write bug fix (Sibley)

[Alexander Belyakov]

On 5/2/07, David Woodhouse <dwmw2@infradead.org> wrote:
>
> Ah, I understand. So this should fix it?
>
> diff --git a/fs/jffs2/wbuf.c b/fs/jffs2/wbuf.c
> index c556e85..91d1d0f 100644
> --- a/fs/jffs2/wbuf.c
> +++ b/fs/jffs2/wbuf.c
> @@ -637,7 +637,10 @@ static int __jffs2_flush_wbuf(struct jffs2_sb_info *c, int pad)
>
>         memset(c->wbuf,0xff,c->wbuf_pagesize);
>         /* adjust write buffer offset, else we get a non contiguous write bug */
> -       c->wbuf_ofs += c->wbuf_pagesize;
> +       if (SECTOR_ADDR(c->wbuf_ofs) == SECTOR_ADDR(c->wbuf_ofs+c->wbuf_pagesize))
> +               c->wbuf_ofs += c->wbuf_pagesize;
> +       else
> +               c->wbuf_ofs = 0xffffffff;
>         c->wbuf_len = 0;
>         return 0;
>  }
>

Yes, it helps.

Thanks,
Alexander

Re: [PATCH] [JFFS2] Non-contiguous write bug fix (Sibley)

[Alexander Belyakov]

On 5/3/07, Alexander Belyakov <abelyako@googlemail.com> wrote:
> On 5/2/07, David Woodhouse <dwmw2@infradead.org> wrote:
> >
> > Ah, I understand. So this should fix it?
> >
>
> Yes, it helps.
>

David,

will you apply your patch to official MTD?

Thanks,
Alexander