Date:	Fri, 18 Mar 2011 20:28:26 -0600
From:	Andreas Dilger <adilger@...ger.ca>
To:	djwong@...ibm.com
Cc:	Dave Chinner <david@...morbit.com>,
	Chris Mason <chris.mason@...cle.com>, Jan Kara <jack@...e.cz>,
	Joel Becker <jlbec@...lplan.org>,
	"Martin K. Petersen" <martin.petersen@...cle.com>,
	Jens Axboe <axboe@...nel.dk>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Mingming Cao <mcao@...ibm.com>,
	linux-scsi <linux-scsi@...r.kernel.org>
Subject: Re: [RFC] block integrity: Fix write after checksum calculation	problem

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@...ibm.com>
>>> ---
>>> 
>>> 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@...morbit.com
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@...r.kernel.org
>> 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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists