[PATCH 0/4] cifs: fix several bugs in oplock queue handling (RFC)

[Jeff Layton-2]

We recently had a customer report panics in the oplock thread:

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

It looks like what happened is that an oplock break raced with a
umount, and tcon got torn down before the oplock release call
happened.

They also reported another panic within DeleteTconOplockQEntries. That
function is removing entries from the list but isn't using
list_for_each_entry_safe.

This set is a draft patchset that tries to fix these problems. I'd
appreciate prompt feedback on them. Does this look like a sane approach
to fixing these problems?

Jeff Layton (4):
  cifs: protect GlobalOplock_Q with its own mutex
  cifs: iterate over cifs_oplock_list using list_for_each_entry_safe
  cifs: take reference to inode for oplock breaks
  cifs: hold cifs_oplock_mutex while doing oplock release call

 fs/cifs/cifsfs.c    |   98 +++++++++++++++++++++++++++++----------------------
 fs/cifs/cifsglob.h  |    6 +++-
 fs/cifs/cifsproto.h |    1 -
 fs/cifs/misc.c      |    9 +++--
 fs/cifs/transport.c |   37 ++++++-------------
 5 files changed, 78 insertions(+), 73 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 mutex
(cifs_oplock_mutex). Also, 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..0d4e0da 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);
+ mutex_lock(&cifs_oplock_mutex);
+ if (list_empty(&cifs_oplock_list)) {
+ mutex_unlock(&cifs_oplock_mutex);
  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);
+ mutex_unlock(&cifs_oplock_mutex);
  DeleteOplockQEntry(oplock_item);
  /* can not grab inode sem here since it would
  deadlock when oplock received on delete
@@ -1055,7 +1055,8 @@ 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);
+ mutex_init(&cifs_oplock_mutex);
 #ifdef CONFIG_CIFS_EXPERIMENTAL
  INIT_LIST_HEAD(&GlobalDnotifyReqList);
  INIT_LIST_HEAD(&GlobalDnotifyRsp_Q);
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 6084d63..a1896e9 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 struct mutex cifs_oplock_mutex;
 
 /* Outstanding dir notify requests */
 GLOBAL_EXTERN struct list_head GlobalDnotifyReqList;
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 0ad3e2d..92e1538 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);
+ mutex_lock(&cifs_oplock_mutex);
+ list_add_tail(&temp->qhead, &cifs_oplock_list);
+ mutex_unlock(&cifs_oplock_mutex);
  }
  return temp;
-
 }
 
 void DeleteOplockQEntry(struct oplock_q_entry *oplockEntry)
 {
- spin_lock(&GlobalMid_Lock);
+ mutex_lock(&cifs_oplock_mutex);
     /* should we check if list empty first? */
  list_del(&oplockEntry->qhead);
- spin_unlock(&GlobalMid_Lock);
+ mutex_unlock(&cifs_oplock_mutex);
  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) {
+ mutex_lock(&cifs_oplock_mutex);
+ 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);
+ mutex_unlock(&cifs_oplock_mutex);
 }
 
 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]

Removing an entry from a list while you're iterating over it can be
problematic and racy, so use the _safe version of the list iteration
routine in DeleteTconOplockQEntries.

Also restructure the oplock_thread loop to make sure that if a
kthread_stop races in just as the thread goes to sleep, then it won't
just sit there for 39s.

Finally, remove DeleteOplockQEntry(). It's only called from one place
and we can avoid a function call this way.

