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 <sprabhu@xxxxxxxxxx> wrote:
> > On Mon, 2014-03-10 at 11:06 -0400, Jeff Layton wrote:
> >> On Fri, 7 Mar 2014 13:29:19 +0000
> >> Sachin Prabhu <sprabhu@xxxxxxxxxx> 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 <sprabhu@xxxxxxxxxx>
> >> > ---
> >> > 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 majordomo@xxxxxxxxxxxxxxx
> > 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
