xfs
[Top] [All Lists]

Re: [PATCH v2] xfs: don't zero partial page cache pages during O_DIRECT

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH v2] xfs: don't zero partial page cache pages during O_DIRECT
From: Chris Mason <clm@xxxxxx>
Date: Tue, 19 Aug 2014 21:54:14 -0400
Cc: <xfs@xxxxxxxxxxx>, Eric Sandeen <sandeen@xxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/simple; d=fb.com; h=message-id : date : from : mime-version : to : cc : subject : references : in-reply-to : content-type : content-transfer-encoding; s=facebook; bh=VA2Gdl1kRRmrwXob03jJ+pMu59Sgfb9/yjKVxp8cji8=; b=gwSdoFsSuOBd8FY1OUGwieq62HzwWXcEq/udgfZ+omcNCKC9bXaRzZ+OU13FB3/lHgCA O5F28lx7xK/JPDYD/AK/rCIb0w+xXcXBQCLhi5S2jGsC2G1P54rhScYxQsRHB+nvaoFQ jOIgIg55+lBveqX+eumD57vC99x+btac2ac=
In-reply-to: <20140819223514.GQ20518@dastard>
References: <53E4E03A.7050101@xxxxxx> <53E61A9C.4020807@xxxxxx> <20140812011743.GU20518@dastard> <53F3A480.6020000@xxxxxx> <20140819223514.GQ20518@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0

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.

It feels like this zeroing and cleaning should be happening in truncate,
but that's because I always blame truncate for fsx bugs.

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.

I think its safe to assume this doesn't happen for you, any hints on
what might make xfstests suddenly love btrfs this much?

-chris

<Prev in Thread] Current Thread [Next in Thread>