Signed-off-by: Jeff Layton <jlayton@...>
---
 fs/cifs/cifsfs.c    |   37 +++++++++++++++++++++++--------------
 fs/cifs/cifsproto.h |    1 -
 fs/cifs/transport.c |   23 +++++------------------
 3 files changed, 28 insertions(+), 33 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 0d4e0da..ab4b373 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -976,7 +976,7 @@ cifs_destroy_mids(void)
 static int cifs_oplock_thread(void *dummyarg)
 {
  struct oplock_q_entry *oplock_item;
- struct cifsTconInfo *pTcon;
+ struct cifsTconInfo *tcon;
  struct inode *inode;
  __u16  netfid;
  int rc, waitrc = 0;
@@ -986,20 +986,24 @@ static int cifs_oplock_thread(void *dummyarg)
  if (try_to_freeze())
  continue;
 
+ /*
+ * can't reasonably use list_for_each macros here. It's
+ * possible that another thread could come along and remove
+ * some of the entries while the lock is released. It's fine
+ * though since we're just popping one off the head on each
+ * iteration anyway.
+ */
  mutex_lock(&cifs_oplock_mutex);
- if (list_empty(&cifs_oplock_list)) {
- mutex_unlock(&cifs_oplock_mutex);
- set_current_state(TASK_INTERRUPTIBLE);
- schedule_timeout(39*HZ);
- } else {
- oplock_item = list_entry(cifs_oplock_list.next,
- struct oplock_q_entry, qhead);
+ while(!list_empty(&cifs_oplock_list)) {
  cFYI(1, ("found oplock item to write out"));
- pTcon = oplock_item->tcon;
+ oplock_item = list_entry(cifs_oplock_list.next,
+ struct oplock_q_entry, qhead);
+ tcon = oplock_item->tcon;
  inode = oplock_item->pinode;
  netfid = oplock_item->netfid;
+ list_del(&oplock_item->qhead);
+ kmem_cache_free(cifs_oplock_cachep, oplock_item);
  mutex_unlock(&cifs_oplock_mutex);
- 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
@@ -1034,16 +1038,21 @@ static int cifs_oplock_thread(void *dummyarg)
  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,
+ if (!tcon->need_reconnect) {
+ rc = CIFSSMBLock(0, tcon, 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 */
+ mutex_lock(&cifs_oplock_mutex);
  }
+ mutex_unlock(&cifs_oplock_mutex);
+ set_current_state(TASK_INTERRUPTIBLE);
+ if (kthread_should_stop())
+ break;
+ /* FIXME: why 39s here? Turn this into a #define constant? */
+ schedule_timeout(39*HZ);
  } while (!kthread_should_stop());
 
  return 0;
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index da8fbf5..b7554a7 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 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);
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 92e1538..59f0e95 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -126,28 +126,15 @@ AllocOplockQEntry(struct inode *pinode, __u16 fid, struct cifsTconInfo *tcon)
  return temp;
 }
 
-void DeleteOplockQEntry(struct oplock_q_entry *oplockEntry)
-{
- mutex_lock(&cifs_oplock_mutex);
-    /* should we check if list empty first? */
- list_del(&oplockEntry->qhead);
- mutex_unlock(&cifs_oplock_mutex);
- kmem_cache_free(cifs_oplock_cachep, oplockEntry);
-}
-
-
 void DeleteTconOplockQEntries(struct cifsTconInfo *tcon)
 {
- struct oplock_q_entry *temp;
-
- if (tcon == NULL)
- return;
+ struct oplock_q_entry *entry, *next;
 
  mutex_lock(&cifs_oplock_mutex);
- 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);
+ list_for_each_entry_safe(entry, next, &cifs_oplock_list, qhead) {
+ if (entry->tcon && entry->tcon == tcon) {
+ list_del(&entry->qhead);
+ kmem_cache_free(cifs_oplock_cachep, entry);
  }
  }
  mutex_unlock(&cifs_oplock_mutex);
--
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 writeback
the inode. 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    |   33 +++++++++++++++------------------
 fs/cifs/misc.c      |    9 +++++----
 fs/cifs/transport.c |    1 +
 3 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index ab4b373..4c724d5 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -977,6 +977,7 @@ static int cifs_oplock_thread(void *dummyarg)
 {
  struct oplock_q_entry *oplock_item;
  struct cifsTconInfo *tcon;
+ struct cifsInodeInfo *cifsi;
  struct inode *inode;
  __u16  netfid;
  int rc, waitrc = 0;
@@ -1004,33 +1005,29 @@ static int cifs_oplock_thread(void *dummyarg)
  list_del(&oplock_item->qhead);
  kmem_cache_free(cifs_oplock_cachep, oplock_item);
  mutex_unlock(&cifs_oplock_mutex);
- /* 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)) {
+
+ if (inode && S_ISREG(inode->i_mode)) {
+ cifsi = CIFS_I(inode);
 #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);
+ if (rc == 0)
+ rc = waitrc;
  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));
+ 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
  of smb session using a now incorrect file
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index e079a91..5bdd81c 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -500,6 +500,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
  struct cifsTconInfo *tcon;
  struct cifsInodeInfo *pCifsInode;
  struct cifsFileInfo *netfile;
+ struct inode *inode;
 
  cFYI(1, ("Checking for oplock break or dnotify response"));
  if ((pSMB->hdr.Command == SMB_COM_NT_TRANSACT) &&
@@ -569,16 +570,16 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
  if (pSMB->Fid != netfile->netfid)
  continue;
 
- write_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;
  pCifsInode->oplockPending = true;
- AllocOplockQEntry(netfile->pInode,
-  netfile->netfid, tcon);
+ inode = igrab(netfile->pInode);
+ write_unlock(&GlobalSMBSeslock);
+ read_unlock(&cifs_tcp_ses_lock);
+ AllocOplockQEntry(inode, netfile->netfid, tcon);
  cFYI(1, ("about to wake up oplock thread"));
  if (oplockThread)
  wake_up_process(oplockThread);
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 59f0e95..c52d27c 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -133,6 +133,7 @@ void DeleteTconOplockQEntries(struct cifsTconInfo *tcon)
  mutex_lock(&cifs_oplock_mutex);
  list_for_each_entry_safe(entry, next, &cifs_oplock_list, qhead) {
  if (entry->tcon && entry->tcon == tcon) {
+ iput(entry->inode);
  list_del(&entry->qhead);
  kmem_cache_free(cifs_oplock_cachep, entry);
  }
--
1.6.0.6

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

[Jeff Layton-2]

We have to ensure that the tcon isn't freed until after this call
completes. After dequeuing an oplock entry, have cifsoplockd hold
the cifs_oplock_mutex until the oplock release call completes.

We don't want to hold the mutex indefinitely however, so release
and reacquire it on each pass through the loop. That should give
other tasks access to the queue.

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

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 4c724d5..745c3ba 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1004,7 +1004,6 @@ static int cifs_oplock_thread(void *dummyarg)
  netfid = oplock_item->netfid;
  list_del(&oplock_item->qhead);
  kmem_cache_free(cifs_oplock_cachep, oplock_item);
- mutex_unlock(&cifs_oplock_mutex);
 
  if (inode && S_ISREG(inode->i_mode)) {
  cifsi = CIFS_I(inode);
@@ -1029,12 +1028,14 @@ static int cifs_oplock_thread(void *dummyarg)
  }
  iput(inode);
 
- /* 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 */
+ /*
+ * 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, netfid,
  0 /* len */ , 0 /* offset */, 0,
@@ -1042,6 +1043,12 @@ static int cifs_oplock_thread(void *dummyarg)
  false /* wait flag */);
  cFYI(1, ("Oplock release rc = %d", rc));
  }
