[linux-cifs-client] [PATCH 2/4] cifs: iterate over cifs_oplock_list using list_for_each_entry_safe

[Jeff Layton]

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 at redhat.com>
---
 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