[v2,1/4] d_walk: optionally lock also parent inode

Commit Message

Ondrej Mosnacek Aug. 1, 2019, 2:02 p.m. UTC

This will be used in a later patch to provide a function to safely
perform d_genocide on live trees.

Signed-off-by: Ondrej Mosnacek <[e-mail]>
---
 fs/dcache.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

Comments

Al Viro Aug. 1, 2019, 4:10 p.m. UTC | #1

On Thu, Aug 01, 2019 at 04:02:40PM +0200, Ondrej Mosnacek wrote:
> @@ -1276,6 +1277,8 @@ static void d_walk(struct dentry *parent, void *data,
>  again:
>  	read_seqbegin_or_lock(&rename_lock, &seq);
>  	this_parent = parent;
> +	if (lock_inode)
> +		inode_lock(this_parent->d_inode);

Suppose we are on the second pass through that thing - with rename_lock held.
What will happen to that inode_lock?

Al Viro Aug. 1, 2019, 4:12 p.m. UTC | #2

On Thu, Aug 01, 2019 at 04:02:40PM +0200, Ondrej Mosnacek wrote:
>  rename_retry:
> -	spin_unlock(&this_parent->d_lock);
>  	rcu_read_unlock();
> +	spin_unlock(&this_parent->d_lock);
> +	if (lock_inode)
> +		inode_unlock(this_parent->d_inode);

... and while we are at it, what's to keep this_parent positive here,
now that you've dropped ->d_lock on it?  Or to prevent it becoming
negative, then posiive _again_, with an unrelated inode?

Patch

diff --git a/fs/dcache.c b/fs/dcache.c
index e88cf0554e65..9ed4c0f99e57 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1259,12 +1259,13 @@  enum d_walk_ret {
 /**
  * d_walk - walk the dentry tree
  * @parent:	start of walk
+ * @lock_inode	whether to lock also parent inode
  * @data:	data passed to @enter() and @finish()
  * @enter:	callback when first entering the dentry
  *
  * The @enter() callbacks are called with d_lock held.
  */
-static void d_walk(struct dentry *parent, void *data,
+static void d_walk(struct dentry *parent, bool lock_inode, void *data,
 		   enum d_walk_ret (*enter)(void *, struct dentry *))
 {
 	struct dentry *this_parent;
@@ -1276,6 +1277,8 @@  static void d_walk(struct dentry *parent, void *data,
 again:
 	read_seqbegin_or_lock(&rename_lock, &seq);
 	this_parent = parent;
+	if (lock_inode)
+		inode_lock(this_parent->d_inode);
 	spin_lock(&this_parent->d_lock);
 
 	ret = enter(data, this_parent);
@@ -1319,9 +1322,21 @@  resume:
 
 		if (!list_empty(&dentry->d_subdirs)) {
 			spin_unlock(&this_parent->d_lock);
-			spin_release(&dentry->d_lock.dep_map, 1, _RET_IP_);
+			if (lock_inode) {
+				spin_unlock(&dentry->d_lock);
+				inode_unlock(this_parent->d_inode);
+			} else {
+				spin_release(&dentry->d_lock.dep_map,
+					     1, _RET_IP_);
+			}
 			this_parent = dentry;
-			spin_acquire(&this_parent->d_lock.dep_map, 0, 1, _RET_IP_);
+			if (lock_inode) {
+				inode_lock(this_parent->d_inode);
+				spin_lock(&this_parent->d_lock);
+			} else {
+				spin_acquire(&this_parent->d_lock.dep_map,
+					     0, 1, _RET_IP_);
+			}
 			goto repeat;
 		}
 		spin_unlock(&dentry->d_lock);
@@ -1336,6 +1351,10 @@  ascend:
 		this_parent = child->d_parent;
 
 		spin_unlock(&child->d_lock);
+		if (lock_inode) {
+			inode_unlock(child->d_inode);
+			inode_lock(this_parent->d_inode);
+		}
 		spin_lock(&this_parent->d_lock);
 
 		/* might go back up the wrong parent if we have had a rename. */
@@ -1357,12 +1376,16 @@  ascend:
 
 out_unlock:
 	spin_unlock(&this_parent->d_lock);
+	if (lock_inode)
+		inode_unlock(this_parent->d_inode);
 	done_seqretry(&rename_lock, seq);
 	return;
 
 rename_retry:
-	spin_unlock(&this_parent->d_lock);
 	rcu_read_unlock();
+	spin_unlock(&this_parent->d_lock);
+	if (lock_inode)
+		inode_unlock(this_parent->d_inode);
 	BUG_ON(seq & 1);
 	if (!retry)
 		return;
@@ -1402,7 +1425,7 @@  int path_has_submounts(const struct path *parent)
 	struct check_mount data = { .mnt = parent->mnt, .mounted = 0 };
 
 	read_seqlock_excl(&mount_lock);
-	d_walk(parent->dentry, &data, path_check_mount);
+	d_walk(parent->dentry, false, &data, path_check_mount);
 	read_sequnlock_excl(&mount_lock);
 
 	return data.mounted;
@@ -1541,7 +1564,7 @@  void shrink_dcache_parent(struct dentry *parent)
 		struct select_data data = {.start = parent};
 
 		INIT_LIST_HEAD(&data.dispose);
-		d_walk(parent, &data, select_collect);
+		d_walk(parent, false, &data, select_collect);
 
 		if (!list_empty(&data.dispose)) {
 			shrink_dentry_list(&data.dispose);
@@ -1552,7 +1575,7 @@  void shrink_dcache_parent(struct dentry *parent)
 		if (!data.found)
 			break;
 		data.victim = NULL;
-		d_walk(parent, &data, select_collect2);
+		d_walk(parent, false, &data, select_collect2);
 		if (data.victim) {
 			struct dentry *parent;
 			spin_lock(&data.victim->d_lock);
@@ -1599,7 +1622,7 @@  static enum d_walk_ret umount_check(void *_data, struct dentry *dentry)
 static void do_one_tree(struct dentry *dentry)
 {
 	shrink_dcache_parent(dentry);
-	d_walk(dentry, dentry, umount_check);
+	d_walk(dentry, false, dentry, umount_check);
 	d_drop(dentry);
 	dput(dentry);
 }
@@ -1656,7 +1679,7 @@  void d_invalidate(struct dentry *dentry)
 	shrink_dcache_parent(dentry);
 	for (;;) {
 		struct dentry *victim = NULL;
-		d_walk(dentry, &victim, find_submount);
+		d_walk(dentry, false, &victim, find_submount);
 		if (!victim) {
 			if (had_submounts)
 				shrink_dcache_parent(dentry);
@@ -3106,7 +3129,7 @@  static enum d_walk_ret d_genocide_kill(void *data, struct dentry *dentry)
 
 void d_genocide(struct dentry *parent)
 {
-	d_walk(parent, parent, d_genocide_kill);
+	d_walk(parent, false, parent, d_genocide_kill);
 }
 
 EXPORT_SYMBOL(d_genocide);