Re: [PATCH] fs: nobh_truncate_page() fix

[Andrew Morton]

On Mon, 5 Mar 2007 13:43:03 -0800 (PST)
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/