lkml.org 
[lkml]   [2007]   [Mar]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] fs: nobh_truncate_page() fix
    On Mon, 5 Mar 2007 13:43:03 -0800 (PST)
    Linus Torvalds <torvalds@linux-foundation.org> wrote:

    >
    >
    > On Mon, 5 Mar 2007, Dave Kleikamp wrote:
    > >
    > > This fixes a regression caused by 22c8ca78f20724676b6006232bf06cc3e9299539.
    > >
    > > nobh_prepare_write() no longer marks the page uptodate, so
    > > nobh_truncate_page() needs to do it.
    >
    > I'm not convinced...
    >
    > If the page wasn't up-to-date from before, it's *not* necessarily
    > up-to-date after the truncate either! So why do we have that at all?

    The thing about nobh mode is that because we have no buffer_heads, we can't
    track the uptodateness of sections of the page. Hence nobh pages are
    basically always uptodate. The only place where we can tolerate partial
    uptodateness is in between prepare_write and commit_write, where we omit
    the initialisation of the section of the page which the caller is writing
    to.

    Of course, this won't perform very well with 64k pages..

    > The same comment is true of "nobh_commit_write()" (which _does_ have the
    > SetPageUptodate() there).

    nobh_prepare_write brings uptodate the sections of the page (0->from) and
    (to->PAGE_CACHE_SIZE), and the nobh_prepare_write() caller brings the
    (from->to) section uptodate. So the page is uptodate at
    nobh_commit_write(). It has to be, because we don't know how to bring a
    non-uptodate nobh page uptodate apart from writing something to every byte
    in it.

    > So I have three questions:
    >
    > - why is that valid in the first place (the page is *not* guaranteed to
    > be up-to-date as far as I can see!)
    >
    > - why is it valid to do in "nobh_commit_write()"
    >
    > - why doesn't "nobh_truncate_page()"
    > (a) call nobh_prepare_write() through an indirect pointer?
    > (b) call nobh_commit_write() at all? (Yeah, I realize it's because
    > of brokenness with i_size, so this is more of a "those
    > functions should be factored out properly" statement rather
    > than a question.

    It's not really appropriate that nobh_truncate_page() call
    ->prepare_write() at all. But it just happened that nobh_prepare_write()
    does exactly what nobh_truncate_page() wants to do, so I just called
    nobh_prepare_write() for code-sharing reasons.

    Perhaps I should have called nobh_prepare_write() directly, or created some
    common private function which both nobh_prepare_write() and
    nobh_truncate_page() internally call.

    > IOW, I'm sure your patch _fixes_ something, but no, it's certainly not
    > obvious to me. A few added comments would be good.. Why is it ok to do
    > this on a page that wasn't up-to-date before (since obviously, if it *was*
    > up-to-date, it's pointless).

    Is OK, I think. nobh_prepare_write() brings the outside-from-and-to
    sections of the page uptodate and memset in nobh_truncate_page() brings the
    rest of the page uptodate.

    We bring the to->PAGE_CACHE_SIZE section uptodate twice, which could be
    optimised.

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

    \
     
     \ /
      Last update: 2007-03-06 00:07    [W:0.026 / U:31.692 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site