[PATCH 0/5] cifs: fix several bugs in oplock queue handling (try #2)

[Jeff Layton-2]

This is a second pass at fixing panics in the oplock queue handling
that were reported here:

https://bugzilla.redhat.com/show_bug.cgi?id=516742

This patchset takes a different approach and has each oplock queue entry
hold a reference to the tcon until the oplock release call goes out onto
the wire.

This set is a draft patchset that tries to fix up the race conditions in
this code and some other issues I identified while working on it.

Comments and suggestions appreciated.

Jeff Layton (5):
  cifs: protect GlobalOplock_Q with its own spinlock
  cifs: take tcon reference for oplock breaks
  cifs: clean up cifs_oplock_thread loop
  cifs: take reference to inode for oplock breaks
  cifs: cancel oplock release callbacks during reconnect

 fs/cifs/cifsfs.c    |   98 +++++++++++++++++++++++++-------------------------
 fs/cifs/cifsglob.h  |   10 ++++-
 fs/cifs/cifsproto.h |    8 ++---
 fs/cifs/connect.c   |   19 ++++++++--
 fs/cifs/misc.c      |   56 +++++++++++++++++++++++++----
 fs/cifs/transport.c |   52 ---------------------------
 6 files changed, 124 insertions(+), 119 deletions(-)

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

[Jeff Layton-2]

Right now, the GlobalOplock_Q is protected by the GlobalMid_Lock. That
lock is also used for completely unrelated purposes (mostly for managing
the global mid queue). Give the list its own dedicated spinlock
(cifs_oplock_lock) and rename the list to cifs_oplock_list to
eliminate the camel-case.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/cifsfs.c    |   13 +++++++------
 fs/cifs/cifsglob.h  |    6 +++++-
 fs/cifs/transport.c |   17 ++++++++---------
 3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index b750aa5..3610e99 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -986,19 +986,19 @@ static int cifs_oplock_thread(void *dummyarg)
  if (try_to_freeze())
  continue;
 
- spin_lock(&GlobalMid_Lock);
- if (list_empty(&GlobalOplock_Q)) {
- spin_unlock(&GlobalMid_Lock);
+ 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(GlobalOplock_Q.next,
+ 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(&GlobalMid_Lock);
+ spin_unlock(&cifs_oplock_lock);
  DeleteOplockQEntry(oplock_item);
  /* can not grab inode sem here since it would
  deadlock when oplock received on delete
@@ -1055,7 +1055,7 @@ init_cifs(void)
  int rc = 0;
  cifs_proc_init();
  INIT_LIST_HEAD(&cifs_tcp_ses_list);
- INIT_LIST_HEAD(&GlobalOplock_Q);
+ INIT_LIST_HEAD(&cifs_oplock_list);
 #ifdef CONFIG_CIFS_EXPERIMENTAL
  INIT_LIST_HEAD(&GlobalDnotifyReqList);
  INIT_LIST_HEAD(&GlobalDnotifyRsp_Q);
@@ -1084,6 +1084,7 @@ 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;
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 6084d63..f100399 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -656,7 +656,11 @@ GLOBAL_EXTERN rwlock_t cifs_tcp_ses_lock;
  */
 GLOBAL_EXTERN rwlock_t GlobalSMBSeslock;
 
-GLOBAL_EXTERN struct list_head GlobalOplock_Q;
+/* 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;
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 0ad3e2d..1da4ab2 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -119,20 +119,19 @@ AllocOplockQEntry(struct inode *pinode, __u16 fid, struct cifsTconInfo *tcon)
  temp->pinode = pinode;
  temp->tcon = tcon;
  temp->netfid = fid;
- spin_lock(&GlobalMid_Lock);
- list_add_tail(&temp->qhead, &GlobalOplock_Q);
- spin_unlock(&GlobalMid_Lock);
+ 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(&GlobalMid_Lock);
+ spin_lock(&cifs_oplock_lock);
     /* should we check if list empty first? */
  list_del(&oplockEntry->qhead);
- spin_unlock(&GlobalMid_Lock);
+ spin_unlock(&cifs_oplock_lock);
  kmem_cache_free(cifs_oplock_cachep, oplockEntry);
 }
 
@@ -144,14 +143,14 @@ void DeleteTconOplockQEntries(struct cifsTconInfo *tcon)
  if (tcon == NULL)
  return;
 
- spin_lock(&GlobalMid_Lock);
- list_for_each_entry(temp, &GlobalOplock_Q, qhead) {
+ 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(&GlobalMid_Lock);
+ spin_unlock(&cifs_oplock_lock);
 }
 
 static int
--
1.6.0.6

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

[Jeff Layton-2]

When an oplock break comes in, we search for a tcon that matches it and
make an oplock queue entry with a pointer to that tcon. There's a
problem here, however. That tcon may disappear before the oplock thread
can get around to using it.

Take a reference to the tcon when we find the matching one and have
the cifs_oplock_thread put it when it's finished with the tcon.

There's a problem though, we can't *put* the last reference to a tcon
within the context of cifsd since that would deadlock. So we need to
take some extra precautions to only take a reference if we can
actually use it.

To do this, we get rid of all of the existing tcon allocation and
deletion routines. The way that they handle the locking is racy, and
in many cases they only have one caller anyway.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/cifsfs.c    |    7 ++---
 fs/cifs/cifsglob.h  |    3 +-
 fs/cifs/cifsproto.h |    8 ++----
 fs/cifs/connect.c   |   10 ++++++--
 fs/cifs/misc.c      |   53 +++++++++++++++++++++++++++++++++++++++++++-------
 fs/cifs/transport.c |   51 -------------------------------------------------
 6 files changed, 60 insertions(+), 72 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 3610e99..7ce8dcd 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -89,8 +89,6 @@ extern mempool_t *cifs_sm_req_poolp;
 extern mempool_t *cifs_req_poolp;
 extern mempool_t *cifs_mid_poolp;
 
-extern struct kmem_cache *cifs_oplock_cachep;
-
 static int
 cifs_read_super(struct super_block *sb, void *data,
  const char *devname, int silent)
@@ -294,7 +292,6 @@ static int cifs_permission(struct inode *inode, int mask)
 static struct kmem_cache *cifs_inode_cachep;
 static struct kmem_cache *cifs_req_cachep;
 static struct kmem_cache *cifs_mid_cachep;
-struct kmem_cache *cifs_oplock_cachep;
 static struct kmem_cache *cifs_sm_req_cachep;
 mempool_t *cifs_sm_req_poolp;
 mempool_t *cifs_req_poolp;
@@ -998,8 +995,9 @@ static int cifs_oplock_thread(void *dummyarg)
  pTcon = oplock_item->tcon;
  inode = oplock_item->pinode;
  netfid = oplock_item->netfid;
+ list_del(&oplock_item->qhead);
  spin_unlock(&cifs_oplock_lock);
- DeleteOplockQEntry(oplock_item);
+ kmem_cache_free(cifs_oplock_cachep, 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
@@ -1041,6 +1039,7 @@ static int cifs_oplock_thread(void *dummyarg)
  false /* wait flag */);
  cFYI(1, ("Oplock release rc = %d", rc));
  }
+ cifs_put_tcon(pTcon);
  set_current_state(TASK_INTERRUPTIBLE);
  schedule_timeout(1);  /* yield in case q were corrupt */
  }
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index f100399..363dbcf 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -369,7 +369,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 */
@@ -656,6 +655,8 @@ GLOBAL_EXTERN rwlock_t cifs_tcp_ses_lock;
  */
 GLOBAL_EXTERN rwlock_t GlobalSMBSeslock;
 
