Re: [linux-cifs-client] [try3][patch] cifs: implement hard mount option behaviour

[Suresh Jayaraman <sjayaraman@suse.de>]

On 05/26/2010 07:43 AM, shirishpargaonkar@gmail.com wrote:
> From 13bc9a6e9808c6a12c26ef25ead8a70210e75bcc Mon Sep 17 00:00:00 2001
> From: Shirish Pargaonkar <shirishpargaonkar@gmail.com>
> Date: Tue, 25 May 2010 21:07:24 -0500
> Subject: [PATCH] add support for implementing hard mount option behaviour

First of, I think the hard mounts behavior for cifs is not well defined
and documented. Along with a descriptive changelog that explains the
past and the behavior this patch introduces would help.
For e.g. whether the requests are retried indefinitely or not (nfs
usually retries indefinitely) or have a timeout, are they interruptible
or not, are there specific calls which are always soft/hard etc.

I think the mount.cifs man page section explaining 'hard' option also
badly needs an update (possible mentioning when hard is
recommended/useful). In current form, I think it is not clear when and
why one should use soft/hard mounts.

> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>
> ---
>  fs/cifs/cifsproto.h |    5 ++-
>  fs/cifs/cifssmb.c   |   99 ++++++++++++++++++++++++++------------------------
>  fs/cifs/connect.c   |    5 ++-
>  fs/cifs/sess.c      |    2 +-
>  fs/cifs/transport.c |   60 ++++++++++++++++++++++++++++---
>  5 files changed, 113 insertions(+), 58 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 2208f06..7ddc602 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1809,6 +1809,9 @@ cifs_get_tcon(struct cifsSesInfo *ses, struct smb_vol *volume_info)
>  		}
>  	}
>  
> +	if (volume_info->retry)
> +		tcon->retry = volume_info->retry;
> +
>  	if (strchr(volume_info->UNC + 3, '\\') == NULL
>  	    && strchr(volume_info->UNC + 3, '/') == NULL) {
>  		cERROR(1, "Missing share name");

> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 82f78c4..eb76d1a 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -311,6 +311,44 @@ static int allocate_mid(struct cifsSesInfo *ses, struct smb_hdr *in_buf,
>  	return 0;
>  }
>  
> +static int
> +wait_for_response_hard(struct cifsSesInfo *ses, struct mid_q_entry *midQ)
> +{
> +	int rc;
> +	bool cmdresp = true;
> +	unsigned long timeout = 60 * HZ;
> +	unsigned long curr_timeout;
> +
> +wait_again:
> +	curr_timeout = timeout + jiffies;
> +	rc = wait_event_interruptible_timeout(ses->server->response_q,
> +			(midQ->midState != MID_REQUEST_SUBMITTED), timeout);
> +
> +	if (rc < 0) {

IIRC, the earlier post checked for only -ERESTARTSYS. Just curious what
caused this change and why the earlier check was not sufficient?

> +		cFYI(1, "command 0x%x interrupted", midQ->command);
> +		return -1;
> +	}
> +	if (!time_before(jiffies, curr_timeout)) {
> +		cmdresp = false;
> +		cFYI(1, "server not responding...");
> +		goto wait_again;
> +	}
> +	spin_lock(&GlobalMid_Lock);

Do we really need to acquire the lock here?

> +	if (midQ->midState != MID_REQUEST_SUBMITTED) {
> +		if (midQ->midState == MID_RESPONSE_RECEIVED) {
> +			if (!cmdresp)
> +				cFYI(1, "server is ok...");

The cFYI message could be little more useful..

> +			rc = 0;
> +		} else {
> +			cFYI(1, "command 0x%x aborted", midQ->command);
> +			rc = -1;
> +		}
> +	}
> +	spin_unlock(&GlobalMid_Lock);
> +	return rc;
> +}
> +
> +

Thanks,

-- 
Suresh Jayaraman