From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Layton Subject: Re: [PATCH 09/18] cifs: clean up sync_mid_result Date: Mon, 20 Dec 2010 08:04:09 -0500 Message-ID: <20101220080409.35112ed8@corrin.poochiereds.net> References: <1292598497-29796-1-git-send-email-jlayton@redhat.com> <1292598497-29796-10-git-send-email-jlayton@redhat.com> <4D0F3DAD.7080801@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Suresh Jayaraman Return-path: In-Reply-To: <4D0F3DAD.7080801-l3A5Bk7waGM@public.gmane.org> Sender: linux-cifs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: On Mon, 20 Dec 2010 16:57:41 +0530 Suresh Jayaraman 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 > > --- > > 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. > > - if (rc != -EHOSTDOWN) { > > - if (mid->midState == MID_RETRY_NEEDED) { > > - rc = -EAGAIN; > > - cFYI(1, "marking request for retry"); > > - } else { > > - rc = -EIO; > > + break; > > } > > + case MID_RETRY_NEEDED: > > + rc = -EAGAIN; > > + break; > > + default: > > + cERROR(1, "%s: invalid mid state mid=%d state=%d", __func__, > > + mid->mid, mid->midState); > > + rc = -EIO; > > } > > spin_unlock(&GlobalMid_Lock); > > > > -- Jeff Layton