[PATCH v3 06/11] fsnotify: don't hold a spin_lock across fsnotify_recalc_mask() calls.

[NeilBrown]

From: NeilBrown <neil@xxxxxxxxxx>

fsnotify_recalc_mask() is normally called without any spin_lock held.
Normally fsnotify_group_lock() is held to keep the connector stable.

However dnotify_recalc_inode_mask() does hold a spin_lock
when calling fsnotify_recalc_mask().  This is problematic as
fsnotify_recalc_mask() can walk the d_children list which can be long.
Holding a spin_lock prevents us from using cond_resched() in that walk.

In some cases it is safe to move the
fsnotify_conn_set_children_dentry_flags() part of fsnotify_recalc_mask()
out of that spin-locked region providing fsnotify_group_lock() is held.
In the case of dnotify_handle_event() fsnotify_group_lock() is NOT held,
but there should be no need to update the child flags as we only need
to set flags (they are cleared lazily) and dnotify_handle_event()
only ever clears flags (for one-shot events), never sets them.

So create fsnotify_recalc_mask_noupdate() which skips the
fsnotify_conn_set_children_dentry_flags() part and instead returns %true
if that is still needed.  Then in places where fsnotify_recalc_mask() is
called with a spinlock held, call fsnotify_recalc_mask_noupdate()
instead and if needed call fsnotify_conn_set_children_dentry_flags()
later when the lock is dropped - except in dnotify_handle_event().

dnotify_recalc_inode_mask() also returns this truth value that that
dnotify_flush() and fcntl_dirnotify() can act accordingly after
dropping the mark->lock.

After this change, the only spin_lock held across the d_children walk
other than the parent's d_lock is the inode's i_lock.  This will be
addressed in a later patch.

Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
---
 fs/notify/dnotify/dnotify.c      | 32 ++++++++++++++++++++++----------
 fs/notify/mark.c                 | 15 ++++++++++-----
 include/linux/fsnotify_backend.h |  3 +++
 3 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 9fb73bafd41d..156f6f821c29 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -54,12 +54,13 @@ struct dnotify_mark {
 /*
  * When a process starts or stops watching an inode the set of events which
  * dnotify cares about for that inode may change.  This function runs the
- * list of everything receiving dnotify events about this directory and calculates
- * the set of all those events.  After it updates what dnotify is interested in
- * it calls the fsnotify function so it can update the set of all events relevant
+ * list of everything receiving dnotify events about this directory and
+ * calculates the set of all those events.  After it updates what dnotify is
+ * interested in it returns true if the fsnotify function should be called
+ * (after dropping the lock) so it can update the set of all events relevant
  * to this inode.
  */
-static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
+static bool dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
 {
 	__u32 new_mask = 0;
 	struct dnotify_struct *dn;
@@ -72,10 +73,9 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
 	for (dn = dn_mark->dn; dn != NULL; dn = dn->dn_next)
 		new_mask |= (dn->dn_mask & ~FS_DN_MULTISHOT);
 	if (fsn_mark->mask == new_mask)
-		return;
+		return false;
 	fsn_mark->mask = new_mask;
-
-	fsnotify_recalc_mask(fsn_mark->connector);
+	return fsnotify_recalc_mask_noupdate(fsn_mark->connector);
 }
 
 /*
@@ -116,10 +116,15 @@ static int dnotify_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
 		else {
 			*prev = dn->dn_next;
 			kmem_cache_free(dnotify_struct_cache, dn);
+			/*
+			 * We ignore return value and never call
+			 * fsnotify_conn_set_children_dentry_flags()
+			 * because it shouldn't be needed and we don't
+			 * hold the needed lock.
+			 */
 			dnotify_recalc_inode_mask(inode_mark);
 		}
 	}
-
 	spin_unlock(&inode_mark->lock);
 
 	return 0;
@@ -156,6 +161,7 @@ void dnotify_flush(struct file *filp, fl_owner_t id)
 	struct dnotify_struct **prev;
 	struct inode *inode;
 	bool free = false;
