On Sun, 17 Dec 2006 21:50:43 -0800 (PST)
Linus Torvalds <[EMAIL PROTECTED]> 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.
Yes, I can't see anything correct about the current behaviour.
But I'm going blue in the face here trying to feed try_to_free_buffers() a
page_mapped(page), without success. pagevec_strip() presumably isn't
triggering.
> 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..
>
> ...
>
> --- 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;
This will (at least) cause truncate to do peculiar things.
do_invalidatepage() runs discard_buffer() against the dirty page and will
then expect try_to_free_buffers() to remove those buffers and then clean
the page. truncate_complete_page() will clean the page, but it still has
those invalidated buffers. We'll end up with a large number of clean,
unused pages on the LRU, with attached buffers. These should eventually
get reaped, but it'll change the page aging dynamics.
-
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/
