[PATCH 0/4] cifs: alternate approach to fixing oplock queue races

[Jeff Layton-2]

This patchset is an alternate approach to fixing the oplock queue
problems. Rather than tracking oplocks with separate structures, this
patchset adds some fields to cifsFileInfo.

When an oplock break comes in, is_valid_oplock_break takes an extra
reference to the cifsFileInfo and queues the oplock break job to the
events workqueue.

This works around the main drawback of the old set, which was that an
oplock break could essentially be ignored if the allocation failed.

This set represents a pretty substantial code reduction since the
dedicated oplock thread is no longer needed with it.

Jeff Layton (4):
  cifs: remove cifsInodeInfo.oplockPending flag
  cifs: take read lock on GlobalSMBSes_lock in is_valid_oplock_break
  cifs: have cifsFileInfo hold an extra inode reference
  cifs: turn oplock breaks into a workqueue job

 fs/cifs/cifsfs.c    |   91 ---------------------------------------------------
 fs/cifs/cifsglob.h  |   15 +++-----
 fs/cifs/cifsproto.h |    5 +--
 fs/cifs/cifssmb.c   |    1 +
 fs/cifs/connect.c   |    1 -
 fs/cifs/dir.c       |    4 ++-
 fs/cifs/file.c      |   52 +++++++++++++++++++++++++++--
 fs/cifs/misc.c      |   27 +++++++++------
 fs/cifs/transport.c |   50 ----------------------------
 9 files changed, 76 insertions(+), 170 deletions(-)

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

