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;
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
