On Mon, 2014-03-10 at 14:16 -0400, Jeffrey Layton wrote:
> On Mon, 10 Mar 2014 16:21:10 +0000
> 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?
> >
>
> 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 <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