It's set on oplock break but nothing ever looks at it.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/cifsglob.h |    1 -
 fs/cifs/misc.c     |    1 -
 2 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 6cfc81a..b8d673c 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -382,7 +382,6 @@ struct cifsInodeInfo {
  unsigned long time; /* jiffies of last update/check of inode */
  bool clientCanCacheRead:1; /* read oplock */
  bool clientCanCacheAll:1; /* read and writebehind oplock */
- bool oplockPending:1;
  bool delete_pending:1; /* DELETE_ON_CLOSE is set */
  u64  server_eof; /* current file size on server */
  u64  uniqueid; /* server inode number */
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index e079a91..f2d508d 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -576,7 +576,6 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
  pCifsInode->clientCanCacheAll = false;
  if (pSMB->OplockLevel == 0)
  pCifsInode->clientCanCacheRead = false;
- pCifsInode->oplockPending = true;
  AllocOplockQEntry(netfile->pInode,
   netfile->netfid, tcon);
  cFYI(1, ("about to wake up oplock thread"));
--
1.6.0.6

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

...rather than a write lock. It doesn't change the list so a read lock
should be sufficient.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/misc.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index f2d508d..191e622 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -562,14 +562,14 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
  continue;
 
  cifs_stats_inc(&tcon->num_oplock_brks);
- write_lock(&GlobalSMBSeslock);
+ read_lock(&GlobalSMBSeslock);
  list_for_each(tmp2, &tcon->openFileList) {
  netfile = list_entry(tmp2, struct cifsFileInfo,
      tlist);
  if (pSMB->Fid != netfile->netfid)
  continue;
 
- write_unlock(&GlobalSMBSeslock);
+ read_unlock(&GlobalSMBSeslock);
  read_unlock(&cifs_tcp_ses_lock);
  cFYI(1, ("file id match, oplock break"));
  pCifsInode = CIFS_I(netfile->pInode);
@@ -584,7 +584,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
 
  return true;
  }
- write_unlock(&GlobalSMBSeslock);
+ read_unlock(&GlobalSMBSeslock);
  read_unlock(&cifs_tcp_ses_lock);
  cFYI(1, ("No matching file for oplock break"));
  return true;
--
1.6.0.6

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

It's possible that this struct will outlive the filp to which it is
attached. If it does and it needs to do some work on the inode, then
it'll need a reference.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/cifsglob.h |    4 +++-
 fs/cifs/dir.c      |    2 +-
 fs/cifs/file.c     |    2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index b8d673c..908b2de 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -365,8 +365,10 @@ static inline void cifsFileInfo_get(struct cifsFileInfo *cifs_file)
 /* Release a reference on the file private data */
 static inline void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 {
- if (atomic_dec_and_test(&cifs_file->count))
+ if (atomic_dec_and_test(&cifs_file->count)) {
+ iput(cifs_file->pInode);
  kfree(cifs_file);
+ }
 }
 
 /*
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index a6424cf..6b45feb 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -147,7 +147,7 @@ cifs_fill_fileinfo(struct inode *newinode, __u16 fileHandle,
 
  pCifsFile->netfid = fileHandle;
  pCifsFile->pid = current->tgid;
- pCifsFile->pInode = newinode;
+ pCifsFile->pInode = igrab(newinode);
  pCifsFile->invalidHandle = false;
  pCifsFile->closePend = false;
  mutex_init(&pCifsFile->fh_mutex);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index fa7beac..9498d2c 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -50,7 +50,7 @@ static inline struct cifsFileInfo *cifs_init_private(
  mutex_init(&private_data->lock_mutex);
  INIT_LIST_HEAD(&private_data->llist);
  private_data->pfile = file; /* needed for writepage */
- private_data->pInode = inode;
+ private_data->pInode = igrab(inode);
  private_data->invalidHandle = false;
  private_data->closePend = false;
  /* Initialize reference count to one.  The private data is
--
1.6.0.6

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

Currently, when an oplock break comes in there's a chance that the
oplock break job won't occur if the allocation of the oplock_q_entry
fails. There are also some rather nasty races in the allocation and
handling these structs.

Rather than allocating oplock queue entries when an oplock break comes
in, add a few extra fields to the cifsFileInfo struct. Get rid of the
dedicated cifs_oplock_thread as well and simply do a schedule_work()
to the global events workqueue when an oplock break comes in.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/cifsfs.c    |   91 ---------------------------------------------------
 fs/cifs/cifsglob.h  |   10 ++----
 fs/cifs/cifsproto.h |    5 +--
 fs/cifs/cifssmb.c   |    1 +
 fs/cifs/connect.c   |    1 -
 fs/cifs/dir.c       |    2 +
 fs/cifs/file.c      |   50 +++++++++++++++++++++++++++-
 fs/cifs/misc.c      |   20 +++++++----
 fs/cifs/transport.c |   50 ----------------------------
 9 files changed, 68 insertions(+), 162 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 3610e99..c4872ad 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -64,9 +64,6 @@ unsigned int multiuser_mount = 0;
 unsigned int extended_security = CIFSSEC_DEF;
 /* unsigned int ntlmv2_support = 0; */
 unsigned int sign_CIFS_PDUs = 1;
-extern struct task_struct *oplockThread; /* remove sparse warning */
-struct task_struct *oplockThread = NULL;
-/* extern struct task_struct * dnotifyThread; remove sparse warning */
 static const struct super_operations cifs_super_ops;
 unsigned int CIFSMaxBufSize = CIFS_MAX_MSGSIZE;
 module_param(CIFSMaxBufSize, int, 0);
@@ -973,89 +970,12 @@ cifs_destroy_mids(void)
  kmem_cache_destroy(cifs_oplock_cachep);
 }
 
