diff mbox series

vfs: rename parent_ino to d_parent_ino and make it use RCU

Message ID 20240627161152.802567-1-mjguzik@gmail.com (mailing list archive)
State New
Headers show
Series vfs: rename parent_ino to d_parent_ino and make it use RCU | expand

Commit Message

Mateusz Guzik June 27, 2024, 4:11 p.m. UTC
The routine is used by procfs through dir_emit_dots.

The combined RCU and lock fallback implementation is too big for an
inline. Given that the routine takes a dentry argument fs/dcache.c seems
like the place to put it in.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---

this shows up with stress-ng --getdent (as one of many things)

build-tested with first adding d_parent_ino, then building allmodconfig
and fixing failures as they pop up. by the end of it grep shows no
remaining occurences, so i don't believe there will be any build regressions.

scheme borrowed from dget_parent

I cared enough to whip out the patch, but I'm not going to particularly
strongly defend it. arguably the getdent bench is not that great and I'm
not confident anything real runs into this as a problem.

 fs/dcache.c            | 30 ++++++++++++++++++++++++++++++
 fs/f2fs/file.c         |  2 +-
 fs/hfsplus/ioctl.c     |  4 ++--
 include/linux/dcache.h |  2 ++
 include/linux/fs.h     | 16 +---------------
 5 files changed, 36 insertions(+), 18 deletions(-)

Comments

Christian Brauner June 27, 2024, 4:30 p.m. UTC | #1
On Thu, Jun 27, 2024 at 06:11:52PM GMT, Mateusz Guzik wrote:
> The routine is used by procfs through dir_emit_dots.
> 
> The combined RCU and lock fallback implementation is too big for an
> inline. Given that the routine takes a dentry argument fs/dcache.c seems
> like the place to put it in.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> ---
> 
> this shows up with stress-ng --getdent (as one of many things)
> 
> build-tested with first adding d_parent_ino, then building allmodconfig
> and fixing failures as they pop up. by the end of it grep shows no
> remaining occurences, so i don't believe there will be any build regressions.
> 
> scheme borrowed from dget_parent

I like it.
diff mbox series

Patch

diff --git a/fs/dcache.c b/fs/dcache.c
index a0a944fd3a1c..38d42f333b35 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -3100,6 +3100,36 @@  void d_tmpfile(struct file *file, struct inode *inode)
 }
 EXPORT_SYMBOL(d_tmpfile);
 
+/*
+ * Obtain inode number of the parent dentry.
+ */
+ino_t d_parent_ino(struct dentry *dentry)
+{
+	struct dentry *parent;
+	struct inode *iparent;
+	unsigned seq;
+	ino_t ret;
+
+	rcu_read_lock();
+	seq = raw_seqcount_begin(&dentry->d_seq);
+	parent = READ_ONCE(dentry->d_parent);
+	iparent = d_inode_rcu(parent);
+	if (likely(iparent)) {
+		ret = iparent->i_ino;
+		if (!read_seqcount_retry(&dentry->d_seq, seq)) {
+			rcu_read_unlock();
+			return ret;
+		}
+	}
+	rcu_read_unlock();
+
+	spin_lock(&dentry->d_lock);
+	ret = dentry->d_parent->d_inode->i_ino;
+	spin_unlock(&dentry->d_lock);
+	return ret;
+}
+EXPORT_SYMBOL(d_parent_ino);
+
 static __initdata unsigned long dhash_entries;
 static int __init set_dhash_entries(char *str)
 {
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index a390b82dd26e..66ab9a859655 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -185,7 +185,7 @@  static int get_parent_ino(struct inode *inode, nid_t *pino)
 	if (!dentry)
 		return 0;
 
-	*pino = parent_ino(dentry);
+	*pino = d_parent_ino(dentry);
 	dput(dentry);
 	return 1;
 }
diff --git a/fs/hfsplus/ioctl.c b/fs/hfsplus/ioctl.c
index 5661a2e24d03..40d04dba13ac 100644
--- a/fs/hfsplus/ioctl.c
+++ b/fs/hfsplus/ioctl.c
@@ -40,7 +40,7 @@  static int hfsplus_ioctl_bless(struct file *file, int __user *user_flags)
 
 	/* Directory containing the bootable system */
 	vh->finder_info[0] = bvh->finder_info[0] =
-		cpu_to_be32(parent_ino(dentry));
+		cpu_to_be32(d_parent_ino(dentry));
 
 	/*
 	 * Bootloader. Just using the inode here breaks in the case of
@@ -51,7 +51,7 @@  static int hfsplus_ioctl_bless(struct file *file, int __user *user_flags)
 
 	/* Per spec, the OS X system folder - same as finder_info[0] here */
 	vh->finder_info[5] = bvh->finder_info[5] =
-		cpu_to_be32(parent_ino(dentry));
+		cpu_to_be32(d_parent_ino(dentry));
 
 	mutex_unlock(&sbi->vh_mutex);
 	return 0;
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 58916b3f53ad..bff956f7b2b9 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -283,6 +283,8 @@  static inline unsigned d_count(const struct dentry *dentry)
 	return dentry->d_lockref.count;
 }
 
+ino_t d_parent_ino(struct dentry *dentry);
+
 /*
  * helper function for dentry_operations.d_dname() members
  */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 32cbcb3443e5..04ee7d7c9621 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3463,20 +3463,6 @@  static inline int kiocb_set_rw_flags(struct kiocb *ki, rwf_t flags)
 	return 0;
 }
 
-static inline ino_t parent_ino(struct dentry *dentry)
-{
-	ino_t res;
-
-	/*
-	 * Don't strictly need d_lock here? If the parent ino could change
-	 * then surely we'd have a deeper race in the caller?
-	 */
-	spin_lock(&dentry->d_lock);
-	res = dentry->d_parent->d_inode->i_ino;
-	spin_unlock(&dentry->d_lock);
-	return res;
-}
-
 /* Transaction based IO helpers */
 
 /*
@@ -3601,7 +3587,7 @@  static inline bool dir_emit_dot(struct file *file, struct dir_context *ctx)
 static inline bool dir_emit_dotdot(struct file *file, struct dir_context *ctx)
 {
 	return ctx->actor(ctx, "..", 2, ctx->pos,
-			  parent_ino(file->f_path.dentry), DT_DIR);
+			  d_parent_ino(file->f_path.dentry), DT_DIR);
 }
 static inline bool dir_emit_dots(struct file *file, struct dir_context *ctx)
 {