[linux-cifs-client] Re: [RFC] patch to add support for leases to cifs

[J. Bruce Fields]

On Wed, Oct 22, 2008 at 03:31:06PM -0500, Steve French wrote:
> Oplock is a guarantee that no one else is changing (or reading) a
> file.  The notification that a file is now changing is different (that
> is SMB change notify), Oplock is a guarantee that you are the only
> writer (or reader/writer) of the file - so if the server wants to
> revoke that (oplock break) - and with the patch we call break_lease
> and don't return on the oplock break until break_lease returns,  and
> __break_lease in fs/locks.c looks synchronous - break_lease async?

Ah!  Yes, you're right.  nfsd always calls break_lease with
O_NONBLOCK....

Makes sense to me, then.

> By the way, the server also gives up on the client if it does not
> respond (but this is a long time - more than 20 seconds for most
> servers) - presumably by flushing the dirty pages, the issues the
> oplock break response.

So it's a little irritating that the server's timeout may not agree with
the local timeout set by that sysctl.  I suppose the lease timeout
should really be a characteristic of the filesystem rather than a global
thing.

--b.

> 
> Following is an updated patch to add support for mount option to
> disable the check for oplock (since not all server support oplock):
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index c6aad77..76919c2 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -618,6 +618,37 @@ static loff_t cifs_llseek(struct file *file,
> loff_t offset, int origin)
>  	return generic_file_llseek_unlocked(file, offset, origin);
>  }
> 
> +#ifdef CONFIG_CIFS_EXPERIMENTAL
> +static int cifs_setlease(struct file *file, long arg, struct file_lock **lease)
> +{
> +	/* note that this is called by vfs setlease with the BKL held
> +	   although I doubt that BKL is needed here in cifs */
> +	struct inode *inode = file->f_path.dentry->d_inode;
> +
> +	if (!(S_ISREG(inode->i_mode)))
> +		return -EINVAL;
> +
> +	/* check if file is oplocked */
> +	if (((arg == F_RDLCK) &&
> +		(CIFS_I(inode)->clientCanCacheRead)) ||
> +	    ((arg == F_WRLCK) &&
> +		(CIFS_I(inode)->clientCanCacheAll)))
> +		return generic_setlease(file, arg, lease);
> +	else if (CIFS_SB(inode->i_sb)->tcon->local_lease &&
> +			!CIFS_I(inode)->clientCanCacheRead)
> +		/* If the server claims to support oplock on this
> +		   file, then we still need to check oplock even
> +		   if the local_lease mount option is set, but there
> +		   are servers which do not support oplock for which
> +		   this mount option may be useful if the user
> +		   knows that the file won't be changed on the server
> +		   by anyone else */
> +		return generic_setlease(file, arg, lease);
> +	else
> +		return -EAGAIN;
> +}
> +#endif
> +
>  struct file_system_type cifs_fs_type = {
>  	.owner = THIS_MODULE,
>  	.name = "cifs",
> @@ -696,6 +727,7 @@ const struct file_operations cifs_file_ops = {
> 
>  #ifdef CONFIG_CIFS_EXPERIMENTAL
>  	.dir_notify = cifs_dir_notify,
> +	.setlease = cifs_setlease,
>  #endif /* CONFIG_CIFS_EXPERIMENTAL */
>  };
> 
> @@ -716,6 +748,7 @@ const struct file_operations cifs_file_direct_ops = {
>  	.llseek = cifs_llseek,
>  #ifdef CONFIG_CIFS_EXPERIMENTAL
>  	.dir_notify = cifs_dir_notify,
> +	.setlease = cifs_setlease,
>  #endif /* CONFIG_CIFS_EXPERIMENTAL */
>  };
>  const struct file_operations cifs_file_nobrl_ops = {
> @@ -736,6 +769,7 @@ const struct file_operations cifs_file_nobrl_ops = {
> 
>  #ifdef CONFIG_CIFS_EXPERIMENTAL
>  	.dir_notify = cifs_dir_notify,
> +	.setlease = cifs_setlease,
>  #endif /* CONFIG_CIFS_EXPERIMENTAL */
>  };
> 
> @@ -755,6 +789,7 @@ const struct file_operations cifs_file_direct_nobrl_ops = {
>  	.llseek = cifs_llseek,
>  #ifdef CONFIG_CIFS_EXPERIMENTAL
>  	.dir_notify = cifs_dir_notify,
> +	.setlease = cifs_setlease,
>  #endif /* CONFIG_CIFS_EXPERIMENTAL */
>  };
> 
> @@ -946,6 +981,12 @@ static int cifs_oplock_thread(void *dummyarg)
>  				the call */
>  			/* mutex_lock(&inode->i_mutex);*/
>  			if (S_ISREG(inode->i_mode)) {
> +#ifdef CONFIG_CIFS_EXPERIMENTAL
> +				if (CIFS_I(inode)->clientCanCacheAll == 0)
> +					break_lease(inode, FMODE_READ);
> +				else if (CIFS_I(inode)->clientCanCacheRead == 0)
> +					break_lease(inode, FMODE_WRITE);
> +#endif
>  				rc = filemap_fdatawrite(inode->i_mapping);
>  				if (CIFS_I(inode)->clientCanCacheRead == 0) {
>  					waitrc = filemap_fdatawait(
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 178f733..c791e5b 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -285,6 +285,7 @@ struct cifsTconInfo {
>  	bool seal:1;      /* transport encryption for this mounted share */
>  	bool unix_ext:1;  /* if false disable Linux extensions to CIFS protocol
>  				for this mount even if server would support */
> +	bool local_lease:1; /* check leases (only) on local system not remote */
>  	/* BB add field for back pointer to sb struct(s)? */
>  };
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 1126f7a..17058c5 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -90,7 +90,8 @@ struct smb_vol {
>  	bool nocase:1;     /* request case insensitive filenames */
>  	bool nobrl:1;      /* disable sending byte range locks to srv */
>  	bool seal:1;       /* request transport encryption on share */
> -	bool nodfs:1;
> +	bool nodfs:1;      /* Do not request DFS, even if available */
> +	bool local_lease:1; /* check leases only on local system, not remote */
>  	unsigned int rsize;
>  	unsigned int wsize;
>  	unsigned int sockopt;
> @@ -1264,6 +1265,8 @@ cifs_parse_mount_options(char *options, const
> char *devname,
>  			vol->no_psx_acl = 0;
>  		} else if (strnicmp(data, "noacl", 5) == 0) {
>  			vol->no_psx_acl = 1;
> +		} else if (strnicmp(data, "locallease", 6) == 0) {
> +			vol->local_lease = 1;
>  		} else if (strnicmp(data, "sign", 4) == 0) {
>  			vol->secFlg |= CIFSSEC_MUST_SIGN;
>  		} else if (strnicmp(data, "seal", 4) == 0) {
> @@ -2162,6 +2165,7 @@ cifs_mount(struct super_block *sb, struct
> cifs_sb_info *cifs_sb,
>  			   for the retry flag is used */
>  			tcon->retry = volume_info.retry;
>  			tcon->nocase = volume_info.nocase;
> +			tcon->local_lease = volume_info.local_lease;
>  			if (tcon->seal != volume_info.seal)
>  				cERROR(1, ("transport encryption setting "
>  					   "conflicts with existing tid"));
> 
> On Wed, Oct 22, 2008 at 3:19 PM, J. Bruce Fields <bfields at fieldses.org> wrote:
> > What guarantees that no other client can modify the file as long as the
> > lease is held?
> >
> > It's not enough to break the lease as soon as you notice the file
> > changes--the holder of the lease has up to 45 seconds (configurable with
> > /proc/sys/fs/lease-break-time) to return the lease before the kernel
> > forcibly revokes it.
> >
> > --b.
> 
> 
> 
> -- 
> Thanks,
> 
> Steve