-static int cifs_oplock_thread(void *dummyarg)
-{
- struct oplock_q_entry *oplock_item;
- struct cifsTconInfo *pTcon;
- struct inode *inode;
- __u16  netfid;
- int rc, waitrc = 0;
-
- set_freezable();
- do {
- if (try_to_freeze())
- continue;
-
- spin_lock(&cifs_oplock_lock);
- if (list_empty(&cifs_oplock_list)) {
- spin_unlock(&cifs_oplock_lock);
- set_current_state(TASK_INTERRUPTIBLE);
- schedule_timeout(39*HZ);
- } else {
- oplock_item = list_entry(cifs_oplock_list.next,
- struct oplock_q_entry, qhead);
- cFYI(1, ("found oplock item to write out"));
- pTcon = oplock_item->tcon;
- inode = oplock_item->pinode;
- netfid = oplock_item->netfid;
- spin_unlock(&cifs_oplock_lock);
- DeleteOplockQEntry(oplock_item);
- /* can not grab inode sem here since it would
- deadlock when oplock received on delete
- since vfs_unlink holds the i_mutex across
- the call */
- /* mutex_lock(&inode->i_mutex);*/
- if (S_ISREG(inode->i_mode)) {
-#ifdef CONFIG_CIFS_EXPERIMENTAL
- if (CIFS_I(inode)->clientCanCacheAll == 0)
- break_lease(inode, FMODE_READ);
- else if (CIFS_I(inode)->clientCanCacheRead == 0)
- break_lease(inode, FMODE_WRITE);
-#endif
- rc = filemap_fdatawrite(inode->i_mapping);
- if (CIFS_I(inode)->clientCanCacheRead == 0) {
- waitrc = filemap_fdatawait(
-      inode->i_mapping);
- invalidate_remote_inode(inode);
- }
- if (rc == 0)
- rc = waitrc;
- } else
- rc = 0;
- /* mutex_unlock(&inode->i_mutex);*/
- if (rc)
- CIFS_I(inode)->write_behind_rc = rc;
- cFYI(1, ("Oplock flush inode %p rc %d",
- inode, 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 in that case */
- if (!pTcon->need_reconnect) {
- rc = CIFSSMBLock(0, pTcon, netfid,
- 0 /* len */ , 0 /* offset */, 0,
- 0, LOCKING_ANDX_OPLOCK_RELEASE,
- false /* wait flag */);
- cFYI(1, ("Oplock release rc = %d", rc));
- }
- set_current_state(TASK_INTERRUPTIBLE);
- schedule_timeout(1);  /* yield in case q were corrupt */
- }
- } while (!kthread_should_stop());
-
- return 0;
-}
-
 static int __init
 init_cifs(void)
 {
  int rc = 0;
  cifs_proc_init();
  INIT_LIST_HEAD(&cifs_tcp_ses_list);
- INIT_LIST_HEAD(&cifs_oplock_list);
 #ifdef CONFIG_CIFS_EXPERIMENTAL
  INIT_LIST_HEAD(&GlobalDnotifyReqList);
  INIT_LIST_HEAD(&GlobalDnotifyRsp_Q);
@@ -1084,7 +1004,6 @@ init_cifs(void)
  rwlock_init(&GlobalSMBSeslock);
  rwlock_init(&cifs_tcp_ses_lock);
  spin_lock_init(&GlobalMid_Lock);
- spin_lock_init(&cifs_oplock_lock);
 
  if (cifs_max_pending < 2) {
  cifs_max_pending = 2;
@@ -1119,18 +1038,9 @@ init_cifs(void)
  if (rc)
  goto out_unregister_key_type;
 #endif
- oplockThread = kthread_run(cifs_oplock_thread, NULL, "cifsoplockd");
- if (IS_ERR(oplockThread)) {
- rc = PTR_ERR(oplockThread);
- cERROR(1, ("error %d create oplock thread", rc));
- goto out_unregister_dfs_key_type;
- }
-
  return 0;
 
- out_unregister_dfs_key_type:
 #ifdef CONFIG_CIFS_DFS_UPCALL
- unregister_key_type(&key_type_dns_resolver);
  out_unregister_key_type:
 #endif
 #ifdef CONFIG_CIFS_UPCALL
@@ -1165,7 +1075,6 @@ exit_cifs(void)
  cifs_destroy_inodecache();
  cifs_destroy_mids();
  cifs_destroy_request_bufs();
- kthread_stop(oplockThread);
 }
 
 MODULE_AUTHOR("Steve French <sfrench@...>");
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 908b2de..2be0471 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -346,14 +346,16 @@ struct cifsFileInfo {
  /* lock scope id (0 if none) */
  struct file *pfile; /* needed for writepage */
  struct inode *pInode; /* needed for oplock break */
+ struct cifsTconInfo *tcon;
  struct mutex lock_mutex;
  struct list_head llist; /* list of byte range locks we have. */
  bool closePend:1; /* file is marked to close */
  bool invalidHandle:1; /* file closed via session abend */
- bool messageMode:1; /* for pipes: message vs byte mode */
+ bool oplock_break_cancelled:1;
  atomic_t count; /* reference count */
  struct mutex fh_mutex; /* prevents reopen race after dead ses*/
  struct cifs_search_info srch_inf;
+ struct work_struct oplock_break; /* workqueue job for oplock breaks */
 };
 
 /* Take a reference on the file private data */
@@ -670,12 +672,6 @@ GLOBAL_EXTERN rwlock_t cifs_tcp_ses_lock;
  */
 GLOBAL_EXTERN rwlock_t GlobalSMBSeslock;
 
-/* Global list of oplocks */
-GLOBAL_EXTERN struct list_head cifs_oplock_list;
-
-/* Protects the cifs_oplock_list */
-GLOBAL_EXTERN spinlock_t cifs_oplock_lock;
-
 /* Outstanding dir notify requests */
 GLOBAL_EXTERN struct list_head GlobalDnotifyReqList;
 /* DirNotify response queue */
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index da8fbf5..b063802 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -86,12 +86,9 @@ extern int CIFS_SessSetup(unsigned int xid, struct cifsSesInfo *ses,
      const int stage,
      const struct nls_table *nls_cp);
 extern __u16 GetNextMid(struct TCP_Server_Info *server);
-extern struct oplock_q_entry *AllocOplockQEntry(struct inode *, u16,
- struct cifsTconInfo *);
-extern void DeleteOplockQEntry(struct oplock_q_entry *);
-extern void DeleteTconOplockQEntries(struct cifsTconInfo *);
 extern struct timespec cifs_NTtimeToUnix(__le64 utc_nanoseconds_since_1601);
 extern u64 cifs_UnixTimeToNT(struct timespec);
+extern void cifs_oplock_break(struct work_struct *work);
 extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time,
       int offset);
 
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 301e307..941441d 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -94,6 +94,7 @@ static void mark_open_files_invalid(struct cifsTconInfo *pTcon)
  list_for_each_safe(tmp, tmp1, &pTcon->openFileList) {
  open_file = list_entry(tmp, struct cifsFileInfo, tlist);
  open_file->invalidHandle = true;
+ open_file->oplock_break_cancelled = true;
  }
  write_unlock(&GlobalSMBSeslock);
  /* BB Add call to invalidate_inodes(sb) for all superblocks mounted
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 9c91c8b..e8b72b8 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1664,7 +1664,6 @@ cifs_put_tcon(struct cifsTconInfo *tcon)
  CIFSSMBTDis(xid, tcon);
  _FreeXid(xid);
 
- DeleteTconOplockQEntries(tcon);
  tconInfoFree(tcon);
  cifs_put_smb_ses(ses);
 }
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 6b45feb..5d70d7a 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -148,12 +148,14 @@ cifs_fill_fileinfo(struct inode *newinode, __u16 fileHandle,
  pCifsFile->netfid = fileHandle;
  pCifsFile->pid = current->tgid;
  pCifsFile->pInode = igrab(newinode);
+ pCifsFile->tcon = tcon;
  pCifsFile->invalidHandle = false;
  pCifsFile->closePend = false;
  mutex_init(&pCifsFile->fh_mutex);
  mutex_init(&pCifsFile->lock_mutex);
  INIT_LIST_HEAD(&pCifsFile->llist);
  atomic_set(&pCifsFile->count, 1);
+ INIT_WORK(&pCifsFile->oplock_break, cifs_oplock_break);
 
  /* set the following in open now
  pCifsFile->pfile = file; */
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 9498d2c..aae5870 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -41,7 +41,7 @@
 
 static inline struct cifsFileInfo *cifs_init_private(
  struct cifsFileInfo *private_data, struct inode *inode,
- struct file *file, __u16 netfid)
+ struct cifsTconInfo *tcon, struct file *file, __u16 netfid)
 {
  memset(private_data, 0, sizeof(struct cifsFileInfo));
  private_data->netfid = netfid;
@@ -51,11 +51,13 @@ static inline struct cifsFileInfo *cifs_init_private(
  INIT_LIST_HEAD(&private_data->llist);
  private_data->pfile = file; /* needed for writepage */
  private_data->pInode = igrab(inode);
+ private_data->tcon = tcon;
  private_data->invalidHandle = false;
  private_data->closePend = false;
  /* Initialize reference count to one.  The private data is
  freed on the release of the last reference */
  atomic_set(&private_data->count, 1);
+ INIT_WORK(&private_data->oplock_break, cifs_oplock_break);
 
  return private_data;
 }
@@ -420,7 +422,8 @@ int cifs_open(struct inode *inode, struct file *file)
  rc = -ENOMEM;
  goto out;
  }
- pCifsFile = cifs_init_private(file->private_data, inode, file, netfid);
+ pCifsFile = cifs_init_private(file->private_data, inode, tcon, file,
+ netfid);
  write_lock(&GlobalSMBSeslock);
  list_add(&pCifsFile->tlist, &tcon->openFileList);
 
@@ -2308,6 +2311,49 @@ out:
  return rc;
 }
 
