On Wed, 25 Oct 2000, Alexander Viro wrote:
>
> Linus, what do you think about that? I can do the remaining filesystems
> and give it initial testing today.
Ok, looks reasonable, if not really pretty. I'd probably prefer
last_page = size >> PAGE_CACHE_SIZE;
last_page_size = size & (PAGE_CACHE_SIZE-1);
and then the three cases would be
if (index < last_page) {
full page case - ok.
}
if (index > last_page || !last_page_size) {
bad case, past the end
}
partial_page.
I see why you did it the way you did, but while it makes it really cheap
to test for "index past the end", it makes for less obvious calculations
in other places, I feel.
The alternative is to have an entirely different approach, where the page
cache itself only knows about the maximum page (in which case your current
"last_page" calculation is right on), and then anybody who needs to know
about partial pages needs to get THAT information from the inode.
I'd almost prefer the alternative approach. Especially as right now the
only real problem we're fighting is to make sure we never return an
invalid page - the sub-page offset really doesn't matter for those things,
and everybody who cares about the sub-page-information already obviously
has it.
Linus
On Thu, 26 Oct 2000, Linus Torvalds wrote:
> The alternative is to have an entirely different approach, where the page
> cache itself only knows about the maximum page (in which case your current
> "last_page" calculation is right on), and then anybody who needs to know
> about partial pages needs to get THAT information from the inode.
>
> I'd almost prefer the alternative approach. Especially as right now the
> only real problem we're fighting is to make sure we never return an
> invalid page - the sub-page offset really doesn't matter for those things,
> and everybody who cares about the sub-page-information already obviously
> has it.
s/everybody/almost &/
There are only two places where we really care. And one of them can be
trivially shot.
* O_APPEND handling. Well, duh - we can take the main loop
of generic_file_write() into a separate function (do_generic_file_write())
and be done with that - grab the semaphore, possibly adjust the ->f_pos,
pass the actor to do_generic_file_write(), be happy.
* do_generic_file_read() should know how much to copy from the
last page. We could push that into the actor. Or we could mirror that
data in struct address_space.
But yes, 99% of cases care only about the index of last page. So
I really don't think that size>>PAGE_CACHE_SHIFT makes sense from VM point
of view.
OK, I have the "push to actor" variant and I'll send it once it
will get some testing. Changing it to "mirror both" is not a problem, anyway.