[PATCHSET] mkfs.xfs: codex-inspired bug fixes, part 2

[PATCHSET] mkfs.xfs: codex-inspired bug fixes, part 2

[Darrick J. Wong]

Hi all,

Here's a batch of mkfs.xfs fixes resulting from Codex reviews.
These are really just the patches from yesterday that needed more tweaks.

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

With a bit of luck, this should all go splendidly.
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=mkfs-codex-fixes2
---
Commits in this patchset:
 * mkfs: pass stat buf pointers around the protofile code
 * mkfs: fix hardlink detection in directory import code
 * mkfs: PQUOTA shouldn't conflict with GQNOENFORCE
---
 mkfs/proto.c    |   47 +++++++++++++++++++++++++++++------------------
 mkfs/xfs_mkfs.c |    2 +-
 2 files changed, 30 insertions(+), 19 deletions(-)

[PATCH 1/3] mkfs: pass stat buf pointers around the protofile code

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Not sure why the author of the new mkfs copy-in code sometimes passed
struct stat objects around by value, but let's clean up the code to pass
(const struct stat *) pointers around instead.  This will make the next
patch (which fixes hardlink detection) more neat.

While we're at it, introduce some helper variables to reduce long lines.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 mkfs/proto.c |   44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)


diff --git a/mkfs/proto.c b/mkfs/proto.c
index e258424ae4140a..e435f2e6d42b4a 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -1285,7 +1285,7 @@ writefsxattrs(
 static void
 writetimestamps(
 	struct xfs_inode	*ip,
-	struct stat		*statbuf)
+	const struct stat	*statbuf)
 {
 	struct timespec64	ts;
 
@@ -1362,20 +1362,25 @@ cleanup_hardlink_tracker(void)
 
 static xfs_ino_t
 get_hardlink_dst_inode(
-	xfs_ino_t	i_ino)
+	const struct stat	*filestat)
 {
-	for (size_t i = 0; i < hardlink_tracker.count; i++) {
-		if (hardlink_tracker.entries[i].src_ino == i_ino)
-			return hardlink_tracker.entries[i].dst_ino;
+	struct hardlink		*h = &hardlink_tracker.entries[0];
+	size_t			i = 0;
+
+	for (; i < hardlink_tracker.count; i++, h++) {
+		if (h->src_ino == filestat->st_ino)
+			return h->dst_ino;
 	}
 	return 0;
 }
 
 static void
 track_hardlink_inode(
-	ino_t	src_ino,
-	xfs_ino_t	dst_ino)
+	const struct stat	*filestat,
+	xfs_ino_t		dst_ino)
 {
+	struct hardlink		*h;
+
 	if (hardlink_tracker.count >= hardlink_tracker.size) {
 		/*
 		 * double for smaller capacity.
@@ -1399,8 +1404,10 @@ track_hardlink_inode(
 		hardlink_tracker.entries = resized_array;
 		hardlink_tracker.size = new_size;
 	}
-	hardlink_tracker.entries[hardlink_tracker.count].src_ino = src_ino;
-	hardlink_tracker.entries[hardlink_tracker.count].dst_ino = dst_ino;
+
+	h = &hardlink_tracker.entries[hardlink_tracker.count];
+	h->src_ino = filestat->st_ino;
+	h->dst_ino = dst_ino;
 	hardlink_tracker.count++;
 }
 
@@ -1415,7 +1422,7 @@ handle_hardlink(
 	struct xfs_mount	*mp,
 	struct xfs_inode	*pip,
 	struct xfs_name		xname,
-	struct stat		file_stat)
+	const struct stat	*file_stat)
 {
 	int			error;
 	xfs_ino_t		dst_ino;
@@ -1429,7 +1436,7 @@ handle_hardlink(
 	 * inode as a regular file type, and later save the source inode in our
 	 * buffer for future consumption.
 	 */
-	dst_ino = get_hardlink_dst_inode(file_stat.st_ino);
+	dst_ino = get_hardlink_dst_inode(file_stat);
 	if (dst_ino == 0)
 		return false;
 
@@ -1536,7 +1543,7 @@ create_nondir_inode(
 	struct cred		creds,
 	struct xfs_name		xname,
 	int			flags,
-	struct stat		file_stat,
+	const struct stat	*file_stat,
 	xfs_dev_t		rdev,
 	int			fd,
 	char			*src_fname)
@@ -1553,7 +1560,8 @@ create_nondir_inode(
 	 * If handle_hardlink() returns true it means the hardlink has been
 	 * correctly found and set, so we don't need to do anything else.
 	 */
-	if (file_stat.st_nlink > 1 && handle_hardlink(mp, pip, xname, file_stat)) {
+	if (file_stat->st_nlink > 1 &&
+	    handle_hardlink(mp, pip, xname, file_stat)) {
 		close(fd);
 		return;
 	}
@@ -1591,7 +1599,7 @@ create_nondir_inode(
 	/*
 	 * Copy over timestamps.
 	 */
-	writetimestamps(ip, &file_stat);
+	writetimestamps(ip, file_stat);
 
 	libxfs_trans_log_inode(tp, ip, flags);
 
@@ -1622,8 +1630,8 @@ create_nondir_inode(
 	 * If we're here it means this is the first time we're encountering an
 	 * hardlink, so we need to store it.
 	 */
-	if (file_stat.st_nlink > 1)
-		track_hardlink_inode(file_stat.st_ino, ip->i_ino);
+	if (file_stat->st_nlink > 1)
+		track_hardlink_inode(file_stat, ip->i_ino);
 
 	libxfs_irele(ip);
 }
@@ -1777,8 +1785,8 @@ handle_direntry(
 		break;
 	}
 
-	create_nondir_inode(mp, pip, fsxp, mode, creds, xname, flags, file_stat,
-			    rdev, fd, fname);
+	create_nondir_inode(mp, pip, fsxp, mode, creds, xname, flags,
+			&file_stat, rdev, fd, fname);
 out:
 	close(pathfd);
 	/* Reset path_buf to original */

Re: [PATCH 1/3] mkfs: pass stat buf pointers around the protofile code

[Christoph Hellwig]

Looks good:

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

[PATCH 2/3] mkfs: fix hardlink detection in directory import code

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

There's a serious problem in the hardlink detection code in the new mkfs
protofile functionality that copies a directory tree into the new
filesystem.  It's been long established that hardlinks can only be
detected by comparing st_ino *and* st_dev, but the new code doesn't do
that.  Fix the detector.

Cc: <linux-xfs@vger.kernel.org> # v6.17.0
Fixes: 8a4ea72724930c ("proto: add ability to populate a filesystem from a directory")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 mkfs/proto.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)


diff --git a/mkfs/proto.c b/mkfs/proto.c
index e435f2e6d42b4a..dd47e41d833e5a 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -1314,6 +1314,7 @@ writetimestamps(
 }
 
 struct hardlink {
+	dev_t		src_dev;
 	ino_t		src_ino;
 	xfs_ino_t	dst_ino;
 };
@@ -1368,7 +1369,8 @@ get_hardlink_dst_inode(
 	size_t			i = 0;
 
 	for (; i < hardlink_tracker.count; i++, h++) {
-		if (h->src_ino == filestat->st_ino)
+		if (h->src_dev == filestat->st_dev &&
+		    h->src_ino == filestat->st_ino)
 			return h->dst_ino;
 	}
 	return 0;
@@ -1406,6 +1408,7 @@ track_hardlink_inode(
 	}
 
 	h = &hardlink_tracker.entries[hardlink_tracker.count];
+	h->src_dev = filestat->st_dev;
 	h->src_ino = filestat->st_ino;
 	h->dst_ino = dst_ino;
 	hardlink_tracker.count++;

Re: [PATCH 2/3] mkfs: fix hardlink detection in directory import code

[Christoph Hellwig]

Looks good:

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

[PATCH 3/3] mkfs: PQUOTA shouldn't conflict with GQNOENFORCE

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Codex points out that project quota isn't incompatible with
non-enforcing group quota in the kernel, so those shouldn't be
incompatible in mkfs either.

Furthermore, project and group quotas are never incompatible on QUOTABIT
and V5 filesystems, so this is clearly a fat-finger error.

Cc: <linux-xfs@vger.kernel.org> # v6.13.0
Fixes: 525f826429a868 ("mkfs: add quota flags when setting up filesystem")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 mkfs/xfs_mkfs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 3da11c664255b1..a4864c37a8821b 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -976,7 +976,7 @@ static struct opt_params mopts = {
 		  .defaultval = 1,
 		},
 		{ .index = M_PQUOTA,
-		  .conflicts = { { &mopts, M_GQNOENFORCE },
+		  .conflicts = { { &mopts, M_PQNOENFORCE },
 				 { NULL, LAST_CONFLICT } },
 		  .minval = 0,
 		  .maxval = 1,

Re: [PATCH 3/3] mkfs: PQUOTA shouldn't conflict with GQNOENFORCE

[Christoph Hellwig]

Looks good:

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