[1/2] Add oplock_break to smb_version_operations

Commit Message

Sachin Prabhu March 7, 2014, 1:29 p.m. UTC

We need to add protocol specific calls to the oplock break thread.

Signed-off-by: Sachin Prabhu <[e-mail]>
---
 fs/cifs/cifsglob.h  |  3 +--
 fs/cifs/cifsproto.h |  2 ++
 fs/cifs/file.c      | 53 +++--------------------------------------------------
 fs/cifs/smb1ops.c   | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/smb2ops.c   | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 106 insertions(+), 52 deletions(-)

Comments

Jeff Layton March 10, 2014, 3:06 p.m. UTC | #1

On Fri,  7 Mar 2014 13:29:19 +0000
Sachin Prabhu <[e-mail]> wrote:

> We need to add protocol specific calls to the oplock break thread.
> 

I'm not necessarily doubting this, but why? AFAICT, the oplock_break
calls look more or less identical...

> Signed-off-by: Sachin Prabhu <[e-mail]>
> ---
>  fs/cifs/cifsglob.h  |  3 +--
>  fs/cifs/cifsproto.h |  2 ++
>  fs/cifs/file.c      | 53 +++--------------------------------------------------
>  fs/cifs/smb1ops.c   | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/cifs/smb2ops.c   | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 106 insertions(+), 52 deletions(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index cf32f03..93a8762 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -402,6 +402,7 @@ struct smb_version_operations {
>  			const struct cifs_fid *, u32 *);
>  	int (*set_acl)(struct cifs_ntsd *, __u32, struct inode *, const char *,
>  			int);
> +	void (*oplock_break)(struct work_struct *);
>  };
>  
>  struct smb_version_values {
> @@ -1557,8 +1558,6 @@ GLOBAL_EXTERN spinlock_t uidsidlock;
>  GLOBAL_EXTERN spinlock_t gidsidlock;
>  #endif /* CONFIG_CIFS_ACL */
>  
> -void cifs_oplock_break(struct work_struct *work);
> -
>  extern const struct slow_work_ops cifs_oplock_break_ops;
>  extern struct workqueue_struct *cifsiod_wq;
>  
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index acc4ee8..e4d0add 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -130,6 +130,8 @@ extern void cifs_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock);
>  extern int cifs_unlock_range(struct cifsFileInfo *cfile,
>  			     struct file_lock *flock, const unsigned int xid);
>  extern int cifs_push_mandatory_locks(struct cifsFileInfo *cfile);
> +extern int cifs_push_locks(struct cifsFileInfo *cfile);
> +bool cifs_has_mand_locks(struct cifsInodeInfo *cinode);
>  
>  extern struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid,
>  					      struct file *file,
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 53c1507..998dec7 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -251,7 +251,7 @@ out:
>  	return rc;
>  }
>  
> -static bool
> +bool
>  cifs_has_mand_locks(struct cifsInodeInfo *cinode)
>  {
>  	struct cifs_fid_locks *cur;
> @@ -304,7 +304,7 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
>  	cfile->f_flags = file->f_flags;
>  	cfile->invalidHandle = false;
>  	cfile->tlink = cifs_get_tlink(tlink);
> -	INIT_WORK(&cfile->oplock_break, cifs_oplock_break);
> +	INIT_WORK(&cfile->oplock_break, server->ops->oplock_break);
>  	mutex_init(&cfile->fh_mutex);
>  
>  	cifs_sb_active(inode->i_sb);
> @@ -1204,7 +1204,7 @@ err_out:
>  	goto out;
>  }
>  
> -static int
> +int
>  cifs_push_locks(struct cifsFileInfo *cfile)
>  {
>  	struct cifs_sb_info *cifs_sb = CIFS_SB(cfile->dentry->d_sb);
> @@ -3656,53 +3656,6 @@ static int cifs_launder_page(struct page *page)
>  	return rc;
>  }
>  
> -void cifs_oplock_break(struct work_struct *work)
> -{
> -	struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
> -						  oplock_break);
> -	struct inode *inode = cfile->dentry->d_inode;
> -	struct cifsInodeInfo *cinode = CIFS_I(inode);
> -	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> -	int rc = 0;
> -
> -	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
> -						cifs_has_mand_locks(cinode)) {
> -		cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
> -			 inode);
> -		cinode->oplock = 0;
> -	}
> -
> -	if (inode && S_ISREG(inode->i_mode)) {
> -		if (CIFS_CACHE_READ(cinode))
> -			break_lease(inode, O_RDONLY);
> -		else
> -			break_lease(inode, O_WRONLY);
> -		rc = filemap_fdatawrite(inode->i_mapping);
> -		if (!CIFS_CACHE_READ(cinode)) {
> -			rc = filemap_fdatawait(inode->i_mapping);
> -			mapping_set_error(inode->i_mapping, rc);
> -			cifs_invalidate_mapping(inode);
> -		}
> -		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
> -	}
> -
> -	rc = cifs_push_locks(cfile);
> -	if (rc)
> -		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> -
> -	/*
> -	 * releasing stale oplock after recent reconnect of smb session using
> -	 * a now incorrect file handle is not a data integrity issue but do
> -	 * not bother sending an oplock release if session to server still is
> -	 * disconnected since oplock already released by the server
> -	 */
> -	if (!cfile->oplock_break_cancelled) {
> -		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> -							     cinode);
> -		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> -	}
> -}
> -
>  /*
>   * The presence of cifs_direct_io() in the address space ops vector
>   * allowes open() O_DIRECT flags which would have failed otherwise.
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index 526fb89..346ee2a 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -23,6 +23,7 @@
>  #include "cifsproto.h"
>  #include "cifs_debug.h"
>  #include "cifspdu.h"
> +#include "cifsfs.h"
>  
>  /*
>   * An NT cancel request header looks just like the original request except:
> @@ -999,6 +1000,53 @@ cifs_is_read_op(__u32 oplock)
>  	return oplock == OPLOCK_READ;
>  }
>  
> +static void cifs_oplock_break(struct work_struct *work)
> +{
> +	struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
> +						  oplock_break);
> +	struct inode *inode = cfile->dentry->d_inode;
> +	struct cifsInodeInfo *cinode = CIFS_I(inode);
> +	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> +	int rc = 0;
> +
> +	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
> +						cifs_has_mand_locks(cinode)) {
> +		cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
> +			 inode);
> +		cinode->oplock = 0;
> +	}
> +
> +	if (inode && S_ISREG(inode->i_mode)) {
> +		if (CIFS_CACHE_READ(cinode))
> +			break_lease(inode, O_RDONLY);
> +		else
> +			break_lease(inode, O_WRONLY);
> +		rc = filemap_fdatawrite(inode->i_mapping);
> +		if (!CIFS_CACHE_READ(cinode)) {
> +			rc = filemap_fdatawait(inode->i_mapping);
> +			mapping_set_error(inode->i_mapping, rc);
> +			cifs_invalidate_mapping(inode);
> +		}
> +		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
> +	}
> +
> +	rc = cifs_push_locks(cfile);
> +	if (rc)
> +		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> +
> +	/*
> +	 * releasing stale oplock after recent reconnect of smb session using
> +	 * a now incorrect file handle is not a data integrity issue but do
> +	 * not bother sending an oplock release if session to server still is
> +	 * disconnected since oplock already released by the server
> +	 */
> +	if (!cfile->oplock_break_cancelled) {
> +		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> +							     cinode);
> +		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> +	}
> +}
> +
>  struct smb_version_operations smb1_operations = {
>  	.send_cancel = send_nt_cancel,
>  	.compare_fids = cifs_compare_fids,
> @@ -1067,6 +1115,7 @@ struct smb_version_operations smb1_operations = {
>  	.query_mf_symlink = cifs_query_mf_symlink,
>  	.create_mf_symlink = cifs_create_mf_symlink,
>  	.is_read_op = cifs_is_read_op,
> +	.oplock_break = cifs_oplock_break,
>  #ifdef CONFIG_CIFS_XATTR
>  	.query_all_EAs = CIFSSMBQAllEAs,
>  	.set_EA = CIFSSMBSetEA,
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 192f51a..e7081cd 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -27,6 +27,7 @@
>  #include "cifs_unicode.h"
>  #include "smb2status.h"
>  #include "smb2glob.h"
> +#include "cifsfs.h"
>  
>  static int
>  change_conf(struct TCP_Server_Info *server)
> @@ -904,6 +905,53 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
>  	return rc;
>  }
>  
> +static void smb2_oplock_break(struct work_struct *work)
> +{
> +	struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
> +						  oplock_break);
> +	struct inode *inode = cfile->dentry->d_inode;
> +	struct cifsInodeInfo *cinode = CIFS_I(inode);
> +	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> +	int rc = 0;
> +
> +	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
> +						cifs_has_mand_locks(cinode)) {
> +		cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
> +			 inode);
> +		cinode->oplock = 0;
> +	}
> +
> +	if (inode && S_ISREG(inode->i_mode)) {
> +		if (CIFS_CACHE_READ(cinode))
> +			break_lease(inode, O_RDONLY);
> +		else
> +			break_lease(inode, O_WRONLY);
> +		rc = filemap_fdatawrite(inode->i_mapping);
> +		if (!CIFS_CACHE_READ(cinode)) {
> +			rc = filemap_fdatawait(inode->i_mapping);
> +			mapping_set_error(inode->i_mapping, rc);
> +			cifs_invalidate_mapping(inode);
> +		}
> +		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
> +	}
> +
> +	rc = cifs_push_locks(cfile);
> +	if (rc)
> +		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> +
> +	/*
> +	 * releasing stale oplock after recent reconnect of smb session using
> +	 * a now incorrect file handle is not a data integrity issue but do
> +	 * not bother sending an oplock release if session to server still is
> +	 * disconnected since oplock already released by the server
> +	 */
> +	if (!cfile->oplock_break_cancelled) {
> +		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> +							     cinode);
> +		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> +	}
> +}
> +
>  static void
>  smb2_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
>  		      unsigned int epoch, bool *purge_cache)
> @@ -1163,6 +1211,7 @@ struct smb_version_operations smb20_operations = {
>  	.create_lease_buf = smb2_create_lease_buf,
>  	.parse_lease_buf = smb2_parse_lease_buf,
>  	.clone_range = smb2_clone_range,
> +	.oplock_break = smb2_oplock_break,
>  };
>  
>  struct smb_version_operations smb21_operations = {
> @@ -1237,6 +1286,7 @@ struct smb_version_operations smb21_operations = {
>  	.create_lease_buf = smb2_create_lease_buf,
>  	.parse_lease_buf = smb2_parse_lease_buf,
>  	.clone_range = smb2_clone_range,
> +	.oplock_break = smb2_oplock_break,
>  };
>  
>  struct smb_version_operations smb30_operations = {
> @@ -1314,6 +1364,7 @@ struct smb_version_operations smb30_operations = {
>  	.parse_lease_buf = smb3_parse_lease_buf,
>  	.clone_range = smb2_clone_range,
>  	.validate_negotiate = smb3_validate_negotiate,
> +	.oplock_break = smb2_oplock_break,
>  };
>  
>  struct smb_version_values smb20_values = {

Sachin Prabhu March 10, 2014, 4:21 p.m. UTC | #2

On Mon, 2014-03-10 at 11:06 -0400, Jeff Layton wrote:
> On Fri,  7 Mar 2014 13:29:19 +0000
> Sachin Prabhu <[e-mail]> wrote:
> 
> > We need to add protocol specific calls to the oplock break thread.
> > 
> 
> I'm not necessarily doubting this, but why? AFAICT, the oplock_break
> calls look more or less identical...

The oplock values for level 2 oplocks are different. For the 2nd fix to
work, we need to downgrade the oplock only once we are sure that no
writers are operating.

We need to downgrade oplocks. Since the constants used to describe
oplock states varies between cifs and smb2, we need to abstract out the
functions required to downgrade the oplocks. For this we need version
specific functions. This could either be the oplock_break function as a
whole or we could define a new function called downgrade_oplock() in
smb_version_operations. I have chosen to use version specific
oplock_break here. downgrade_oplock() could instead be used here which
would require fewer changes.

Should I implement this using a downgrade_oplock() function instead?

Sachin Prabhu

> 
> > Signed-off-by: Sachin Prabhu <[e-mail]>
> > ---
> >  fs/cifs/cifsglob.h  |  3 +--
> >  fs/cifs/cifsproto.h |  2 ++
> >  fs/cifs/file.c      | 53 +++--------------------------------------------------
> >  fs/cifs/smb1ops.c   | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/cifs/smb2ops.c   | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 106 insertions(+), 52 deletions(-)
> > 
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index cf32f03..93a8762 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -402,6 +402,7 @@ struct smb_version_operations {
> >  			const struct cifs_fid *, u32 *);
> >  	int (*set_acl)(struct cifs_ntsd *, __u32, struct inode *, const char *,
> >  			int);
> > +	void (*oplock_break)(struct work_struct *);
> >  };
> >  
> >  struct smb_version_values {
> > @@ -1557,8 +1558,6 @@ GLOBAL_EXTERN spinlock_t uidsidlock;
> >  GLOBAL_EXTERN spinlock_t gidsidlock;
> >  #endif /* CONFIG_CIFS_ACL */
> >  
> > -void cifs_oplock_break(struct work_struct *work);
> > -
> >  extern const struct slow_work_ops cifs_oplock_break_ops;
> >  extern struct workqueue_struct *cifsiod_wq;
> >  
> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> > index acc4ee8..e4d0add 100644
> > --- a/fs/cifs/cifsproto.h
> > +++ b/fs/cifs/cifsproto.h
> > @@ -130,6 +130,8 @@ extern void cifs_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock);
> >  extern int cifs_unlock_range(struct cifsFileInfo *cfile,
> >  			     struct file_lock *flock, const unsigned int xid);
> >  extern int cifs_push_mandatory_locks(struct cifsFileInfo *cfile);
> > +extern int cifs_push_locks(struct cifsFileInfo *cfile);
> > +bool cifs_has_mand_locks(struct cifsInodeInfo *cinode);
> >  
> >  extern struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid,
> >  					      struct file *file,
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index 53c1507..998dec7 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -251,7 +251,7 @@ out:
> >  	return rc;
> >  }
> >  
> > -static bool
> > +bool
> >  cifs_has_mand_locks(struct cifsInodeInfo *cinode)
> >  {
> >  	struct cifs_fid_locks *cur;
> > @@ -304,7 +304,7 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
> >  	cfile->f_flags = file->f_flags;
> >  	cfile->invalidHandle = false;
> >  	cfile->tlink = cifs_get_tlink(tlink);
> > -	INIT_WORK(&cfile->oplock_break, cifs_oplock_break);
> > +	INIT_WORK(&cfile->oplock_break, server->ops->oplock_break);
> >  	mutex_init(&cfile->fh_mutex);
> >  
> >  	cifs_sb_active(inode->i_sb);
> > @@ -1204,7 +1204,7 @@ err_out:
> >  	goto out;
> >  }
> >  
> > -static int
> > +int
> >  cifs_push_locks(struct cifsFileInfo *cfile)
> >  {
> >  	struct cifs_sb_info *cifs_sb = CIFS_SB(cfile->dentry->d_sb);
> > @@ -3656,53 +3656,6 @@ static int cifs_launder_page(struct page *page)
> >  	return rc;
> >  }
> >  
> > -void cifs_oplock_break(struct work_struct *work)
> > -{
> > -	struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
> > -						  oplock_break);
> > -	struct inode *inode = cfile->dentry->d_inode;
> > -	struct cifsInodeInfo *cinode = CIFS_I(inode);
> > -	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> > -	int rc = 0;
> > -
> > -	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
> > -						cifs_has_mand_locks(cinode)) {
> > -		cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
> > -			 inode);
> > -		cinode->oplock = 0;
> > -	}
> > -
> > -	if (inode && S_ISREG(inode->i_mode)) {
> > -		if (CIFS_CACHE_READ(cinode))
> > -			break_lease(inode, O_RDONLY);
> > -		else
> > -			break_lease(inode, O_WRONLY);
> > -		rc = filemap_fdatawrite(inode->i_mapping);
> > -		if (!CIFS_CACHE_READ(cinode)) {
> > -			rc = filemap_fdatawait(inode->i_mapping);
> > -			mapping_set_error(inode->i_mapping, rc);
> > -			cifs_invalidate_mapping(inode);
> > -		}
> > -		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
> > -	}
> > -
> > -	rc = cifs_push_locks(cfile);
> > -	if (rc)
> > -		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> > -
> > -	/*
> > -	 * releasing stale oplock after recent reconnect of smb session using
> > -	 * a now incorrect file handle is not a data integrity issue but do
> > -	 * not bother sending an oplock release if session to server still is
> > -	 * disconnected since oplock already released by the server
> > -	 */
> > -	if (!cfile->oplock_break_cancelled) {
> > -		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> > -							     cinode);
> > -		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> > -	}
> > -}
> > -
> >  /*
> >   * The presence of cifs_direct_io() in the address space ops vector
> >   * allowes open() O_DIRECT flags which would have failed otherwise.
> > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> > index 526fb89..346ee2a 100644
> > --- a/fs/cifs/smb1ops.c
> > +++ b/fs/cifs/smb1ops.c
> > @@ -23,6 +23,7 @@
> >  #include "cifsproto.h"
> >  #include "cifs_debug.h"
> >  #include "cifspdu.h"
> > +#include "cifsfs.h"
> >  
> >  /*
> >   * An NT cancel request header looks just like the original request except:
> > @@ -999,6 +1000,53 @@ cifs_is_read_op(__u32 oplock)
> >  	return oplock == OPLOCK_READ;
> >  }
> >  
> > +static void cifs_oplock_break(struct work_struct *work)
> > +{
> > +	struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
> > +						  oplock_break);
> > +	struct inode *inode = cfile->dentry->d_inode;
> > +	struct cifsInodeInfo *cinode = CIFS_I(inode);
> > +	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> > +	int rc = 0;
> > +
> > +	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
> > +						cifs_has_mand_locks(cinode)) {
> > +		cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
> > +			 inode);
> > +		cinode->oplock = 0;
> > +	}
> > +
> > +	if (inode && S_ISREG(inode->i_mode)) {
> > +		if (CIFS_CACHE_READ(cinode))
> > +			break_lease(inode, O_RDONLY);
> > +		else
> > +			break_lease(inode, O_WRONLY);
> > +		rc = filemap_fdatawrite(inode->i_mapping);
> > +		if (!CIFS_CACHE_READ(cinode)) {
> > +			rc = filemap_fdatawait(inode->i_mapping);
> > +			mapping_set_error(inode->i_mapping, rc);
> > +			cifs_invalidate_mapping(inode);
> > +		}
> > +		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
> > +	}
> > +
> > +	rc = cifs_push_locks(cfile);
> > +	if (rc)
> > +		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> > +
> > +	/*
> > +	 * releasing stale oplock after recent reconnect of smb session using
> > +	 * a now incorrect file handle is not a data integrity issue but do
> > +	 * not bother sending an oplock release if session to server still is
> > +	 * disconnected since oplock already released by the server
> > +	 */
> > +	if (!cfile->oplock_break_cancelled) {
> > +		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> > +							     cinode);
> > +		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> > +	}
> > +}
> > +
> >  struct smb_version_operations smb1_operations = {
> >  	.send_cancel = send_nt_cancel,
> >  	.compare_fids = cifs_compare_fids,
> > @@ -1067,6 +1115,7 @@ struct smb_version_operations smb1_operations = {
> >  	.query_mf_symlink = cifs_query_mf_symlink,
> >  	.create_mf_symlink = cifs_create_mf_symlink,
> >  	.is_read_op = cifs_is_read_op,
> > +	.oplock_break = cifs_oplock_break,
> >  #ifdef CONFIG_CIFS_XATTR
> >  	.query_all_EAs = CIFSSMBQAllEAs,
> >  	.set_EA = CIFSSMBSetEA,
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index 192f51a..e7081cd 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -27,6 +27,7 @@
> >  #include "cifs_unicode.h"
> >  #include "smb2status.h"
> >  #include "smb2glob.h"
> > +#include "cifsfs.h"
> >  
> >  static int
> >  change_conf(struct TCP_Server_Info *server)
> > @@ -904,6 +905,53 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
> >  	return rc;
> >  }
> >  
> > +static void smb2_oplock_break(struct work_struct *work)
> > +{
> > +	struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
> > +						  oplock_break);
> > +	struct inode *inode = cfile->dentry->d_inode;
> > +	struct cifsInodeInfo *cinode = CIFS_I(inode);
> > +	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> > +	int rc = 0;
> > +
> > +	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
> > +						cifs_has_mand_locks(cinode)) {
> > +		cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
> > +			 inode);
> > +		cinode->oplock = 0;
> > +	}
> > +
> > +	if (inode && S_ISREG(inode->i_mode)) {
> > +		if (CIFS_CACHE_READ(cinode))
> > +			break_lease(inode, O_RDONLY);
> > +		else
> > +			break_lease(inode, O_WRONLY);
> > +		rc = filemap_fdatawrite(inode->i_mapping);
> > +		if (!CIFS_CACHE_READ(cinode)) {
> > +			rc = filemap_fdatawait(inode->i_mapping);
> > +			mapping_set_error(inode->i_mapping, rc);
> > +			cifs_invalidate_mapping(inode);
> > +		}
> > +		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
> > +	}
> > +
> > +	rc = cifs_push_locks(cfile);
> > +	if (rc)
> > +		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> > +
> > +	/*
> > +	 * releasing stale oplock after recent reconnect of smb session using
> > +	 * a now incorrect file handle is not a data integrity issue but do
> > +	 * not bother sending an oplock release if session to server still is
> > +	 * disconnected since oplock already released by the server
> > +	 */
> > +	if (!cfile->oplock_break_cancelled) {
> > +		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> > +							     cinode);
> > +		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> > +	}
> > +}
> > +
> >  static void
> >  smb2_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
> >  		      unsigned int epoch, bool *purge_cache)
> > @@ -1163,6 +1211,7 @@ struct smb_version_operations smb20_operations = {
> >  	.create_lease_buf = smb2_create_lease_buf,
> >  	.parse_lease_buf = smb2_parse_lease_buf,
> >  	.clone_range = smb2_clone_range,
> > +	.oplock_break = smb2_oplock_break,
> >  };
> >  
> >  struct smb_version_operations smb21_operations = {
> > @@ -1237,6 +1286,7 @@ struct smb_version_operations smb21_operations = {
> >  	.create_lease_buf = smb2_create_lease_buf,
> >  	.parse_lease_buf = smb2_parse_lease_buf,
> >  	.clone_range = smb2_clone_range,
> > +	.oplock_break = smb2_oplock_break,
> >  };
> >  
> >  struct smb_version_operations smb30_operations = {
> > @@ -1314,6 +1364,7 @@ struct smb_version_operations smb30_operations = {
> >  	.parse_lease_buf = smb3_parse_lease_buf,
> >  	.clone_range = smb2_clone_range,
> >  	.validate_negotiate = smb3_validate_negotiate,
> > +	.oplock_break = smb2_oplock_break,
> >  };
> >  
> >  struct smb_version_values smb20_values = {
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Steve French March 10, 2014, 4:28 p.m. UTC | #3

Since this might have to go to stable (or be backported) isn't the smaller
fix size a big consideration?

On Mon, Mar 10, 2014 at 11:21 AM, Sachin Prabhu <[e-mail]> wrote:
> On Mon, 2014-03-10 at 11:06 -0400, Jeff Layton wrote:
>> On Fri,  7 Mar 2014 13:29:19 +0000
>> Sachin Prabhu <[e-mail]> wrote:
>>
>> > We need to add protocol specific calls to the oplock break thread.
>> >
>>
>> I'm not necessarily doubting this, but why? AFAICT, the oplock_break
>> calls look more or less identical...
>
> The oplock values for level 2 oplocks are different. For the 2nd fix to
> work, we need to downgrade the oplock only once we are sure that no
> writers are operating.
>
> We need to downgrade oplocks. Since the constants used to describe
> oplock states varies between cifs and smb2, we need to abstract out the
> functions required to downgrade the oplocks. For this we need version
> specific functions. This could either be the oplock_break function as a
> whole or we could define a new function called downgrade_oplock() in
> smb_version_operations. I have chosen to use version specific
> oplock_break here. downgrade_oplock() could instead be used here which
> would require fewer changes.
>
> Should I implement this using a downgrade_oplock() function instead?
>
> Sachin Prabhu
>
>>
>> > Signed-off-by: Sachin Prabhu <[e-mail]>
>> > ---
>> >  fs/cifs/cifsglob.h  |  3 +--
>> >  fs/cifs/cifsproto.h |  2 ++
>> >  fs/cifs/file.c      | 53 +++--------------------------------------------------
>> >  fs/cifs/smb1ops.c   | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>> >  fs/cifs/smb2ops.c   | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  5 files changed, 106 insertions(+), 52 deletions(-)
>> >
>> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> > index cf32f03..93a8762 100644
>> > --- a/fs/cifs/cifsglob.h
>> > +++ b/fs/cifs/cifsglob.h
>> > @@ -402,6 +402,7 @@ struct smb_version_operations {
>> >                     const struct cifs_fid *, u32 *);
>> >     int (*set_acl)(struct cifs_ntsd *, __u32, struct inode *, const char *,
>> >                     int);
>> > +   void (*oplock_break)(struct work_struct *);
>> >  };
>> >
>> >  struct smb_version_values {
>> > @@ -1557,8 +1558,6 @@ GLOBAL_EXTERN spinlock_t uidsidlock;
>> >  GLOBAL_EXTERN spinlock_t gidsidlock;
>> >  #endif /* CONFIG_CIFS_ACL */
>> >
>> > -void cifs_oplock_break(struct work_struct *work);
>> > -
>> >  extern const struct slow_work_ops cifs_oplock_break_ops;
>> >  extern struct workqueue_struct *cifsiod_wq;
>> >
>> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>> > index acc4ee8..e4d0add 100644
>> > --- a/fs/cifs/cifsproto.h
>> > +++ b/fs/cifs/cifsproto.h
>> > @@ -130,6 +130,8 @@ extern void cifs_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock);
>> >  extern int cifs_unlock_range(struct cifsFileInfo *cfile,
>> >                          struct file_lock *flock, const unsigned int xid);
>> >  extern int cifs_push_mandatory_locks(struct cifsFileInfo *cfile);
>> > +extern int cifs_push_locks(struct cifsFileInfo *cfile);
>> > +bool cifs_has_mand_locks(struct cifsInodeInfo *cinode);
>> >
>> >  extern struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid,
>> >                                           struct file *file,
>> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> > index 53c1507..998dec7 100644
>> > --- a/fs/cifs/file.c
>> > +++ b/fs/cifs/file.c
>> > @@ -251,7 +251,7 @@ out:
>> >     return rc;
>> >  }
>> >
>> > -static bool
>> > +bool
>> >  cifs_has_mand_locks(struct cifsInodeInfo *cinode)
>> >  {
>> >     struct cifs_fid_locks *cur;
>> > @@ -304,7 +304,7 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
>> >     cfile->f_flags = file->f_flags;
>> >     cfile->invalidHandle = false;
>> >     cfile->tlink = cifs_get_tlink(tlink);
>> > -   INIT_WORK(&cfile->oplock_break, cifs_oplock_break);
>> > +   INIT_WORK(&cfile->oplock_break, server->ops->oplock_break);
>> >     mutex_init(&cfile->fh_mutex);
>> >
>> >     cifs_sb_active(inode->i_sb);
>> > @@ -1204,7 +1204,7 @@ err_out:
>> >     goto out;
>> >  }
>> >
>> > -static int
>> > +int
>> >  cifs_push_locks(struct cifsFileInfo *cfile)
>> >  {
>> >     struct cifs_sb_info *cifs_sb = CIFS_SB(cfile->dentry->d_sb);
>> > @@ -3656,53 +3656,6 @@ static int cifs_launder_page(struct page *page)
>> >     return rc;
>> >  }
>> >
>> > -void cifs_oplock_break(struct work_struct *work)
>> > -{
>> > -   struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
>> > -                                             oplock_break);
>> > -   struct inode *inode = cfile->dentry->d_inode;
>> > -   struct cifsInodeInfo *cinode = CIFS_I(inode);
>> > -   struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
>> > -   int rc = 0;
>> > -
>> > -   if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
>> > -                                           cifs_has_mand_locks(cinode)) {
>> > -           cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
>> > -                    inode);
>> > -           cinode->oplock = 0;
>> > -   }
>> > -
>> > -   if (inode && S_ISREG(inode->i_mode)) {
>> > -           if (CIFS_CACHE_READ(cinode))
>> > -                   break_lease(inode, O_RDONLY);
>> > -           else
>> > -                   break_lease(inode, O_WRONLY);
>> > -           rc = filemap_fdatawrite(inode->i_mapping);
>> > -           if (!CIFS_CACHE_READ(cinode)) {
>> > -                   rc = filemap_fdatawait(inode->i_mapping);
>> > -                   mapping_set_error(inode->i_mapping, rc);
>> > -                   cifs_invalidate_mapping(inode);
>> > -           }
>> > -           cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
>> > -   }
>> > -
>> > -   rc = cifs_push_locks(cfile);
>> > -   if (rc)
>> > -           cifs_dbg(VFS, "Push locks rc = %d\n", rc);
>> > -
>> > -   /*
>> > -    * releasing stale oplock after recent reconnect of smb session using
>> > -    * a now incorrect file handle is not a data integrity issue but do
>> > -    * not bother sending an oplock release if session to server still is
>> > -    * disconnected since oplock already released by the server
>> > -    */
>> > -   if (!cfile->oplock_break_cancelled) {
>> > -           rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
>> > -                                                        cinode);
>> > -           cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
>> > -   }
>> > -}
>> > -
>> >  /*
>> >   * The presence of cifs_direct_io() in the address space ops vector
>> >   * allowes open() O_DIRECT flags which would have failed otherwise.
>> > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
>> > index 526fb89..346ee2a 100644
>> > --- a/fs/cifs/smb1ops.c
>> > +++ b/fs/cifs/smb1ops.c
>> > @@ -23,6 +23,7 @@
>> >  #include "cifsproto.h"
>> >  #include "cifs_debug.h"
>> >  #include "cifspdu.h"
>> > +#include "cifsfs.h"
>> >
>> >  /*
>> >   * An NT cancel request header looks just like the original request except:
>> > @@ -999,6 +1000,53 @@ cifs_is_read_op(__u32 oplock)
>> >     return oplock == OPLOCK_READ;
>> >  }
>> >
>> > +static void cifs_oplock_break(struct work_struct *work)
>> > +{
>> > +   struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
>> > +                                             oplock_break);
>> > +   struct inode *inode = cfile->dentry->d_inode;
>> > +   struct cifsInodeInfo *cinode = CIFS_I(inode);
>> > +   struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
>> > +   int rc = 0;
>> > +
>> > +   if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
>> > +                                           cifs_has_mand_locks(cinode)) {
>> > +           cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
>> > +                    inode);
>> > +           cinode->oplock = 0;
>> > +   }
>> > +
>> > +   if (inode && S_ISREG(inode->i_mode)) {
>> > +           if (CIFS_CACHE_READ(cinode))
>> > +                   break_lease(inode, O_RDONLY);
>> > +           else
>> > +                   break_lease(inode, O_WRONLY);
>> > +           rc = filemap_fdatawrite(inode->i_mapping);
>> > +           if (!CIFS_CACHE_READ(cinode)) {
>> > +                   rc = filemap_fdatawait(inode->i_mapping);
>> > +                   mapping_set_error(inode->i_mapping, rc);
>> > +                   cifs_invalidate_mapping(inode);
>> > +           }
>> > +           cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
>> > +   }
>> > +
>> > +   rc = cifs_push_locks(cfile);
>> > +   if (rc)
>> > +           cifs_dbg(VFS, "Push locks rc = %d\n", rc);
>> > +
>> > +   /*
>> > +    * releasing stale oplock after recent reconnect of smb session using
>> > +    * a now incorrect file handle is not a data integrity issue but do
>> > +    * not bother sending an oplock release if session to server still is
>> > +    * disconnected since oplock already released by the server
>> > +    */
>> > +   if (!cfile->oplock_break_cancelled) {
>> > +           rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
>> > +                                                        cinode);
>> > +           cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
>> > +   }
>> > +}
>> > +
>> >  struct smb_version_operations smb1_operations = {
>> >     .send_cancel = send_nt_cancel,
>> >     .compare_fids = cifs_compare_fids,
>> > @@ -1067,6 +1115,7 @@ struct smb_version_operations smb1_operations = {
>> >     .query_mf_symlink = cifs_query_mf_symlink,
>> >     .create_mf_symlink = cifs_create_mf_symlink,
>> >     .is_read_op = cifs_is_read_op,
>> > +   .oplock_break = cifs_oplock_break,
>> >  #ifdef CONFIG_CIFS_XATTR
>> >     .query_all_EAs = CIFSSMBQAllEAs,
>> >     .set_EA = CIFSSMBSetEA,
>> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
>> > index 192f51a..e7081cd 100644
>> > --- a/fs/cifs/smb2ops.c
>> > +++ b/fs/cifs/smb2ops.c
>> > @@ -27,6 +27,7 @@
>> >  #include "cifs_unicode.h"
>> >  #include "smb2status.h"
>> >  #include "smb2glob.h"
>> > +#include "cifsfs.h"
>> >
>> >  static int
>> >  change_conf(struct TCP_Server_Info *server)
>> > @@ -904,6 +905,53 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
>> >     return rc;
>> >  }
>> >
>> > +static void smb2_oplock_break(struct work_struct *work)
>> > +{
>> > +   struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
>> > +                                             oplock_break);
>> > +   struct inode *inode = cfile->dentry->d_inode;
>> > +   struct cifsInodeInfo *cinode = CIFS_I(inode);
>> > +   struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
>> > +   int rc = 0;
>> > +
>> > +   if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
>> > +                                           cifs_has_mand_locks(cinode)) {
>> > +           cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
>> > +                    inode);
>> > +           cinode->oplock = 0;
>> > +   }
>> > +
>> > +   if (inode && S_ISREG(inode->i_mode)) {
>> > +           if (CIFS_CACHE_READ(cinode))
>> > +                   break_lease(inode, O_RDONLY);
>> > +           else
>> > +                   break_lease(inode, O_WRONLY);
>> > +           rc = filemap_fdatawrite(inode->i_mapping);
>> > +           if (!CIFS_CACHE_READ(cinode)) {
>> > +                   rc = filemap_fdatawait(inode->i_mapping);
>> > +                   mapping_set_error(inode->i_mapping, rc);
>> > +                   cifs_invalidate_mapping(inode);
>> > +           }
>> > +           cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
>> > +   }
>> > +
>> > +   rc = cifs_push_locks(cfile);
>> > +   if (rc)
>> > +           cifs_dbg(VFS, "Push locks rc = %d\n", rc);
>> > +
>> > +   /*
>> > +    * releasing stale oplock after recent reconnect of smb session using
>> > +    * a now incorrect file handle is not a data integrity issue but do
>> > +    * not bother sending an oplock release if session to server still is
>> > +    * disconnected since oplock already released by the server
>> > +    */
>> > +   if (!cfile->oplock_break_cancelled) {
>> > +           rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
>> > +                                                        cinode);
>> > +           cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
>> > +   }
>> > +}
>> > +
>> >  static void
>> >  smb2_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
>> >                   unsigned int epoch, bool *purge_cache)
>> > @@ -1163,6 +1211,7 @@ struct smb_version_operations smb20_operations = {
>> >     .create_lease_buf = smb2_create_lease_buf,
>> >     .parse_lease_buf = smb2_parse_lease_buf,
>> >     .clone_range = smb2_clone_range,
>> > +   .oplock_break = smb2_oplock_break,
>> >  };
>> >
>> >  struct smb_version_operations smb21_operations = {
>> > @@ -1237,6 +1286,7 @@ struct smb_version_operations smb21_operations = {
>> >     .create_lease_buf = smb2_create_lease_buf,
>> >     .parse_lease_buf = smb2_parse_lease_buf,
>> >     .clone_range = smb2_clone_range,
>> > +   .oplock_break = smb2_oplock_break,
>> >  };
>> >
>> >  struct smb_version_operations smb30_operations = {
>> > @@ -1314,6 +1364,7 @@ struct smb_version_operations smb30_operations = {
>> >     .parse_lease_buf = smb3_parse_lease_buf,
>> >     .clone_range = smb2_clone_range,
>> >     .validate_negotiate = smb3_validate_negotiate,
>> > +   .oplock_break = smb2_oplock_break,
>> >  };
>> >
>> >  struct smb_version_values smb20_values = {
>>
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sachin Prabhu March 10, 2014, 4:47 p.m. UTC | #4

On Mon, 2014-03-10 at 11:28 -0500, Steve French wrote:
> Since this might have to go to stable (or be backported) isn't the smaller
> fix size a big consideration?

Yes. It may be better. The only reason I split oplock_break into version
specific options was because I thought we may need to do it any how for
other reasons in the future.

I'll post a new version using a version specific downgrade_oplock()
instead.

Sachin Prabhu

> 
> On Mon, Mar 10, 2014 at 11:21 AM, Sachin Prabhu <[e-mail]> wrote:
> > On Mon, 2014-03-10 at 11:06 -0400, Jeff Layton wrote:
> >> On Fri,  7 Mar 2014 13:29:19 +0000
> >> Sachin Prabhu <[e-mail]> wrote:
> >>
> >> > We need to add protocol specific calls to the oplock break thread.
> >> >
> >>
> >> I'm not necessarily doubting this, but why? AFAICT, the oplock_break
> >> calls look more or less identical...
> >
> > The oplock values for level 2 oplocks are different. For the 2nd fix to
> > work, we need to downgrade the oplock only once we are sure that no
> > writers are operating.
> >
> > We need to downgrade oplocks. Since the constants used to describe
> > oplock states varies between cifs and smb2, we need to abstract out the
> > functions required to downgrade the oplocks. For this we need version
> > specific functions. This could either be the oplock_break function as a
> > whole or we could define a new function called downgrade_oplock() in
> > smb_version_operations. I have chosen to use version specific
> > oplock_break here. downgrade_oplock() could instead be used here which
> > would require fewer changes.
> >
> > Should I implement this using a downgrade_oplock() function instead?
> >
> > Sachin Prabhu
> >
> >>
> >> > Signed-off-by: Sachin Prabhu <[e-mail]>
> >> > ---
> >> >  fs/cifs/cifsglob.h  |  3 +--
> >> >  fs/cifs/cifsproto.h |  2 ++
> >> >  fs/cifs/file.c      | 53 +++--------------------------------------------------
> >> >  fs/cifs/smb1ops.c   | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> >> >  fs/cifs/smb2ops.c   | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >  5 files changed, 106 insertions(+), 52 deletions(-)
> >> >
> >> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >> > index cf32f03..93a8762 100644
> >> > --- a/fs/cifs/cifsglob.h
> >> > +++ b/fs/cifs/cifsglob.h
> >> > @@ -402,6 +402,7 @@ struct smb_version_operations {
> >> >                     const struct cifs_fid *, u32 *);
> >> >     int (*set_acl)(struct cifs_ntsd *, __u32, struct inode *, const char *,
> >> >                     int);
> >> > +   void (*oplock_break)(struct work_struct *);
> >> >  };
> >> >
> >> >  struct smb_version_values {
> >> > @@ -1557,8 +1558,6 @@ GLOBAL_EXTERN spinlock_t uidsidlock;
> >> >  GLOBAL_EXTERN spinlock_t gidsidlock;
> >> >  #endif /* CONFIG_CIFS_ACL */
> >> >
> >> > -void cifs_oplock_break(struct work_struct *work);
> >> > -
> >> >  extern const struct slow_work_ops cifs_oplock_break_ops;
> >> >  extern struct workqueue_struct *cifsiod_wq;
> >> >
> >> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> >> > index acc4ee8..e4d0add 100644
> >> > --- a/fs/cifs/cifsproto.h
> >> > +++ b/fs/cifs/cifsproto.h
> >> > @@ -130,6 +130,8 @@ extern void cifs_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock);
> >> >  extern int cifs_unlock_range(struct cifsFileInfo *cfile,
> >> >                          struct file_lock *flock, const unsigned int xid);
> >> >  extern int cifs_push_mandatory_locks(struct cifsFileInfo *cfile);
> >> > +extern int cifs_push_locks(struct cifsFileInfo *cfile);
> >> > +bool cifs_has_mand_locks(struct cifsInodeInfo *cinode);
> >> >
> >> >  extern struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid,
> >> >                                           struct file *file,
> >> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> >> > index 53c1507..998dec7 100644
> >> > --- a/fs/cifs/file.c
> >> > +++ b/fs/cifs/file.c
> >> > @@ -251,7 +251,7 @@ out:
> >> >     return rc;
> >> >  }
> >> >
> >> > -static bool
> >> > +bool
> >> >  cifs_has_mand_locks(struct cifsInodeInfo *cinode)
> >> >  {
> >> >     struct cifs_fid_locks *cur;
> >> > @@ -304,7 +304,7 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
> >> >     cfile->f_flags = file->f_flags;
> >> >     cfile->invalidHandle = false;
> >> >     cfile->tlink = cifs_get_tlink(tlink);
> >> > -   INIT_WORK(&cfile->oplock_break, cifs_oplock_break);
> >> > +   INIT_WORK(&cfile->oplock_break, server->ops->oplock_break);
> >> >     mutex_init(&cfile->fh_mutex);
> >> >
> >> >     cifs_sb_active(inode->i_sb);
> >> > @@ -1204,7 +1204,7 @@ err_out:
> >> >     goto out;
> >> >  }
> >> >
> >> > -static int
> >> > +int
> >> >  cifs_push_locks(struct cifsFileInfo *cfile)
> >> >  {
> >> >     struct cifs_sb_info *cifs_sb = CIFS_SB(cfile->dentry->d_sb);
> >> > @@ -3656,53 +3656,6 @@ static int cifs_launder_page(struct page *page)
> >> >     return rc;
> >> >  }
> >> >
> >> > -void cifs_oplock_break(struct work_struct *work)
> >> > -{
> >> > -   struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
> >> > -                                             oplock_break);
> >> > -   struct inode *inode = cfile->dentry->d_inode;
> >> > -   struct cifsInodeInfo *cinode = CIFS_I(inode);
> >> > -   struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> >> > -   int rc = 0;
> >> > -
> >> > -   if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
> >> > -                                           cifs_has_mand_locks(cinode)) {
> >> > -           cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
> >> > -                    inode);
> >> > -           cinode->oplock = 0;
> >> > -   }
> >> > -
> >> > -   if (inode && S_ISREG(inode->i_mode)) {
> >> > -           if (CIFS_CACHE_READ(cinode))
> >> > -                   break_lease(inode, O_RDONLY);
> >> > -           else
> >> > -                   break_lease(inode, O_WRONLY);
> >> > -           rc = filemap_fdatawrite(inode->i_mapping);
> >> > -           if (!CIFS_CACHE_READ(cinode)) {
> >> > -                   rc = filemap_fdatawait(inode->i_mapping);
> >> > -                   mapping_set_error(inode->i_mapping, rc);
> >> > -                   cifs_invalidate_mapping(inode);
> >> > -           }
> >> > -           cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
> >> > -   }
> >> > -
> >> > -   rc = cifs_push_locks(cfile);
> >> > -   if (rc)
> >> > -           cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> >> > -
> >> > -   /*
> >> > -    * releasing stale oplock after recent reconnect of smb session using
> >> > -    * a now incorrect file handle is not a data integrity issue but do
> >> > -    * not bother sending an oplock release if session to server still is
> >> > -    * disconnected since oplock already released by the server
> >> > -    */
> >> > -   if (!cfile->oplock_break_cancelled) {
> >> > -           rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> >> > -                                                        cinode);
> >> > -           cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> >> > -   }
> >> > -}
> >> > -
> >> >  /*
> >> >   * The presence of cifs_direct_io() in the address space ops vector
> >> >   * allowes open() O_DIRECT flags which would have failed otherwise.
> >> > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> >> > index 526fb89..346ee2a 100644
> >> > --- a/fs/cifs/smb1ops.c
> >> > +++ b/fs/cifs/smb1ops.c
> >> > @@ -23,6 +23,7 @@
> >> >  #include "cifsproto.h"
> >> >  #include "cifs_debug.h"
> >> >  #include "cifspdu.h"
> >> > +#include "cifsfs.h"
> >> >
> >> >  /*
> >> >   * An NT cancel request header looks just like the original request except:
> >> > @@ -999,6 +1000,53 @@ cifs_is_read_op(__u32 oplock)
> >> >     return oplock == OPLOCK_READ;
> >> >  }
> >> >
> >> > +static void cifs_oplock_break(struct work_struct *work)
> >> > +{
> >> > +   struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
> >> > +                                             oplock_break);
> >> > +   struct inode *inode = cfile->dentry->d_inode;
> >> > +   struct cifsInodeInfo *cinode = CIFS_I(inode);
> >> > +   struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> >> > +   int rc = 0;
> >> > +
> >> > +   if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
> >> > +                                           cifs_has_mand_locks(cinode)) {
> >> > +           cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
> >> > +                    inode);
> >> > +           cinode->oplock = 0;
> >> > +   }
> >> > +
> >> > +   if (inode && S_ISREG(inode->i_mode)) {
> >> > +           if (CIFS_CACHE_READ(cinode))
> >> > +                   break_lease(inode, O_RDONLY);
> >> > +           else
> >> > +                   break_lease(inode, O_WRONLY);
> >> > +           rc = filemap_fdatawrite(inode->i_mapping);
> >> > +           if (!CIFS_CACHE_READ(cinode)) {
> >> > +                   rc = filemap_fdatawait(inode->i_mapping);
> >> > +                   mapping_set_error(inode->i_mapping, rc);
> >> > +                   cifs_invalidate_mapping(inode);
> >> > +           }
> >> > +           cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
> >> > +   }
> >> > +
> >> > +   rc = cifs_push_locks(cfile);
> >> > +   if (rc)
> >> > +           cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> >> > +
> >> > +   /*
> >> > +    * releasing stale oplock after recent reconnect of smb session using
> >> > +    * a now incorrect file handle is not a data integrity issue but do
> >> > +    * not bother sending an oplock release if session to server still is
> >> > +    * disconnected since oplock already released by the server
> >> > +    */
> >> > +   if (!cfile->oplock_break_cancelled) {
> >> > +           rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> >> > +                                                        cinode);
> >> > +           cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> >> > +   }
> >> > +}
> >> > +
> >> >  struct smb_version_operations smb1_operations = {
> >> >     .send_cancel = send_nt_cancel,
> >> >     .compare_fids = cifs_compare_fids,
> >> > @@ -1067,6 +1115,7 @@ struct smb_version_operations smb1_operations = {
> >> >     .query_mf_symlink = cifs_query_mf_symlink,
> >> >     .create_mf_symlink = cifs_create_mf_symlink,
> >> >     .is_read_op = cifs_is_read_op,
> >> > +   .oplock_break = cifs_oplock_break,
> >> >  #ifdef CONFIG_CIFS_XATTR
> >> >     .query_all_EAs = CIFSSMBQAllEAs,
> >> >     .set_EA = CIFSSMBSetEA,
> >> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> >> > index 192f51a..e7081cd 100644
> >> > --- a/fs/cifs/smb2ops.c
> >> > +++ b/fs/cifs/smb2ops.c
> >> > @@ -27,6 +27,7 @@
> >> >  #include "cifs_unicode.h"
> >> >  #include "smb2status.h"
> >> >  #include "smb2glob.h"
> >> > +#include "cifsfs.h"
> >> >
> >> >  static int
> >> >  change_conf(struct TCP_Server_Info *server)
> >> > @@ -904,6 +905,53 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
> >> >     return rc;
> >> >  }
> >> >
> >> > +static void smb2_oplock_break(struct work_struct *work)
> >> > +{
> >> > +   struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
> >> > +                                             oplock_break);
> >> > +   struct inode *inode = cfile->dentry->d_inode;
> >> > +   struct cifsInodeInfo *cinode = CIFS_I(inode);
> >> > +   struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> >> > +   int rc = 0;
> >> > +
> >> > +   if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
> >> > +                                           cifs_has_mand_locks(cinode)) {
> >> > +           cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
> >> > +                    inode);
> >> > +           cinode->oplock = 0;
> >> > +   }
> >> > +
> >> > +   if (inode && S_ISREG(inode->i_mode)) {
> >> > +           if (CIFS_CACHE_READ(cinode))
> >> > +                   break_lease(inode, O_RDONLY);
> >> > +           else
> >> > +                   break_lease(inode, O_WRONLY);
> >> > +           rc = filemap_fdatawrite(inode->i_mapping);
> >> > +           if (!CIFS_CACHE_READ(cinode)) {
> >> > +                   rc = filemap_fdatawait(inode->i_mapping);
> >> > +                   mapping_set_error(inode->i_mapping, rc);
> >> > +                   cifs_invalidate_mapping(inode);
> >> > +           }
> >> > +           cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
> >> > +   }
> >> > +
> >> > +   rc = cifs_push_locks(cfile);
> >> > +   if (rc)
> >> > +           cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> >> > +
> >> > +   /*
> >> > +    * releasing stale oplock after recent reconnect of smb session using
> >> > +    * a now incorrect file handle is not a data integrity issue but do
> >> > +    * not bother sending an oplock release if session to server still is
> >> > +    * disconnected since oplock already released by the server
> >> > +    */
> >> > +   if (!cfile->oplock_break_cancelled) {
> >> > +           rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> >> > +                                                        cinode);
> >> > +           cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> >> > +   }
> >> > +}
> >> > +
> >> >  static void
> >> >  smb2_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
> >> >                   unsigned int epoch, bool *purge_cache)
> >> > @@ -1163,6 +1211,7 @@ struct smb_version_operations smb20_operations = {
> >> >     .create_lease_buf = smb2_create_lease_buf,
> >> >     .parse_lease_buf = smb2_parse_lease_buf,
> >> >     .clone_range = smb2_clone_range,
> >> > +   .oplock_break = smb2_oplock_break,
> >> >  };
> >> >
> >> >  struct smb_version_operations smb21_operations = {
> >> > @@ -1237,6 +1286,7 @@ struct smb_version_operations smb21_operations = {
> >> >     .create_lease_buf = smb2_create_lease_buf,
> >> >     .parse_lease_buf = smb2_parse_lease_buf,
> >> >     .clone_range = smb2_clone_range,
> >> > +   .oplock_break = smb2_oplock_break,
> >> >  };
> >> >
> >> >  struct smb_version_operations smb30_operations = {
> >> > @@ -1314,6 +1364,7 @@ struct smb_version_operations smb30_operations = {
> >> >     .parse_lease_buf = smb3_parse_lease_buf,
> >> >     .clone_range = smb2_clone_range,
> >> >     .validate_negotiate = smb3_validate_negotiate,
> >> > +   .oplock_break = smb2_oplock_break,
> >> >  };
> >> >
> >> >  struct smb_version_values smb20_values = {
> >>
> >>
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Layton March 10, 2014, 6:16 p.m. UTC | #5

On Mon, 10 Mar 2014 16:21:10 +0000
Sachin Prabhu <[e-mail]> wrote:

> On Mon, 2014-03-10 at 11:06 -0400, Jeff Layton wrote:
> > On Fri,  7 Mar 2014 13:29:19 +0000
> > Sachin Prabhu <[e-mail]> wrote:
> > 
> > > We need to add protocol specific calls to the oplock break thread.
> > > 
> > 
> > I'm not necessarily doubting this, but why? AFAICT, the oplock_break
> > calls look more or less identical...
> 
> The oplock values for level 2 oplocks are different. For the 2nd fix
> to work, we need to downgrade the oplock only once we are sure that no
> writers are operating.
> 
> We need to downgrade oplocks. Since the constants used to describe
> oplock states varies between cifs and smb2, we need to abstract out
> the functions required to downgrade the oplocks. For this we need
> version specific functions. This could either be the oplock_break
> function as a whole or we could define a new function called
> downgrade_oplock() in smb_version_operations. I have chosen to use
> version specific oplock_break here. downgrade_oplock() could instead
> be used here which would require fewer changes.
> 
> Should I implement this using a downgrade_oplock() function instead?
> 

I guess I'm unclear on why the set_oplock_level operation (possibly
combined with some new struct smb_version_values fields) wouldn't be a
better solution.

I'll grant however that the oplock handing code is a bit of a mess. It
seems like it could benefit from some rethink and redesign...

> Sachin Prabhu
> 
> > 
> > > Signed-off-by: Sachin Prabhu <[e-mail]>
> > > ---
> > >  fs/cifs/cifsglob.h  |  3 +--
> > >  fs/cifs/cifsproto.h |  2 ++
> > >  fs/cifs/file.c      | 53
> > > +++--------------------------------------------------
> > > fs/cifs/smb1ops.c   | 49
> > > +++++++++++++++++++++++++++++++++++++++++++++++++
> > > fs/cifs/smb2ops.c   | 51
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files
> > > changed, 106 insertions(+), 52 deletions(-)
> > > 
> > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > index cf32f03..93a8762 100644
> > > --- a/fs/cifs/cifsglob.h
> > > +++ b/fs/cifs/cifsglob.h
> > > @@ -402,6 +402,7 @@ struct smb_version_operations {
> > >  			const struct cifs_fid *, u32 *);
> > >  	int (*set_acl)(struct cifs_ntsd *, __u32, struct inode
> > > *, const char *, int);
> > > +	void (*oplock_break)(struct work_struct *);
> > >  };
> > >  
> > >  struct smb_version_values {
> > > @@ -1557,8 +1558,6 @@ GLOBAL_EXTERN spinlock_t uidsidlock;
> > >  GLOBAL_EXTERN spinlock_t gidsidlock;
> > >  #endif /* CONFIG_CIFS_ACL */
> > >  
> > > -void cifs_oplock_break(struct work_struct *work);
> > > -
> > >  extern const struct slow_work_ops cifs_oplock_break_ops;
> > >  extern struct workqueue_struct *cifsiod_wq;
> > >  
> > > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> > > index acc4ee8..e4d0add 100644
> > > --- a/fs/cifs/cifsproto.h
> > > +++ b/fs/cifs/cifsproto.h
> > > @@ -130,6 +130,8 @@ extern void cifs_set_oplock_level(struct
> > > cifsInodeInfo *cinode, __u32 oplock); extern int
> > > cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock
> > > *flock, const unsigned int xid); extern int
> > > cifs_push_mandatory_locks(struct cifsFileInfo *cfile); +extern
> > > int cifs_push_locks(struct cifsFileInfo *cfile); +bool
> > > cifs_has_mand_locks(struct cifsInodeInfo *cinode); 
> > >  extern struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid
> > > *fid, struct file *file,
> > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > > index 53c1507..998dec7 100644
> > > --- a/fs/cifs/file.c
> > > +++ b/fs/cifs/file.c
> > > @@ -251,7 +251,7 @@ out:
> > >  	return rc;
> > >  }
> > >  
> > > -static bool
> > > +bool
> > >  cifs_has_mand_locks(struct cifsInodeInfo *cinode)
> > >  {
> > >  	struct cifs_fid_locks *cur;
> > > @@ -304,7 +304,7 @@ cifs_new_fileinfo(struct cifs_fid *fid,
> > > struct file *file, cfile->f_flags = file->f_flags;
> > >  	cfile->invalidHandle = false;
> > >  	cfile->tlink = cifs_get_tlink(tlink);
> > > -	INIT_WORK(&cfile->oplock_break, cifs_oplock_break);
> > > +	INIT_WORK(&cfile->oplock_break,
> > > server->ops->oplock_break); mutex_init(&cfile->fh_mutex);
> > >  
> > >  	cifs_sb_active(inode->i_sb);
> > > @@ -1204,7 +1204,7 @@ err_out:
> > >  	goto out;
> > >  }
> > >  
> > > -static int
> > > +int
> > >  cifs_push_locks(struct cifsFileInfo *cfile)
> > >  {
> > >  	struct cifs_sb_info *cifs_sb =
> > > CIFS_SB(cfile->dentry->d_sb); @@ -3656,53 +3656,6 @@ static int
> > > cifs_launder_page(struct page *page) return rc;
> > >  }
> > >  
> > > -void cifs_oplock_break(struct work_struct *work)
> > > -{
> > > -	struct cifsFileInfo *cfile = container_of(work, struct
> > > cifsFileInfo,
> > > -						  oplock_break);
> > > -	struct inode *inode = cfile->dentry->d_inode;
> > > -	struct cifsInodeInfo *cinode = CIFS_I(inode);
> > > -	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> > > -	int rc = 0;
> > > -
> > > -	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode)
> > > &&
> > > -
> > > cifs_has_mand_locks(cinode)) {
> > > -		cifs_dbg(FYI, "Reset oplock to None for inode=%p
> > > due to mand locks\n",
> > > -			 inode);
> > > -		cinode->oplock = 0;
> > > -	}
> > > -
> > > -	if (inode && S_ISREG(inode->i_mode)) {
> > > -		if (CIFS_CACHE_READ(cinode))
> > > -			break_lease(inode, O_RDONLY);
> > > -		else
> > > -			break_lease(inode, O_WRONLY);
> > > -		rc = filemap_fdatawrite(inode->i_mapping);
> > > -		if (!CIFS_CACHE_READ(cinode)) {
> > > -			rc = filemap_fdatawait(inode->i_mapping);
> > > -			mapping_set_error(inode->i_mapping, rc);
> > > -			cifs_invalidate_mapping(inode);
> > > -		}
> > > -		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n",
> > > inode, rc);
> > > -	}
> > > -
> > > -	rc = cifs_push_locks(cfile);
> > > -	if (rc)
> > > -		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> > > -
> > > -	/*
> > > -	 * releasing stale oplock after recent reconnect of smb
> > > session using
> > > -	 * a now incorrect file handle is not a data integrity
> > > issue but do
> > > -	 * not bother sending an oplock release if session to
> > > server still is
> > > -	 * disconnected since oplock already released by the
> > > server
> > > -	 */
> > > -	if (!cfile->oplock_break_cancelled) {
> > > -		rc =
> > > tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> > > -
> > > cinode);
> > > -		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> > > -	}
> > > -}
> > > -
> > >  /*
> > >   * The presence of cifs_direct_io() in the address space ops
> > > vector
> > >   * allowes open() O_DIRECT flags which would have failed
> > > otherwise. diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> > > index 526fb89..346ee2a 100644
> > > --- a/fs/cifs/smb1ops.c
> > > +++ b/fs/cifs/smb1ops.c
> > > @@ -23,6 +23,7 @@
> > >  #include "cifsproto.h"
> > >  #include "cifs_debug.h"
> > >  #include "cifspdu.h"
> > > +#include "cifsfs.h"
> > >  
> > >  /*
> > >   * An NT cancel request header looks just like the original
> > > request except: @@ -999,6 +1000,53 @@ cifs_is_read_op(__u32
> > > oplock) return oplock == OPLOCK_READ;
> > >  }
> > >  
> > > +static void cifs_oplock_break(struct work_struct *work)
> > > +{
> > > +	struct cifsFileInfo *cfile = container_of(work, struct
> > > cifsFileInfo,
> > > +						  oplock_break);
> > > +	struct inode *inode = cfile->dentry->d_inode;
> > > +	struct cifsInodeInfo *cinode = CIFS_I(inode);
> > > +	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> > > +	int rc = 0;
> > > +
> > > +	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode)
> > > &&
> > > +
> > > cifs_has_mand_locks(cinode)) {
> > > +		cifs_dbg(FYI, "Reset oplock to None for inode=%p
> > > due to mand locks\n",
> > > +			 inode);
> > > +		cinode->oplock = 0;
> > > +	}
> > > +
> > > +	if (inode && S_ISREG(inode->i_mode)) {
> > > +		if (CIFS_CACHE_READ(cinode))
> > > +			break_lease(inode, O_RDONLY);
> > > +		else
> > > +			break_lease(inode, O_WRONLY);
> > > +		rc = filemap_fdatawrite(inode->i_mapping);
> > > +		if (!CIFS_CACHE_READ(cinode)) {
> > > +			rc = filemap_fdatawait(inode->i_mapping);
> > > +			mapping_set_error(inode->i_mapping, rc);
> > > +			cifs_invalidate_mapping(inode);
> > > +		}
> > > +		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n",
> > > inode, rc);
> > > +	}
> > > +
> > > +	rc = cifs_push_locks(cfile);
> > > +	if (rc)
> > > +		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> > > +
> > > +	/*
> > > +	 * releasing stale oplock after recent reconnect of smb
> > > session using
> > > +	 * a now incorrect file handle is not a data integrity
> > > issue but do
> > > +	 * not bother sending an oplock release if session to
> > > server still is
> > > +	 * disconnected since oplock already released by the
> > > server
> > > +	 */
> > > +	if (!cfile->oplock_break_cancelled) {
> > > +		rc =
> > > tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> > > +
> > > cinode);
> > > +		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> > > +	}
> > > +}
> > > +
> > >  struct smb_version_operations smb1_operations = {
> > >  	.send_cancel = send_nt_cancel,
> > >  	.compare_fids = cifs_compare_fids,
> > > @@ -1067,6 +1115,7 @@ struct smb_version_operations
> > > smb1_operations = { .query_mf_symlink = cifs_query_mf_symlink,
> > >  	.create_mf_symlink = cifs_create_mf_symlink,
> > >  	.is_read_op = cifs_is_read_op,
> > > +	.oplock_break = cifs_oplock_break,
> > >  #ifdef CONFIG_CIFS_XATTR
> > >  	.query_all_EAs = CIFSSMBQAllEAs,
> > >  	.set_EA = CIFSSMBSetEA,
> > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > > index 192f51a..e7081cd 100644
> > > --- a/fs/cifs/smb2ops.c
> > > +++ b/fs/cifs/smb2ops.c
> > > @@ -27,6 +27,7 @@
> > >  #include "cifs_unicode.h"
> > >  #include "smb2status.h"
> > >  #include "smb2glob.h"
> > > +#include "cifsfs.h"
> > >  
> > >  static int
> > >  change_conf(struct TCP_Server_Info *server)
> > > @@ -904,6 +905,53 @@ smb2_query_symlink(const unsigned int xid,
> > > struct cifs_tcon *tcon, return rc;
> > >  }
> > >  
> > > +static void smb2_oplock_break(struct work_struct *work)
> > > +{
> > > +	struct cifsFileInfo *cfile = container_of(work, struct
> > > cifsFileInfo,
> > > +						  oplock_break);
> > > +	struct inode *inode = cfile->dentry->d_inode;
> > > +	struct cifsInodeInfo *cinode = CIFS_I(inode);
> > > +	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> > > +	int rc = 0;
> > > +
> > > +	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode)
> > > &&
> > > +
> > > cifs_has_mand_locks(cinode)) {
> > > +		cifs_dbg(FYI, "Reset oplock to None for inode=%p
> > > due to mand locks\n",
> > > +			 inode);
> > > +		cinode->oplock = 0;
> > > +	}
> > > +
> > > +	if (inode && S_ISREG(inode->i_mode)) {
> > > +		if (CIFS_CACHE_READ(cinode))
> > > +			break_lease(inode, O_RDONLY);
> > > +		else
> > > +			break_lease(inode, O_WRONLY);
> > > +		rc = filemap_fdatawrite(inode->i_mapping);
> > > +		if (!CIFS_CACHE_READ(cinode)) {
> > > +			rc = filemap_fdatawait(inode->i_mapping);
> > > +			mapping_set_error(inode->i_mapping, rc);
> > > +			cifs_invalidate_mapping(inode);
> > > +		}
> > > +		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n",
> > > inode, rc);
> > > +	}
> > > +
> > > +	rc = cifs_push_locks(cfile);
> > > +	if (rc)
> > > +		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> > > +
> > > +	/*
> > > +	 * releasing stale oplock after recent reconnect of smb
> > > session using
> > > +	 * a now incorrect file handle is not a data integrity
> > > issue but do
> > > +	 * not bother sending an oplock release if session to
> > > server still is
> > > +	 * disconnected since oplock already released by the
> > > server
> > > +	 */
> > > +	if (!cfile->oplock_break_cancelled) {
> > > +		rc =
> > > tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> > > +
> > > cinode);
> > > +		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> > > +	}
> > > +}
> > > +
> > >  static void
> > >  smb2_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
> > >  		      unsigned int epoch, bool *purge_cache)
> > > @@ -1163,6 +1211,7 @@ struct smb_version_operations
> > > smb20_operations = { .create_lease_buf = smb2_create_lease_buf,
> > >  	.parse_lease_buf = smb2_parse_lease_buf,
> > >  	.clone_range = smb2_clone_range,
> > > +	.oplock_break = smb2_oplock_break,
> > >  };
> > >  
> > >  struct smb_version_operations smb21_operations = {
> > > @@ -1237,6 +1286,7 @@ struct smb_version_operations
> > > smb21_operations = { .create_lease_buf = smb2_create_lease_buf,
> > >  	.parse_lease_buf = smb2_parse_lease_buf,
> > >  	.clone_range = smb2_clone_range,
> > > +	.oplock_break = smb2_oplock_break,
> > >  };
> > >  
> > >  struct smb_version_operations smb30_operations = {
> > > @@ -1314,6 +1364,7 @@ struct smb_version_operations
> > > smb30_operations = { .parse_lease_buf = smb3_parse_lease_buf,
> > >  	.clone_range = smb2_clone_range,
> > >  	.validate_negotiate = smb3_validate_negotiate,
> > > +	.oplock_break = smb2_oplock_break,
> > >  };
> > >  
> > >  struct smb_version_values smb20_values = {
> > 
> > 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs"
> in the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sachin Prabhu March 11, 2014, 10:25 a.m. UTC | #6

On Mon, 2014-03-10 at 14:16 -0400, Jeffrey Layton wrote:
> On Mon, 10 Mar 2014 16:21:10 +0000
> Sachin Prabhu <[e-mail]> wrote:
> 
> > On Mon, 2014-03-10 at 11:06 -0400, Jeff Layton wrote:
> > > On Fri,  7 Mar 2014 13:29:19 +0000
> > > Sachin Prabhu <[e-mail]> wrote:
> > > 
> > > > We need to add protocol specific calls to the oplock break thread.
> > > > 
> > > 
> > > I'm not necessarily doubting this, but why? AFAICT, the oplock_break
> > > calls look more or less identical...
> > 
> > The oplock values for level 2 oplocks are different. For the 2nd fix
> > to work, we need to downgrade the oplock only once we are sure that no
> > writers are operating.
> > 
> > We need to downgrade oplocks. Since the constants used to describe
> > oplock states varies between cifs and smb2, we need to abstract out
> > the functions required to downgrade the oplocks. For this we need
> > version specific functions. This could either be the oplock_break
> > function as a whole or we could define a new function called
> > downgrade_oplock() in smb_version_operations. I have chosen to use
> > version specific oplock_break here. downgrade_oplock() could instead
> > be used here which would require fewer changes.
> > 
> > Should I implement this using a downgrade_oplock() function instead?
> > 
> 
> I guess I'm unclear on why the set_oplock_level operation (possibly
> combined with some new struct smb_version_values fields) wouldn't be a
> better solution.
> 
> I'll grant however that the oplock handing code is a bit of a mess. It
> seems like it could benefit from some rethink and redesign...
> 

The constants used to denote oplock values are not consistent across the
versions

In this case, a level 2 oplock for smb1 is 
#define OPLOCK_READ      3  /* level 2 oplock */
for smb2, it is
#define SMB2_OPLOCK_LEVEL_II            0x01

similarly the values for other states are different too. 

We could abstract these but it sounded like overkill in this situation.

Sachin Prabhu







> > Sachin Prabhu
> > 
> > > 
> > > > Signed-off-by: Sachin Prabhu <[e-mail]>
> > > > ---
> > > >  fs/cifs/cifsglob.h  |  3 +--
> > > >  fs/cifs/cifsproto.h |  2 ++
> > > >  fs/cifs/file.c      | 53
> > > > +++--------------------------------------------------
> > > > fs/cifs/smb1ops.c   | 49
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > fs/cifs/smb2ops.c   | 51
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files
> > > > changed, 106 insertions(+), 52 deletions(-)
> > > > 
> > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > > index cf32f03..93a8762 100644
> > > > --- a/fs/cifs/cifsglob.h
> > > > +++ b/fs/cifs/cifsglob.h
> > > > @@ -402,6 +402,7 @@ struct smb_version_operations {
> > > >  			const struct cifs_fid *, u32 *);
> > > >  	int (*set_acl)(struct cifs_ntsd *, __u32, struct inode
> > > > *, const char *, int);
> > > > +	void (*oplock_break)(struct work_struct *);
> > > >  };
> > > >  
> > > >  struct smb_version_values {
> > > > @@ -1557,8 +1558,6 @@ GLOBAL_EXTERN spinlock_t uidsidlock;
> > > >  GLOBAL_EXTERN spinlock_t gidsidlock;
> > > >  #endif /* CONFIG_CIFS_ACL */
> > > >  
> > > > -void cifs_oplock_break(struct work_struct *work);
> > > > -
> > > >  extern const struct slow_work_ops cifs_oplock_break_ops;
> > > >  extern struct workqueue_struct *cifsiod_wq;
> > > >  
> > > > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> > > > index acc4ee8..e4d0add 100644
> > > > --- a/fs/cifs/cifsproto.h
> > > > +++ b/fs/cifs/cifsproto.h
> > > > @@ -130,6 +130,8 @@ extern void cifs_set_oplock_level(struct
> > > > cifsInodeInfo *cinode, __u32 oplock); extern int
> > > > cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock
> > > > *flock, const unsigned int xid); extern int
> > > > cifs_push_mandatory_locks(struct cifsFileInfo *cfile); +extern
> > > > int cifs_push_locks(struct cifsFileInfo *cfile); +bool
> > > > cifs_has_mand_locks(struct cifsInodeInfo *cinode); 
> > > >  extern struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid
> > > > *fid, struct file *file,
> > > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > > > index 53c1507..998dec7 100644
> > > > --- a/fs/cifs/file.c
> > > > +++ b/fs/cifs/file.c
> > > > @@ -251,7 +251,7 @@ out:
> > > >  	return rc;
> > > >  }
> > > >  
> > > > -static bool
> > > > +bool
> > > >  cifs_has_mand_locks(struct cifsInodeInfo *cinode)
> > > >  {
> > > >  	struct cifs_fid_locks *cur;
> > > > @@ -304,7 +304,7 @@ cifs_new_fileinfo(struct cifs_fid *fid,
> > > > struct file *file, cfile->f_flags = file->f_flags;
> > > >  	cfile->invalidHandle = false;
> > > >  	cfile->tlink = cifs_get_tlink(tlink);
> > > > -	INIT_WORK(&cfile->oplock_break, cifs_oplock_break);
> > > > +	INIT_WORK(&cfile->oplock_break,
> > > > server->ops->oplock_break); mutex_init(&cfile->fh_mutex);
> > > >  
> > > >  	cifs_sb_active(inode->i_sb);
> > > > @@ -1204,7 +1204,7 @@ err_out:
> > > >  	goto out;
> > > >  }
> > > >  
> > > > -static int
> > > > +int
> > > >  cifs_push_locks(struct cifsFileInfo *cfile)
> > > >  {
> > > >  	struct cifs_sb_info *cifs_sb =
> > > > CIFS_SB(cfile->dentry->d_sb); @@ -3656,53 +3656,6 @@ static int
> > > > cifs_launder_page(struct page *page) return rc;
> > > >  }
> > > >  
> > > > -void cifs_oplock_break(struct work_struct *work)
> > > > -{
> > > > -	struct cifsFileInfo *cfile = container_of(work, struct
> > > > cifsFileInfo,
> > > > -						  oplock_break);
> > > > -	struct inode *inode = cfile->dentry->d_inode;
> > > > -	struct cifsInodeInfo *cinode = CIFS_I(inode);
> > > > -	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> > > > -	int rc = 0;
> > > > -
> > > > -	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode)
> > > > &&
> > > > -
> > > > cifs_has_mand_locks(cinode)) {
> > > > -		cifs_dbg(FYI, "Reset oplock to None for inode=%p
> > > > due to mand locks\n",
> > > > -			 inode);
> > > > -		cinode->oplock = 0;
> > > > -	}
> > > > -
> > > > -	if (inode && S_ISREG(inode->i_mode)) {
> > > > -		if (CIFS_CACHE_READ(cinode))
> > > > -			break_lease(inode, O_RDONLY);
> > > > -		else
> > > > -			break_lease(inode, O_WRONLY);
> > > > -		rc = filemap_fdatawrite(inode->i_mapping);
> > > > -		if (!CIFS_CACHE_READ(cinode)) {
> > > > -			rc = filemap_fdatawait(inode->i_mapping);
> > > > -			mapping_set_error(inode->i_mapping, rc);
> > > > -			cifs_invalidate_mapping(inode);
> > > > -		}
> > > > -		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n",
> > > > inode, rc);
> > > > -	}
> > > > -
> > > > -	rc = cifs_push_locks(cfile);
> > > > -	if (rc)
> > > > -		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> > > > -
> > > > -	/*
> > > > -	 * releasing stale oplock after recent reconnect of smb
> > > > session using
> > > > -	 * a now incorrect file handle is not a data integrity
> > > > issue but do
> > > > -	 * not bother sending an oplock release if session to
> > > > server still is
> > > > -	 * disconnected since oplock already released by the
> > > > server
> > > > -	 */
> > > > -	if (!cfile->oplock_break_cancelled) {
> > > > -		rc =
> > > > tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> > > > -
> > > > cinode);
> > > > -		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> > > > -	}
> > > > -}
> > > > -
> > > >  /*
> > > >   * The presence of cifs_direct_io() in the address space ops
> > > > vector
> > > >   * allowes open() O_DIRECT flags which would have failed
> > > > otherwise. diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> > > > index 526fb89..346ee2a 100644
> > > > --- a/fs/cifs/smb1ops.c
> > > > +++ b/fs/cifs/smb1ops.c
> > > > @@ -23,6 +23,7 @@
> > > >  #include "cifsproto.h"
> > > >  #include "cifs_debug.h"
> > > >  #include "cifspdu.h"
> > > > +#include "cifsfs.h"
> > > >  
> > > >  /*
> > > >   * An NT cancel request header looks just like the original
> > > > request except: @@ -999,6 +1000,53 @@ cifs_is_read_op(__u32
> > > > oplock) return oplock == OPLOCK_READ;
> > > >  }
> > > >  
> > > > +static void cifs_oplock_break(struct work_struct *work)
> > > > +{
> > > > +	struct cifsFileInfo *cfile = container_of(work, struct
> > > > cifsFileInfo,
> > > > +						  oplock_break);
> > > > +	struct inode *inode = cfile->dentry->d_inode;
> > > > +	struct cifsInodeInfo *cinode = CIFS_I(inode);
> > > > +	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> > > > +	int rc = 0;
> > > > +
> > > > +	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode)
> > > > &&
> > > > +
> > > > cifs_has_mand_locks(cinode)) {
> > > > +		cifs_dbg(FYI, "Reset oplock to None for inode=%p
> > > > due to mand locks\n",
> > > > +			 inode);
> > > > +		cinode->oplock = 0;
> > > > +	}
> > > > +
> > > > +	if (inode && S_ISREG(inode->i_mode)) {
> > > > +		if (CIFS_CACHE_READ(cinode))
> > > > +			break_lease(inode, O_RDONLY);
> > > > +		else
> > > > +			break_lease(inode, O_WRONLY);
> > > > +		rc = filemap_fdatawrite(inode->i_mapping);
> > > > +		if (!CIFS_CACHE_READ(cinode)) {
> > > > +			rc = filemap_fdatawait(inode->i_mapping);
> > > > +			mapping_set_error(inode->i_mapping, rc);
> > > > +			cifs_invalidate_mapping(inode);
> > > > +		}
> > > > +		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n",
> > > > inode, rc);
> > > > +	}
> > > > +
> > > > +	rc = cifs_push_locks(cfile);
> > > > +	if (rc)
> > > > +		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> > > > +
> > > > +	/*
> > > > +	 * releasing stale oplock after recent reconnect of smb
> > > > session using
> > > > +	 * a now incorrect file handle is not a data integrity
> > > > issue but do
> > > > +	 * not bother sending an oplock release if session to
> > > > server still is
> > > > +	 * disconnected since oplock already released by the
> > > > server
> > > > +	 */
> > > > +	if (!cfile->oplock_break_cancelled) {
> > > > +		rc =
> > > > tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> > > > +
> > > > cinode);
> > > > +		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> > > > +	}
> > > > +}
> > > > +
> > > >  struct smb_version_operations smb1_operations = {
> > > >  	.send_cancel = send_nt_cancel,
> > > >  	.compare_fids = cifs_compare_fids,
> > > > @@ -1067,6 +1115,7 @@ struct smb_version_operations
> > > > smb1_operations = { .query_mf_symlink = cifs_query_mf_symlink,
> > > >  	.create_mf_symlink = cifs_create_mf_symlink,
> > > >  	.is_read_op = cifs_is_read_op,
> > > > +	.oplock_break = cifs_oplock_break,
> > > >  #ifdef CONFIG_CIFS_XATTR
> > > >  	.query_all_EAs = CIFSSMBQAllEAs,
> > > >  	.set_EA = CIFSSMBSetEA,
> > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > > > index 192f51a..e7081cd 100644
> > > > --- a/fs/cifs/smb2ops.c
> > > > +++ b/fs/cifs/smb2ops.c
> > > > @@ -27,6 +27,7 @@
> > > >  #include "cifs_unicode.h"
> > > >  #include "smb2status.h"
> > > >  #include "smb2glob.h"
> > > > +#include "cifsfs.h"
> > > >  
> > > >  static int
> > > >  change_conf(struct TCP_Server_Info *server)
> > > > @@ -904,6 +905,53 @@ smb2_query_symlink(const unsigned int xid,
> > > > struct cifs_tcon *tcon, return rc;
> > > >  }
> > > >  
> > > > +static void smb2_oplock_break(struct work_struct *work)
> > > > +{
> > > > +	struct cifsFileInfo *cfile = container_of(work, struct
> > > > cifsFileInfo,
> > > > +						  oplock_break);
> > > > +	struct inode *inode = cfile->dentry->d_inode;
> > > > +	struct cifsInodeInfo *cinode = CIFS_I(inode);
> > > > +	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> > > > +	int rc = 0;
> > > > +
> > > > +	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode)
> > > > &&
> > > > +
> > > > cifs_has_mand_locks(cinode)) {
> > > > +		cifs_dbg(FYI, "Reset oplock to None for inode=%p
> > > > due to mand locks\n",
> > > > +			 inode);
> > > > +		cinode->oplock = 0;
> > > > +	}
> > > > +
> > > > +	if (inode && S_ISREG(inode->i_mode)) {
> > > > +		if (CIFS_CACHE_READ(cinode))
> > > > +			break_lease(inode, O_RDONLY);
> > > > +		else
> > > > +			break_lease(inode, O_WRONLY);
> > > > +		rc = filemap_fdatawrite(inode->i_mapping);
> > > > +		if (!CIFS_CACHE_READ(cinode)) {
> > > > +			rc = filemap_fdatawait(inode->i_mapping);
> > > > +			mapping_set_error(inode->i_mapping, rc);
> > > > +			cifs_invalidate_mapping(inode);
> > > > +		}
> > > > +		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n",
> > > > inode, rc);
> > > > +	}
> > > > +
> > > > +	rc = cifs_push_locks(cfile);
> > > > +	if (rc)
> > > > +		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> > > > +
> > > > +	/*
> > > > +	 * releasing stale oplock after recent reconnect of smb
> > > > session using
> > > > +	 * a now incorrect file handle is not a data integrity
> > > > issue but do
> > > > +	 * not bother sending an oplock release if session to
> > > > server still is
> > > > +	 * disconnected since oplock already released by the
> > > > server
> > > > +	 */
> > > > +	if (!cfile->oplock_break_cancelled) {
> > > > +		rc =
> > > > tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> > > > +
> > > > cinode);
> > > > +		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> > > > +	}
> > > > +}
> > > > +
> > > >  static void
> > > >  smb2_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
> > > >  		      unsigned int epoch, bool *purge_cache)
> > > > @@ -1163,6 +1211,7 @@ struct smb_version_operations
> > > > smb20_operations = { .create_lease_buf = smb2_create_lease_buf,
> > > >  	.parse_lease_buf = smb2_parse_lease_buf,
> > > >  	.clone_range = smb2_clone_range,
> > > > +	.oplock_break = smb2_oplock_break,
> > > >  };
> > > >  
> > > >  struct smb_version_operations smb21_operations = {
> > > > @@ -1237,6 +1286,7 @@ struct smb_version_operations
> > > > smb21_operations = { .create_lease_buf = smb2_create_lease_buf,
> > > >  	.parse_lease_buf = smb2_parse_lease_buf,
> > > >  	.clone_range = smb2_clone_range,
> > > > +	.oplock_break = smb2_oplock_break,
> > > >  };
> > > >  
> > > >  struct smb_version_operations smb30_operations = {
> > > > @@ -1314,6 +1364,7 @@ struct smb_version_operations
> > > > smb30_operations = { .parse_lease_buf = smb3_parse_lease_buf,
> > > >  	.clone_range = smb2_clone_range,
> > > >  	.validate_negotiate = smb3_validate_negotiate,
> > > > +	.oplock_break = smb2_oplock_break,
> > > >  };
> > > >  
> > > >  struct smb_version_values smb20_values = {
> > > 
> > > 
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs"
> > in the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Layton March 11, 2014, 11:08 a.m. UTC | #7

On Tue, 11 Mar 2014 10:25:49 +0000
Sachin Prabhu <[e-mail]> wrote:

> On Mon, 2014-03-10 at 14:16 -0400, Jeffrey Layton wrote:
> > On Mon, 10 Mar 2014 16:21:10 +0000
> > Sachin Prabhu <[e-mail]> wrote:
> > 
> > > On Mon, 2014-03-10 at 11:06 -0400, Jeff Layton wrote:
> > > > On Fri,  7 Mar 2014 13:29:19 +0000
> > > > Sachin Prabhu <[e-mail]> wrote:
> > > > 
> > > > > We need to add protocol specific calls to the oplock break thread.
> > > > > 
> > > > 
> > > > I'm not necessarily doubting this, but why? AFAICT, the oplock_break
> > > > calls look more or less identical...
> > > 
> > > The oplock values for level 2 oplocks are different. For the 2nd fix
> > > to work, we need to downgrade the oplock only once we are sure that no
> > > writers are operating.
> > > 
> > > We need to downgrade oplocks. Since the constants used to describe
> > > oplock states varies between cifs and smb2, we need to abstract out
> > > the functions required to downgrade the oplocks. For this we need
> > > version specific functions. This could either be the oplock_break
> > > function as a whole or we could define a new function called
> > > downgrade_oplock() in smb_version_operations. I have chosen to use
> > > version specific oplock_break here. downgrade_oplock() could instead
> > > be used here which would require fewer changes.
> > > 
> > > Should I implement this using a downgrade_oplock() function instead?
> > > 
> > 
> > I guess I'm unclear on why the set_oplock_level operation (possibly
> > combined with some new struct smb_version_values fields) wouldn't be a
> > better solution.
> > 
> > I'll grant however that the oplock handing code is a bit of a mess. It
> > seems like it could benefit from some rethink and redesign...
> > 
> 
> The constants used to denote oplock values are not consistent across the
> versions
> 
> In this case, a level 2 oplock for smb1 is 
> #define OPLOCK_READ      3  /* level 2 oplock */
> for smb2, it is
> #define SMB2_OPLOCK_LEVEL_II            0x01
> 
> similarly the values for other states are different too. 
> 
> We could abstract these but it sounded like overkill in this situation.
> 
> Sachin Prabhu
> 
> 

Yeah, that's the part that's a mess. We have a generic set_oplock_level
operation but that requires you to pass down oplock levels that are
version specific. Typically when you have a generic operation, you want
it to take a set of generic values as arguments.

> 
> 
> 
> 
> 
> > > Sachin Prabhu
> > > 
> > > > 
> > > > > Signed-off-by: Sachin Prabhu <[e-mail]>
> > > > > ---
> > > > >  fs/cifs/cifsglob.h  |  3 +--
> > > > >  fs/cifs/cifsproto.h |  2 ++
> > > > >  fs/cifs/file.c      | 53
> > > > > +++--------------------------------------------------
> > > > > fs/cifs/smb1ops.c   | 49
> > > > > +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > fs/cifs/smb2ops.c   | 51
> > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files
> > > > > changed, 106 insertions(+), 52 deletions(-)
> > > > > 
> > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > > > index cf32f03..93a8762 100644
> > > > > --- a/fs/cifs/cifsglob.h
> > > > > +++ b/fs/cifs/cifsglob.h
> > > > > @@ -402,6 +402,7 @@ struct smb_version_operations {
> > > > >  			const struct cifs_fid *, u32 *);
> > > > >  	int (*set_acl)(struct cifs_ntsd *, __u32, struct inode
> > > > > *, const char *, int);
> > > > > +	void (*oplock_break)(struct work_struct *);
> > > > >  };
> > > > >  
> > > > >  struct smb_version_values {
> > > > > @@ -1557,8 +1558,6 @@ GLOBAL_EXTERN spinlock_t uidsidlock;
> > > > >  GLOBAL_EXTERN spinlock_t gidsidlock;
> > > > >  #endif /* CONFIG_CIFS_ACL */
> > > > >  
> > > > > -void cifs_oplock_break(struct work_struct *work);
> > > > > -
> > > > >  extern const struct slow_work_ops cifs_oplock_break_ops;
> > > > >  extern struct workqueue_struct *cifsiod_wq;
> > > > >  
> > > > > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> > > > > index acc4ee8..e4d0add 100644
> > > > > --- a/fs/cifs/cifsproto.h
> > > > > +++ b/fs/cifs/cifsproto.h
> > > > > @@ -130,6 +130,8 @@ extern void cifs_set_oplock_level(struct
> > > > > cifsInodeInfo *cinode, __u32 oplock); extern int
> > > > > cifs_unlock_range(struct cifsFileInfo *cfile, struct file_lock
> > > > > *flock, const unsigned int xid); extern int
> > > > > cifs_push_mandatory_locks(struct cifsFileInfo *cfile); +extern
> > > > > int cifs_push_locks(struct cifsFileInfo *cfile); +bool
> > > > > cifs_has_mand_locks(struct cifsInodeInfo *cinode); 
> > > > >  extern struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid
> > > > > *fid, struct file *file,
> > > > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > > > > index 53c1507..998dec7 100644
> > > > > --- a/fs/cifs/file.c
> > > > > +++ b/fs/cifs/file.c
> > > > > @@ -251,7 +251,7 @@ out:
> > > > >  	return rc;
> > > > >  }
> > > > >  
> > > > > -static bool
> > > > > +bool
> > > > >  cifs_has_mand_locks(struct cifsInodeInfo *cinode)
> > > > >  {
> > > > >  	struct cifs_fid_locks *cur;
> > > > > @@ -304,7 +304,7 @@ cifs_new_fileinfo(struct cifs_fid *fid,
> > > > > struct file *file, cfile->f_flags = file->f_flags;
> > > > >  	cfile->invalidHandle = false;
> > > > >  	cfile->tlink = cifs_get_tlink(tlink);
> > > > > -	INIT_WORK(&cfile->oplock_break, cifs_oplock_break);
> > > > > +	INIT_WORK(&cfile->oplock_break,
> > > > > server->ops->oplock_break); mutex_init(&cfile->fh_mutex);
> > > > >  
> > > > >  	cifs_sb_active(inode->i_sb);
> > > > > @@ -1204,7 +1204,7 @@ err_out:
> > > > >  	goto out;
> > > > >  }
> > > > >  
> > > > > -static int
> > > > > +int
> > > > >  cifs_push_locks(struct cifsFileInfo *cfile)
> > > > >  {
> > > > >  	struct cifs_sb_info *cifs_sb =
> > > > > CIFS_SB(cfile->dentry->d_sb); @@ -3656,53 +3656,6 @@ static int
> > > > > cifs_launder_page(struct page *page) return rc;
> > > > >  }
> > > > >  
> > > > > -void cifs_oplock_break(struct work_struct *work)
> > > > > -{
> > > > > -	struct cifsFileInfo *cfile = container_of(work, struct
> > > > > cifsFileInfo,
> > > > > -						  oplock_break);
> > > > > -	struct inode *inode = cfile->dentry->d_inode;
> > > > > -	struct cifsInodeInfo *cinode = CIFS_I(inode);
> > > > > -	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> > > > > -	int rc = 0;
> > > > > -
> > > > > -	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode)
> > > > > &&
> > > > > -
> > > > > cifs_has_mand_locks(cinode)) {
> > > > > -		cifs_dbg(FYI, "Reset oplock to None for inode=%p
> > > > > due to mand locks\n",
> > > > > -			 inode);
> > > > > -		cinode->oplock = 0;
> > > > > -	}
> > > > > -
> > > > > -	if (inode && S_ISREG(inode->i_mode)) {
> > > > > -		if (CIFS_CACHE_READ(cinode))
> > > > > -			break_lease(inode, O_RDONLY);
> > > > > -		else
> > > > > -			break_lease(inode, O_WRONLY);
> > > > > -		rc = filemap_fdatawrite(inode->i_mapping);
> > > > > -		if (!CIFS_CACHE_READ(cinode)) {
> > > > > -			rc = filemap_fdatawait(inode->i_mapping);
> > > > > -			mapping_set_error(inode->i_mapping, rc);
> > > > > -			cifs_invalidate_mapping(inode);
> > > > > -		}
> > > > > -		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n",
> > > > > inode, rc);
> > > > > -	}
> > > > > -
> > > > > -	rc = cifs_push_locks(cfile);
> > > > > -	if (rc)
> > > > > -		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> > > > > -
> > > > > -	/*
> > > > > -	 * releasing stale oplock after recent reconnect of smb
> > > > > session using
> > > > > -	 * a now incorrect file handle is not a data integrity
> > > > > issue but do
> > > > > -	 * not bother sending an oplock release if session to
> > > > > server still is
> > > > > -	 * disconnected since oplock already released by the
> > > > > server
> > > > > -	 */
> > > > > -	if (!cfile->oplock_break_cancelled) {
> > > > > -		rc =
> > > > > tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> > > > > -
> > > > > cinode);
> > > > > -		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> > > > > -	}
> > > > > -}
> > > > > -
> > > > >  /*
> > > > >   * The presence of cifs_direct_io() in the address space ops
> > > > > vector
> > > > >   * allowes open() O_DIRECT flags which would have failed
> > > > > otherwise. diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> > > > > index 526fb89..346ee2a 100644
> > > > > --- a/fs/cifs/smb1ops.c
> > > > > +++ b/fs/cifs/smb1ops.c
> > > > > @@ -23,6 +23,7 @@
> > > > >  #include "cifsproto.h"
> > > > >  #include "cifs_debug.h"
> > > > >  #include "cifspdu.h"
> > > > > +#include "cifsfs.h"
> > > > >  
> > > > >  /*
> > > > >   * An NT cancel request header looks just like the original
> > > > > request except: @@ -999,6 +1000,53 @@ cifs_is_read_op(__u32
> > > > > oplock) return oplock == OPLOCK_READ;
> > > > >  }
> > > > >  
> > > > > +static void cifs_oplock_break(struct work_struct *work)
> > > > > +{
> > > > > +	struct cifsFileInfo *cfile = container_of(work, struct
> > > > > cifsFileInfo,
> > > > > +						  oplock_break);
> > > > > +	struct inode *inode = cfile->dentry->d_inode;
> > > > > +	struct cifsInodeInfo *cinode = CIFS_I(inode);
> > > > > +	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> > > > > +	int rc = 0;
> > > > > +
> > > > > +	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode)
> > > > > &&
> > > > > +
> > > > > cifs_has_mand_locks(cinode)) {
> > > > > +		cifs_dbg(FYI, "Reset oplock to None for inode=%p
> > > > > due to mand locks\n",
> > > > > +			 inode);
> > > > > +		cinode->oplock = 0;
> > > > > +	}
> > > > > +
> > > > > +	if (inode && S_ISREG(inode->i_mode)) {
> > > > > +		if (CIFS_CACHE_READ(cinode))
> > > > > +			break_lease(inode, O_RDONLY);
> > > > > +		else
> > > > > +			break_lease(inode, O_WRONLY);
> > > > > +		rc = filemap_fdatawrite(inode->i_mapping);
> > > > > +		if (!CIFS_CACHE_READ(cinode)) {
> > > > > +			rc = filemap_fdatawait(inode->i_mapping);
> > > > > +			mapping_set_error(inode->i_mapping, rc);
> > > > > +			cifs_invalidate_mapping(inode);
> > > > > +		}
> > > > > +		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n",
> > > > > inode, rc);
> > > > > +	}
> > > > > +
> > > > > +	rc = cifs_push_locks(cfile);
> > > > > +	if (rc)
> > > > > +		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> > > > > +
> > > > > +	/*
> > > > > +	 * releasing stale oplock after recent reconnect of smb
> > > > > session using
> > > > > +	 * a now incorrect file handle is not a data integrity
> > > > > issue but do
> > > > > +	 * not bother sending an oplock release if session to
> > > > > server still is
> > > > > +	 * disconnected since oplock already released by the
> > > > > server
> > > > > +	 */
> > > > > +	if (!cfile->oplock_break_cancelled) {
> > > > > +		rc =
> > > > > tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> > > > > +
> > > > > cinode);
> > > > > +		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> > > > > +	}
> > > > > +}
> > > > > +
> > > > >  struct smb_version_operations smb1_operations = {
> > > > >  	.send_cancel = send_nt_cancel,
> > > > >  	.compare_fids = cifs_compare_fids,
> > > > > @@ -1067,6 +1115,7 @@ struct smb_version_operations
> > > > > smb1_operations = { .query_mf_symlink = cifs_query_mf_symlink,
> > > > >  	.create_mf_symlink = cifs_create_mf_symlink,
> > > > >  	.is_read_op = cifs_is_read_op,
> > > > > +	.oplock_break = cifs_oplock_break,
> > > > >  #ifdef CONFIG_CIFS_XATTR
> > > > >  	.query_all_EAs = CIFSSMBQAllEAs,
> > > > >  	.set_EA = CIFSSMBSetEA,
> > > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > > > > index 192f51a..e7081cd 100644
> > > > > --- a/fs/cifs/smb2ops.c
> > > > > +++ b/fs/cifs/smb2ops.c
> > > > > @@ -27,6 +27,7 @@
> > > > >  #include "cifs_unicode.h"
> > > > >  #include "smb2status.h"
> > > > >  #include "smb2glob.h"
> > > > > +#include "cifsfs.h"
> > > > >  
> > > > >  static int
> > > > >  change_conf(struct TCP_Server_Info *server)
> > > > > @@ -904,6 +905,53 @@ smb2_query_symlink(const unsigned int xid,
> > > > > struct cifs_tcon *tcon, return rc;
> > > > >  }
> > > > >  
> > > > > +static void smb2_oplock_break(struct work_struct *work)
> > > > > +{
> > > > > +	struct cifsFileInfo *cfile = container_of(work, struct
> > > > > cifsFileInfo,
> > > > > +						  oplock_break);
> > > > > +	struct inode *inode = cfile->dentry->d_inode;
> > > > > +	struct cifsInodeInfo *cinode = CIFS_I(inode);
> > > > > +	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> > > > > +	int rc = 0;
> > > > > +
> > > > > +	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode)
> > > > > &&
> > > > > +
> > > > > cifs_has_mand_locks(cinode)) {
> > > > > +		cifs_dbg(FYI, "Reset oplock to None for inode=%p
> > > > > due to mand locks\n",
> > > > > +			 inode);
> > > > > +		cinode->oplock = 0;
> > > > > +	}
> > > > > +
> > > > > +	if (inode && S_ISREG(inode->i_mode)) {
> > > > > +		if (CIFS_CACHE_READ(cinode))
> > > > > +			break_lease(inode, O_RDONLY);
> > > > > +		else
> > > > > +			break_lease(inode, O_WRONLY);
> > > > > +		rc = filemap_fdatawrite(inode->i_mapping);
> > > > > +		if (!CIFS_CACHE_READ(cinode)) {
> > > > > +			rc = filemap_fdatawait(inode->i_mapping);
> > > > > +			mapping_set_error(inode->i_mapping, rc);
> > > > > +			cifs_invalidate_mapping(inode);
> > > > > +		}
> > > > > +		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n",
> > > > > inode, rc);
> > > > > +	}
> > > > > +
> > > > > +	rc = cifs_push_locks(cfile);
> > > > > +	if (rc)
> > > > > +		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
> > > > > +
> > > > > +	/*
> > > > > +	 * releasing stale oplock after recent reconnect of smb
> > > > > session using
> > > > > +	 * a now incorrect file handle is not a data integrity
> > > > > issue but do
> > > > > +	 * not bother sending an oplock release if session to
> > > > > server still is
> > > > > +	 * disconnected since oplock already released by the
> > > > > server
> > > > > +	 */
> > > > > +	if (!cfile->oplock_break_cancelled) {
> > > > > +		rc =
> > > > > tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
> > > > > +
> > > > > cinode);
> > > > > +		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
> > > > > +	}
> > > > > +}
> > > > > +
> > > > >  static void
> > > > >  smb2_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
> > > > >  		      unsigned int epoch, bool *purge_cache)
> > > > > @@ -1163,6 +1211,7 @@ struct smb_version_operations
> > > > > smb20_operations = { .create_lease_buf = smb2_create_lease_buf,
> > > > >  	.parse_lease_buf = smb2_parse_lease_buf,
> > > > >  	.clone_range = smb2_clone_range,
> > > > > +	.oplock_break = smb2_oplock_break,
> > > > >  };
> > > > >  
> > > > >  struct smb_version_operations smb21_operations = {
> > > > > @@ -1237,6 +1286,7 @@ struct smb_version_operations
> > > > > smb21_operations = { .create_lease_buf = smb2_create_lease_buf,
> > > > >  	.parse_lease_buf = smb2_parse_lease_buf,
> > > > >  	.clone_range = smb2_clone_range,
> > > > > +	.oplock_break = smb2_oplock_break,
> > > > >  };
> > > > >  
> > > > >  struct smb_version_operations smb30_operations = {
> > > > > @@ -1314,6 +1364,7 @@ struct smb_version_operations
> > > > > smb30_operations = { .parse_lease_buf = smb3_parse_lease_buf,
> > > > >  	.clone_range = smb2_clone_range,
> > > > >  	.validate_negotiate = smb3_validate_negotiate,
> > > > > +	.oplock_break = smb2_oplock_break,
> > > > >  };
> > > > >  
> > > > >  struct smb_version_values smb20_values = {
> > > > 
> > > > 
> > > 
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-cifs"
> > > in the body of a message to [e-mail]
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> > 
> 
>

Sachin Prabhu March 11, 2014, 3:05 p.m. UTC | #8

On Tue, 2014-03-11 at 07:08 -0400, Jeff Layton wrote:
> On Tue, 11 Mar 2014 10:25:49 +0000
> Sachin Prabhu <[e-mail]> wrote:
> 
> > On Mon, 2014-03-10 at 14:16 -0400, Jeffrey Layton wrote:
> > > On Mon, 10 Mar 2014 16:21:10 +0000
> > > Sachin Prabhu <[e-mail]> wrote:
> > > 
> > > > On Mon, 2014-03-10 at 11:06 -0400, Jeff Layton wrote:
> > > > > On Fri,  7 Mar 2014 13:29:19 +0000
> > > > > Sachin Prabhu <[e-mail]> wrote:
> > > > > 
> > > > > > We need to add protocol specific calls to the oplock break thread.
> > > > > > 
> > > > > 
> > > > > I'm not necessarily doubting this, but why? AFAICT, the oplock_break
> > > > > calls look more or less identical...
> > > > 
> > > > The oplock values for level 2 oplocks are different. For the 2nd fix
> > > > to work, we need to downgrade the oplock only once we are sure that no
> > > > writers are operating.
> > > > 
> > > > We need to downgrade oplocks. Since the constants used to describe
> > > > oplock states varies between cifs and smb2, we need to abstract out
> > > > the functions required to downgrade the oplocks. For this we need
> > > > version specific functions. This could either be the oplock_break
> > > > function as a whole or we could define a new function called
> > > > downgrade_oplock() in smb_version_operations. I have chosen to use
> > > > version specific oplock_break here. downgrade_oplock() could instead
> > > > be used here which would require fewer changes.
> > > > 
> > > > Should I implement this using a downgrade_oplock() function instead?
> > > > 
> > > 
> > > I guess I'm unclear on why the set_oplock_level operation (possibly
> > > combined with some new struct smb_version_values fields) wouldn't be a
> > > better solution.
> > > 
> > > I'll grant however that the oplock handing code is a bit of a mess. It
> > > seems like it could benefit from some rethink and redesign...
> > > 
> > 
> > The constants used to denote oplock values are not consistent across the
> > versions
> > 
> > In this case, a level 2 oplock for smb1 is 
> > #define OPLOCK_READ      3  /* level 2 oplock */
> > for smb2, it is
> > #define SMB2_OPLOCK_LEVEL_II            0x01
> > 
> > similarly the values for other states are different too. 
> > 
> > We could abstract these but it sounded like overkill in this situation.
> > 
> > Sachin Prabhu
> > 
> > 
> 
> Yeah, that's the part that's a mess. We have a generic set_oplock_level
> operation but that requires you to pass down oplock levels that are
> version specific. Typically when you have a generic operation, you want
> it to take a set of generic values as arguments.

I have a patch ready with the downgrade_oplock version specific
function. Since this problem leads to data corruption and needs to be
included in stable, I propose we go ahead with this version for now. The
other option to fix the set_oplock_levels() call will require a lot more
changes and may not be suitable for the stable kernels.
Once this issue has been fixed, we can work to fix set_oplock_level and
remove downgrade_oplock().

Sachin Prabhu

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index cf32f03..93a8762 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -402,6 +402,7 @@  struct smb_version_operations {
 			const struct cifs_fid *, u32 *);
 	int (*set_acl)(struct cifs_ntsd *, __u32, struct inode *, const char *,
 			int);
+	void (*oplock_break)(struct work_struct *);
 };
 
 struct smb_version_values {
@@ -1557,8 +1558,6 @@  GLOBAL_EXTERN spinlock_t uidsidlock;
 GLOBAL_EXTERN spinlock_t gidsidlock;
 #endif /* CONFIG_CIFS_ACL */
 
-void cifs_oplock_break(struct work_struct *work);
-
 extern const struct slow_work_ops cifs_oplock_break_ops;
 extern struct workqueue_struct *cifsiod_wq;
 
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index acc4ee8..e4d0add 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -130,6 +130,8 @@  extern void cifs_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock);
 extern int cifs_unlock_range(struct cifsFileInfo *cfile,
 			     struct file_lock *flock, const unsigned int xid);
 extern int cifs_push_mandatory_locks(struct cifsFileInfo *cfile);
+extern int cifs_push_locks(struct cifsFileInfo *cfile);
+bool cifs_has_mand_locks(struct cifsInodeInfo *cinode);
 
 extern struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid,
 					      struct file *file,
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 53c1507..998dec7 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -251,7 +251,7 @@  out:
 	return rc;
 }
 
