On Fri, Oct 18, 2019 at 5:21 PM David Wysochanski <dwysocha@xxxxxxxxxx> wrote: > > On Fri, Oct 18, 2019 at 4:59 PM Pavel Shilovskiy <pshilov@xxxxxxxxxxxxx> wrote: > > > > Thanks for the good news that the patch is stable in your workload! > > > The attached patch I ran on top of 5.4-rc3 for over 5 hrs today on the > reboot test - before it would crash after a few minutes tops. This is great! Thanks for verifying the fix. > > The extra flag may not be necessary and we may rely on a MID state but we would need to handle two states actually: MID_RETRY_NEEDED and MID_SHUTDOWN - see clean_demultiplex_info() which is doing the same things with mid as cifs_reconnect(). Please add ref counting to both functions since they both can race with system call threads. > > > I agree that loop has the same problem. I can add that you're ok with the mid_state approach. I think the only other option is probably a flag like Ronnie > suggested. > I will have to review the state machine more when I am more alert if you are concerned about possible subtle regressions. I am ok with both approaches as long as the stable patch is minimal. Thinking about this conditional assignment of the mid retry state: I don't think there is any case in the current code base where the WARN_ON you proposed would fire but I can't be sure about all possible stable kernel that the stable patch is going to be applied. Another more general thought: we have cifs_delete_mid -> DeleteMidQEntry -> _cifs_mid_q_entry_release chain of calls and every function frees its own part of the mid entry. I think we should merge the last two at least. It would allow us to guarantee that holding a reference to the mid means: 1) the mid itself is valid; 2) the mid response buffer is valid; 3) the mid is in a list if it is REQUEST_SUBMITTED, RETRY_NEEDED or SHUTDOWN and is not in a list if it is ALLOCATED, RESPONSE_RECEIVED, RESPONSE_MALFORMED or FREE; the release function should remove the mid from the list or warn appropriately depending on a state of the mid. The mid state and list location are changed only when the GlobalMid_Lock is held. In this case cifs_delete_mid is not needed too because all what it does will be done in the release function. I think this would allow to avoid all the problems discussed in this thread but looks too risky for stable. -- Best regards, Pavel Shilovsky