+GLOBAL_EXTERN struct kmem_cache *cifs_oplock_cachep;
+
 /* Global list of oplocks */
 GLOBAL_EXTERN struct list_head cifs_oplock_list;
 
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index da8fbf5..623d928 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -64,7 +64,8 @@ extern int SendReceiveBlockingLock(const unsigned int xid,
  int *bytes_returned);
 extern int checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length);
 extern bool is_valid_oplock_break(struct smb_hdr *smb,
-  struct TCP_Server_Info *);
+  struct TCP_Server_Info *,
+  struct oplock_q_entry **);
 extern bool is_size_safe_to_change(struct cifsInodeInfo *, __u64 eof);
 extern struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *);
 #ifdef CONFIG_CIFS_EXPERIMENTAL
@@ -86,10 +87,6 @@ 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 struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time,
@@ -117,6 +114,7 @@ extern void cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb,
       const char *path, const __u16 *pfid);
 extern int mode_to_acl(struct inode *inode, const char *path, __u64);
 
+void cifs_put_tcon(struct cifsTconInfo *tcon);
 extern int cifs_mount(struct super_block *, struct cifs_sb_info *, char *,
  const char *);
 extern int cifs_umount(struct super_block *, struct cifs_sb_info *);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index edf8765..f49304d 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -331,6 +331,7 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
  struct cifsSesInfo *ses;
  struct task_struct *task_to_wake = NULL;
  struct mid_q_entry *mid_entry;
+ struct oplock_q_entry *oplock_entry = NULL;
  char temp;
  bool isLargeBuf = false;
  bool isMultiRsp;
@@ -626,7 +627,8 @@ multi_t2_fnd:
  smallbuf = NULL;
  }
  wake_up_process(task_to_wake);
- } else if (!is_valid_oplock_break(smb_buffer, server) &&
+ } else if (!is_valid_oplock_break(smb_buffer, server,
+  &oplock_entry) &&
    !isMultiRsp) {
  cERROR(1, ("No task to wake, unknown frame received! "
    "NumMids %d", midCount.counter));
@@ -640,6 +642,9 @@ multi_t2_fnd:
  }
  } /* end while !EXITING */
 
+ if (oplock_entry)
+ kmem_cache_free(cifs_oplock_cachep, oplock_entry);
+
  /* take it off the list, if it's not already */
  write_lock(&cifs_tcp_ses_lock);
  list_del_init(&server->tcp_ses_list);
@@ -1624,7 +1629,7 @@ cifs_find_tcon(struct cifsSesInfo *ses, const char *unc)
  return NULL;
 }
 
-static void
+void
 cifs_put_tcon(struct cifsTconInfo *tcon)
 {
  int xid;
@@ -1643,7 +1648,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/misc.c b/fs/cifs/misc.c
index e079a91..7221af9 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -492,7 +492,8 @@ checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length)
 }
 
 bool
-is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
+is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
+      struct oplock_q_entry **oplock_entry)
 {
  struct smb_com_lock_req *pSMB = (struct smb_com_lock_req *)buf;
  struct list_head *tmp, *tmp1, *tmp2;
@@ -500,6 +501,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
  struct cifsTconInfo *tcon;
  struct cifsInodeInfo *pCifsInode;
  struct cifsFileInfo *netfile;
+ struct oplock_q_entry *oplock;
 
  cFYI(1, ("Checking for oplock break or dnotify response"));
  if ((pSMB->hdr.Command == SMB_COM_NT_TRANSACT) &&
@@ -552,8 +554,23 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
  if (!(pSMB->LockType & LOCKING_ANDX_OPLOCK_RELEASE))
  return false;
 
+ /*
+ * If we succeed in finding a matching tcon and fid, then we'll need
+ * an allocated oplock queue entry. But at that point we'll have an
+ * active tcon reference. If the allocation fails, we cannot put
+ * the reference within the context of cifs_demultiplex_thread.
+ *
+ * So, we must instead pre-allocate this entry in case it's needed.
+ * If it isn't however, then we can just hold on to it until one is.
+ */
+ if (!*oplock_entry)
+ *oplock_entry = (struct oplock_q_entry *)
+        kmem_cache_alloc(cifs_oplock_cachep,
+ GFP_KERNEL);
+ oplock = *oplock_entry;
+
  /* look up tcon based on tid & uid */
- read_lock(&cifs_tcp_ses_lock);
+ write_lock(&cifs_tcp_ses_lock);
  list_for_each(tmp, &srv->smb_ses_list) {
  ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
  list_for_each(tmp1, &ses->tcon_list) {
@@ -569,16 +586,36 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
  if (pSMB->Fid != netfile->netfid)
  continue;
 
+ /*
+ * can't put reference later if we don't have
+ * an oplock entry. So only take a reference
+ * if we had a successful allocation.
+ */
+ if (oplock)
+ ++tcon->tc_count;
  write_unlock(&GlobalSMBSeslock);
- read_unlock(&cifs_tcp_ses_lock);
+ write_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;
- pCifsInode->oplockPending = true;
- AllocOplockQEntry(netfile->pInode,
-  netfile->netfid, tcon);
+
+ if (!oplock) {
+ cERROR(1, ("Unable to allocate "
+   "queue entry!"));
+ return true;
+ }
+
+ oplock->tcon = tcon;
+ oplock->pinode = netfile->pInode;
+ oplock->netfid = netfile->netfid;
+ spin_lock(&cifs_oplock_lock);
+ list_add_tail(&oplock->qhead,
+      &cifs_oplock_list);
+ spin_unlock(&cifs_oplock_lock);
+ *oplock_entry = NULL;
+
  cFYI(1, ("about to wake up oplock thread"));
  if (oplockThread)
  wake_up_process(oplockThread);
@@ -586,12 +623,12 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
  return true;
  }
  write_unlock(&GlobalSMBSeslock);
- read_unlock(&cifs_tcp_ses_lock);
+ write_unlock(&cifs_tcp_ses_lock);
  cFYI(1, ("No matching file for oplock break"));
  return true;
  }
  }
- read_unlock(&cifs_tcp_ses_lock);
+ write_unlock(&cifs_tcp_ses_lock);
  cFYI(1, ("Can not process oplock break for non-existent connection"));
  return true;
 }
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 1da4ab2..ae22ff2 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -34,7 +34,6 @@
 #include "cifs_debug.h"
 
 extern mempool_t *cifs_mid_poolp;