-static bool
+bool
 cifs_has_mand_locks(struct cifsInodeInfo *cinode)
 {
 	struct cifs_fid_locks *cur;
@@ -304,7 +304,7 @@  cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
 	cfile->f_flags = file->f_flags;
 	cfile->invalidHandle = false;
 	cfile->tlink = cifs_get_tlink(tlink);
-	INIT_WORK(&cfile->oplock_break, cifs_oplock_break);
+	INIT_WORK(&cfile->oplock_break, server->ops->oplock_break);
 	mutex_init(&cfile->fh_mutex);
 
 	cifs_sb_active(inode->i_sb);
@@ -1204,7 +1204,7 @@  err_out:
 	goto out;
 }
 
-static int
+int
 cifs_push_locks(struct cifsFileInfo *cfile)
 {
 	struct cifs_sb_info *cifs_sb = CIFS_SB(cfile->dentry->d_sb);
@@ -3656,53 +3656,6 @@  static int cifs_launder_page(struct page *page)
 	return rc;
 }
 
-void cifs_oplock_break(struct work_struct *work)
-{
-	struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
-						  oplock_break);
-	struct inode *inode = cfile->dentry->d_inode;
-	struct cifsInodeInfo *cinode = CIFS_I(inode);
-	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
-	int rc = 0;
-
-	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
-						cifs_has_mand_locks(cinode)) {
-		cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
-			 inode);
-		cinode->oplock = 0;
-	}
-
-	if (inode && S_ISREG(inode->i_mode)) {
-		if (CIFS_CACHE_READ(cinode))
-			break_lease(inode, O_RDONLY);
-		else
-			break_lease(inode, O_WRONLY);
-		rc = filemap_fdatawrite(inode->i_mapping);
-		if (!CIFS_CACHE_READ(cinode)) {
-			rc = filemap_fdatawait(inode->i_mapping);
-			mapping_set_error(inode->i_mapping, rc);
-			cifs_invalidate_mapping(inode);
-		}
-		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
-	}
-
-	rc = cifs_push_locks(cfile);
-	if (rc)
-		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
-
-	/*
-	 * releasing stale oplock after recent reconnect of smb session using
-	 * a now incorrect file handle is not a data integrity issue but do
-	 * not bother sending an oplock release if session to server still is
-	 * disconnected since oplock already released by the server
-	 */
-	if (!cfile->oplock_break_cancelled) {
-		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
-							     cinode);
-		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
-	}
-}
-
 /*
  * The presence of cifs_direct_io() in the address space ops vector
  * allowes open() O_DIRECT flags which would have failed otherwise.
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index 526fb89..346ee2a 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -23,6 +23,7 @@ 
 #include "cifsproto.h"
 #include "cifs_debug.h"
 #include "cifspdu.h"
+#include "cifsfs.h"
 
 /*
  * An NT cancel request header looks just like the original request except:
@@ -999,6 +1000,53 @@  cifs_is_read_op(__u32 oplock)
 	return oplock == OPLOCK_READ;
 }
 
+static void cifs_oplock_break(struct work_struct *work)
+{
+	struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
+						  oplock_break);
+	struct inode *inode = cfile->dentry->d_inode;
+	struct cifsInodeInfo *cinode = CIFS_I(inode);
+	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
+	int rc = 0;
+
+	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
+						cifs_has_mand_locks(cinode)) {
+		cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
+			 inode);
+		cinode->oplock = 0;
+	}
+
+	if (inode && S_ISREG(inode->i_mode)) {
+		if (CIFS_CACHE_READ(cinode))
+			break_lease(inode, O_RDONLY);
+		else
+			break_lease(inode, O_WRONLY);
+		rc = filemap_fdatawrite(inode->i_mapping);
+		if (!CIFS_CACHE_READ(cinode)) {
+			rc = filemap_fdatawait(inode->i_mapping);
+			mapping_set_error(inode->i_mapping, rc);
+			cifs_invalidate_mapping(inode);
+		}
+		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
+	}
+
+	rc = cifs_push_locks(cfile);
+	if (rc)
+		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
+
+	/*
+	 * releasing stale oplock after recent reconnect of smb session using
+	 * a now incorrect file handle is not a data integrity issue but do
+	 * not bother sending an oplock release if session to server still is
+	 * disconnected since oplock already released by the server
+	 */
+	if (!cfile->oplock_break_cancelled) {
+		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
+							     cinode);
+		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
+	}
+}
+
 struct smb_version_operations smb1_operations = {
 	.send_cancel = send_nt_cancel,
 	.compare_fids = cifs_compare_fids,
@@ -1067,6 +1115,7 @@  struct smb_version_operations smb1_operations = {
 	.query_mf_symlink = cifs_query_mf_symlink,
 	.create_mf_symlink = cifs_create_mf_symlink,
 	.is_read_op = cifs_is_read_op,
+	.oplock_break = cifs_oplock_break,
 #ifdef CONFIG_CIFS_XATTR
 	.query_all_EAs = CIFSSMBQAllEAs,
 	.set_EA = CIFSSMBSetEA,
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 192f51a..e7081cd 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -27,6 +27,7 @@ 
 #include "cifs_unicode.h"
 #include "smb2status.h"
 #include "smb2glob.h"
+#include "cifsfs.h"
 
 static int
 change_conf(struct TCP_Server_Info *server)
@@ -904,6 +905,53 @@  smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
 	return rc;
 }
 
+static void smb2_oplock_break(struct work_struct *work)
+{
+	struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
+						  oplock_break);
+	struct inode *inode = cfile->dentry->d_inode;
+	struct cifsInodeInfo *cinode = CIFS_I(inode);
+	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
+	int rc = 0;
+
+	if (!CIFS_CACHE_WRITE(cinode) && CIFS_CACHE_READ(cinode) &&
+						cifs_has_mand_locks(cinode)) {
+		cifs_dbg(FYI, "Reset oplock to None for inode=%p due to mand locks\n",
+			 inode);
+		cinode->oplock = 0;
+	}
+
+	if (inode && S_ISREG(inode->i_mode)) {
+		if (CIFS_CACHE_READ(cinode))
+			break_lease(inode, O_RDONLY);
+		else
+			break_lease(inode, O_WRONLY);
+		rc = filemap_fdatawrite(inode->i_mapping);
+		if (!CIFS_CACHE_READ(cinode)) {
+			rc = filemap_fdatawait(inode->i_mapping);
+			mapping_set_error(inode->i_mapping, rc);
+			cifs_invalidate_mapping(inode);
+		}
+		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
+	}
+
+	rc = cifs_push_locks(cfile);
+	if (rc)
+		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
+
+	/*
+	 * releasing stale oplock after recent reconnect of smb session using
+	 * a now incorrect file handle is not a data integrity issue but do
+	 * not bother sending an oplock release if session to server still is
+	 * disconnected since oplock already released by the server
+	 */
+	if (!cfile->oplock_break_cancelled) {
+		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
+							     cinode);
+		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
+	}
+}
+
 static void
 smb2_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
 		      unsigned int epoch, bool *purge_cache)
@@ -1163,6 +1211,7 @@  struct smb_version_operations smb20_operations = {
 	.create_lease_buf = smb2_create_lease_buf,
 	.parse_lease_buf = smb2_parse_lease_buf,
 	.clone_range = smb2_clone_range,
+	.oplock_break = smb2_oplock_break,
 };
 
 struct smb_version_operations smb21_operations = {
@@ -1237,6 +1286,7 @@  struct smb_version_operations smb21_operations = {
 	.create_lease_buf = smb2_create_lease_buf,
 	.parse_lease_buf = smb2_parse_lease_buf,
 	.clone_range = smb2_clone_range,
+	.oplock_break = smb2_oplock_break,
 };
 
 struct smb_version_operations smb30_operations = {
@@ -1314,6 +1364,7 @@  struct smb_version_operations smb30_operations = {
 	.parse_lease_buf = smb3_parse_lease_buf,
 	.clone_range = smb2_clone_range,
 	.validate_negotiate = smb3_validate_negotiate,
+	.oplock_break = smb2_oplock_break,
 };
 
 struct smb_version_values smb20_values = {