+void
+cifs_oplock_break(struct work_struct *work)
+{
+ struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
+  oplock_break);
+ struct inode *inode = cfile->pInode;
+ struct cifsInodeInfo *cinode = CIFS_I(inode);
+ int rc, waitrc = 0;
+
+ if (inode && S_ISREG(inode->i_mode)) {
+#ifdef CONFIG_CIFS_EXPERIMENTAL
+ if (cinode->clientCanCacheAll == 0)
+ break_lease(inode, FMODE_READ);
+ else if (cinode->clientCanCacheRead == 0)
+ break_lease(inode, FMODE_WRITE);
+#endif
+ rc = filemap_fdatawrite(inode->i_mapping);
+ if (cinode->clientCanCacheRead == 0) {
+ waitrc = filemap_fdatawait(inode->i_mapping);
+ invalidate_remote_inode(inode);
+ }
+ if (!rc)
+ rc = waitrc;
+ if (rc)
+ cinode->write_behind_rc = rc;
+ cFYI(1, ("Oplock flush inode %p rc %d", inode, 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->closePend && !cfile->oplock_break_cancelled) {
+ rc = CIFSSMBLock(0, cfile->tcon, cfile->netfid, 0, 0, 0, 0,
+ LOCKING_ANDX_OPLOCK_RELEASE, false);
+ cFYI(1, ("Oplock release rc = %d", rc));
+ }
+
+ cifsFileInfo_put(cfile);
+}
+
 const struct address_space_operations cifs_addr_ops = {
  .readpage = cifs_readpage,
  .readpages = cifs_readpages,
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 191e622..8e098e6 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -32,7 +32,6 @@
 
 extern mempool_t *cifs_sm_req_poolp;
 extern mempool_t *cifs_req_poolp;
-extern struct task_struct *oplockThread;
 
 /* The xid serves as a useful identifier for each incoming vfs request,
    in a similar way to the mid which is useful to track each sent smb,
@@ -569,6 +568,17 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
  if (pSMB->Fid != netfile->netfid)
  continue;
 
+ /*
+ * don't do anything if file is about to be
+ * closed anyway.
+ */
+ if (netfile->closePend) {
+ read_unlock(&GlobalSMBSeslock);
+ read_unlock(&cifs_tcp_ses_lock);
+ return true;
+ }
+
+ cifsFileInfo_get(netfile);
  read_unlock(&GlobalSMBSeslock);
  read_unlock(&cifs_tcp_ses_lock);
  cFYI(1, ("file id match, oplock break"));
@@ -576,12 +586,8 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
  pCifsInode->clientCanCacheAll = false;
  if (pSMB->OplockLevel == 0)
  pCifsInode->clientCanCacheRead = false;
- AllocOplockQEntry(netfile->pInode,
-  netfile->netfid, tcon);
- cFYI(1, ("about to wake up oplock thread"));
- if (oplockThread)
- wake_up_process(oplockThread);
-
+ if (schedule_work(&netfile->oplock_break))
+ netfile->oplock_break_cancelled = false;
  return true;
  }
  read_unlock(&GlobalSMBSeslock);
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 1da4ab2..07b8e71 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -103,56 +103,6 @@ DeleteMidQEntry(struct mid_q_entry *midEntry)
  mempool_free(midEntry, cifs_mid_poolp);
 }
 
