On 17 Oct 2017, at 14:19, Jeff Layton wrote: > On Tue, 2017-10-17 at 13:46 -0400, Benjamin Coddington wrote: >> On 17 Oct 2017, at 12:39, Jeff Layton wrote: >> >>> On Tue, 2017-10-17 at 09:55 -0400, Benjamin Coddington wrote: >>>> While running generic/089 on NFSv4.1, the following on-the-wire >>>> exchange >>>> occurs: >>>> >>>> Client Server >>>> ---------- ---------- >>>> OPEN (owner A) -> >>>> <- OPEN response: state A1 >>>> CLOSE (state A1)-> >>>> OPEN (owner A) -> >>>> <- CLOSE response: state A2 >>>> <- OPEN response: state A3 >>>> LOCK (state A3) -> >>>> <- LOCK response: NFS4_BAD_STATEID >>>> >>>> The server should not be returning state A3 in response to the second >>>> OPEN >>>> after processing the CLOSE and sending back state A2. The problem is >>>> that >>>> nfsd4_process_open2() is able to find an existing open state just >>>> after >>>> nfsd4_close() has incremented the state's sequence number but before >>>> calling unhash_ol_stateid(). >>>> >>>> Fix this by using the state's sc_type field to identify closed state, >>>> and >>>> protect the update of that field with st_mutex. >>>> nfsd4_find_existing_open() >>>> will return with the st_mutex held, so that the state will not >>>> transition >>>> between being looked up and then updated in nfsd4_process_open2(). >>>> >>>> Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx> >>>> --- >>>> fs/nfsd/nfs4state.c | 29 +++++++++++++++++++---------- >>>> 1 file changed, 19 insertions(+), 10 deletions(-) >>>> >>> >>> The problem is real, but I'm not thrilled with the fix. It seems more >>> lock thrashy... >> >> Really? I don't think I increased any call counts to spinlocks or mutex >> locks. The locking overhead should be equivalent. >> > > init_open_stateid will now end up taking fi_lock twice. Yes, that's true unless there's an existing state. > Also we now have to take the st_mutex in nfsd4_find_existing_open, just to > check sc_type. Neither of those are probably unreasonable, it's just > messier than I'd like. It is indeed messy.. no argument. I'll spin up your suggestion to unhash the stateid before updating and take it for a ride and let you know the results. Thanks for looking at this. Ben -- 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