+
+ /*
+ * release and reacquire the oplock mutex in order to
+ * allow tasks waiting on it to have a chance.
+ */
+ mutex_unlock(&cifs_oplock_mutex);
  mutex_lock(&cifs_oplock_mutex);
  }
  mutex_unlock(&cifs_oplock_mutex);
--
1.6.0.6

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

[Shirish Pargaonkar]

On Mon, Aug 17, 2009 at 7:16 AM, Jeff Layton<jlayton@...> wrote:

> Removing an entry from a list while you're iterating over it can be
> problematic and racy, so use the _safe version of the list iteration
> routine in DeleteTconOplockQEntries.
>
> Also restructure the oplock_thread loop to make sure that if a
> kthread_stop races in just as the thread goes to sleep, then it won't
> just sit there for 39s.
>
> Finally, remove DeleteOplockQEntry(). It's only called from one place
> and we can avoid a function call this way.
>
> Signed-off-by: Jeff Layton <jlayton@...>
> ---
>  fs/cifs/cifsfs.c    |   37 +++++++++++++++++++++++--------------
>  fs/cifs/cifsproto.h |    1 -
>  fs/cifs/transport.c |   23 +++++------------------
>  3 files changed, 28 insertions(+), 33 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 0d4e0da..ab4b373 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -976,7 +976,7 @@ cifs_destroy_mids(void)
>  static int cifs_oplock_thread(void *dummyarg)
>  {
>        struct oplock_q_entry *oplock_item;
> -       struct cifsTconInfo *pTcon;
> +       struct cifsTconInfo *tcon;
>        struct inode *inode;
>        __u16  netfid;
>        int rc, waitrc = 0;
> @@ -986,20 +986,24 @@ static int cifs_oplock_thread(void *dummyarg)
>                if (try_to_freeze())
>                        continue;
>
> +               /*
> +                * can't reasonably use list_for_each macros here. It's
> +                * possible that another thread could come along and remove
> +                * some of the entries while the lock is released. It's fine
> +                * though since we're just popping one off the head on each
> +                * iteration anyway.
> +                */
>                mutex_lock(&cifs_oplock_mutex);
> -               if (list_empty(&cifs_oplock_list)) {
> -                       mutex_unlock(&cifs_oplock_mutex);
> -                       set_current_state(TASK_INTERRUPTIBLE);
> -                       schedule_timeout(39*HZ);
> -               } else {
> -                       oplock_item = list_entry(cifs_oplock_list.next,
> -                                               struct oplock_q_entry, qhead);
> +               while(!list_empty(&cifs_oplock_list)) {
>                        cFYI(1, ("found oplock item to write out"));
> -                       pTcon = oplock_item->tcon;
> +                       oplock_item = list_entry(cifs_oplock_list.next,
> +                                                struct oplock_q_entry, qhead);
> +                       tcon = oplock_item->tcon;
>                        inode = oplock_item->pinode;
>                        netfid = oplock_item->netfid;
> +                       list_del(&oplock_item->qhead);
> +                       kmem_cache_free(cifs_oplock_cachep, oplock_item);
>                        mutex_unlock(&cifs_oplock_mutex);

Is not very clear with changes and indentations, but we do take mutex
lock within the new while loop right?

> -                       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
> @@ -1034,16 +1038,21 @@ static int cifs_oplock_thread(void *dummyarg)
>                                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,
> +                       if (!tcon->need_reconnect) {
> +                               rc = CIFSSMBLock(0, tcon, 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 */
> +                       mutex_lock(&cifs_oplock_mutex);
>                }
> +               mutex_unlock(&cifs_oplock_mutex);
> +               set_current_state(TASK_INTERRUPTIBLE);
> +               if (kthread_should_stop())
> +                       break;
> +               /* FIXME: why 39s here? Turn this into a #define constant? */
> +               schedule_timeout(39*HZ);
>        } while (!kthread_should_stop());
>
>        return 0;
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index da8fbf5..b7554a7 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 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);
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 92e1538..59f0e95 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -126,28 +126,15 @@ AllocOplockQEntry(struct inode *pinode, __u16 fid, struct cifsTconInfo *tcon)
>        return temp;
>  }
>
> -void DeleteOplockQEntry(struct oplock_q_entry *oplockEntry)
> -{
> -       mutex_lock(&cifs_oplock_mutex);
> -    /* should we check if list empty first? */
> -       list_del(&oplockEntry->qhead);
> -       mutex_unlock(&cifs_oplock_mutex);
> -       kmem_cache_free(cifs_oplock_cachep, oplockEntry);
> -}
> -
> -
>  void DeleteTconOplockQEntries(struct cifsTconInfo *tcon)
>  {
> -       struct oplock_q_entry *temp;
> -
> -       if (tcon == NULL)
> -               return;
> +       struct oplock_q_entry *entry, *next;
>
>        mutex_lock(&cifs_oplock_mutex);
> -       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);
> +       list_for_each_entry_safe(entry, next, &cifs_oplock_list, qhead) {

Just a nitpick, since we do not use next anywhere, may be use NULL instead?

> +               if (entry->tcon && entry->tcon == tcon) {
> +                       list_del(&entry->qhead);
> +                       kmem_cache_free(cifs_oplock_cachep, entry);
>                }
>        }
>        mutex_unlock(&cifs_oplock_mutex);
> --
> 1.6.0.6
>
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@...
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>

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

[Shirish Pargaonkar]

On Mon, Aug 17, 2009 at 7:16 AM, Jeff Layton<jlayton@...> wrote:

> We have to ensure that the tcon isn't freed until after this call
> completes. After dequeuing an oplock entry, have cifsoplockd hold
> the cifs_oplock_mutex until the oplock release call completes.
>
> We don't want to hold the mutex indefinitely however, so release
> and reacquire it on each pass through the loop. That should give
> other tasks access to the queue.
>
> Signed-off-by: Jeff Layton <jlayton@...>
> ---
>  fs/cifs/cifsfs.c |   21 ++++++++++++++-------
>  1 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 4c724d5..745c3ba 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -1004,7 +1004,6 @@ static int cifs_oplock_thread(void *dummyarg)
>                        netfid = oplock_item->netfid;
>                        list_del(&oplock_item->qhead);
>                        kmem_cache_free(cifs_oplock_cachep, oplock_item);
> -                       mutex_unlock(&cifs_oplock_mutex);
>
>                        if (inode && S_ISREG(inode->i_mode)) {
>                                cifsi = CIFS_I(inode);
> @@ -1029,12 +1028,14 @@ static int cifs_oplock_thread(void *dummyarg)
>                        }
>                        iput(inode);
>
> -                               /* 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 */
> +                       /*
> +                        * 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, netfid,
>                                                0 /* len */ , 0 /* offset */, 0,
> @@ -1042,6 +1043,12 @@ static int cifs_oplock_thread(void *dummyarg)
>                                                false /* wait flag */);
>                                cFYI(1, ("Oplock release rc = %d", rc));

