BUG at mm/filemap.c:1749 (2.6.24, jffs2, unionfs)

BUG at mm/filemap.c:1749 (2.6.24, jffs2, unionfs)

[Erez Zadok]

David,

I'm testing unionfs on top of jffs2, using 2.6.24 as of linus's commit
4fa4d23fa20de67df919030c1216295664866ad7.  All of my unionfs tests pass when
unionfs is stacked on top of jffs2, other than my truncate test -- whic
tries to truncate files up/down (through the union, which then is passed
through to the lower jffs2 f/s).  The same truncate test passes on all other
file systems I've tried unionfs/2.6.24 with, as well as all of the earlier
kernels that unionfs runs on (2.6.9--2.6.23).  So I tend to think this bug
is more probably due to something else going on in 2.6.24, possibly wrt
jffs2/mtd.  (Of course, it's still possible that unionfs isn't doing
something right -- any pointers?)

The oops trace is included below.  Is this a known issue and if so, any
fixes?  If this is the first you hear of this problem, let me know and I'll
try to narrow it down further.

Thanks,
Erez.

------------[ cut here ]------------
kernel BUG at mm/filemap.c:1749!
invalid opcode: 0000 [#1] DEBUG_PAGEALLOC
Modules linked in: block2mtd mtdblock jffs2 mtd_blkdevs mtd zlib_deflate
zlib_inflate nfsd exportfs auth_rpcgss nfs lockd nfs_acl sunrpc pcnet32
CPU:    0
EIP:    0060:[<c012f03d>]    Not tainted VLI
EFLAGS: 00010287   (2.6.23-unionfs2-2.6.24-rc0-pre #9)
EIP is at iov_iter_advance+0x13/0x5d
eax: c538fdec   ebx: 00001000   ecx: c538fdec   edx: 00001000
esi: 00000fa0   edi: 00000000   ebp: c538fd7c   esp: c538fd6c
ds: 007b   es: 007b   fs: 0000  gs: 0033  ss: 0068
Process dd (pid: 11484, ti=c538e000 task=c4cb05b0 task.ti=c538e000)
Stack: 00001000 00001000 00000fa0 00000000 c538fe10 c01307b7 00000fa0 00000000 
       00000060 00000060 c16fb0c0 c690edc8 c690eed4 00000000 c538fed4 6238f40 
       c690eed4 f890a4c0 c690edc8 00000000 00000060 00000fa0 f890a4c0 c16fb0a0 
Call Trace:
 [<c0102bc2>] show_trace_log_lvl+0x1a/0x2f
 [<c0102c72>] show_stack_log_lvl+0x9b/0xa3
 [<c0102e2e>] show_registers+0x1b4/0x285
 [<c0102fff>] die+0x100/0x21d
 [<c01031a5>] do_trap+0x89/0xa2
 [<c010347d>] do_invalid_op+0x88/0x92
 [<c026a412>] error_code+0x6a/0x70
 [<c01307b7>] generic_file_buffered_write+0x163/0x51d
 [<c0130f60>] __generic_file_aio_write_nolock+0x3ef/0x43f
 [<c0131008>] generic_file_aio_write+0x58/0xb6
 [<c01498f8>] do_sync_write+0xc4/0x101
 [<c014a04b>] vfs_write+0x90/0x119
 [<c014a500>] sys_write+0x3d/0x61
 [<c0102586>] sysenter_past_esp+0x5f/0x91
 =======================
Code: ff 39 c2 74 0c 89 da 8a 41 ff 88 45 f7 89 d0 eb 02 31 c0 5a 5b 5e 5d
c3 55 89 c1 89 e5 57 56 53 83 ec 04 89 55 f0 39 50 0c 73 04 <0f> 0b eb fe 83
78 04 01 75 08 8b 45 f0 01 41 08 eb 2c 8b 38 8b 
EIP: [<c012f03d>] iov_iter_advance+0x13/0x5d SS:ESP 0068:c538fd6c

Re: BUG at mm/filemap.c:1749 (2.6.24, jffs2, unionfs)

[Nick Piggin]

[-- Attachment #1: Type: text/plain, Size: 1367 bytes --]

On Friday 19 October 2007 16:05, Erez Zadok wrote:
> David,
>
> I'm testing unionfs on top of jffs2, using 2.6.24 as of linus's commit
> 4fa4d23fa20de67df919030c1216295664866ad7.  All of my unionfs tests pass
> when unionfs is stacked on top of jffs2, other than my truncate test --
> whic tries to truncate files up/down (through the union, which then is
> passed through to the lower jffs2 f/s).  The same truncate test passes on
> all other file systems I've tried unionfs/2.6.24 with, as well as all of
> the earlier kernels that unionfs runs on (2.6.9--2.6.23).  So I tend to
> think this bug is more probably due to something else going on in 2.6.24,
> possibly wrt jffs2/mtd.  (Of course, it's still possible that unionfs isn't
> doing something right -- any pointers?)
>
> The oops trace is included below.  Is this a known issue and if so, any
> fixes?  If this is the first you hear of this problem, let me know and I'll
> try to narrow it down further.

It's had quite a lot of recent changes in that area -- the "new aops"
patches.

They've been getting quite a bit of testing in -mm and no such problems,
but I doubt anyone was doing much unionfs over jffs2, or even much jffs2
testing with -mm.

The bug smells like jffs2 is actually passing back a "written" length
greater than the length we passed into it.

The following might show what's happening.

[-- Attachment #2: mm-debug.patch --]
[-- Type: text/x-diff, Size: 738 bytes --]

Index: linux-2.6/mm/filemap.c
===================================================================
--- linux-2.6.orig/mm/filemap.c
+++ linux-2.6/mm/filemap.c
@@ -2184,6 +2184,7 @@ fs_write_aop_error:
 	return written ? written : status;
 }
 
+#include <linux/kallsyms.h>
 static ssize_t generic_perform_write(struct file *file,
 				struct iov_iter *i, loff_t pos)
 {
@@ -2243,6 +2244,13 @@ again:
 						page, fsdata);
 		if (unlikely(status < 0))
 			break;
+		if (status > copied) {
+			print_symbol("%s returned more than it should!\n", (unsigned long)a_ops->write_end);
+			printk("status = %ld, copied = %lu\n", status, copied);
+			dump_stack();
+			status = copied; /* try to fix */
+		}
+
 		copied = status;
 
 		cond_resched();

Re: BUG at mm/filemap.c:1749 (2.6.24, jffs2, unionfs)

[Nick Piggin]

[-- Attachment #1: Type: text/plain, Size: 2095 bytes --]

On Friday 19 October 2007 17:03, Nick Piggin wrote:
> On Friday 19 October 2007 16:05, Erez Zadok wrote:
> > David,
> >
> > I'm testing unionfs on top of jffs2, using 2.6.24 as of linus's commit
> > 4fa4d23fa20de67df919030c1216295664866ad7.  All of my unionfs tests pass
> > when unionfs is stacked on top of jffs2, other than my truncate test --
> > whic tries to truncate files up/down (through the union, which then is
> > passed through to the lower jffs2 f/s).  The same truncate test passes on
> > all other file systems I've tried unionfs/2.6.24 with, as well as all of
> > the earlier kernels that unionfs runs on (2.6.9--2.6.23).  So I tend to
> > think this bug is more probably due to something else going on in 2.6.24,
> > possibly wrt jffs2/mtd.  (Of course, it's still possible that unionfs
> > isn't doing something right -- any pointers?)
> >
> > The oops trace is included below.  Is this a known issue and if so, any
> > fixes?  If this is the first you hear of this problem, let me know and
> > I'll try to narrow it down further.
>
> It's had quite a lot of recent changes in that area -- the "new aops"
> patches.
>
> They've been getting quite a bit of testing in -mm and no such problems,
> but I doubt anyone was doing much unionfs over jffs2, or even much jffs2
> testing with -mm.
>
> The bug smells like jffs2 is actually passing back a "written" length
> greater than the length we passed into it.
>
> The following might show what's happening.

Hmm, looks like jffs2_write_end is writing more than we actually ask it
to, and returns that back.

        unsigned aligned_start = start & ~3;

and

        if (end == PAGE_CACHE_SIZE) {
                /* When writing out the end of a page, write out the
                   _whole_ page. This helps to reduce the number of
                   nodes in files which have many short writes, like
                   syslog files. */
                start = aligned_start = 0;
        }

These "longer" writes are fine, but they shouldn't get propagated back
to the vm/vfs. Something like the following patch might fix it.


[-- Attachment #2: jffs2-writtenlen-fix.patch --]
[-- Type: text/x-diff, Size: 1136 bytes --]

Index: linux-2.6/fs/jffs2/file.c
===================================================================
--- linux-2.6.orig/fs/jffs2/file.c
+++ linux-2.6/fs/jffs2/file.c
@@ -255,7 +255,7 @@ static int jffs2_write_end(struct file *
 		   _whole_ page. This helps to reduce the number of
 		   nodes in files which have many short writes, like
 		   syslog files. */
-		start = aligned_start = 0;
+		aligned_start = 0;
 	}
 
 	ri = jffs2_alloc_raw_inode();
@@ -291,14 +291,11 @@ static int jffs2_write_end(struct file *
 	}
 
 	/* Adjust writtenlen for the padding we did, so we don't confuse our caller */
-	if (writtenlen < (start&3))
-		writtenlen = 0;
-	else
-		writtenlen -= (start&3);
+	writtenlen -= min(writtenlen, (start - aligned_start));
 
 	if (writtenlen) {
-		if (inode->i_size < (pg->index << PAGE_CACHE_SHIFT) + start + writtenlen) {
-			inode->i_size = (pg->index << PAGE_CACHE_SHIFT) + start + writtenlen;
+		if (inode->i_size < pos + start + writtenlen) {
+			inode->i_size = pos + start + writtenlen;
 			inode->i_blocks = (inode->i_size + 511) >> 9;
 
 			inode->i_ctime = inode->i_mtime = ITIME(je32_to_cpu(ri->ctime));

Re: BUG at mm/filemap.c:1749 (2.6.24, jffs2, unionfs)

[Erez Zadok]

In message <200710191716.53470.nickpiggin@yahoo.com.au>, Nick Piggin writes:
[...]
> Hmm, looks like jffs2_write_end is writing more than we actually ask it
> to, and returns that back.
> 
>         unsigned aligned_start = start & ~3;
> 
> and
> 
>         if (end == PAGE_CACHE_SIZE) {
>                 /* When writing out the end of a page, write out the
>                    _whole_ page. This helps to reduce the number of
>                    nodes in files which have many short writes, like
>                    syslog files. */
>                 start = aligned_start = 0;
>         }
> 
> These "longer" writes are fine, but they shouldn't get propagated back
> to the vm/vfs. Something like the following patch might fix it.
> 
> 
> --Boundary-00=_lnFGHwOggSRGKPd
> Content-Type: text/x-diff;
>   charset="utf-8";
>   name="jffs2-writtenlen-fix.patch"
> Content-Transfer-Encoding: 7bit
> Content-Disposition: attachment;
> 	filename="jffs2-writtenlen-fix.patch"

Nick, the patch worked.  All of my unionfs-over-jffs2 tests passed.

Thanks,
Erez.

Re: BUG at mm/filemap.c:1749 (2.6.24, jffs2, unionfs)

[David Woodhouse]

On Fri, 2007-10-19 at 13:38 -0400, Erez Zadok wrote:
> Nick, the patch worked.  All of my unionfs-over-jffs2 tests passed.

Can I have a Signed-off-by: for it please?

-- 
dwmw2

Re: BUG at mm/filemap.c:1749 (2.6.24, jffs2, unionfs)

[David Woodhouse]

On Fri, 2007-10-19 at 17:16 +1000, Nick Piggin wrote:
> 
>         if (writtenlen) {
> -               if (inode->i_size < (pg->index << PAGE_CACHE_SHIFT) + start + writtenlen) {
> -                       inode->i_size = (pg->index << PAGE_CACHE_SHIFT) + start + writtenlen;
> +               if (inode->i_size < pos + start + writtenlen) {
> +                       inode->i_size = pos + start + writtenlen;

This part seems wrong. Shouldn't it just be pos + writtenlen -- which is
basically what it was already: pos==(pg->index<<PAGE_CACHE_SHIFT)+start

-- 
dwmw2

Re: BUG at mm/filemap.c:1749 (2.6.24, jffs2, unionfs)

[Nick Piggin]

On Sunday 21 October 2007 18:55, David Woodhouse wrote:
> On Fri, 2007-10-19 at 17:16 +1000, Nick Piggin wrote:
> >         if (writtenlen) {
> > -               if (inode->i_size < (pg->index << PAGE_CACHE_SHIFT) +
> > start + writtenlen) { -                       inode->i_size = (pg->index
> > << PAGE_CACHE_SHIFT) + start + writtenlen; +               if
> > (inode->i_size < pos + start + writtenlen) {
> > +                       inode->i_size = pos + start + writtenlen;
>
> This part seems wrong. Shouldn't it just be pos + writtenlen -- which is
> basically what it was already: pos==(pg->index<<PAGE_CACHE_SHIFT)+start

Yeah you're right, thanks.