Re: [PATCH 09/18] cifs: clean up sync_mid_result

[Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>]

On 12/20/2010 06:34 PM, Jeff Layton wrote:
> On Mon, 20 Dec 2010 16:57:41 +0530
> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> 
>> On 12/17/2010 08:38 PM, Jeff Layton wrote:
>>> Make it use a switch statement based on the value of the midStatus. If
>>> the resp_buf is set, then MID_RESPONSE_RECEIVED is too.
>>>
>>> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  fs/cifs/transport.c |   35 ++++++++++++++++++-----------------
>>>  1 files changed, 18 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>>> index f65cdec..6abd144 100644
>>> --- a/fs/cifs/transport.c
>>> +++ b/fs/cifs/transport.c
>>> @@ -363,28 +363,29 @@ sync_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
>>>  {
>>>  	int rc = 0;
>>>  
>>> -	spin_lock(&GlobalMid_Lock);
>>> +	cFYI(1, "%s: cmd=%d mid=%d state=%d", __func__, mid->command,
>>> +		mid->mid, mid->midState);
>>>  
>>> -	if (mid->resp_buf) {
>>> +	spin_lock(&GlobalMid_Lock);
>>> +	switch (mid->midState) {
>>> +	case MID_RESPONSE_RECEIVED:
>>>  		spin_unlock(&GlobalMid_Lock);
>>>  		return rc;
>>> -	}
>>> -
>>> -	cERROR(1, "No response to cmd %d mid %d", mid->command, mid->mid);
>>> -	if (mid->midState == MID_REQUEST_SUBMITTED) {
>>> -		if (server->tcpStatus == CifsExiting)
>>> +	case MID_REQUEST_SUBMITTED:
>>> +		/* socket is going down, reject all calls */
>>> +		if (server->tcpStatus == CifsExiting) {
>>> +			cERROR(1, "%s: canceling mid=%d cmd=0x%x state=%d",
>>> +			       __func__, mid->mid, mid->command, mid->midState);
>>>  			rc = -EHOSTDOWN;
>>> -		else
>>> -			mid->midState = MID_RETRY_NEEDED;
>>> -	}
>>
>> When midState is MID_REQUEST_SUBMITTED and tcpStatus is not CifsExiting,
>> the old code used to set midState to MID_RETRY_NEEDED. This is not set
>> now - is this intentional?
>>
> 
> I don't think it matters much. wait_for_response does this:
> 
>         error = wait_event_killable(server->response_q,
>                                     midQ->midState != MID_REQUEST_SUBMITTED);
> 
> ...so you can only get out of that wait with MID_REQUEST_SUBMITTED by
> fatal signal. At that point, the caller is going to do send the cancel
> and return without calling handle_mid_result.
> 
> So TBH, I think that this MID_REQUEST_SUBMITTED clause is going to be
> unused, but we could reset the value to MID_RETRY_NEEDED if we think
> it'll be better for future-proofness.
> 

I don't think we'll need it. But, if Steve or others see any need, it's
fine to keep it as well.


-- 
Suresh Jayaraman