[PATCH 4/5] fsnotify: dont call fsnotify_destroy_[inode|vfsmount]_mark() with mark lock held.

[Lino Sanfilippo]

Dont call fsnotify_destroy_[inode|vfsmount]_mark() with the mark lock already held,
but take the lock within these functions.

Also dont put inode in mark_destroy() but in inode_mark_destroy().

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@xxxxxx>
---
 fs/notify/inode_mark.c    |   27 ++++++++++++++---
 fs/notify/mark.c          |   72 +++++++++++++-------------------------------
 fs/notify/vfsmount_mark.c |   10 +++---
 3 files changed, 48 insertions(+), 61 deletions(-)

diff --git a/fs/notify/inode_mark.c b/fs/notify/inode_mark.c
index b357e4b..ea015c7 100644
--- a/fs/notify/inode_mark.c
+++ b/fs/notify/inode_mark.c
@@ -61,21 +61,38 @@ void fsnotify_recalc_inode_mask(struct inode *inode)
 void fsnotify_destroy_inode_mark(struct fsnotify_mark *mark,
 				 struct inode *inode)
 {
-	assert_spin_locked(&mark->lock);
+	spin_lock(&mark->lock);
 
 	spin_lock(&inode->i_lock);
-
 	hlist_del_init_rcu(&mark->i.i_list);
-	mark->i.inode = NULL;
-
 	/*
 	 * this mark is now off the inode->i_fsnotify_marks list and we
 	 * hold the inode->i_lock, so this is the perfect time to update the
 	 * inode->i_fsnotify_mask
 	 */
 	fsnotify_recalc_inode_mask_locked(inode);
-
 	spin_unlock(&inode->i_lock);
+
+	if (mark->flags & FSNOTIFY_MARK_FLAG_OBJECT_PINNED) {
+		/*
+		 * __fsnotify_update_child_dentry_flags(inode);
+		 *
+		 * I really want to call that, but we can't, we have no idea if
+		 * the inode still exists the second we drop the mark->lock.
+		 *
+		 * The next time an event arrive to this inode from one of it's
+		 * children
+		 * __fsnotify_parent will see that the inode doesn't care about
+		 * it's children and will update all of these flags then.  So
+		 * really this is just a lazy update (and could be a perf
+		 * win...)
+		 */
+		iput(inode);
+		mark->flags &= ~FSNOTIFY_MARK_FLAG_OBJECT_PINNED;
+	}
+	mark->i.inode = NULL;
+	mark->flags &= ~FSNOTIFY_MARK_FLAG_INODE;
+	spin_unlock(&mark->lock);
 }
 
 /*
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 57f8dd6..8743532 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -112,6 +112,7 @@ void fsnotify_destroy_mark(struct fsnotify_mark *mark)
 {
 	struct fsnotify_group *group;
 	struct inode *inode = NULL;
+	struct vfsmount *mnt = NULL;
 
 	spin_lock(&mark->lock);
 	/* something else already called this function on this mark */
@@ -119,27 +120,26 @@ void fsnotify_destroy_mark(struct fsnotify_mark *mark)
 		spin_unlock(&mark->lock);
 		return;
 	}
+	if (mark->flags & FSNOTIFY_MARK_FLAG_INODE)
+		inode = mark->i.inode;
+	else if (mark->flags & FSNOTIFY_MARK_FLAG_VFSMOUNT)
+		mnt = mark->m.mnt;
+	else
+		BUG();
 	mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE;
 	group = mark->group;
 	spin_unlock(&mark->lock);
 
 	mutex_lock(&group->mark_mutex);
-	spin_lock(&mark->lock);
-
 	/* 1 from caller and 1 for being on i_list/g_list */
 	BUG_ON(atomic_read(&mark->refcnt) < 2);
 
-	if (mark->flags & FSNOTIFY_MARK_FLAG_INODE) {
-		inode = mark->i.inode;
+	if (inode)
 		fsnotify_destroy_inode_mark(mark, inode);
-	} else if (mark->flags & FSNOTIFY_MARK_FLAG_VFSMOUNT)
-		fsnotify_destroy_vfsmount_mark(mark, mark->m.mnt);
-	else
-		BUG();
-	spin_unlock(&mark->lock);
+	else /* mount */
+		fsnotify_destroy_vfsmount_mark(mark, mnt);
 
 	list_del_init(&mark->g_list);
-
 	mutex_unlock(&group->mark_mutex);
 
 	spin_lock(&destroy_lock);
