On 2011-03-18, at 6:07 PM, Darrick J. Wong wrote:
>> Ok, here's what I have so far. I took everyone's suggestions of where to add
>> calls to wait_on_page_writeback, which seems to handle the multiple-write case
>> adequately. Unfortunately, it is still possible to generate checksum errors by
>> scribbling furiously on a mmap'd region, even after adding the writeback wait
>> in the ext4 writepage function. Oddly, I couldn't break btrfs with mmap by
>> removing its wait_for_page_writeback call, so I suspect there's a bit more
>> going on in btrfs than I've been able to figure out.
> I wonder, is it possible for this to happen:
>
> 1. Thread A mmaps a page and tries to write to it. ext4_page_mkwrite executes,
> but there's no ongoing writeback, so it returns without delay.
> 2. Thread A starts writing furiously to the page.
> 3. Thread B runs fsync() or something that results in the page being
> checksummed and scheduled for writeout.
> 4. Thread A continues to write furiously(!) on that same page before the
> controller finishes the DMA transfer.
Right, page_mkwrite() is only called for the ro->rw transition.
> 5. Disk gets the page, which now doesn't match its checksum, and *boom*
>
> After letting the stress tool run for a few days, I can say fairly confidently
> that the write() case doesn't seem to fail regardless of the O_DIRECT setting.
> However, with writes to mmap regions, failures happen about once every 20-40
> minutes, even with O_DIRECT set. To me this suggests some sort of race
> condition that we seem to win except once every 20 minutes.
>
> I then thought, if page_mkwrite contains a wait_on_page_writeback, then perhaps
> there's something that I could do just prior to calculating the DIF checksum
> that would cause any subsequent write attempts to be shuffled back into
> page_mkwrite. I tried the set_memory_ro thing again, though that led to some
> recursive lock errors and I noticed that those functions only seem to exist in
> arch/x86/. Next I tried directly mucking with PTEs, in addition to feeling
> messy, only seemed to corrupt memory. :)
This seems like the best solution, IMHO, to ensure that mmap is blocked in page_mkwrite() before it has any chance to dirty the page undergoing checksum. The trick is that you need to set_page_writeback() before setting the page read-only, otherwise the race still exists.
> Is there a "correct" way to take a writeable page and make it so that any
> process trying to write to it ends up hitting the page fault handler where we
> can then wait for writeback? Or perhaps I am simply barking up the wrong tree?
>
> (Just FYI I took the old copy-everything-to-bounce-buffers patch that few
> people liked for a second spin, and the errors did not surface regardless of
> what combination of write/mmap and directio/bufferedio I told it to use.)
I wouldn't be so much against memcpy() for mmap pages, but it does seem kind of gross that mmap is forcing data copies when a major reason to use mmap is to AVOID data copies.
>>> The set_memory_ro debugging trick didn't ferret out any write paths that I
>>> didn't catch... though it did have the effect of causing occasional fsync()
>>> deadlocks. I suppose I could sprinkle in a few more of those write calls to
>>> see what happens.
>>>
>>> Either way, I'm emailing to ask everyone's advice since I've run out of ideas.
>>> Or: Did I miss something?
>>>
>>> Thanks all for the feedback so far!
>>>
>>> --
>>> fs: Wait for page writeback when rewrite detected
>>>
>>> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
>>> ---
>>>
>>> fs/buffer.c | 4 +++-
>>> fs/ext4/inode.c | 3 +++
>>> mm/filemap.c | 15 +++++++++++++--
>>> 3 files changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/buffer.c b/fs/buffer.c
>>> index 2219a76..39e934c 100644
>>> --- a/fs/buffer.c
>>> +++ b/fs/buffer.c
>>> @@ -2379,8 +2379,10 @@ block_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf,
>>> ret = VM_FAULT_OOM;
>>> else /* -ENOSPC, -EIO, etc */
>>> ret = VM_FAULT_SIGBUS;
>>> - } else
>>> + } else {
>>> + wait_on_page_writeback(page);
>>> ret = VM_FAULT_LOCKED;
>>> + }
>>
>> I think this needs to wait before the __block_write_begin() call,
>> not after it. i.e. wait before the page is mapped, not afterwards.
>>
>> ....
>>> diff --git a/mm/filemap.c b/mm/filemap.c
>>> index 83a45d3..f201d80 100644
>>> --- a/mm/filemap.c
>>> +++ b/mm/filemap.c
>>> @@ -2217,8 +2217,8 @@ EXPORT_SYMBOL(generic_file_direct_write);
>>> * Find or create a page at the given pagecache position. Return the locked
>>> * page. This function is specifically for buffered writes.
>>> */
>>> -struct page *grab_cache_page_write_begin(struct address_space *mapping,
>>> - pgoff_t index, unsigned flags)
>>> +struct page *__grab_cache_page_write_begin(struct address_space *mapping,
>>> + pgoff_t index, unsigned flags)
>>> {
>>> int status;
>>> struct page *page;
>>> @@ -2243,6 +2243,17 @@ repeat:
>>> }
>>> return page;
>>> }
>>> +struct page *grab_cache_page_write_begin(struct address_space *mapping,
>>> + pgoff_t index, unsigned flags)
>>> +{
>>> + struct page *p;
>>> +
>>> + p = __grab_cache_page_write_begin(mapping, index, flags);
>>> + if (p)
>>> + wait_on_page_writeback(p);
>>> +
>>> + return p;
>>> +}
>>> EXPORT_SYMBOL(grab_cache_page_write_begin);
>>
>> Not much point in add in a wrapper when nothing else calls
>> __grab_cache_page_write_begin(), which should also be static....
>>
>> Cheers,
>>
>> Dave.
>> --
>> Dave Chinner
>> david@xxxxxxxxxxxxx
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers, Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