-struct oplock_q_entry *
-AllocOplockQEntry(struct inode *pinode, __u16 fid, struct cifsTconInfo *tcon)
-{
- struct oplock_q_entry *temp;
- if ((pinode == NULL) || (tcon == NULL)) {
- cERROR(1, ("Null parms passed to AllocOplockQEntry"));
- return NULL;
- }
- temp = (struct oplock_q_entry *) kmem_cache_alloc(cifs_oplock_cachep,
-       GFP_KERNEL);
- if (temp == NULL)
- return temp;
- else {
- temp->pinode = pinode;
- temp->tcon = tcon;
- temp->netfid = fid;
- spin_lock(&cifs_oplock_lock);
- list_add_tail(&temp->qhead, &cifs_oplock_list);
- spin_unlock(&cifs_oplock_lock);
- }
- return temp;
-}
-
-void DeleteOplockQEntry(struct oplock_q_entry *oplockEntry)
-{
- spin_lock(&cifs_oplock_lock);
-    /* should we check if list empty first? */
- list_del(&oplockEntry->qhead);
- spin_unlock(&cifs_oplock_lock);
- kmem_cache_free(cifs_oplock_cachep, oplockEntry);
-}
-
-
-void DeleteTconOplockQEntries(struct cifsTconInfo *tcon)
-{
- struct oplock_q_entry *temp;
-
- if (tcon == NULL)
- return;
-
- spin_lock(&cifs_oplock_lock);
- list_for_each_entry(temp, &cifs_oplock_list, qhead) {
- if ((temp->tcon) && (temp->tcon == tcon)) {
- list_del(&temp->qhead);
- kmem_cache_free(cifs_oplock_cachep, temp);
- }
- }
- spin_unlock(&cifs_oplock_lock);
-}
-
 static int
 smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
 {
--
1.6.0.6

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Steve French-2]