-extern struct kmem_cache *cifs_oplock_cachep;
 
 static struct mid_q_entry *
 AllocMidQEntry(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
@@ -103,56 +102,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

[Jeff Layton-2]

Minor cleanups and optimizations mostly. Also fix it so that if a
kthread_stop races in at the right time, it won't make the thread
sleep unnecessarily for 39s.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/cifsfs.c |   66 +++++++++++++++++++++++++++++------------------------
 1 files changed, 36 insertions(+), 30 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 7ce8dcd..2fcc722 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -972,10 +972,10 @@ cifs_destroy_mids(void)
 
 static int cifs_oplock_thread(void *dummyarg)
 {
- struct oplock_q_entry *oplock_item;
- struct cifsTconInfo *pTcon;
+ struct oplock_q_entry *oplock;
+ struct cifsTconInfo *tcon;
  struct inode *inode;
- __u16  netfid;
+ struct cifsInodeInfo *cifsi;
  int rc, waitrc = 0;
 
  set_freezable();
@@ -984,34 +984,30 @@ static int cifs_oplock_thread(void *dummyarg)
  continue;
 
  spin_lock(&cifs_oplock_lock);
- if (list_empty(&cifs_oplock_list)) {
+ while(!list_empty(&cifs_oplock_list)) {
+ oplock = list_entry(cifs_oplock_list.next,
+    struct oplock_q_entry, qhead);
+ list_del(&oplock->qhead);
  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;
- list_del(&oplock_item->qhead);
- spin_unlock(&cifs_oplock_lock);
- kmem_cache_free(cifs_oplock_cachep, oplock_item);
+ tcon = oplock->tcon;
+ inode = oplock->pinode;
  /* 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);*/
+ cifsi = CIFS_I(inode);
  if (S_ISREG(inode->i_mode)) {
 #ifdef CONFIG_CIFS_EXPERIMENTAL
- if (CIFS_I(inode)->clientCanCacheAll == 0)
+ if (cifsi->clientCanCacheAll == 0)
  break_lease(inode, FMODE_READ);
- else if (CIFS_I(inode)->clientCanCacheRead == 0)
+ else if (cifsi->clientCanCacheRead == 0)
  break_lease(inode, FMODE_WRITE);
 #endif
  rc = filemap_fdatawrite(inode->i_mapping);
- if (CIFS_I(inode)->clientCanCacheRead == 0) {
+ if (cifsi->clientCanCacheRead == 0) {
  waitrc = filemap_fdatawait(
       inode->i_mapping);
  invalidate_remote_inode(inode);
@@ -1022,27 +1018,37 @@ static int cifs_oplock_thread(void *dummyarg)
  rc = 0;
  /* mutex_unlock(&inode->i_mutex);*/
  if (rc)
- CIFS_I(inode)->write_behind_rc = rc;
+ cifsi->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,
+ /*
+ * 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 (!tcon->need_reconnect) {
+ rc = CIFSSMBLock(0, tcon, oplock->netfid,
  0 /* len */ , 0 /* offset */, 0,
  0, LOCKING_ANDX_OPLOCK_RELEASE,
  false /* wait flag */);
  cFYI(1, ("Oplock release rc = %d", rc));
  }
- cifs_put_tcon(pTcon);
- set_current_state(TASK_INTERRUPTIBLE);
- schedule_timeout(1);  /* yield in case q were corrupt */
+ cifs_put_tcon(tcon);
+ kmem_cache_free(cifs_oplock_cachep, oplock);
+ spin_lock(&cifs_oplock_lock);
+ }
+ spin_unlock(&cifs_oplock_lock);
+ set_current_state(TASK_INTERRUPTIBLE);
+ if (kthread_should_stop()) {
+ set_current_state(TASK_RUNNING);
+ break;
  }
+ /* FIXME: Why 39s here? This should be a named constant. */
+ schedule_timeout(39*HZ);
  } while (!kthread_should_stop());
 
  return 0;
--
1.6.0.6

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

[Jeff Layton-2]

When an oplock break comes in, cifs needs to do things like write out
dirty data for it. It doesn't hold a reference to that inode in this
case however. Get an active reference to the inode when an oplock break
comes in. If we don't get a reference, we still need to create an oplock
queue entry so that the oplock release call gets done, but we'll want to
skip writeback in that case.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/cifsfs.c |   26 ++++++++++----------------
 fs/cifs/misc.c   |    4 +++-
 2 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 2fcc722..647c5bc 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -993,13 +993,8 @@ static int cifs_oplock_thread(void *dummyarg)
  cFYI(1, ("found oplock item to write out"));
  tcon = oplock->tcon;
  inode = oplock->pinode;
- /* 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);*/
- cifsi = CIFS_I(inode);
- if (S_ISREG(inode->i_mode)) {
+ if (inode && S_ISREG(inode->i_mode)) {
+ cifsi = CIFS_I(inode);
 #ifdef CONFIG_CIFS_EXPERIMENTAL
  if (cifsi->clientCanCacheAll == 0)
  break_lease(inode, FMODE_READ);
@@ -1010,17 +1005,16 @@ static int cifs_oplock_thread(void *dummyarg)
  if (cifsi->clientCanCacheRead == 0) {
  waitrc = filemap_fdatawait(
       inode->i_mapping);
+ if (rc == 0)
+ rc = waitrc;
  invalidate_remote_inode(inode);
  }
- if (rc == 0)
- rc = waitrc;
- } else
- rc = 0;
- /* mutex_unlock(&inode->i_mutex);*/
- if (rc)
- cifsi->write_behind_rc = rc;
- cFYI(1, ("Oplock flush inode %p rc %d",
- inode, rc));
+ if (rc)
+ cifsi->write_behind_rc = rc;
+ cFYI(1, ("Oplock flush inode %p rc %d",
+ inode, rc));
+ }
+ iput(inode);
 
  /*
  * releasing stale oplock after recent reconnect
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 7221af9..3bf3a52 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -502,6 +502,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
  struct cifsInodeInfo *pCifsInode;
  struct cifsFileInfo *netfile;
  struct oplock_q_entry *oplock;
+ struct inode *inode;
 
  cFYI(1, ("Checking for oplock break or dnotify response"));
  if ((pSMB->hdr.Command == SMB_COM_NT_TRANSACT) &&
@@ -600,6 +601,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
  pCifsInode->clientCanCacheAll = false;
  if (pSMB->OplockLevel == 0)
  pCifsInode->clientCanCacheRead = false;
+ inode = igrab(netfile->pInode);
 
  if (!oplock) {
  cERROR(1, ("Unable to allocate "
@@ -608,7 +610,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
  }
 
  oplock->tcon = tcon;
- oplock->pinode = netfile->pInode;
+ oplock->pinode = inode;
  oplock->netfid = netfile->netfid;
  spin_lock(&cifs_oplock_lock);
  list_add_tail(&oplock->qhead,
--
1.6.0.6

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

[Jeff Layton-2]

cifs_oplock_thread has a check for pTcon->needs_reconnect and will skip
the CIFSSMBLock call if it's set.

Problem: what if the tcon has this set and then gets reconnected before
the call goes out on the wire? The oplock release isn't needed and could
be a bad thing at that point if the filehandle was reclaimed.

Cancel oplock release calls during a reconnect event.

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

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 647c5bc..92e06c1 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1024,7 +1024,7 @@ static int cifs_oplock_thread(void *dummyarg)
  * to server still is disconnected since oplock
  * already released by the server in that case
  */
- if (!tcon->need_reconnect) {
+ if (!oplock->cancel) {
  rc = CIFSSMBLock(0, tcon, oplock->netfid,
  0 /* len */ , 0 /* offset */, 0,
  0, LOCKING_ANDX_OPLOCK_RELEASE,
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 363dbcf..676c107 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -448,6 +448,7 @@ struct oplock_q_entry {
  struct inode *pinode;
  struct cifsTconInfo *tcon;
  __u16 netfid;
+ bool cancel;
 };
 
 /* for pending dnotify requests */
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index f49304d..b7b67e9 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -121,6 +121,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
  struct cifsSesInfo *ses;
  struct cifsTconInfo *tcon;
  struct mid_q_entry *mid_entry;
+ struct oplock_q_entry *oplock;
 
  spin_lock(&GlobalMid_Lock);
  if (server->tcpStatus == CifsExiting) {
@@ -137,6 +138,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
 
  /* before reconnecting the tcp session, mark the smb session (uid)
  and the tid bad so they are not used until reconnected */
+ spin_lock(&cifs_oplock_lock);
  read_lock(&cifs_tcp_ses_lock);
  list_for_each(tmp, &server->smb_ses_list) {
  ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
@@ -145,9 +147,16 @@ cifs_reconnect(struct TCP_Server_Info *server)
  list_for_each(tmp2, &ses->tcon_list) {
  tcon = list_entry(tmp2, struct cifsTconInfo, tcon_list);
  tcon->need_reconnect = true;
+ list_for_each_entry(oplock, &cifs_oplock_list, qhead) {
+ if (oplock->tcon == tcon)
+ oplock->cancel = true;
+ }
  }
+
  }
  read_unlock(&cifs_tcp_ses_lock);
+ spin_unlock(&cifs_oplock_lock);
+
  /* do not want to be sending data on a socket we are freeing */
  mutex_lock(&server->srv_mutex);
  if (server->ssocket) {
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 3bf3a52..4a2d297 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -612,6 +612,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
  oplock->tcon = tcon;
  oplock->pinode = inode;
  oplock->netfid = netfile->netfid;
+ oplock->cancel = false;
  spin_lock(&cifs_oplock_lock);
  list_add_tail(&oplock->qhead,
       &cifs_oplock_list);
--
1.6.0.6

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

[Steve French-2]

Starting to look through this set

ACK on this one.   Doesn't look like this would affect behavior, but slight cosmetic improvement

On Tue, Aug 18, 2009 at 1:06 PM, Jeff Layton <jlayton@...> wrote:

Right now, the GlobalOplock_Q is protected by the GlobalMid_Lock. That
lock is also used for completely unrelated purposes (mostly for managing
the global mid queue). Give the list its own dedicated spinlock
(cifs_oplock_lock) and rename the list to cifs_oplock_list to
eliminate the camel-case.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/cifsfs.c    |   13 +++++++------
 fs/cifs/cifsglob.h  |    6 +++++-
 fs/cifs/transport.c |   17 ++++++++---------
 3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index b750aa5..3610e99 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -986,19 +986,19 @@ static int cifs_oplock_thread(void *dummyarg)
               if (try_to_freeze())
                       continue;

-               spin_lock(&GlobalMid_Lock);
-               if (list_empty(&GlobalOplock_Q)) {
-                       spin_unlock(&GlobalMid_Lock);
+               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(GlobalOplock_Q.next,
+                       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(&GlobalMid_Lock);
+                       spin_unlock(&cifs_oplock_lock);
                       DeleteOplockQEntry(oplock_item);
                       /* can not grab inode sem here since it would
                               deadlock when oplock received on delete
@@ -1055,7 +1055,7 @@ init_cifs(void)
       int rc = 0;
       cifs_proc_init();
       INIT_LIST_HEAD(&cifs_tcp_ses_list);
-       INIT_LIST_HEAD(&GlobalOplock_Q);
+       INIT_LIST_HEAD(&cifs_oplock_list);
 #ifdef CONFIG_CIFS_EXPERIMENTAL
       INIT_LIST_HEAD(&GlobalDnotifyReqList);
       INIT_LIST_HEAD(&GlobalDnotifyRsp_Q);
@@ -1084,6 +1084,7 @@ 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;
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 6084d63..f100399 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -656,7 +656,11 @@ GLOBAL_EXTERN rwlock_t             cifs_tcp_ses_lock;
 */
 GLOBAL_EXTERN rwlock_t GlobalSMBSeslock;

-GLOBAL_EXTERN struct list_head GlobalOplock_Q;
+/* 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;
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 0ad3e2d..1da4ab2 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -119,20 +119,19 @@ AllocOplockQEntry(struct inode *pinode, __u16 fid, struct cifsTconInfo *tcon)
               temp->pinode = pinode;
               temp->tcon = tcon;
               temp->netfid = fid;
-               spin_lock(&GlobalMid_Lock);
-               list_add_tail(&temp->qhead, &GlobalOplock_Q);
-               spin_unlock(&GlobalMid_Lock);
+               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(&GlobalMid_Lock);
+       spin_lock(&cifs_oplock_lock);
    /* should we check if list empty first? */
       list_del(&oplockEntry->qhead);
-       spin_unlock(&GlobalMid_Lock);
+       spin_unlock(&cifs_oplock_lock);
       kmem_cache_free(cifs_oplock_cachep, oplockEntry);
 }

@@ -144,14 +143,14 @@ void DeleteTconOplockQEntries(struct cifsTconInfo *tcon)
       if (tcon == NULL)
               return;

-       spin_lock(&GlobalMid_Lock);
-       list_for_each_entry(temp, &GlobalOplock_Q, qhead) {
+       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(&GlobalMid_Lock);
+       spin_unlock(&cifs_oplock_lock);
 }

 static int
--
1.6.0.6

--
Thanks,

Steve

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

[Steve French-2]

Why don't we have a reference to this inode already?   We can't have an oplock break unless the file is open, if the file is open then we have a reference to the inode ...

On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton <jlayton@...> wrote:

When an oplock break comes in, cifs needs to do things like write out
dirty data for it. It doesn't hold a reference to that inode in this
case however. Get an active reference to the inode when an oplock break
comes in. If we don't get a reference, we still need to create an oplock
queue entry so that the oplock release call gets done, but we'll want to
skip writeback in that case.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/cifsfs.c |   26 ++++++++++----------------
 fs/cifs/misc.c   |    4 +++-
 2 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 2fcc722..647c5bc 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -993,13 +993,8 @@ static int cifs_oplock_thread(void *dummyarg)
                       cFYI(1, ("found oplock item to write out"));
                       tcon = oplock->tcon;
                       inode = oplock->pinode;
-                       /* 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);*/
-                       cifsi = CIFS_I(inode);
-                       if (S_ISREG(inode->i_mode)) {
+                       if (inode && S_ISREG(inode->i_mode)) {
+                               cifsi = CIFS_I(inode);
 #ifdef CONFIG_CIFS_EXPERIMENTAL
                               if (cifsi->clientCanCacheAll == 0)
                                       break_lease(inode, FMODE_READ);
@@ -1010,17 +1005,16 @@ static int cifs_oplock_thread(void *dummyarg)
                               if (cifsi->clientCanCacheRead == 0) {
                                       waitrc = filemap_fdatawait(
                                                             inode->i_mapping);
+                                       if (rc == 0)
+                                               rc = waitrc;
                                       invalidate_remote_inode(inode);
                               }
-                               if (rc == 0)
-                                       rc = waitrc;
-                       } else
-                               rc = 0;
-                       /* mutex_unlock(&inode->i_mutex);*/
-                       if (rc)
-                               cifsi->write_behind_rc = rc;
-                       cFYI(1, ("Oplock flush inode %p rc %d",
-                               inode, rc));
+                               if (rc)
+                                       cifsi->write_behind_rc = rc;
+                               cFYI(1, ("Oplock flush inode %p rc %d",
+                                        inode, rc));
+                       }
+                       iput(inode);

                       /*
                        * releasing stale oplock after recent reconnect
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 7221af9..3bf3a52 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -502,6 +502,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
       struct cifsInodeInfo *pCifsInode;
       struct cifsFileInfo *netfile;
       struct oplock_q_entry *oplock;
+       struct inode *inode;

       cFYI(1, ("Checking for oplock break or dnotify response"));
       if ((pSMB->hdr.Command == SMB_COM_NT_TRANSACT) &&
@@ -600,6 +601,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
                               pCifsInode->clientCanCacheAll = false;
                               if (pSMB->OplockLevel == 0)
                                       pCifsInode->clientCanCacheRead = false;
+                               inode = igrab(netfile->pInode);

                               if (!oplock) {
                                       cERROR(1, ("Unable to allocate "
@@ -608,7 +610,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
                               }

                               oplock->tcon = tcon;
-                               oplock->pinode = netfile->pInode;
+                               oplock->pinode = inode;
                               oplock->netfid = netfile->netfid;
                               spin_lock(&cifs_oplock_lock);
                               list_add_tail(&oplock->qhead,
--
1.6.0.6

--
Thanks,

Steve

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

[Steve French-2]

ACK -  (although an extra oplock break on an incorrect file handle is not a big deal, slightly slower, and would be very rare - this approach is cleaner)

On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton <jlayton@...> wrote:

cifs_oplock_thread has a check for pTcon->needs_reconnect and will skip
the CIFSSMBLock call if it's set.

Problem: what if the tcon has this set and then gets reconnected before
the call goes out on the wire? The oplock release isn't needed and could
be a bad thing at that point if the filehandle was reclaimed.

Cancel oplock release calls during a reconnect event.

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

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 647c5bc..92e06c1 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1024,7 +1024,7 @@ static int cifs_oplock_thread(void *dummyarg)
                        * to server still is disconnected since oplock
                        * already released by the server in that case
                        */
-                       if (!tcon->need_reconnect) {
+                       if (!oplock->cancel) {
                               rc = CIFSSMBLock(0, tcon, oplock->netfid,
                                               0 /* len */ , 0 /* offset */, 0,
                                               0, LOCKING_ANDX_OPLOCK_RELEASE,
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 363dbcf..676c107 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -448,6 +448,7 @@ struct oplock_q_entry {
       struct inode *pinode;
       struct cifsTconInfo *tcon;
       __u16 netfid;
+       bool cancel;
 };

 /* for pending dnotify requests */
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index f49304d..b7b67e9 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -121,6 +121,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
       struct cifsSesInfo *ses;
       struct cifsTconInfo *tcon;
       struct mid_q_entry *mid_entry;
+       struct oplock_q_entry *oplock;

       spin_lock(&GlobalMid_Lock);
       if (server->tcpStatus == CifsExiting) {
@@ -137,6 +138,7 @@ cifs_reconnect(struct TCP_Server_Info *server)

       /* before reconnecting the tcp session, mark the smb session (uid)
               and the tid bad so they are not used until reconnected */
+       spin_lock(&cifs_oplock_lock);
       read_lock(&cifs_tcp_ses_lock);
       list_for_each(tmp, &server->smb_ses_list) {
               ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
@@ -145,9 +147,16 @@ cifs_reconnect(struct TCP_Server_Info *server)
               list_for_each(tmp2, &ses->tcon_list) {
                       tcon = list_entry(tmp2, struct cifsTconInfo, tcon_list);
                       tcon->need_reconnect = true;
+                       list_for_each_entry(oplock, &cifs_oplock_list, qhead) {
+                               if (oplock->tcon == tcon)
+                                       oplock->cancel = true;
+                       }
               }
+
       }
       read_unlock(&cifs_tcp_ses_lock);
+       spin_unlock(&cifs_oplock_lock);
+
       /* do not want to be sending data on a socket we are freeing */
       mutex_lock(&server->srv_mutex);
       if (server->ssocket) {
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 3bf3a52..4a2d297 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -612,6 +612,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
                               oplock->tcon = tcon;
                               oplock->pinode = inode;
                               oplock->netfid = netfile->netfid;
+                               oplock->cancel = false;
                               spin_lock(&cifs_oplock_lock);
                               list_add_tail(&oplock->qhead,
                                             &cifs_oplock_list);
--
1.6.0.6

--
Thanks,

Steve

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

[Jeff Layton-2]

On Thu, 20 Aug 2009 19:32:17 -0500
Steve French <smfrench@...> wrote:

> Starting to look through this set
>
> ACK on this one.   Doesn't look like this would affect behavior, but slight
> cosmetic improvement
>

Well, it could affect behavior. It changes the lock from the
GlobalMid_lock to a completely different one. It's always hard to tell
exactly what the effect will be of breaking up a coarse-grained lock
like that.

The locks are taken in the same place however. If there truly is no
relationship between the cifs_oplock_list and the other things
protected by the mid lock, then yes there should be no behavior change.

>
> On Tue, Aug 18, 2009 at 1:06 PM, Jeff Layton <jlayton@...> wrote:
>
> > Right now, the GlobalOplock_Q is protected by the GlobalMid_Lock. That
> > lock is also used for completely unrelated purposes (mostly for managing
> > the global mid queue). Give the list its own dedicated spinlock
> > (cifs_oplock_lock) and rename the list to cifs_oplock_list to
> > eliminate the camel-case.
> >
> > Signed-off-by: Jeff Layton <jlayton@...>
> > ---
> >  fs/cifs/cifsfs.c    |   13 +++++++------
> >  fs/cifs/cifsglob.h  |    6 +++++-
> >  fs/cifs/transport.c |   17 ++++++++---------
> >  3 files changed, 20 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index b750aa5..3610e99 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -986,19 +986,19 @@ static int cifs_oplock_thread(void *dummyarg)
> >                if (try_to_freeze())
> >                        continue;
> >
> > -               spin_lock(&GlobalMid_Lock);
> > -               if (list_empty(&GlobalOplock_Q)) {
> > -                       spin_unlock(&GlobalMid_Lock);
> > +               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(GlobalOplock_Q.next,
> > +                       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(&GlobalMid_Lock);
> > +                       spin_unlock(&cifs_oplock_lock);
> >                        DeleteOplockQEntry(oplock_item);
> >                        /* can not grab inode sem here since it would
> >                                deadlock when oplock received on delete
> > @@ -1055,7 +1055,7 @@ init_cifs(void)
> >        int rc = 0;
> >        cifs_proc_init();
> >        INIT_LIST_HEAD(&cifs_tcp_ses_list);
> > -       INIT_LIST_HEAD(&GlobalOplock_Q);
> > +       INIT_LIST_HEAD(&cifs_oplock_list);
> >  #ifdef CONFIG_CIFS_EXPERIMENTAL
> >        INIT_LIST_HEAD(&GlobalDnotifyReqList);
> >        INIT_LIST_HEAD(&GlobalDnotifyRsp_Q);
> > @@ -1084,6 +1084,7 @@ 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;
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 6084d63..f100399 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -656,7 +656,11 @@ GLOBAL_EXTERN rwlock_t             cifs_tcp_ses_lock;
> >  */
> >  GLOBAL_EXTERN rwlock_t GlobalSMBSeslock;
> >
> > -GLOBAL_EXTERN struct list_head GlobalOplock_Q;
> > +/* 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;
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index 0ad3e2d..1da4ab2 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -119,20 +119,19 @@ AllocOplockQEntry(struct inode *pinode, __u16 fid,
> > struct cifsTconInfo *tcon)
> >                temp->pinode = pinode;
> >                temp->tcon = tcon;
> >                temp->netfid = fid;
> > -               spin_lock(&GlobalMid_Lock);
> > -               list_add_tail(&temp->qhead, &GlobalOplock_Q);
> > -               spin_unlock(&GlobalMid_Lock);
> > +               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(&GlobalMid_Lock);
> > +       spin_lock(&cifs_oplock_lock);
> >     /* should we check if list empty first? */
> >        list_del(&oplockEntry->qhead);
> > -       spin_unlock(&GlobalMid_Lock);
> > +       spin_unlock(&cifs_oplock_lock);
> >        kmem_cache_free(cifs_oplock_cachep, oplockEntry);
> >  }
> >
> > @@ -144,14 +143,14 @@ void DeleteTconOplockQEntries(struct cifsTconInfo
> > *tcon)
> >        if (tcon == NULL)
> >                return;
> >
> > -       spin_lock(&GlobalMid_Lock);
> > -       list_for_each_entry(temp, &GlobalOplock_Q, qhead) {
> > +       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(&GlobalMid_Lock);
> > +       spin_unlock(&cifs_oplock_lock);
> >  }
> >
> >  static int
> > --
> > 1.6.0.6
> >
> >
>
>

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

[Jeff Layton-2]

On Thu, 20 Aug 2009 19:42:42 -0500
Steve French <smfrench@...> wrote:

> Why don't we have a reference to this inode already?   We can't have an
> oplock break unless the file is open, if the file is open then we have a
> reference to the inode ...
>

Well, you have a reference when the entry goes onto the list. There's
no guarantee that you'll still have one when cifs_oplock_thread goes to
take it off the list however. The file could have been closed by then
and the inode flushed out of the cache. Since the cifs_oplock_thread
happens outside of any "normal" process behavior, we really need to
take an inode reference here.

> On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton <jlayton@...> wrote:
>
> > When an oplock break comes in, cifs needs to do things like write out
> > dirty data for it. It doesn't hold a reference to that inode in this
> > case however. Get an active reference to the inode when an oplock break
> > comes in. If we don't get a reference, we still need to create an oplock
> > queue entry so that the oplock release call gets done, but we'll want to
> > skip writeback in that case.
> >
> > Signed-off-by: Jeff Layton <jlayton@...>
> > ---
> >  fs/cifs/cifsfs.c |   26 ++++++++++----------------
> >  fs/cifs/misc.c   |    4 +++-
> >  2 files changed, 13 insertions(+), 17 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 2fcc722..647c5bc 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -993,13 +993,8 @@ static int cifs_oplock_thread(void *dummyarg)
> >                        cFYI(1, ("found oplock item to write out"));
> >                        tcon = oplock->tcon;
> >                        inode = oplock->pinode;
> > -                       /* 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);*/
> > -                       cifsi = CIFS_I(inode);
> > -                       if (S_ISREG(inode->i_mode)) {
> > +                       if (inode && S_ISREG(inode->i_mode)) {
> > +                               cifsi = CIFS_I(inode);
> >  #ifdef CONFIG_CIFS_EXPERIMENTAL
> >                                if (cifsi->clientCanCacheAll == 0)
> >                                        break_lease(inode, FMODE_READ);
> > @@ -1010,17 +1005,16 @@ static int cifs_oplock_thread(void *dummyarg)
> >                                if (cifsi->clientCanCacheRead == 0) {
> >                                        waitrc = filemap_fdatawait(
> >
> >  inode->i_mapping);
> > +                                       if (rc == 0)
> > +                                               rc = waitrc;
> >                                        invalidate_remote_inode(inode);
> >                                }
> > -                               if (rc == 0)
> > -                                       rc = waitrc;
> > -                       } else
> > -                               rc = 0;
> > -                       /* mutex_unlock(&inode->i_mutex);*/
> > -                       if (rc)
> > -                               cifsi->write_behind_rc = rc;
> > -                       cFYI(1, ("Oplock flush inode %p rc %d",
> > -                               inode, rc));
> > +                               if (rc)
> > +                                       cifsi->write_behind_rc = rc;
> > +                               cFYI(1, ("Oplock flush inode %p rc %d",
> > +                                        inode, rc));
> > +                       }
> > +                       iput(inode);
> >
> >                        /*
> >                         * releasing stale oplock after recent reconnect
> > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> > index 7221af9..3bf3a52 100644
> > --- a/fs/cifs/misc.c
> > +++ b/fs/cifs/misc.c
> > @@ -502,6 +502,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct
> > TCP_Server_Info *srv,
> >        struct cifsInodeInfo *pCifsInode;
> >        struct cifsFileInfo *netfile;
> >        struct oplock_q_entry *oplock;
> > +       struct inode *inode;
> >
> >        cFYI(1, ("Checking for oplock break or dnotify response"));
> >        if ((pSMB->hdr.Command == SMB_COM_NT_TRANSACT) &&
> > @@ -600,6 +601,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct
> > TCP_Server_Info *srv,
> >                                pCifsInode->clientCanCacheAll = false;
> >                                if (pSMB->OplockLevel == 0)
> >                                        pCifsInode->clientCanCacheRead =
> > false;
> > +                               inode = igrab(netfile->pInode);
> >
> >                                if (!oplock) {
> >                                        cERROR(1, ("Unable to allocate "
> > @@ -608,7 +610,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct
> > TCP_Server_Info *srv,
> >                                }
> >
> >                                oplock->tcon = tcon;
> > -                               oplock->pinode = netfile->pInode;
> > +                               oplock->pinode = inode;
> >                                oplock->netfid = netfile->netfid;
> >                                spin_lock(&cifs_oplock_lock);
> >                                list_add_tail(&oplock->qhead,
> > --
> > 1.6.0.6
> >
> >
>
>

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

[Jeff Layton-2]

On Thu, 20 Aug 2009 19:45:15 -0500
Steve French <smfrench@...> wrote:

> ACK -  (although an extra oplock break on an incorrect file handle is not a
> big deal, slightly slower, and would be very rare - this approach is
> cleaner)
>

I was also worried about what happens if you get an oplock break and
then a reconnect causes the fid to change to a completely different
file. The client would send the oplock release and then proceed
thinking that it had an oplock on the other file at that point when it
really didn't.

Not an extremely likely race, but simple enough to prevent.

> On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton <jlayton@...> wrote:
>
> > cifs_oplock_thread has a check for pTcon->needs_reconnect and will skip
> > the CIFSSMBLock call if it's set.
> >
> > Problem: what if the tcon has this set and then gets reconnected before
> > the call goes out on the wire? The oplock release isn't needed and could
> > be a bad thing at that point if the filehandle was reclaimed.
> >
> > Cancel oplock release calls during a reconnect event.
> >
> > Signed-off-by: Jeff Layton <jlayton@...>
> > ---
> >  fs/cifs/cifsfs.c   |    2 +-
> >  fs/cifs/cifsglob.h |    1 +
> >  fs/cifs/connect.c  |    9 +++++++++
> >  fs/cifs/misc.c     |    1 +
> >  4 files changed, 12 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 647c5bc..92e06c1 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -1024,7 +1024,7 @@ static int cifs_oplock_thread(void *dummyarg)
> >                         * to server still is disconnected since oplock
> >                         * already released by the server in that case
> >                         */
> > -                       if (!tcon->need_reconnect) {
> > +                       if (!oplock->cancel) {
> >                                rc = CIFSSMBLock(0, tcon, oplock->netfid,
> >                                                0 /* len */ , 0 /* offset
> > */, 0,
> >                                                0,
> > LOCKING_ANDX_OPLOCK_RELEASE,
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 363dbcf..676c107 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -448,6 +448,7 @@ struct oplock_q_entry {
> >        struct inode *pinode;
> >        struct cifsTconInfo *tcon;
> >        __u16 netfid;
> > +       bool cancel;
> >  };
> >
> >  /* for pending dnotify requests */
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index f49304d..b7b67e9 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -121,6 +121,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >        struct cifsSesInfo *ses;
> >        struct cifsTconInfo *tcon;
> >        struct mid_q_entry *mid_entry;
> > +       struct oplock_q_entry *oplock;
> >
> >        spin_lock(&GlobalMid_Lock);
> >        if (server->tcpStatus == CifsExiting) {
> > @@ -137,6 +138,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >
> >        /* before reconnecting the tcp session, mark the smb session (uid)
> >                and the tid bad so they are not used until reconnected */
> > +       spin_lock(&cifs_oplock_lock);
> >        read_lock(&cifs_tcp_ses_lock);
> >        list_for_each(tmp, &server->smb_ses_list) {
> >                ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
> > @@ -145,9 +147,16 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >                list_for_each(tmp2, &ses->tcon_list) {
> >                        tcon = list_entry(tmp2, struct cifsTconInfo,
> > tcon_list);
> >                        tcon->need_reconnect = true;
> > +                       list_for_each_entry(oplock, &cifs_oplock_list,
> > qhead) {
> > +                               if (oplock->tcon == tcon)
> > +                                       oplock->cancel = true;
> > +                       }
> >                }
> > +
> >        }
> >        read_unlock(&cifs_tcp_ses_lock);
> > +       spin_unlock(&cifs_oplock_lock);
> > +
> >        /* do not want to be sending data on a socket we are freeing */
> >        mutex_lock(&server->srv_mutex);
> >        if (server->ssocket) {
> > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> > index 3bf3a52..4a2d297 100644
> > --- a/fs/cifs/misc.c
> > +++ b/fs/cifs/misc.c
> > @@ -612,6 +612,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct
> > TCP_Server_Info *srv,
> >                                oplock->tcon = tcon;
> >                                oplock->pinode = inode;
> >                                oplock->netfid = netfile->netfid;
> > +                               oplock->cancel = false;
> >                                spin_lock(&cifs_oplock_lock);
> >                                list_add_tail(&oplock->qhead,
> >                                              &cifs_oplock_list);
> > --
> > 1.6.0.6
> >
> >
>
>

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

[Steve French-2]

On Fri, Aug 21, 2009 at 5:36 AM, Jeff Layton <jlayton@...> wrote:

On Thu, 20 Aug 2009 19:42:42 -0500
Steve French <smfrench@...> wrote:

> Why don't we have a reference to this inode already?   We can't have an
> oplock break unless the file is open, if the file is open then we have a
> reference to the inode ...
>

Well, you have a reference when the entry goes onto the list. There's
no guarantee that you'll still have one when cifs_oplock_thread goes to
take it off the list however. The file could have been closed by then

An oplock response is handle based so we need to make sure the fid
is valid (if not, throw away the oplock response, except for the case
of batch oplock which we don't use yet).  Seems odd to lock the
inode when we really need the fid (file struct)

We could mark the file struct (similar to the write pending flag) and
delete such an entry off the oplockq in close (if we are closing
a file with an oplock pending)

--
Thanks,

Steve

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

[Jeff Layton-2]

On Fri, 21 Aug 2009 10:18:16 -0500
Steve French <smfrench@...> wrote:

> On Fri, Aug 21, 2009 at 5:36 AM, Jeff Layton <jlayton@...> wrote:
>
> > On Thu, 20 Aug 2009 19:42:42 -0500
> > Steve French <smfrench@...> wrote:
> >
> > > Why don't we have a reference to this inode already?   We can't have an
> > > oplock break unless the file is open, if the file is open then we have a
> > > reference to the inode ...
> > >
> >
> > Well, you have a reference when the entry goes onto the list. There's
> > no guarantee that you'll still have one when cifs_oplock_thread goes to
> > take it off the list however. The file could have been closed by then
> >
>
> An oplock response is handle based so we need to make sure the fid
> is valid (if not, throw away the oplock response, except for the case
> of batch oplock which we don't use yet).  Seems odd to lock the
> inode when we really need the fid (file struct)
>

True. Though that's sort of a design issue with the oplock queue
itself. The entry tracks an inode and not a filp.

The goal with this set is to fix the use-after-free problems without
changing the underlying design too much. I'm more inclined to leave
this as-is until the whole approach can be redesigned.

> We could mark the file struct (similar to the write pending flag) and
> delete such an entry off the oplockq in close (if we are closing
> a file with an oplock pending)
>

As long as you can do so in a non-racy way, then I'm not opposed to
that long-term. The problem though is that I don't have a lot of
confidence in the open file tracking code. It's extremely hard to
follow and definitely has races. I don't think it's really possible to
do what you suggest safely until the open file tracking code has been
fixed.

For now, I'm pretty sure this set should fix the problems that users
are hitting in the oplock codepath today. I'd like to fix that first
before we embark on a redesign of it.

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

[Steve French-2]

On Fri, Aug 21, 2009 at 10:48 AM, Jeff Layton <jlayton@...> wrote:

As long as you can do so in a non-racy way, then I'm not opposed to
that long-term. The problem though is that I don't have a lot of
confidence in the open file tracking code. It's extremely hard to
follow and definitely has races. I don't think it's really possible to
do what you suggest safely until the open file tracking code has been
fixed.

For now, I'm pretty sure this set should fix the problems that users
are hitting in the oplock codepath today. I'd like to fix that first
before we embark on a redesign of it.

You may be right that this should be two stages, but your 2nd and 3rd patch
are already large, so I doubt that it would grow (and might make it easier
to follow and more logical).

--
Thanks,

Steve

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

[Steve French-2]

On Fri, Aug 21, 2009 at 2:01 PM, Steve French <smfrench@...> wrote:

On Fri, Aug 21, 2009 at 10:48 AM, Jeff Layton <jlayton@...> wrote:

As long as you can do so in a non-racy way, then I'm not opposed to
that long-term. The problem though is that I don't have a lot of
confidence in the open file tracking code. It's extremely hard to
follow and definitely has races. I don't think it's really possible to
do what you suggest safely until the open file tracking code has been
fixed.

For now, I'm pretty sure this set should fix the problems that users
are hitting in the oplock codepath today. I'd like to fix that first
before we embark on a redesign of it.

You may be right that this should be two stages, but your 2nd and 3rd patch
are already large, so I doubt that it would grow (and might make it easier
to follow and more logical).

There is one other thing to consider (which might lean toward your inode
based approach rather than what I suggested about tie of the
oplock to the file struct for the fid) ... we need to move to 1st attempting
"batch oplocks" (as was discussed in June) to allow safer caching
across close (and would also helps with multiple opens from the same
client) - this will necessitate making the routine which frees inodes
aware of oplocks.   In the short run I was planning on trying this approach
(grab one batch oplock on open, and only rely on the other type of oplocks
when we can't get or lose the batch oplock) with the smb2 code and
after a few months if it works ok consider fixing the cifs code.  In the
meantime if we uses patches 2 through 4 as is (one and five are
obviously fine), it would be great if we could get at least one more ack.
I haven't found any problems yet but they are big.


--
Thanks,

Steve

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

[Jeff Layton-2]

On Fri, 21 Aug 2009 14:08:24 -0500
Steve French <smfrench@...> wrote:

> On Fri, Aug 21, 2009 at 2:01 PM, Steve French <smfrench@...> wrote:
>
> >
> >
> > On Fri, Aug 21, 2009 at 10:48 AM, Jeff Layton <jlayton@...> wrote:
> >
> >>
> >> As long as you can do so in a non-racy way, then I'm not opposed to
> >> that long-term. The problem though is that I don't have a lot of
> >> confidence in the open file tracking code. It's extremely hard to
> >> follow and definitely has races. I don't think it's really possible to
> >> do what you suggest safely until the open file tracking code has been
> >> fixed.
> >>
> >> For now, I'm pretty sure this set should fix the problems that users
> >> are hitting in the oplock codepath today. I'd like to fix that first
> >> before we embark on a redesign of it.
> >>
> >
> > You may be right that this should be two stages, but your 2nd and 3rd patch
> > are already large, so I doubt that it would grow (and might make it easier
> > to follow and more logical).
> >
>
> There is one other thing to consider (which might lean toward your inode
> based approach rather than what I suggested about tie of the
> oplock to the file struct for the fid) ... we need to move to 1st attempting
> "batch oplocks" (as was discussed in June) to allow safer caching
> across close (and would also helps with multiple opens from the same
> client) - this will necessitate making the routine which frees inodes
> aware of oplocks.   In the short run I was planning on trying this approach
> (grab one batch oplock on open, and only rely on the other type of oplocks
> when we can't get or lose the batch oplock) with the smb2 code and
> after a few months if it works ok consider fixing the cifs code.  In the
> meantime if we uses patches 2 through 4 as is (one and five are
> obviously fine), it would be great if we could get at least one more ack.
> I haven't found any problems yet but they are big.
>
>

Sorry about the size, but there were a number of races and potential
use-after-free holes in that code. I tried to do the set so that it
made a logical progression of changes, but the patches do touch the
same code numerous times. If it would be more helpful, I can send you
the set as a single large patch.

Thanks again for reviewing it promptly.

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

[Jeff Layton-2]

On Fri, 21 Aug 2009 14:01:21 -0500
Steve French <smfrench@...> wrote:

> On Fri, Aug 21, 2009 at 10:48 AM, Jeff Layton <jlayton@...> wrote:
>
> >
> > As long as you can do so in a non-racy way, then I'm not opposed to
> > that long-term. The problem though is that I don't have a lot of
> > confidence in the open file tracking code. It's extremely hard to
> > follow and definitely has races. I don't think it's really possible to
> > do what you suggest safely until the open file tracking code has been
> > fixed.
> >
> > For now, I'm pretty sure this set should fix the problems that users
> > are hitting in the oplock codepath today. I'd like to fix that first
> > before we embark on a redesign of it.
> >
>
> You may be right that this should be two stages, but your 2nd and 3rd patch
> are already large, so I doubt that it would grow (and might make it easier
> to follow and more logical).
>

Any more thoughts on this patchset?

I've been looking over what it would take to fix up the open file
handle tracking and it's going to be a substantial set of patches (on
the order of the size of the cifs_iget patchset). I'd still like to do
that, but don't think we can wait until then to fix this set of
problems.

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

[Shirish Pargaonkar]

On Tue, Aug 18, 2009 at 1:06 PM, Jeff Layton<jlayton@...> wrote:

> Right now, the GlobalOplock_Q is protected by the GlobalMid_Lock. That
> lock is also used for completely unrelated purposes (mostly for managing
> the global mid queue). Give the list its own dedicated spinlock
> (cifs_oplock_lock) and rename the list to cifs_oplock_list to
> eliminate the camel-case.
>
> Signed-off-by: Jeff Layton <jlayton@...>
> ---
>  fs/cifs/cifsfs.c    |   13 +++++++------
>  fs/cifs/cifsglob.h  |    6 +++++-
>  fs/cifs/transport.c |   17 ++++++++---------
>  3 files changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index b750aa5..3610e99 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -986,19 +986,19 @@ static int cifs_oplock_thread(void *dummyarg)
>                if (try_to_freeze())
>                        continue;
>
> -               spin_lock(&GlobalMid_Lock);
> -               if (list_empty(&GlobalOplock_Q)) {
> -                       spin_unlock(&GlobalMid_Lock);
> +               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(GlobalOplock_Q.next,
> +                       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(&GlobalMid_Lock);
> +                       spin_unlock(&cifs_oplock_lock);
>                        DeleteOplockQEntry(oplock_item);
>                        /* can not grab inode sem here since it would
>                                deadlock when oplock received on delete
> @@ -1055,7 +1055,7 @@ init_cifs(void)
>        int rc = 0;
>        cifs_proc_init();
>        INIT_LIST_HEAD(&cifs_tcp_ses_list);
> -       INIT_LIST_HEAD(&GlobalOplock_Q);
> +       INIT_LIST_HEAD(&cifs_oplock_list);
>  #ifdef CONFIG_CIFS_EXPERIMENTAL
>        INIT_LIST_HEAD(&GlobalDnotifyReqList);
>        INIT_LIST_HEAD(&GlobalDnotifyRsp_Q);
> @@ -1084,6 +1084,7 @@ 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;
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 6084d63..f100399 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -656,7 +656,11 @@ GLOBAL_EXTERN rwlock_t             cifs_tcp_ses_lock;
>  */
>  GLOBAL_EXTERN rwlock_t GlobalSMBSeslock;
>
> -GLOBAL_EXTERN struct list_head GlobalOplock_Q;
> +/* 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;
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 0ad3e2d..1da4ab2 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -119,20 +119,19 @@ AllocOplockQEntry(struct inode *pinode, __u16 fid, struct cifsTconInfo *tcon)
>                temp->pinode = pinode;
>                temp->tcon = tcon;
>                temp->netfid = fid;
> -               spin_lock(&GlobalMid_Lock);
> -               list_add_tail(&temp->qhead, &GlobalOplock_Q);
> -               spin_unlock(&GlobalMid_Lock);
> +               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(&GlobalMid_Lock);
> +       spin_lock(&cifs_oplock_lock);
>     /* should we check if list empty first? */
>        list_del(&oplockEntry->qhead);
> -       spin_unlock(&GlobalMid_Lock);
> +       spin_unlock(&cifs_oplock_lock);
>        kmem_cache_free(cifs_oplock_cachep, oplockEntry);
>  }
>
> @@ -144,14 +143,14 @@ void DeleteTconOplockQEntries(struct cifsTconInfo *tcon)
>        if (tcon == NULL)
>                return;
>
> -       spin_lock(&GlobalMid_Lock);
> -       list_for_each_entry(temp, &GlobalOplock_Q, qhead) {
> +       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(&GlobalMid_Lock);
> +       spin_unlock(&cifs_oplock_lock);
>  }
>
>  static int
> --
> 1.6.0.6
>
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@...
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>

Acked-by: Shirish Pargaonkar <shirishpargaonkar@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Shirish Pargaonkar]

On Tue, Aug 18, 2009 at 1:07 PM, Jeff Layton<jlayton@...> wrote:

> cifs_oplock_thread has a check for pTcon->needs_reconnect and will skip
> the CIFSSMBLock call if it's set.
>
> Problem: what if the tcon has this set and then gets reconnected before
> the call goes out on the wire? The oplock release isn't needed and could
> be a bad thing at that point if the filehandle was reclaimed.
>
> Cancel oplock release calls during a reconnect event.
>
> Signed-off-by: Jeff Layton <jlayton@...>
> ---
>  fs/cifs/cifsfs.c   |    2 +-
>  fs/cifs/cifsglob.h |    1 +
>  fs/cifs/connect.c  |    9 +++++++++
>  fs/cifs/misc.c     |    1 +
>  4 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 647c5bc..92e06c1 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -1024,7 +1024,7 @@ static int cifs_oplock_thread(void *dummyarg)
>                         * to server still is disconnected since oplock
>                         * already released by the server in that case
>                         */
> -                       if (!tcon->need_reconnect) {
> +                       if (!oplock->cancel) {
>                                rc = CIFSSMBLock(0, tcon, oplock->netfid,
>                                                0 /* len */ , 0 /* offset */, 0,
>                                                0, LOCKING_ANDX_OPLOCK_RELEASE,
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 363dbcf..676c107 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -448,6 +448,7 @@ struct oplock_q_entry {
>        struct inode *pinode;
>        struct cifsTconInfo *tcon;
>        __u16 netfid;
> +       bool cancel;
>  };
>
>  /* for pending dnotify requests */
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index f49304d..b7b67e9 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -121,6 +121,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>        struct cifsSesInfo *ses;
>        struct cifsTconInfo *tcon;
>        struct mid_q_entry *mid_entry;
> +       struct oplock_q_entry *oplock;
>
>        spin_lock(&GlobalMid_Lock);
>        if (server->tcpStatus == CifsExiting) {
> @@ -137,6 +138,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
>
>        /* before reconnecting the tcp session, mark the smb session (uid)
>                and the tid bad so they are not used until reconnected */
> +       spin_lock(&cifs_oplock_lock);
>        read_lock(&cifs_tcp_ses_lock);
>        list_for_each(tmp, &server->smb_ses_list) {
>                ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
> @@ -145,9 +147,16 @@ cifs_reconnect(struct TCP_Server_Info *server)
>                list_for_each(tmp2, &ses->tcon_list) {
>                        tcon = list_entry(tmp2, struct cifsTconInfo, tcon_list);
>                        tcon->need_reconnect = true;
> +                       list_for_each_entry(oplock, &cifs_oplock_list, qhead) {
> +                               if (oplock->tcon == tcon)
> +                                       oplock->cancel = true;
> +                       }
>                }
> +
>        }
>        read_unlock(&cifs_tcp_ses_lock);
> +       spin_unlock(&cifs_oplock_lock);
> +
>        /* do not want to be sending data on a socket we are freeing */
>        mutex_lock(&server->srv_mutex);
>        if (server->ssocket) {
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index 3bf3a52..4a2d297 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -612,6 +612,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv,
>                                oplock->tcon = tcon;
>                                oplock->pinode = inode;
>                                oplock->netfid = netfile->netfid;
> +                               oplock->cancel = false;
>                                spin_lock(&cifs_oplock_lock);
>                                list_add_tail(&oplock->qhead,
>                                              &cifs_oplock_list);
> --
> 1.6.0.6
>
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@...
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>

Acked-by: Shirish Pargaonkar <shiishpargaonkar@...>

with a question, does what you say about pTcon->needs_reconnect  apply
to oplock->cancel also?
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client