On 08/26/2011 01:39 PM, J. Bruce Fields wrote: > On Fri, Aug 12, 2011 at 05:12:20PM -0700, Boaz Harrosh wrote: >> >> locks_in_grace() was called twice one for the "yes" case second >> for the "no" case. If the status changes between these two calls >> the Server would do the wrong thing. Sample it only once. > > I don't see how this fixes any bug. The only thing that could happen in > between those two checks is that the grace period could end. In that > case op_claim_type must be NFS4_OPEN_CLAIM_PREVIOUS (otherwise we would > have jumped to out and not hit the second case). The second condition > will therefore be true and we'll fail with err_no_grace. I don't see > that is incorrect, as in fact the grace period is now over. OK You are right it is not NFS incorrect. But I still think a single sample is better coding practice. no? Though I agree that the commit log wording should be changed. What about the other part of the patch? The print. Should I send a separate patch for that? > > There may be a different bug: if the grace period ends *any time* after > locks_in_grace() is called but before we actually do the lock or open, > then a reclaim could be incorrectly granted. The state lock prevents > this happening between two nfsv4 clients, but it could happen between > a v4 client and a lockd client, I think. In more detail: > > > lockd client NFSv4 client > ------------ ------------ > > reclaim request passes grace check > -- grace period ends -- > gets conflicting lock > drops conflicting lock > reclaim request granted > > One fix might be to take some sort of reference count as long as you're > processing a reclaim request, and not end the grace period till that > count goes to zero. > > Another might be push the grace checks down into the core lock code and > make sure there's a lock that provides mutual exclusion between the > locks_end_grace() call and lock reclaims. > You might get by, by rechecking the grace period at the end of the processing and if passed issue a "reclaim request failed" anyway. So the first check is only for optimization but the final disposition is the post-check. (Just as if you dropped that refcount above) Boaz > --b. > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