On Tue, Sep 8, 2009 at 9:12 AM, Jeff Layton<jlayton@...> wrote:
> This patchset is an alternate approach to fixing the oplock queue
> problems. Rather than tracking oplocks with separate structures, this
> patchset adds some fields to cifsFileInfo.
>
> When an oplock break comes in, is_valid_oplock_break takes an extra
> reference to the cifsFileInfo and queues the oplock break job to the
> events workqueue.

This looks excellent - the 4th patch has to be tried carefully but
easier to read than alternatives.   Any idea when the schedule_work
would get dispatched (is the current task put to sleep immediately?)

--
Thanks,

Steve
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

On Tue, 8 Sep 2009 12:12:22 -0500
Steve French <smfrench@...> wrote:

> On Tue, Sep 8, 2009 at 9:12 AM, Jeff Layton<jlayton@...> wrote:
> > This patchset is an alternate approach to fixing the oplock queue
> > problems. Rather than tracking oplocks with separate structures, this
> > patchset adds some fields to cifsFileInfo.
> >
> > When an oplock break comes in, is_valid_oplock_break takes an extra
> > reference to the cifsFileInfo and queues the oplock break job to the
> > events workqueue.
>
> This looks excellent - the 4th patch has to be tried carefully but
> easier to read than alternatives.   Any idea when the schedule_work
> would get dispatched (is the current task put to sleep immediately?)
>

When it actually runs is sort of dependent on what other jobs are
running in queue, how many CPU's are in the box etc. In practice, I
don't think this set will materially change when the oplock break
actually happens, unless there is a lot of stuff sitting in the events
queue beforehand.

It may also help performance if a lot of oplock breaks come in at once
since they won't be serialized. You could have one running for each CPU
you have.

Another thing we could consider is using the new slow_work stuff that
dhowells added recently. An oplock break task could take a while to
run if there is a lot of data to be flushed, so that may be a better
scheme (though it does add a build-time kconfig dependency to CIFS).

LWN has a good writeup on the slow_work infrastructure:

http://lwn.net/Articles/329464/

--
Jeff Layton <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

On Tue, 8 Sep 2009 13:28:58 -0400
Jeff Layton <jlayton@...> wrote:

