diff mbox series

[16/32] xfs: create a hashname function for parent pointers

Message ID 171270969823.3631889.1348496929393481589.stgit@frogsfrogsfrogs (mailing list archive)
State Superseded
Headers show
Series [01/32] xfs: rearrange xfs_attr_match parameters | expand

Commit Message

Darrick J. Wong April 10, 2024, 12:57 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Although directory entry and parent pointer recordsets look very similar
(name -> ino), there's one major difference between them: a file can be
hardlinked from multiple parent directories with the same filename.
This is common in shared container environments where a base directory
tree might be hardlink-copied multiple times.  IOWs the same 'ls'
program might be hardlinked to multiple /srv/*/bin/ls paths.

We don't want parent pointer operations to bog down on hash collisions
between the same dirent name, so create a special hash function that
mixes in the parent directory inode number.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c   |    3 +++
 fs/xfs/libxfs/xfs_parent.c |   49 ++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_parent.h |    5 ++++
 fs/xfs/scrub/attr.c        |    4 ++++
 4 files changed, 61 insertions(+)

Comments

Christoph Hellwig April 10, 2024, 5:33 a.m. UTC | #1 
> +	/*
> +	 * Use the same dirent name hash as would be used on the directory, but
> +	 * mix in the parent inode number.
> +	 */
> +	ret = xfs_dir2_hashname(mp, &xname);
> +	ret ^= upper_32_bits(parent_ino);
> +	ret ^= lower_32_bits(parent_ino);
> +	return ret;

Totally superficial nit, but wouldn't this read a little nicer as:

	return xfs_dir2_hashname(mp, &xname) ^
		lower_32_bits(parent_ino) ^
		upper_32_bits(parent_ino);

?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong April 10, 2024, 9:39 p.m. UTC | #2 
On Tue, Apr 09, 2024 at 10:33:45PM -0700, Christoph Hellwig wrote:
> > +	/*
> > +	 * Use the same dirent name hash as would be used on the directory, but
> > +	 * mix in the parent inode number.
> > +	 */
> > +	ret = xfs_dir2_hashname(mp, &xname);
> > +	ret ^= upper_32_bits(parent_ino);
> > +	ret ^= lower_32_bits(parent_ino);
> > +	return ret;
> 
> Totally superficial nit, but wouldn't this read a little nicer as:
> 
> 	return xfs_dir2_hashname(mp, &xname) ^
> 		lower_32_bits(parent_ino) ^
> 		upper_32_bits(parent_ino);
> 
> ?

Yeah, will change.

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

--D
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 41de6a135d907..99930472e59da 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -439,6 +439,9 @@  xfs_attr_hashval(
 {
 	ASSERT(xfs_attr_check_namespace(attr_flags));
 
+	if (attr_flags & XFS_ATTR_PARENT)
+		return xfs_parent_hashattr(mp, name, namelen, value, valuelen);
+
 	return xfs_attr_hashname(name, namelen);
 }
 
diff --git a/fs/xfs/libxfs/xfs_parent.c b/fs/xfs/libxfs/xfs_parent.c
index 5961fa8c85615..d24104821a090 100644
--- a/fs/xfs/libxfs/xfs_parent.c
+++ b/fs/xfs/libxfs/xfs_parent.c
@@ -90,3 +90,52 @@  xfs_parent_valuecheck(
 
 	return true;
 }
+
+/* Compute the attribute name hash for a parent pointer. */
+xfs_dahash_t
+xfs_parent_hashval(
+	struct xfs_mount		*mp,
+	const uint8_t			*name,
+	int				namelen,
+	xfs_ino_t			parent_ino)
+{
+	struct xfs_name			xname = {
+		.name			= name,
+		.len			= namelen,
+	};
+	xfs_dahash_t			ret;
+
+	/*
+	 * Use the same dirent name hash as would be used on the directory, but
+	 * mix in the parent inode number.
+	 */
+	ret = xfs_dir2_hashname(mp, &xname);
+	ret ^= upper_32_bits(parent_ino);
+	ret ^= lower_32_bits(parent_ino);
+	return ret;
+}
+
+/* Compute the attribute name hash from the xattr components. */
+xfs_dahash_t
+xfs_parent_hashattr(
+	struct xfs_mount		*mp,
+	const uint8_t			*name,
+	int				namelen,
+	const void			*value,
+	int				valuelen)
+{
+	const struct xfs_parent_rec	*rec = value;
+
+	/* Requires a local attr value in xfs_parent_rec format */
+	if (valuelen != sizeof(struct xfs_parent_rec)) {
+		ASSERT(valuelen == sizeof(struct xfs_parent_rec));
+		return 0;
+	}
+
+	if (!value) {
+		ASSERT(value != NULL);
+		return 0;
+	}
+
+	return xfs_parent_hashval(mp, name, namelen, be64_to_cpu(rec->p_ino));
+}
diff --git a/fs/xfs/libxfs/xfs_parent.h b/fs/xfs/libxfs/xfs_parent.h
index ef8aff8607801..6a4028871b72a 100644
--- a/fs/xfs/libxfs/xfs_parent.h
+++ b/fs/xfs/libxfs/xfs_parent.h
@@ -12,4 +12,9 @@  bool xfs_parent_namecheck(unsigned int attr_flags, const void *name,
 bool xfs_parent_valuecheck(struct xfs_mount *mp, const void *value,
 		size_t valuelen);
 
+xfs_dahash_t xfs_parent_hashval(struct xfs_mount *mp, const uint8_t *name,
+		int namelen, xfs_ino_t parent_ino);
+xfs_dahash_t xfs_parent_hashattr(struct xfs_mount *mp, const uint8_t *name,
+		int namelen, const void *value, int valuelen);
+
 #endif /* __XFS_PARENT_H__ */
diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
index d5c2e73be8623..fe51a17661831 100644
--- a/fs/xfs/scrub/attr.c
+++ b/fs/xfs/scrub/attr.c
@@ -536,6 +536,10 @@  xchk_xattr_rec(
 			xchk_da_set_corrupt(ds, level);
 			goto out;
 		}
+		if (ent->flags & XFS_ATTR_PARENT) {
+			xchk_da_set_corrupt(ds, level);
+			goto out;
+		}
 		calc_hash = xfs_attr_hashval(mp, ent->flags, rentry->name,
 					     rentry->namelen, NULL,
 					     be32_to_cpu(rentry->valuelen));