From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suresh Jayaraman Subject: Re: [PATCH 09/18] cifs: clean up sync_mid_result Date: Mon, 20 Dec 2010 22:05:49 +0530 Message-ID: <4D0F85E5.50500@suse.de> References: <1292598497-29796-1-git-send-email-jlayton@redhat.com> <1292598497-29796-10-git-send-email-jlayton@redhat.com> <4D0F3DAD.7080801@suse.de> <20101220080409.35112ed8@corrin.poochiereds.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Cc: smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Jeff Layton Return-path: In-Reply-To: <20101220080409.35112ed8-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org> Sender: linux-cifs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: On 12/20/2010 06:34 PM, Jeff Layton wrote: > 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. > 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