> On Tue, 8 Sep 2009 12:12:22 -0500
> Steve French <smfrench@...> wrote:
>
> > On Tue, Sep 8, 2009 at 9:12 AM, Jeff Layton<jlayton@...> wrote:
> > > This patchset is an alternate approach to fixing the oplock queue
> > > problems. Rather than tracking oplocks with separate structures, this
> > > patchset adds some fields to cifsFileInfo.
> > >
> > > When an oplock break comes in, is_valid_oplock_break takes an extra
> > > reference to the cifsFileInfo and queues the oplock break job to the
> > > events workqueue.
> >
> > This looks excellent - the 4th patch has to be tried carefully but
> > easier to read than alternatives.   Any idea when the schedule_work
> > would get dispatched (is the current task put to sleep immediately?)
> >
>
> When it actually runs is sort of dependent on what other jobs are
> running in queue, how many CPU's are in the box etc. In practice, I
> don't think this set will materially change when the oplock break
> actually happens, unless there is a lot of stuff sitting in the events
> queue beforehand.
>
> It may also help performance if a lot of oplock breaks come in at once
> since they won't be serialized. You could have one running for each CPU
> you have.
>
> Another thing we could consider is using the new slow_work stuff that
> dhowells added recently. An oplock break task could take a while to
> run if there is a lot of data to be flushed, so that may be a better
> scheme (though it does add a build-time kconfig dependency to CIFS).
>
> LWN has a good writeup on the slow_work infrastructure:
>
> http://lwn.net/Articles/329464/
>

Here's a quick conversion to slow_work that I did this morning. This
patch applies on top of the set I sent yesterday. Tested with
connectathon and seems to work correctly (sniffing traffic showed the
oplock break response go out onto the wire).

I like this better than queueing to the events queue -- seems less
likely to block more critical tasks.

Thoughts?
--
Jeff Layton <jlayton@...>

---

From 46ae620b364db83b3db79508364303d4b01780b3 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@...>
Date: Wed, 9 Sep 2009 11:44:16 -0400
Subject: [PATCH 5/4] cifs: convert oplock_break job to slow_work

Seems like a good candidate for this.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/Kconfig     |    1 +
 fs/cifs/cifsfs.c    |    6 ++++++
 fs/cifs/cifsglob.h  |    4 +++-
 fs/cifs/cifsproto.h |    1 -
 fs/cifs/dir.c       |    3 ++-
 fs/cifs/file.c      |   27 ++++++++++++++++++++++++---
 fs/cifs/misc.c      |   13 +++++++++----
 7 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig
index 6994a0f..80f3525 100644
--- a/fs/cifs/Kconfig
+++ b/fs/cifs/Kconfig
@@ -2,6 +2,7 @@ config CIFS
  tristate "CIFS support (advanced network filesystem, SMBFS successor)"
  depends on INET
  select NLS
+ select SLOW_WORK
  help
   This is the client VFS module for the Common Internet File System
   (CIFS) protocol which is the successor to the Server Message Block
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index c4872ad..89142b3 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1038,8 +1038,14 @@ init_cifs(void)
  if (rc)
  goto out_unregister_key_type;
 #endif
+ rc = slow_work_register_user();
+ if (rc)
+ goto out_unregister_resolver_key;
+
  return 0;
 
+ out_unregister_resolver_key:
+ unregister_key_type(&key_type_dns_resolver);
 #ifdef CONFIG_CIFS_DFS_UPCALL
  out_unregister_key_type:
 #endif
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 2be0471..c3280fa 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -18,6 +18,7 @@
  */
 #include <linux/in.h>
 #include <linux/in6.h>
+#include <linux/slow-work.h>
 #include "cifs_fs_sb.h"
 #include "cifsacl.h"
 /*
@@ -355,7 +356,7 @@ struct cifsFileInfo {
  atomic_t count; /* reference count */
  struct mutex fh_mutex; /* prevents reopen race after dead ses*/
  struct cifs_search_info srch_inf;
- struct work_struct oplock_break; /* workqueue job for oplock breaks */
+ struct slow_work oplock_break; /* slow_work job for oplock breaks */
 };
 
 /* Take a reference on the file private data */
@@ -722,3 +723,4 @@ GLOBAL_EXTERN unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
 GLOBAL_EXTERN unsigned int cifs_min_small;  /* min size of small buf pool */
 GLOBAL_EXTERN unsigned int cifs_max_pending; /* MAX requests at once to server*/
 
