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
_______________________________________________ Pvfs2-developers mailing list [email protected] http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
