[Pvfs2-developers] Review: experimental readcaching code

[Murali Vilayannur]

Hi Pete,
Thanks for the reviews!

>
> Names COPY_TO_ADDRESSES and rw->copy_to_user are confusing, as those
> are also checked in the "from" path.  Maybe COPY_USER_ADDRESSES and
> rw->copy_user_buffers might help future readers.


Hmm. So I was using COPY_TO_ADDRESSES and COPY_TO_PAGES as indicative of the
destination
addresses, i.e. they can be pages or plain addresses. If they are pages, it
does not  matter what value ->copy_to_user
is set to, but if they are addresses we need to know if they are kernel
virtual addresses or user virtual addresses.
I will replace those with the names you suggest.

Your pagecache_read_actor() isn't really an actor, i.e. it's not a
> callback from some other function.  Just needs a different name,
> like "copy_pagecache_to_user".


heh.. indeed. Sorry about that. I was thinking about calling the kernel's
pagecache read actor function
and I forgot to rename it when I realized that it was not exported to
modules.. :)
I will change the name.

Use of pvfs2_readpages_fill_cb is interesting, and probably wants
> a comment.  Normally this function issues IO, but you just
> gather up all the would-be-issued IOs to ship later as a big chunk,
> for performance reasons.


Okay. Will add a comment.

The error handling may not do the right thing.  If you get a read
> error on your issue_pages, you go through and error out the entire
> page_list, even though the previously cached ones are probably fine.


So, the only ones that are in the page_list are the ones that were not in
the page-cache and were newly allocated/added
to the page-cache. So we want those pages to be marked as erroneous in case
another I/O happens to hit on those pages, no?

Little stuff:  no need to cast return value of kmalloc().  There's
> also kzalloc() if you're going to do a memset(,0,) next.


Is kzalloc() available on older kernels or should I add a configure time
option for that?
Will change this too..

I don't understand how you can do write buffering safely.  There's
> no equivalent to IS_IMMUTABLE(), is there?  That seems like a scary
> next step, although I understand some people do want it.


Yep. this is the question I am grappling with.
Even with close-to-open semantics, write buffering seems like it will be
prone to correctness issues.
There is no equivalent to IS_IMMUTABLE, but we could do it for IS_CTOO or
something like that to go through the write buffering
path.
In order to do write buffering efficiently, we should try to avoid doing a
read before we modify the page and also keep track of the dirty
ranges, but that seems like a hard thing to do from a module without vfs
changes. Aggregating writebacks for efficiency is also
an issue, but I suspect we could look at the ->writepages callback and see
if that will work for us.
Maybe I am not thinking through this correctly. If you have any suggestions,
do let me know.
thanks again for the detailed comments..
BTW: Do not try the patch just yet. I have done zero testing on it and it
might crash your machine.
Murali

                -- Pete
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://www.beowulf-underground.org/pipermail/pvfs2-developers/attachments/20061101/5f039d0d/attachment.htm