On Fri, Oct 18, 2019 at 6:45 PM Pavel Shilovskiy <pshilov@xxxxxxxxxxxxx> wrote: > > 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. > Right but look at it this way. If we conditionally set the state, then what is preventing a duplicate list_del_init call? Let's say we get into the special case that you're not sure it could happen (mid_entry->mid_state == MID_REQUEST_SUBMITTED is false), and so the mid_state does not get set to MID_RETRY_NEEDED inside cifs_reconnect but yet the mid gets added to retry_list. In that case both the cifs_reconnect code path will call list_del_init as well as the other code paths which we're adding the conditional tests and that will cause a blowup again because cifs_reconnect retry_list loop will end up in a singleton list and exhaust the refcount, leading to the same crash. This is exactly why the refcount only patch crashed again - it's erroneous to think it's ok to modify mid_entry->qhead without a) taking globalMid_Lock and b) checking mid_state is what you think it should be. But if you're really concerned about that 'if' condition and want to leave it, and you want a stable patch, then the extra flag seems like the way to go. But that has the downside that it's only being done for stable, so a later patch will likely remove it (presumably). I am not sure what such policy is or if that is even acceptable or allowed. > 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