+	bool need_recalc = false;
 
 	inode = file_inode(filp);
 	if (!S_ISDIR(inode->i_mode))
@@ -174,13 +180,16 @@ void dnotify_flush(struct file *filp, fl_owner_t id)
 		if ((dn->dn_owner == id) && (dn->dn_filp == filp)) {
 			*prev = dn->dn_next;
 			kmem_cache_free(dnotify_struct_cache, dn);
-			dnotify_recalc_inode_mask(fsn_mark);
+			if (dnotify_recalc_inode_mask(fsn_mark))
+				need_recalc = true;
 			break;
 		}
 		prev = &dn->dn_next;
 	}
 
 	spin_unlock(&fsn_mark->lock);
+	if (need_recalc)
+		fsnotify_conn_set_children_dentry_flags(fsn_mark->connector);
 
 	/* nothing else could have found us thanks to the dnotify_groups
 	   mark_mutex */
@@ -265,6 +274,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned int arg)
 	fl_owner_t id = current->files;
 	struct file *f = NULL;
 	int destroy = 0, error = 0;
+	bool need_recalc = false;
 	__u32 mask;
 
 	/* we use these to tell if we need to kfree */
@@ -377,9 +387,11 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned int arg)
 	else if (error == -EEXIST)
 		error = 0;
 
-	dnotify_recalc_inode_mask(fsn_mark);
+	need_recalc = dnotify_recalc_inode_mask(fsn_mark);
 out:
 	spin_unlock(&fsn_mark->lock);
+	if (need_recalc)
+		fsnotify_conn_set_children_dentry_flags(fsn_mark->connector);
 
 	if (destroy)
 		fsnotify_detach_mark(fsn_mark);
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index b2640d836a71..fadaec74dbde 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -307,8 +307,8 @@ static bool fsnotify_conn_watches_children(
 	return fsnotify_inode_watches_children(fsnotify_conn_inode(conn));
 }
 
-static void fsnotify_conn_set_children_dentry_flags(
-					struct fsnotify_mark_connector *conn)
+void fsnotify_conn_set_children_dentry_flags(
+	struct fsnotify_mark_connector *conn)
 {
 	if (conn->type != FSNOTIFY_OBJ_TYPE_INODE)
 		return;
@@ -322,12 +322,12 @@ static void fsnotify_conn_set_children_dentry_flags(
  * this by holding a mark->lock or mark->group->mark_mutex for a mark on this
  * list.
  */
-void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
+bool fsnotify_recalc_mask_noupdate(struct fsnotify_mark_connector *conn)
 {
 	bool update_children;
 
 	if (!conn)
-		return;
+		return false;
 
 	spin_lock(&conn->lock);
 	update_children = !fsnotify_conn_watches_children(conn);
@@ -339,7 +339,12 @@ void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 	 * When parent stops watching, we clear false positive PARENT_WATCHED
 	 * flags lazily in __fsnotify_parent().
 	 */
-	if (update_children)
+	return update_children;
+}
+
+void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
+{
+	if (fsnotify_recalc_mask_noupdate(conn))
 		fsnotify_conn_set_children_dentry_flags(conn);
 }
 
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 618eed4d6d72..4e2708e39a2f 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -887,8 +887,11 @@ static inline __u32 fsnotify_calc_mask(struct fsnotify_mark *mark)
 extern __u32 fsnotify_conn_mask(struct fsnotify_mark_connector *conn);
 /* Calculate mask of events for a list of marks */
 extern void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn);
+extern bool fsnotify_recalc_mask_noupdate(struct fsnotify_mark_connector *conn);
 extern void fsnotify_init_mark(struct fsnotify_mark *mark,
 			       struct fsnotify_group *group);
+extern void fsnotify_conn_set_children_dentry_flags(
+	struct fsnotify_mark_connector *conn);
 /* Find mark belonging to given group in the list of marks */
 struct fsnotify_mark *fsnotify_find_mark(void *obj, unsigned int obj_type,
 					 struct fsnotify_group *group);
-- 
2.50.0.107.gf914562f5916.dirty