Is it OK to make an SMB call whiile holding the mutex lock?
Also wonder if rc has any meaning i.e. can it fail in any way and if so does
it have any import in this function.

>                        }
> +
> +                       /*
> +                        * release and reacquire the oplock mutex in order to
> +                        * allow tasks waiting on it to have a chance.
> +                        */
> +                       mutex_unlock(&cifs_oplock_mutex);
>                        mutex_lock(&cifs_oplock_mutex);
>                }
>                mutex_unlock(&cifs_oplock_mutex);
> --
> 1.6.0.6
>
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@...
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>

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

[Jeff Layton-2]

On Mon, 17 Aug 2009 10:09:56 -0500
Shirish Pargaonkar <shirishpargaonkar@...> wrote:

> On Mon, Aug 17, 2009 at 7:16 AM, Jeff Layton<jlayton@...> wrote:
> > Removing an entry from a list while you're iterating over it can be
> > problematic and racy, so use the _safe version of the list iteration
> > routine in DeleteTconOplockQEntries.
> >
> > Also restructure the oplock_thread loop to make sure that if a
> > kthread_stop races in just as the thread goes to sleep, then it won't
> > just sit there for 39s.
> >
> > Finally, remove DeleteOplockQEntry(). It's only called from one place
> > and we can avoid a function call this way.
> >
> > Signed-off-by: Jeff Layton <jlayton@...>
> > ---
> >  fs/cifs/cifsfs.c    |   37 +++++++++++++++++++++++--------------
> >  fs/cifs/cifsproto.h |    1 -
> >  fs/cifs/transport.c |   23 +++++------------------
> >  3 files changed, 28 insertions(+), 33 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 0d4e0da..ab4b373 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -976,7 +976,7 @@ cifs_destroy_mids(void)
> >  static int cifs_oplock_thread(void *dummyarg)
> >  {
> >        struct oplock_q_entry *oplock_item;
> > -       struct cifsTconInfo *pTcon;
> > +       struct cifsTconInfo *tcon;
> >        struct inode *inode;
> >        __u16  netfid;
> >        int rc, waitrc = 0;
> > @@ -986,20 +986,24 @@ static int cifs_oplock_thread(void *dummyarg)
> >                if (try_to_freeze())
> >                        continue;
> >
> > +               /*
> > +                * can't reasonably use list_for_each macros here. It's
> > +                * possible that another thread could come along and remove
> > +                * some of the entries while the lock is released. It's fine
> > +                * though since we're just popping one off the head on each
> > +                * iteration anyway.
> > +                */
> >                mutex_lock(&cifs_oplock_mutex);
> > -               if (list_empty(&cifs_oplock_list)) {
> > -                       mutex_unlock(&cifs_oplock_mutex);
> > -                       set_current_state(TASK_INTERRUPTIBLE);
> > -                       schedule_timeout(39*HZ);
> > -               } else {
> > -                       oplock_item = list_entry(cifs_oplock_list.next,
> > -                                               struct oplock_q_entry, qhead);
> > +               while(!list_empty(&cifs_oplock_list)) {
> >                        cFYI(1, ("found oplock item to write out"));
> > -                       pTcon = oplock_item->tcon;
> > +                       oplock_item = list_entry(cifs_oplock_list.next,
> > +                                                struct oplock_q_entry, qhead);
> > +                       tcon = oplock_item->tcon;
> >                        inode = oplock_item->pinode;
> >                        netfid = oplock_item->netfid;
> > +                       list_del(&oplock_item->qhead);
> > +                       kmem_cache_free(cifs_oplock_cachep, oplock_item);
> >                        mutex_unlock(&cifs_oplock_mutex);
>
> Is not very clear with changes and indentations, but we do take mutex
> lock within the new while loop right?
>

Right. In a later patch, I have the thread hold the mutex while the
call goes out on the wire as well (in order to prevent the tcon from
getting ripped out from under it).

> > -                       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
> > @@ -1034,16 +1038,21 @@ static int cifs_oplock_thread(void *dummyarg)
> >                                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,
> > +                       if (!tcon->need_reconnect) {
> > +                               rc = CIFSSMBLock(0, tcon, 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 */
> > +                       mutex_lock(&cifs_oplock_mutex);
> >                }
> > +               mutex_unlock(&cifs_oplock_mutex);
> > +               set_current_state(TASK_INTERRUPTIBLE);
> > +               if (kthread_should_stop())
> > +                       break;
> > +               /* FIXME: why 39s here? Turn this into a #define constant? */
> > +               schedule_timeout(39*HZ);
> >        } while (!kthread_should_stop());
> >
> >        return 0;
> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> > index da8fbf5..b7554a7 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 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);
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index 92e1538..59f0e95 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -126,28 +126,15 @@ AllocOplockQEntry(struct inode *pinode, __u16 fid, struct cifsTconInfo *tcon)
> >        return temp;
> >  }
> >
> > -void DeleteOplockQEntry(struct oplock_q_entry *oplockEntry)
> > -{
> > -       mutex_lock(&cifs_oplock_mutex);
> > -    /* should we check if list empty first? */
> > -       list_del(&oplockEntry->qhead);
> > -       mutex_unlock(&cifs_oplock_mutex);
> > -       kmem_cache_free(cifs_oplock_cachep, oplockEntry);
> > -}
> > -
> > -
> >  void DeleteTconOplockQEntries(struct cifsTconInfo *tcon)
> >  {
> > -       struct oplock_q_entry *temp;
> > -
> > -       if (tcon == NULL)
> > -               return;
> > +       struct oplock_q_entry *entry, *next;
> >
> >        mutex_lock(&cifs_oplock_mutex);
> > -       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);
> > +       list_for_each_entry_safe(entry, next, &cifs_oplock_list, qhead) {
>
> Just a nitpick, since we do not use next anywhere, may be use NULL instead?
>

No. The "next" pointer is used internally by the
list_for_each_entry_safe macro. It's not safe to use
list_for_each_entry if you're removing the entry from the list and
freeing it within the loop. The _safe versions of those routines use
dereference the "next" pointer first, so that it doesn't have to touch
the original entry on the next pass.

> > +               if (entry->tcon && entry->tcon == tcon) {
> > +                       list_del(&entry->qhead);
> > +                       kmem_cache_free(cifs_oplock_cachep, entry);
> >                }
> >        }
> >        mutex_unlock(&cifs_oplock_mutex);
> > --
> > 1.6.0.6
> >
> > _______________________________________________
> > linux-cifs-client mailing list
> > linux-cifs-client@...
> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
> >

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

[Jeff Layton-2]

On Mon, 17 Aug 2009 10:18:51 -0500
Shirish Pargaonkar <shirishpargaonkar@...> wrote:

> On Mon, Aug 17, 2009 at 7:16 AM, Jeff Layton<jlayton@...> wrote:
> > We have to ensure that the tcon isn't freed until after this call
> > completes. After dequeuing an oplock entry, have cifsoplockd hold
> > the cifs_oplock_mutex until the oplock release call completes.
> >
> > We don't want to hold the mutex indefinitely however, so release
> > and reacquire it on each pass through the loop. That should give
> > other tasks access to the queue.
> >
> > Signed-off-by: Jeff Layton <jlayton@...>
> > ---
> >  fs/cifs/cifsfs.c |   21 ++++++++++++++-------
> >  1 files changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 4c724d5..745c3ba 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -1004,7 +1004,6 @@ static int cifs_oplock_thread(void *dummyarg)
> >                        netfid = oplock_item->netfid;
> >                        list_del(&oplock_item->qhead);
> >                        kmem_cache_free(cifs_oplock_cachep, oplock_item);
> > -                       mutex_unlock(&cifs_oplock_mutex);
> >
> >                        if (inode && S_ISREG(inode->i_mode)) {
> >                                cifsi = CIFS_I(inode);
> > @@ -1029,12 +1028,14 @@ static int cifs_oplock_thread(void *dummyarg)
> >                        }
> >                        iput(inode);
> >
> > -                               /* 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 */
> > +                       /*
> > +                        * 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, netfid,
> >                                                0 /* len */ , 0 /* offset */, 0,
> > @@ -1042,6 +1043,12 @@ static int cifs_oplock_thread(void *dummyarg)
> >                                                false /* wait flag */);
> >                                cFYI(1, ("Oplock release rc = %d", rc));
>
> Is it OK to make an SMB call whiile holding the mutex lock?
> Also wonder if rc has any meaning i.e. can it fail in any way and if so does
> it have any import in this function.
>

