On Tue, Aug 19, 2014 at 09:54:14PM -0400, Chris Mason wrote: > On 08/19/2014 06:35 PM, Dave Chinner wrote: > > On Tue, Aug 19, 2014 at 03:24:48PM -0400, Chris Mason wrote: > >> On 08/11/2014 09:17 PM, Dave Chinner wrote: > >>> On Sat, Aug 09, 2014 at 08:57:00AM -0400, Chris Mason wrote: > >>>> > >>>> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > >>>> index 1f66779..023d575 100644 > >>>> --- a/fs/xfs/xfs_file.c > >>>> +++ b/fs/xfs/xfs_file.c > >>>> @@ -295,7 +295,8 @@ xfs_file_read_iter( > >>>> xfs_rw_iunlock(ip, XFS_IOLOCK_EXCL); > >>>> return ret; > >>>> } > >>>> - truncate_pagecache_range(VFS_I(ip), pos, -1); > >>>> + invalidate_inode_pages2_range(VFS_I(ip)->i_mapping, > >>>> + pos >> PAGE_CACHE_SHIFT, -1); > >>>> } > >>>> xfs_rw_ilock_demote(ip, XFS_IOLOCK_EXCL); > >>>> } > >>> > >>> I added the WARN_ON_ONCE(ret) check to this and I am seeing it fire > >>> occasionally. It always fires immediately before some other ASSERT() > >>> they fires with a block map/page cache inconsistency. It usually > >>> fires in a test that runs fsx or fsstress. The fsx failures are new > >>> regressions caused by this patch. e.g. generic/263 hasn't failed for > >>> months on any of my systems and this patch causes it to fail > >>> reliably on my 1k block size test config. > >>> > >>> I'm going to assume at this point that this is uncovering some other > >>> existing bug, but it means I'm not going to push this fix until I > >>> understand what is actually happening here. It is possible that what > >>> I'm seeing is related to Brian's collapse range bug fixes, but until > >>> I applied this direct IO patch I'd never seen fsx throw ASSERTs in > >>> xfs_bmap_shift_extents().... > >>> > >>> Either way, more testing and understanding is needed. > >> > >> Do you have the output from xfs and the command line args it used? For > >> my device, it picks: > >> > >> -r 4096 -t 512 -w 512 -Z > >> > >> And for a blocksize 1024 test I did mkfs.xfs -b size=1024 > > > > I'm running: > > > > $ mkfs.xfs -f -m crc=1,finobt=1 -b size=1k /dev/vda > > $ mount /dev/vda /mnt/test > > $ ltp/fsx -o 128000 -l 500000 -r 4096 -t 512 -w 512 -Z -d /mnt/test/foo > > > >> But I can't trigger failures with or without the invalidate_inode_pages2 > >> change. I was hoping to trigger on 3.16, and then jump back to 3.10 + > >> my patch to see if the patch alone was at fault. > > > > I am seeing failures at operation 1192. > > > > Yesterday, I found a new class of bufferhead state coherency issues > > to do with EOF handling that are causing the problems. Basically, > > when the page cache marks a page dirty, the generic code marks all > > the buffers on the page dirty, even when they are beyond EOF. > > > > As a result, when we go to invalidate the page that spans EOF, it > > cannot be invalidated because there are dirty buffers on the page. > > Those buffers persist in that state because they are beyond EOF, > > have no blocks allocated to them, and cannot be written. And so when > > we do a direct IO that spans the current EOF, it now fails to > > invalidate that page and so triggers the warning. > > > > Worse is that it appears that these bufferheads can leak into the > > internal blocks into the file when the file is extended, leading to > > all sorts of other ASSERT failures (which I've been seeing for a > > while now). > > > > I've got the above fsx command to run for somewhere between 100k and > > 110k operations with the fixes I currently have, but I haven't found > > the cause of the dirty buffer beyond EOF state leaking into the > > interior of the file from extend operations yet. > > > > Once I have something that passes a few million fsx ops.... > > I have to admit, I'm not sure where this leaves us in terms of safely > applying my patch to our 3.10 or mainline kernel... Failing to > invalidate the page and zeroing the page are really both wrong. Well, yes they are, and that's one of the reasons why I wanted to ensure we caught failures. As such, I think this invalidation bug goes back a *long time* because we've never, ever checked for invalidation failure before this patch. However, I think I've got a self-contained fix that can be backported for the invalidation problem now - it's well past 10 million fsx ops at this point with both read and write DIO invalidation fixes included. I'll post my series when I've done some more robust testing on it.... > It feels like this zeroing and cleaning should be happening in truncate, > but that's because I always blame truncate for fsx bugs. The problem is the other way around - buffers beyond EOF should never be marked dirty because they can never be written and hence never be marked clean in a consistent and sane manner. > Totally unrelated, but I'll abuse this email thread anyway. With a pull > to today's xfstests so I can send my shiny new test, ./check -g auto now > only runs the btrfs tests and nothing else. Works just fine for me. Did you reformat you test dev to xfs first? Do you have stray env vars set? What does ./setup tell you about your config? And maybe you can bisect - it was only ~20 commits in the last push. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
