Re: 2.6.19 file content corruption on ext3

[Andrei Popa <andrei.popa@i-neo.ro>]

I tried latest git with the patch from this email and it still get file
content corruption. If I can help you further debug the problem tell me
what to do.

On Sun, 2006-12-17 at 21:50 -0800, Linus Torvalds wrote:
> 
> On Mon, 18 Dec 2006, Nick Piggin wrote:
> > 
> > I can't see how that's exactly a problem -- so long as the page does not
> > get reclaimed (it won't, because we have a ref on it) then all that matters
> > is that the page eventually gets marked dirty.
> 
> But the point being that "try_to_free_buffers()" marks it clean 
> AFTERWARDS.
> 
> So yes, the page gets marked dirty in the pte's - the hardware generally 
> does that for us, so we don't have to worry about that part going on.
> 
> But "try_to_free_buffers()" seems to clear those dirty bits without 
> serializing it really any way. It just says "ok, I will now clear them". 
> Without knowing whether the dirty bits got set before the IO that cleared 
> the buffer head dirty bits or not.
> 
> What is _that_ serialization? As far as I can see, the only way to 
> guarantee that to happen (since the dirty bits in the page tables will get 
> set without us ever even being notified) is that the page tables 
> themselves must simply never contain that page in a writable form at all.
> 
> And that seems to be lacking.
> 
> Anyway, I have what I consider a much simpler solution: just don't DO all 
> that crap in try_to_free_buffers() at all. I sent it out to some people 
> already, not not very widely. 
> 
> I reproduce my suggestion here for you (and maybe others too who weren't 
> cc'd in that other discussion group) to comment on..
> 
> 		Linus
> 
> ---
> 
> So I think your patch is really broken, how about this one instead?
> 
> It's really my previous patch, BUT it also adds a 
> 
> 	if (PageDirty(page) ..
> 		return 0;
> 
> case, on the assumption that since PageDirty() measn that one of the 
> buffers should be dirty, there's no point in even _trying_ drop_buffers, 
> since that should just fail anyway.
> 
> Now, that assumption is obviously wrong _if_ the buffers have been cleaned 
> by something else. So in that case, we now don't remove the buffer heads, 
> but who really cares? The page will remain on the dirty list, and 
> something should be trying to write it out, but since now all the buffers 
> are clean, once that happens, there is no actual IO to happen.
> 
> Hmm? So this means that we simply don't remove the buffers early from such 
> pages, but there shouldn't be any real downside.
> 
> Now, the only question would be if the page is marked dirty _while_ this 
> is running. We do hold the page lock, but page dirtying doesn't get the 
> lock, does it? But at least we won't mark the page _clean_ when it 
> shouldn't be.. And we still are atomic wrt the actual buffer lists 
> (mapping->private_lock), so I think this should all be ok, and 
> drop_buffers() will do the right thing.
> 
> So no race possible either.
> 
> At least as far as I can see. And the patch certainly is simple.
> 
> Now the question whether this actually _fixes_ any problems does remain, 
> but I think this should be a pretty good solution if the bug really is 
> here. Andrew?
> 
> 		Linus
> 
> ----
> diff --git a/fs/buffer.c b/fs/buffer.c
> index d1f1b54..263f88e 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -2834,7 +2834,7 @@ int try_to_free_buffers(struct page *page)
>  	int ret = 0;
>  
>  	BUG_ON(!PageLocked(page));
> -	if (PageWriteback(page))
> +	if (PageDirty(page) || PageWriteback(page))
>  		return 0;
>  
>  	if (mapping == NULL) {		/* can this still happen? */
> @@ -2845,22 +2845,6 @@ int try_to_free_buffers(struct page *page)
>  	spin_lock(&mapping->private_lock);
>  	ret = drop_buffers(page, &buffers_to_free);
>  	spin_unlock(&mapping->private_lock);
> -	if (ret) {
> -		/*
> -		 * If the filesystem writes its buffers by hand (eg ext3)
> -		 * then we can have clean buffers against a dirty page.  We
> -		 * clean the page here; otherwise later reattachment of buffers
> -		 * could encounter a non-uptodate page, which is unresolvable.
> -		 * This only applies in the rare case where try_to_free_buffers
> -		 * succeeds but the page is not freed.
> -		 *
> -		 * Also, during truncate, discard_buffer will have marked all
> -		 * the page's buffers clean.  We discover that here and clean
> -		 * the page also.
> -		 */
> -		if (test_clear_page_dirty(page))
> -			task_io_account_cancelled_write(PAGE_CACHE_SIZE);
> -	}
>  out:
>  	if (buffers_to_free) {
>  		struct buffer_head *bh = buffers_to_free;
>