I believe it's safe. The mutex is only intended to protect the list.

That said, the locking in these codepaths is very complex, so a
critical eye for deadlocks would be a good thing here.

> >                        }
> > +
> > +                       /*
> > +                        * release and reacquire the oplock mutex in order to
> > +                        * allow tasks waiting on it to have a chance.
> > +                        */
> > +                       mutex_unlock(&cifs_oplock_mutex);
> >                        mutex_lock(&cifs_oplock_mutex);
> >                }
> >                mutex_unlock(&cifs_oplock_mutex);
> > --
> > 1.6.0.6
> >
> > _______________________________________________
> > linux-cifs-client mailing list
> > linux-cifs-client@...
> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
> >

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

[Jeff Layton-2]

On Mon, 17 Aug 2009 11:49:03 -0400
Jeff Layton <jlayton@...> wrote:

> On Mon, 17 Aug 2009 10:18:51 -0500
> Shirish Pargaonkar <shirishpargaonkar@...> wrote:
>
> > On Mon, Aug 17, 2009 at 7:16 AM, Jeff Layton<jlayton@...> wrote:
> > > We have to ensure that the tcon isn't freed until after this call
> > > completes. After dequeuing an oplock entry, have cifsoplockd hold
> > > the cifs_oplock_mutex until the oplock release call completes.
> > >
> > > We don't want to hold the mutex indefinitely however, so release
> > > and reacquire it on each pass through the loop. That should give
> > > other tasks access to the queue.
> > >
> > > Signed-off-by: Jeff Layton <jlayton@...>
> > > ---
> > >  fs/cifs/cifsfs.c |   21 ++++++++++++++-------
> > >  1 files changed, 14 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > > index 4c724d5..745c3ba 100644
> > > --- a/fs/cifs/cifsfs.c
> > > +++ b/fs/cifs/cifsfs.c
> > > @@ -1004,7 +1004,6 @@ static int cifs_oplock_thread(void *dummyarg)
> > >                        netfid = oplock_item->netfid;
> > >                        list_del(&oplock_item->qhead);
> > >                        kmem_cache_free(cifs_oplock_cachep, oplock_item);
> > > -                       mutex_unlock(&cifs_oplock_mutex);
> > >
> > >                        if (inode && S_ISREG(inode->i_mode)) {
> > >                                cifsi = CIFS_I(inode);
> > > @@ -1029,12 +1028,14 @@ static int cifs_oplock_thread(void *dummyarg)
> > >                        }
> > >                        iput(inode);
> > >
> > > -                               /* 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 */
> > > +                       /*
> > > +                        * 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, netfid,
> > >                                                0 /* len */ , 0 /* offset */, 0,
> > > @@ -1042,6 +1043,12 @@ static int cifs_oplock_thread(void *dummyarg)
> > >                                                false /* wait flag */);
> > >                                cFYI(1, ("Oplock release rc = %d", rc));
> >
> > Is it OK to make an SMB call whiile holding the mutex lock?
> > Also wonder if rc has any meaning i.e. can it fail in any way and if so does
> > it have any import in this function.
> >
>
> I believe it's safe. The mutex is only intended to protect the list.
>
> That said, the locking in these codepaths is very complex, so a
> critical eye for deadlocks would be a good thing here.
>

Actually...now that I look with a fresh eye. The locking changes seem
safe enough, but there's still a potential race with this set. In
is_valid_oplock_break(), the code drops the cifs_tcp_ses_lock and then
calls AllocOplockQEntry. The problem there is that the tcon may be
invalid by the time you create the new entry.

I think I need to rework this set to just take a reference on the tcon
after all and have cifs_oplock_thread put it as necessary. Let me
respin and retest and I'll re-post in a few days.

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