@@ -156,21 +156,6 @@ void fsnotify_destroy_mark(struct fsnotify_mark *mark)
 		group->ops->freeing_mark(mark, group);
 
 	/*
-	 * __fsnotify_update_child_dentry_flags(inode);
-	 *
-	 * I really want to call that, but we can't, we have no idea if the inode
-	 * still exists the second we drop the mark->lock.
-	 *
-	 * The next time an event arrive to this inode from one of it's children
-	 * __fsnotify_parent will see that the inode doesn't care about it's
-	 * children and will update all of these flags then.  So really this
-	 * is just a lazy update (and could be a perf win...)
-	 */
-
-	if (inode && (mark->flags & FSNOTIFY_MARK_FLAG_OBJECT_PINNED))
-		iput(inode);
-
-	/*
 	 * it's possible that this group tried to destroy itself, but this
 	 * this mark was simultaneously being freed by inode.  If that's the
 	 * case, we finish freeing the group here.
@@ -217,7 +202,6 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
 	if (ret)
 		goto err;
 
-
 	if (inode)
 		__fsnotify_update_child_dentry_flags(inode);
 
@@ -265,29 +249,30 @@ void fsnotify_remove_mark_locked(struct fsnotify_mark *mark,
 				 struct fsnotify_group *group)
 {
 	struct inode *inode = NULL;
+	struct vfsmount *mnt = NULL;
 
 	spin_lock(&mark->lock);
-
 	/* something else already called this function on this mark */
 	if (!(mark->flags & FSNOTIFY_MARK_FLAG_ALIVE)) {
 		spin_unlock(&mark->lock);
 		return;
 	}
-
+	if (mark->flags & FSNOTIFY_MARK_FLAG_INODE)
+		inode = mark->i.inode;
+	else if (mark->flags & FSNOTIFY_MARK_FLAG_VFSMOUNT)
+		mnt = mark->m.mnt;
+	else
+		BUG();
 	mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE;
+	spin_unlock(&mark->lock);
 
 	/* 1 from caller and 1 for being on i_list/g_list */
 	BUG_ON(atomic_read(&mark->refcnt) < 2);
 
-
-	if (mark->flags & FSNOTIFY_MARK_FLAG_INODE) {
-		inode = mark->i.inode;
+	if (inode)
 		fsnotify_destroy_inode_mark(mark, inode);
-	} else if (mark->flags & FSNOTIFY_MARK_FLAG_VFSMOUNT)
-		fsnotify_destroy_vfsmount_mark(mark, mark->m.mnt);
-	else
-		BUG();
-	spin_unlock(&mark->lock);
+	else  /* mount */
+		fsnotify_destroy_vfsmount_mark(mark, mnt);
 
 	list_del_init(&mark->g_list);
 
@@ -305,21 +290,6 @@ void fsnotify_remove_mark_locked(struct fsnotify_mark *mark,
 		group->ops->freeing_mark(mark, group);
 
 	/*
-	 * __fsnotify_update_child_dentry_flags(inode);
-	 *
-	 * I really want to call that, but we can't, we have no idea if the inode
-	 * still exists the second we drop the mark->lock.
-	 *
-	 * The next time an event arrive to this inode from one of it's children
-	 * __fsnotify_parent will see that the inode doesn't care about it's
-	 * children and will update all of these flags then.  So really this
-	 * is just a lazy update (and could be a perf win...)
-	 */
-
-	if (inode && (mark->flags & FSNOTIFY_MARK_FLAG_OBJECT_PINNED))
-		iput(inode);
-
-	/*
 	 * it's possible that this group tried to destroy itself, but this
 	 * this mark was simultaneously being freed by inode.  If that's the
 	 * case, we finish freeing the group here.
diff --git a/fs/notify/vfsmount_mark.c b/fs/notify/vfsmount_mark.c
index a6fe562..71b6f64 100644
--- a/fs/notify/vfsmount_mark.c
+++ b/fs/notify/vfsmount_mark.c
@@ -85,16 +85,16 @@ void fsnotify_recalc_vfsmount_mask(struct vfsmount *mnt)
 void fsnotify_destroy_vfsmount_mark(struct fsnotify_mark *mark,
 				    struct vfsmount *mnt)
 {
-	assert_spin_locked(&mark->lock);
+	spin_lock(&mark->lock);
 
 	spin_lock(&mnt->mnt_root->d_lock);
-
 	hlist_del_init_rcu(&mark->m.m_list);
-	mark->m.mnt = NULL;
-
 	fsnotify_recalc_vfsmount_mask_locked(mnt);
-
 	spin_unlock(&mnt->mnt_root->d_lock);
+
+	mark->m.mnt = NULL;
+	mark->flags &= ~FSNOTIFY_MARK_FLAG_VFSMOUNT;
+	spin_unlock(&mark->lock);
 }
 
 static struct fsnotify_mark *fsnotify_find_vfsmount_mark_locked(struct fsnotify_group *group,
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/