+extern const struct slow_work_ops cifs_oplock_break_ops;
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index b063802..f2a6241 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -88,7 +88,6 @@ extern int CIFS_SessSetup(unsigned int xid, struct cifsSesInfo *ses,
 extern __u16 GetNextMid(struct TCP_Server_Info *server);
 extern struct timespec cifs_NTtimeToUnix(__le64 utc_nanoseconds_since_1601);
 extern u64 cifs_UnixTimeToNT(struct timespec);
-extern void cifs_oplock_break(struct work_struct *work);
 extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time,
       int offset);
 
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 5d70d7a..ae09e5c 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -31,6 +31,7 @@
 #include "cifs_debug.h"
 #include "cifs_fs_sb.h"
 
+
 static void
 renew_parental_timestamps(struct dentry *direntry)
 {
@@ -155,7 +156,7 @@ cifs_fill_fileinfo(struct inode *newinode, __u16 fileHandle,
  mutex_init(&pCifsFile->lock_mutex);
  INIT_LIST_HEAD(&pCifsFile->llist);
  atomic_set(&pCifsFile->count, 1);
- INIT_WORK(&pCifsFile->oplock_break, cifs_oplock_break);
+ slow_work_init(&pCifsFile->oplock_break, &cifs_oplock_break_ops);
 
  /* set the following in open now
  pCifsFile->pfile = file; */
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index aae5870..04aab66 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -57,7 +57,7 @@ static inline struct cifsFileInfo *cifs_init_private(
  /* Initialize reference count to one.  The private data is
  freed on the release of the last reference */
  atomic_set(&private_data->count, 1);
- INIT_WORK(&private_data->oplock_break, cifs_oplock_break);
+ slow_work_init(&private_data->oplock_break, &cifs_oplock_break_ops);
 
  return private_data;
 }
@@ -2311,8 +2311,8 @@ out:
  return rc;
 }
 
-void
-cifs_oplock_break(struct work_struct *work)
+static void
+cifs_oplock_break(struct slow_work *work)
 {
  struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
   oplock_break);
@@ -2350,10 +2350,31 @@ cifs_oplock_break(struct work_struct *work)
  LOCKING_ANDX_OPLOCK_RELEASE, false);
  cFYI(1, ("Oplock release rc = %d", rc));
  }
+}
 
+static int
+cifs_oplock_break_get(struct slow_work *work)
+{
+ struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
+  oplock_break);
+ cifsFileInfo_get(cfile);
+ return 0;
+}
+
+static void
+cifs_oplock_break_put(struct slow_work *work)
+{
+ struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
+  oplock_break);
  cifsFileInfo_put(cfile);
 }
 
+const struct slow_work_ops cifs_oplock_break_ops = {
+ .get_ref = cifs_oplock_break_get,
+ .put_ref = cifs_oplock_break_put,
+ .execute = cifs_oplock_break,
+};
+
 const struct address_space_operations cifs_addr_ops = {
  .readpage = cifs_readpage,
  .readpages = cifs_readpages,
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 8e098e6..0241b25 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -499,6 +499,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
  struct cifsTconInfo *tcon;
  struct cifsInodeInfo *pCifsInode;
  struct cifsFileInfo *netfile;
+ int rc;
 
  cFYI(1, ("Checking for oplock break or dnotify response"));
  if ((pSMB->hdr.Command == SMB_COM_NT_TRANSACT) &&
@@ -578,16 +579,20 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
  return true;
  }
 
- cifsFileInfo_get(netfile);
- read_unlock(&GlobalSMBSeslock);
- read_unlock(&cifs_tcp_ses_lock);
  cFYI(1, ("file id match, oplock break"));
  pCifsInode = CIFS_I(netfile->pInode);
  pCifsInode->clientCanCacheAll = false;
  if (pSMB->OplockLevel == 0)
  pCifsInode->clientCanCacheRead = false;
- if (schedule_work(&netfile->oplock_break))
+ rc = slow_work_enqueue(&netfile->oplock_break);
+ if (rc) {
+ cERROR(1, ("failed to enqueue oplock "
+   "break: %d\n", rc));
+ } else {
  netfile->oplock_break_cancelled = false;
+ }
+ read_unlock(&GlobalSMBSeslock);
+ read_unlock(&cifs_tcp_ses_lock);
  return true;
  }
  read_unlock(&GlobalSMBSeslock);
--
1.6.0.6

---

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client