Trond Myklebust wrote:
On Wed, 2008-06-18 at 16:40 -0400, Chuck Lever wrote:
No. If the page is being written out, then we _must_ release all locks,
wait on the request, and try again. This is not a change compared to the
previous incarnation.
Right, understood... but I think the logic added here is harder to
understand and thus more prone to break when something changes than the
previous incarnation was.
I don't understand: The behaviour and the results are exactly the same
as before. All that has happened is that the existing code to update a
request got shuffled into a separate function. If the request is locked,
so that we can't update it, we still wait for the request to become
unlocked, then release it and try the lookup again. There is no new
logic...
Sorry, I'm not being clear. I do realize that this patch is intended as
a clean up and not a behavioral change.
What I'm trying to say is the new function with the EAGAIN return code
is spaghetti. Don't get me wrong, I like Italian food.
I do agree that nfs_update_request() in its current state deserves some
clarification and clean up. I'm not sure I like the new code any more
than the old, however.
One alternative might be to move the code that waits on the lock into
nfs_try_to_update_request(), but I don't like that idea for two reasons:
1. waiting on the lock isn't related to updating the request
2. Once the request has been unlocked, you _still_ have to return
to nfs_setup_write_request() because the old request might have
completed, and been removed from the inode altogether. IOW: you
still don't get rid of the need to signal the EAGAIN.
begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard
