[PATCHSET v2] xfs: proposed bug fixes for 6.13

[PATCHSET v2] xfs: proposed bug fixes for 6.13

[Darrick J. Wong]

Hi all,

Here are even more bugfixes for 6.13 that have been accumulating since
6.12 was released.

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

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=proposed-fixes-6.13
---
Commits in this patchset:
 * xfs: don't move nondir/nonreg temporary repair files to the metadir namespace
 * xfs: don't crash on corrupt /quotas dirent
 * xfs: check pre-metadir fields correctly
 * xfs: fix zero byte checking in the superblock scrubber
 * xfs: return from xfs_symlink_verify early on V4 filesystems
 * xfs: port xfs_ioc_start_commit to multigrain timestamps
---
 fs/xfs/libxfs/xfs_symlink_remote.c |    4 ++
 fs/xfs/scrub/agheader.c            |   66 ++++++++++++++++++++++++++++--------
 fs/xfs/scrub/tempfile.c            |    3 ++
 fs/xfs/xfs_exchrange.c             |   14 ++++----
 fs/xfs/xfs_qm.c                    |    7 ++++
 5 files changed, 71 insertions(+), 23 deletions(-)

[PATCH 1/6] xfs: don't move nondir/nonreg temporary repair files to the metadir namespace

[Darrick J. Wong]

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

Only directories or regular files are allowed in the metadata directory
tree.  Don't move the repair tempfile to the metadir namespace if this
is not true; this will cause the inode verifiers to trip.

Cc: <stable@vger.kernel.org> # v6.13-rc1
Fixes: 9dc31acb01a1c7 ("xfs: move repair temporary files to the metadata directory tree")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/scrub/tempfile.c |    3 +++
 1 file changed, 3 insertions(+)


diff --git a/fs/xfs/scrub/tempfile.c b/fs/xfs/scrub/tempfile.c
index dc3802c7f678ce..82ecbb654fbb39 100644
--- a/fs/xfs/scrub/tempfile.c
+++ b/fs/xfs/scrub/tempfile.c
@@ -204,6 +204,9 @@ xrep_tempfile_adjust_directory_tree(
 
 	if (!sc->ip || !xfs_is_metadir_inode(sc->ip))
 		return 0;
+	if (!S_ISDIR(VFS_I(sc->tempip)->i_mode) &&
+	    !S_ISREG(VFS_I(sc->tempip)->i_mode))
+		return 0;
 
 	xfs_ilock(sc->tempip, XFS_IOLOCK_EXCL);
 	sc->temp_ilock_flags |= XFS_IOLOCK_EXCL;

[PATCH 2/6] xfs: don't crash on corrupt /quotas dirent

[Darrick J. Wong]

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

If the /quotas dirent points to an inode but the inode isn't loadable
(and hence mkdir returns -EEXIST), don't crash, just bail out.

Cc: <stable@vger.kernel.org> # v6.13-rc1
Fixes: e80fbe1ad8eff7 ("xfs: use metadir for quota inodes")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/xfs_qm.c |    7 +++++++
 1 file changed, 7 insertions(+)


diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 69b70c3e999d72..dc8b1010d4d332 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -731,6 +731,13 @@ xfs_qm_create_metadir_qinos(
 		error = xfs_dqinode_mkdir_parent(mp, &qi->qi_dirip);
 		if (error && error != -EEXIST)
 			return error;
+		/*
+		 * If the /quotas dirent points to an inode that isn't
+		 * loadable, qi_dirip will be NULL but mkdir_parent will return
+		 * -EEXIST.  In this case the metadir is corrupt, so bail out.
+		 */
+		if (XFS_IS_CORRUPT(mp, qi->qi_dirip == NULL))
+			return -EFSCORRUPTED;
 	}
 
 	if (XFS_IS_UQUOTA_ON(mp) && !qi->qi_uquotaip) {

[PATCH 3/6] xfs: check pre-metadir fields correctly

[Darrick J. Wong]

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

The checks that were added to the superblock scrubber for metadata
directories aren't quite right -- the old inode pointers are now defined
to be zeroes until someone else reuses them.  Also consolidate the new
metadir field checks to one place; they were inexplicably scattered
around.

Cc: <stable@vger.kernel.org> # v6.13-rc1
Fixes: 28d756d4d562dc ("xfs: update sb field checks when metadir is turned on")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/scrub/agheader.c |   40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)


diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
index 1d41b85478da9d..88063d67cb5fd4 100644
--- a/fs/xfs/scrub/agheader.c
+++ b/fs/xfs/scrub/agheader.c
@@ -145,8 +145,11 @@ xchk_superblock(
 		xchk_block_set_preen(sc, bp);
 
 	if (xfs_has_metadir(sc->mp)) {
-		if (sb->sb_metadirino != cpu_to_be64(mp->m_sb.sb_metadirino))
-			xchk_block_set_preen(sc, bp);
+		if (sb->sb_rbmino != cpu_to_be64(0))
+			xchk_block_set_corrupt(sc, bp);
+
+		if (sb->sb_rsumino != cpu_to_be64(0))
+			xchk_block_set_corrupt(sc, bp);
 	} else {
 		if (sb->sb_rbmino != cpu_to_be64(mp->m_sb.sb_rbmino))
 			xchk_block_set_preen(sc, bp);
@@ -229,7 +232,13 @@ xchk_superblock(
 	 * sb_icount, sb_ifree, sb_fdblocks, sb_frexents
 	 */
 
-	if (!xfs_has_metadir(mp)) {
+	if (xfs_has_metadir(mp)) {
+		if (sb->sb_uquotino != cpu_to_be64(0))
+			xchk_block_set_corrupt(sc, bp);
+
+		if (sb->sb_gquotino != cpu_to_be64(0))
+			xchk_block_set_preen(sc, bp);
+	} else {
 		if (sb->sb_uquotino != cpu_to_be64(mp->m_sb.sb_uquotino))
 			xchk_block_set_preen(sc, bp);
 
@@ -281,15 +290,8 @@ xchk_superblock(
 		if (!!(sb->sb_features2 & cpu_to_be32(~v2_ok)))
 			xchk_block_set_corrupt(sc, bp);
 
-		if (xfs_has_metadir(mp)) {
-			if (sb->sb_rgblklog != mp->m_sb.sb_rgblklog)
-				xchk_block_set_corrupt(sc, bp);
-			if (memchr_inv(sb->sb_pad, 0, sizeof(sb->sb_pad)))
-				xchk_block_set_preen(sc, bp);
-		} else {
-			if (sb->sb_features2 != sb->sb_bad_features2)
-				xchk_block_set_preen(sc, bp);
-		}
+		if (sb->sb_features2 != sb->sb_bad_features2)
+			xchk_block_set_preen(sc, bp);
 	}
 
 	/* Check sb_features2 flags that are set at mkfs time. */
@@ -351,7 +353,10 @@ xchk_superblock(
 		if (sb->sb_spino_align != cpu_to_be32(mp->m_sb.sb_spino_align))
 			xchk_block_set_corrupt(sc, bp);
 
-		if (!xfs_has_metadir(mp)) {
+		if (xfs_has_metadir(mp)) {
+			if (sb->sb_pquotino != cpu_to_be64(0))
+				xchk_block_set_corrupt(sc, bp);
+		} else {
 			if (sb->sb_pquotino != cpu_to_be64(mp->m_sb.sb_pquotino))
 				xchk_block_set_preen(sc, bp);
 		}
@@ -366,11 +371,20 @@ xchk_superblock(
 	}
 
 	if (xfs_has_metadir(mp)) {
+		if (sb->sb_metadirino != cpu_to_be64(mp->m_sb.sb_metadirino))
+			xchk_block_set_preen(sc, bp);
+
 		if (sb->sb_rgcount != cpu_to_be32(mp->m_sb.sb_rgcount))
 			xchk_block_set_corrupt(sc, bp);
 
 		if (sb->sb_rgextents != cpu_to_be32(mp->m_sb.sb_rgextents))
 			xchk_block_set_corrupt(sc, bp);
+
+		if (sb->sb_rgblklog != mp->m_sb.sb_rgblklog)
+			xchk_block_set_corrupt(sc, bp);
+
+		if (memchr_inv(sb->sb_pad, 0, sizeof(sb->sb_pad)))
+			xchk_block_set_corrupt(sc, bp);
 	}
 
 	/* Everything else must be zero. */

[PATCH 4/6] xfs: fix zero byte checking in the superblock scrubber

[Darrick J. Wong]

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

The logic to check that the region past the end of the superblock is all
zeroes is wrong -- we don't want to check only the bytes past the end of
the maximally sized ondisk superblock structure as currently defined in
xfs_format.h; we want to check the bytes beyond the end of the ondisk as
defined by the feature bits.

Port the superblock size logic from xfs_repair and then put it to use in
xfs_scrub.

Cc: <stable@vger.kernel.org> # v4.15
Fixes: 21fb4cb1981ef7 ("xfs: scrub the secondary superblocks")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/scrub/agheader.c |   26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
index 88063d67cb5fd4..190d56f8134463 100644
--- a/fs/xfs/scrub/agheader.c
+++ b/fs/xfs/scrub/agheader.c
@@ -59,6 +59,27 @@ xchk_superblock_xref(
 	/* scrub teardown will take care of sc->sa for us */
 }
 
+/* Calculate the ondisk superblock size in bytes */
+STATIC size_t
+xchk_superblock_ondisk_size(
+	struct xfs_mount	*mp)
+{
+	if (xfs_has_metadir(mp))
+		return offsetofend(struct xfs_dsb, sb_pad);
+	if (xfs_has_metauuid(mp))
+		return offsetofend(struct xfs_dsb, sb_meta_uuid);
+	if (xfs_has_crc(mp))
+		return offsetofend(struct xfs_dsb, sb_lsn);
+	if (xfs_sb_version_hasmorebits(&mp->m_sb))
+		return offsetofend(struct xfs_dsb, sb_bad_features2);
+	if (xfs_has_logv2(mp))
+		return offsetofend(struct xfs_dsb, sb_logsunit);
+	if (xfs_has_sector(mp))
+		return offsetofend(struct xfs_dsb, sb_logsectsize);
+	/* only support dirv2 or more recent */
+	return offsetofend(struct xfs_dsb, sb_dirblklog);
+}
+
 /*
  * Scrub the filesystem superblock.
  *
@@ -75,6 +96,7 @@ xchk_superblock(
 	struct xfs_buf		*bp;
 	struct xfs_dsb		*sb;
 	struct xfs_perag	*pag;
+	size_t			sblen;
 	xfs_agnumber_t		agno;
 	uint32_t		v2_ok;
 	__be32			features_mask;
@@ -388,8 +410,8 @@ xchk_superblock(
 	}
 
 	/* Everything else must be zero. */
-	if (memchr_inv(sb + 1, 0,
-			BBTOB(bp->b_length) - sizeof(struct xfs_dsb)))
+	sblen = xchk_superblock_ondisk_size(mp);
+	if (memchr_inv((char *)sb + sblen, 0, BBTOB(bp->b_length) - sblen))
 		xchk_block_set_corrupt(sc, bp);
 
 	xchk_superblock_xref(sc, bp);

[PATCH 5/6] xfs: return from xfs_symlink_verify early on V4 filesystems

[Darrick J. Wong]

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

V4 symlink blocks didn't have headers, so return early if this is a V4
filesystem.

Cc: <stable@vger.kernel.org> # v5.1
Fixes: 39708c20ab5133 ("xfs: miscellaneous verifier magic value fixups")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_symlink_remote.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c
index f228127a88ff26..fb47a76ead18c2 100644
--- a/fs/xfs/libxfs/xfs_symlink_remote.c
+++ b/fs/xfs/libxfs/xfs_symlink_remote.c
@@ -92,8 +92,10 @@ xfs_symlink_verify(
 	struct xfs_mount	*mp = bp->b_mount;
 	struct xfs_dsymlink_hdr	*dsl = bp->b_addr;
 
+	/* no verification of non-crc buffers */
 	if (!xfs_has_crc(mp))
-		return __this_address;
+		return NULL;
+
 	if (!xfs_verify_magic(bp, dsl->sl_magic))
 		return __this_address;
 	if (!uuid_equal(&dsl->sl_uuid, &mp->m_sb.sb_meta_uuid))

[PATCH 6/6] xfs: port xfs_ioc_start_commit to multigrain timestamps

[Darrick J. Wong]

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

Take advantage of the multigrain timestamp APIs to ensure that nobody
can sneak in and write things to a file between starting a file update
operation and committing the results.  This should have been part of the
multigrain timestamp merge, but I forgot to fling it at jlayton when he
resubmitted the patchset due to developer bandwidth problems.

Cc: jlayton@kernel.org
Cc: <stable@vger.kernel.org> # v6.13-rc1
Fixes: 4e40eff0b5737c ("fs: add infrastructure for multigrain timestamps")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_exchrange.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)


diff --git a/fs/xfs/xfs_exchrange.c b/fs/xfs/xfs_exchrange.c
index 9ab05ad224d127..dd24de420714ab 100644
--- a/fs/xfs/xfs_exchrange.c
+++ b/fs/xfs/xfs_exchrange.c
@@ -854,7 +854,7 @@ xfs_ioc_start_commit(
 	struct xfs_commit_range __user	*argp)
 {
 	struct xfs_commit_range		args = { };
-	struct timespec64		ts;
+	struct kstat			kstat;
 	struct xfs_commit_range_fresh	*kern_f;
 	struct xfs_commit_range_fresh	__user *user_f;
 	struct inode			*inode2 = file_inode(file);
@@ -871,12 +871,12 @@ xfs_ioc_start_commit(
 	memcpy(&kern_f->fsid, ip2->i_mount->m_fixedfsid, sizeof(xfs_fsid_t));
 
 	xfs_ilock(ip2, lockflags);
-	ts = inode_get_ctime(inode2);
-	kern_f->file2_ctime		= ts.tv_sec;
-	kern_f->file2_ctime_nsec	= ts.tv_nsec;
-	ts = inode_get_mtime(inode2);
-	kern_f->file2_mtime		= ts.tv_sec;
-	kern_f->file2_mtime_nsec	= ts.tv_nsec;
+	/* Force writing of a distinct ctime if any writes happen. */
+	fill_mg_cmtime(&kstat, STATX_CTIME | STATX_MTIME, inode2);
+	kern_f->file2_ctime		= kstat.ctime.tv_sec;
+	kern_f->file2_ctime_nsec	= kstat.ctime.tv_nsec;
+	kern_f->file2_mtime		= kstat.mtime.tv_sec;
+	kern_f->file2_mtime_nsec	= kstat.mtime.tv_nsec;
 	kern_f->file2_ino		= ip2->i_ino;
 	kern_f->file2_gen		= inode2->i_generation;
 	kern_f->magic			= XCR_FRESH_MAGIC;

Re: [PATCH 6/6] xfs: port xfs_ioc_start_commit to multigrain timestamps

[Jeff Layton]

On Tue, 2024-12-03 at 19:03 -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Take advantage of the multigrain timestamp APIs to ensure that nobody
> can sneak in and write things to a file between starting a file update
> operation and committing the results.  This should have been part of the
> multigrain timestamp merge, but I forgot to fling it at jlayton when he
> resubmitted the patchset due to developer bandwidth problems.
> 
> Cc: jlayton@kernel.org
> Cc: <stable@vger.kernel.org> # v6.13-rc1
> Fixes: 4e40eff0b5737c ("fs: add infrastructure for multigrain timestamps")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_exchrange.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_exchrange.c b/fs/xfs/xfs_exchrange.c
> index 9ab05ad224d127..dd24de420714ab 100644
> --- a/fs/xfs/xfs_exchrange.c
> +++ b/fs/xfs/xfs_exchrange.c
> @@ -854,7 +854,7 @@ xfs_ioc_start_commit(
>  	struct xfs_commit_range __user	*argp)
>  {
>  	struct xfs_commit_range		args = { };
> -	struct timespec64		ts;
> +	struct kstat			kstat;
>  	struct xfs_commit_range_fresh	*kern_f;
>  	struct xfs_commit_range_fresh	__user *user_f;
>  	struct inode			*inode2 = file_inode(file);
> @@ -871,12 +871,12 @@ xfs_ioc_start_commit(
>  	memcpy(&kern_f->fsid, ip2->i_mount->m_fixedfsid, sizeof(xfs_fsid_t));
>  
>  	xfs_ilock(ip2, lockflags);
> -	ts = inode_get_ctime(inode2);
> -	kern_f->file2_ctime		= ts.tv_sec;
> -	kern_f->file2_ctime_nsec	= ts.tv_nsec;
> -	ts = inode_get_mtime(inode2);
> -	kern_f->file2_mtime		= ts.tv_sec;
> -	kern_f->file2_mtime_nsec	= ts.tv_nsec;
> +	/* Force writing of a distinct ctime if any writes happen. */
> +	fill_mg_cmtime(&kstat, STATX_CTIME | STATX_MTIME, inode2);
> +	kern_f->file2_ctime		= kstat.ctime.tv_sec;
> +	kern_f->file2_ctime_nsec	= kstat.ctime.tv_nsec;
> +	kern_f->file2_mtime		= kstat.mtime.tv_sec;
> +	kern_f->file2_mtime_nsec	= kstat.mtime.tv_nsec;
>  	kern_f->file2_ino		= ip2->i_ino;
>  	kern_f->file2_gen		= inode2->i_generation;
>  	kern_f->magic			= XCR_FRESH_MAGIC;
> 

Looks good!

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/6] xfs: don't move nondir/nonreg temporary repair files to the metadir namespace

[Christoph Hellwig]

On Tue, Dec 03, 2024 at 07:02:29PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Only directories or regular files are allowed in the metadata directory
> tree.  Don't move the repair tempfile to the metadir namespace if this
> is not true; this will cause the inode verifiers to trip.

Shouldn't this be an error instead of silently returning?  Either way
the function could probably use a lot more comments explaining what is
doing and why.

Re: [PATCH 2/6] xfs: don't crash on corrupt /quotas dirent

[Christoph Hellwig]

On Tue, Dec 03, 2024 at 07:02:45PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If the /quotas dirent points to an inode but the inode isn't loadable
> (and hence mkdir returns -EEXIST), don't crash, just bail out.

Looks good:

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

Re: [PATCH 3/6] xfs: check pre-metadir fields correctly

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 4/6] xfs: fix zero byte checking in the superblock scrubber

[Christoph Hellwig]

On Tue, Dec 03, 2024 at 07:03:16PM -0800, Darrick J. Wong wrote:
> +/* Calculate the ondisk superblock size in bytes */
> +STATIC size_t
> +xchk_superblock_ondisk_size(
> +	struct xfs_mount	*mp)
> +{
> +	if (xfs_has_metadir(mp))
> +		return offsetofend(struct xfs_dsb, sb_pad);
> +	if (xfs_has_metauuid(mp))
> +		return offsetofend(struct xfs_dsb, sb_meta_uuid);
> +	if (xfs_has_crc(mp))
> +		return offsetofend(struct xfs_dsb, sb_lsn);
> +	if (xfs_sb_version_hasmorebits(&mp->m_sb))
> +		return offsetofend(struct xfs_dsb, sb_bad_features2);
> +	if (xfs_has_logv2(mp))
> +		return offsetofend(struct xfs_dsb, sb_logsunit);
> +	if (xfs_has_sector(mp))
> +		return offsetofend(struct xfs_dsb, sb_logsectsize);
> +	/* only support dirv2 or more recent */
> +	return offsetofend(struct xfs_dsb, sb_dirblklog);

This really should be libxfs so tht it can be shared with
secondary_sb_whack in xfsrepair.  The comment at the end of
the xfs_dsb definition should also be changed to point to this
libxfs version.

> +}
>  	/* Everything else must be zero. */
> -	if (memchr_inv(sb + 1, 0,
> -			BBTOB(bp->b_length) - sizeof(struct xfs_dsb)))
> +	sblen = xchk_superblock_ondisk_size(mp);
> +	if (memchr_inv((char *)sb + sblen, 0, BBTOB(bp->b_length) - sblen))

This could be simplified to

	if (memchr_inv(bp->b_addr + sblen, 0, BBTOB(bp->b_length) - sblen))

Otherwise looks good:

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

Re: [PATCH 5/6] xfs: return from xfs_symlink_verify early on V4 filesystems

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 6/6] xfs: port xfs_ioc_start_commit to multigrain timestamps

[Christoph Hellwig]

Looks good:

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

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Bill O'Donnell]

On Tue, Dec 03, 2024 at 07:02:23PM -0800, Darrick J. Wong wrote:
> Hi all,
> 
> Here are even more bugfixes for 6.13 that have been accumulating since
> 6.12 was released.
> 
> 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

Hi Darrick-

I must ask, why are these constant bug fixes and fixes for fixes, and fixes for fixes for fixes
often appearing? It's worrying that xfs is becoming rather dodgy these days. Do things need to be
this complicated?

-Bill


> 
> kernel git tree:
> https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=proposed-fixes-6.13
> ---
> Commits in this patchset:
>  * xfs: don't move nondir/nonreg temporary repair files to the metadir namespace
>  * xfs: don't crash on corrupt /quotas dirent
>  * xfs: check pre-metadir fields correctly
>  * xfs: fix zero byte checking in the superblock scrubber
>  * xfs: return from xfs_symlink_verify early on V4 filesystems
>  * xfs: port xfs_ioc_start_commit to multigrain timestamps
> ---
>  fs/xfs/libxfs/xfs_symlink_remote.c |    4 ++
>  fs/xfs/scrub/agheader.c            |   66 ++++++++++++++++++++++++++++--------
>  fs/xfs/scrub/tempfile.c            |    3 ++
>  fs/xfs/xfs_exchrange.c             |   14 ++++----
>  fs/xfs/xfs_qm.c                    |    7 ++++
>  5 files changed, 71 insertions(+), 23 deletions(-)
> 
> 

Re: [PATCH 4/6] xfs: fix zero byte checking in the superblock scrubber

[Darrick J. Wong]

On Wed, Dec 04, 2024 at 12:27:38AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 03, 2024 at 07:03:16PM -0800, Darrick J. Wong wrote:
> > +/* Calculate the ondisk superblock size in bytes */
> > +STATIC size_t
> > +xchk_superblock_ondisk_size(
> > +	struct xfs_mount	*mp)
> > +{
> > +	if (xfs_has_metadir(mp))
> > +		return offsetofend(struct xfs_dsb, sb_pad);
> > +	if (xfs_has_metauuid(mp))
> > +		return offsetofend(struct xfs_dsb, sb_meta_uuid);
> > +	if (xfs_has_crc(mp))
> > +		return offsetofend(struct xfs_dsb, sb_lsn);
> > +	if (xfs_sb_version_hasmorebits(&mp->m_sb))
> > +		return offsetofend(struct xfs_dsb, sb_bad_features2);
> > +	if (xfs_has_logv2(mp))
> > +		return offsetofend(struct xfs_dsb, sb_logsunit);
> > +	if (xfs_has_sector(mp))
> > +		return offsetofend(struct xfs_dsb, sb_logsectsize);
> > +	/* only support dirv2 or more recent */
> > +	return offsetofend(struct xfs_dsb, sb_dirblklog);
> 
> This really should be libxfs so tht it can be shared with
> secondary_sb_whack in xfsrepair.  The comment at the end of
> the xfs_dsb definition should also be changed to point to this
> libxfs version.

The xfs_repair version of this is subtlely different -- given a
secondary ondisk superblock, it figures out the size of the ondisk
superblock given the features set *in that alleged superblock*.  From
there it validates the secondary superblock.  The featureset in the
alleged superblock doesn't even have to match the primary super, but
it'll go zero things all the same before copying the incore super back
to disk:

	if (xfs_sb_version_hasmetadir(sb))
		size = offsetofend(struct xfs_dsb, sb_pad);
	else if (xfs_sb_version_hasmetauuid(sb))
		size = offsetofend(struct xfs_dsb, sb_meta_uuid);

This version in online computes the size of the secondary ondisk
superblock object given the features set in the *primary* superblock
that we used to mount the filesystem.

Also if I did that we'd have to recopy the xfs_sb_version_hasXXXX
functions back into libxfs after ripping most of them out.  Or we'd have
to encode the logic manually.  But even then, the xfs_repair and
xfs_scrub functions are /not quite/ switching on the same thing.

> > +}
> >  	/* Everything else must be zero. */
> > -	if (memchr_inv(sb + 1, 0,
> > -			BBTOB(bp->b_length) - sizeof(struct xfs_dsb)))
> > +	sblen = xchk_superblock_ondisk_size(mp);
> > +	if (memchr_inv((char *)sb + sblen, 0, BBTOB(bp->b_length) - sblen))
> 
> This could be simplified to
> 
> 	if (memchr_inv(bp->b_addr + sblen, 0, BBTOB(bp->b_length) - sblen))

<nod>

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

Thanks!

--D

Re: [PATCH 1/6] xfs: don't move nondir/nonreg temporary repair files to the metadir namespace

[Darrick J. Wong]

On Wed, Dec 04, 2024 at 12:24:38AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 03, 2024 at 07:02:29PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Only directories or regular files are allowed in the metadata directory
> > tree.  Don't move the repair tempfile to the metadir namespace if this
> > is not true; this will cause the inode verifiers to trip.
> 
> Shouldn't this be an error instead of silently returning?  Either way
> the function could probably use a lot more comments explaining what is
> doing and why.

The function opportunistically moves sc->tempip from the regular
directory tree to the metadata directory tree if sc->ip is part of the
metadata directory tree.  However, the scrub setup functions grab sc->ip
and create sc->tempip before we actually get around to checking if the
file is the right type for the scrubber.

IOWs, you can invoke the symlink scrubber with the file handle of a
subdirectory in the metadir.  xrep_setup_symlink will create a temporary
symlink file, xrep_tempfile_adjust_directory_tree will foolishly try to
set the METADATA flag on the temp symlink, which trips the inode
verifier in the inode item precommit, which shuts down the filesystem
when expensive checks are turned on.  If they're /not/ turned on, then
xchk_symlink will return ENOENT when it sees that it's been passed a
symlink.

I considered modifying xchk_setup_inode_contents to check the mode if
desired and return ENOENT to abort the scrub without calling
_adjust_directory_tree, but it seemed simpler to leave the tempfile code
inside tempfile.c.

<shrug> I'm ok doing it that way too.

--D

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Darrick J. Wong]

On Wed, Dec 04, 2024 at 07:26:54PM -0600, Bill O'Donnell wrote:
> On Tue, Dec 03, 2024 at 07:02:23PM -0800, Darrick J. Wong wrote:
> > Hi all,
> > 
> > Here are even more bugfixes for 6.13 that have been accumulating since
> > 6.12 was released.
> > 
> > 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
> 
> Hi Darrick-
> 
> I must ask, why are these constant bug fixes and fixes for fixes, and
> fixes for fixes for fixes often appearing? It's worrying that xfs is

Roughly speaking, the ~35 bugfixes can be split into three categories:

1) Our vaunted^Wshitty review process didn't catch various coding bugs,
and testing didn't trip over them until I started (ab)using precommit
hooks for spot checking of inode/dquot/buffer log items.

2) Most of the metadir/rtgroups fixes are for things that hch reworked
towards the end of the six years the patchset has been under
development, and that introduced bugs.  Did it make things easier for a
second person to understand?  Yes.

3) The rest are mostly cases of the authors not fully understanding the
subtleties of that which they were constructing (myself included!) and
lucking out that the errors cancelled each other out until someone
started wanting to use that code for a slightly different purpose, which
wouldn't be possible until the bug finally got fixed.

4) The dquot buffer changes have been a known problem since dchinner
decided that RMW cycles in the AIL with inode buffers was having very
bad effects on reclaim performance.  Nobody stepped up to convert dquots
(even though I noted this at the time) so here I am years later because
the mm got pissy at us in 6.12.

5) XFS lit up a lot of new functionality this year, which means the code
is ripe with bugfixing opportunities where cognitive friction comes into
play.

> becoming rather dodgy these days. Do things need to be this
> complicated?

Yeah, they do.  We left behind the kindly old world where people didn't
feed computers fuzzed datafiles and nobody got fired for a computer
crashing periodically.  Nowadays it seems that everything has to be
bulletproofed AND fast. :(

--D

> -Bill
> 
> 
> > 
> > kernel git tree:
> > https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=proposed-fixes-6.13
> > ---
> > Commits in this patchset:
> >  * xfs: don't move nondir/nonreg temporary repair files to the metadir namespace
> >  * xfs: don't crash on corrupt /quotas dirent
> >  * xfs: check pre-metadir fields correctly
> >  * xfs: fix zero byte checking in the superblock scrubber
> >  * xfs: return from xfs_symlink_verify early on V4 filesystems
> >  * xfs: port xfs_ioc_start_commit to multigrain timestamps
> > ---
> >  fs/xfs/libxfs/xfs_symlink_remote.c |    4 ++
> >  fs/xfs/scrub/agheader.c            |   66 ++++++++++++++++++++++++++++--------
> >  fs/xfs/scrub/tempfile.c            |    3 ++
> >  fs/xfs/xfs_exchrange.c             |   14 ++++----
> >  fs/xfs/xfs_qm.c                    |    7 ++++
> >  5 files changed, 71 insertions(+), 23 deletions(-)
> > 
> > 
> 
> 

Re: [PATCH 1/6] xfs: don't move nondir/nonreg temporary repair files to the metadir namespace

[Christoph Hellwig]

On Wed, Dec 04, 2024 at 10:14:50PM -0800, Darrick J. Wong wrote:
> The function opportunistically moves sc->tempip from the regular
> directory tree to the metadata directory tree if sc->ip is part of the
> metadata directory tree.  However, the scrub setup functions grab sc->ip
> and create sc->tempip before we actually get around to checking if the
> file is the right type for the scrubber.
> 
> IOWs, you can invoke the symlink scrubber with the file handle of a
> subdirectory in the metadir.  xrep_setup_symlink will create a temporary
> symlink file, xrep_tempfile_adjust_directory_tree will foolishly try to
> set the METADATA flag on the temp symlink, which trips the inode
> verifier in the inode item precommit, which shuts down the filesystem
> when expensive checks are turned on.  If they're /not/ turned on, then
> xchk_symlink will return ENOENT when it sees that it's been passed a
> symlink.

Maybe just write this down in a big fat comment?

Re: [PATCH 4/6] xfs: fix zero byte checking in the superblock scrubber

[Christoph Hellwig]

On Wed, Dec 04, 2024 at 09:54:51PM -0800, Darrick J. Wong wrote:
> > This really should be libxfs so tht it can be shared with
> > secondary_sb_whack in xfsrepair.  The comment at the end of
> > the xfs_dsb definition should also be changed to point to this
> > libxfs version.
> 
> The xfs_repair version of this is subtlely different -- given a
> secondary ondisk superblock, it figures out the size of the ondisk
> superblock given the features set *in that alleged superblock*.  From
> there it validates the secondary superblock.  The featureset in the
> alleged superblock doesn't even have to match the primary super, but
> it'll go zero things all the same before copying the incore super back
> to disk:
> 
> 	if (xfs_sb_version_hasmetadir(sb))
> 		size = offsetofend(struct xfs_dsb, sb_pad);
> 	else if (xfs_sb_version_hasmetauuid(sb))
> 		size = offsetofend(struct xfs_dsb, sb_meta_uuid);
> 
> This version in online computes the size of the secondary ondisk
> superblock object given the features set in the *primary* superblock
> that we used to mount the filesystem.

Well, it considers the size for the passed in superblock.  Where the
passed in one happens to be the primary one and the usage is for the
second.

> Also if I did that we'd have to recopy the xfs_sb_version_hasXXXX
> functions back into libxfs after ripping most of them out.  Or we'd have
> to encode the logic manually.  But even then, the xfs_repair and
> xfs_scrub functions are /not quite/ switching on the same thing.

We don't really need the helpers and could just check the flag vs
the field directly.

I'd personally prefer to share this code, but I also don't want to
hold off the fix for it.  So if you prefer to stick to this
version maybe just clearly document why these two are different
with a comment that has the above information?

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Bill O'Donnell]

On Wed, Dec 04, 2024 at 10:42:43PM -0800, Darrick J. Wong wrote:
> On Wed, Dec 04, 2024 at 07:26:54PM -0600, Bill O'Donnell wrote:
> > On Tue, Dec 03, 2024 at 07:02:23PM -0800, Darrick J. Wong wrote:
> > > Hi all,
> > > 
> > > Here are even more bugfixes for 6.13 that have been accumulating since
> > > 6.12 was released.
> > > 
> > > 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
> > 
> > Hi Darrick-
> > 
> > I must ask, why are these constant bug fixes and fixes for fixes, and
> > fixes for fixes for fixes often appearing? It's worrying that xfs is
> 
> Roughly speaking, the ~35 bugfixes can be split into three categories:
> 
> 1) Our vaunted^Wshitty review process didn't catch various coding bugs,
> and testing didn't trip over them until I started (ab)using precommit
> hooks for spot checking of inode/dquot/buffer log items.

You give little time for the review process.

> 
> 2) Most of the metadir/rtgroups fixes are for things that hch reworked
> towards the end of the six years the patchset has been under
> development, and that introduced bugs.  Did it make things easier for a
> second person to understand?  Yes.

No.

> 
> 3) The rest are mostly cases of the authors not fully understanding the
> subtleties of that which they were constructing (myself included!) and
> lucking out that the errors cancelled each other out until someone
> started wanting to use that code for a slightly different purpose, which
> wouldn't be possible until the bug finally got fixed.
> 
> 4) The dquot buffer changes have been a known problem since dchinner
> decided that RMW cycles in the AIL with inode buffers was having very
> bad effects on reclaim performance.  Nobody stepped up to convert dquots
> (even though I noted this at the time) so here I am years later because
> the mm got pissy at us in 6.12.
> 
> 5) XFS lit up a lot of new functionality this year, which means the code
> is ripe with bugfixing opportunities where cognitive friction comes into
> play.

I call bullshit. You guys are fast and loose with your patches. Giving
little time for review and soaking.

> 
> > becoming rather dodgy these days. Do things need to be this
> > complicated?
> 
> Yeah, they do.  We left behind the kindly old world where people didn't
> feed computers fuzzed datafiles and nobody got fired for a computer
> crashing periodically.  Nowadays it seems that everything has to be
> bulletproofed AND fast. :(

Cop-out answer.

> 
> --D
> 
> > -Bill
> > 
> > 
> > > 
> > > kernel git tree:
> > > https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=proposed-fixes-6.13
> > > ---
> > > Commits in this patchset:
> > >  * xfs: don't move nondir/nonreg temporary repair files to the metadir namespace
> > >  * xfs: don't crash on corrupt /quotas dirent
> > >  * xfs: check pre-metadir fields correctly
> > >  * xfs: fix zero byte checking in the superblock scrubber
> > >  * xfs: return from xfs_symlink_verify early on V4 filesystems
> > >  * xfs: port xfs_ioc_start_commit to multigrain timestamps
> > > ---
> > >  fs/xfs/libxfs/xfs_symlink_remote.c |    4 ++
> > >  fs/xfs/scrub/agheader.c            |   66 ++++++++++++++++++++++++++++--------
> > >  fs/xfs/scrub/tempfile.c            |    3 ++
> > >  fs/xfs/xfs_exchrange.c             |   14 ++++----
> > >  fs/xfs/xfs_qm.c                    |    7 ++++
> > >  5 files changed, 71 insertions(+), 23 deletions(-)
> > > 
> > > 
> > 
> > 
> 

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Christoph Hellwig]

On Thu, Dec 05, 2024 at 12:52:25AM -0600, Bill O'Donnell wrote:
> > 1) Our vaunted^Wshitty review process didn't catch various coding bugs,
> > and testing didn't trip over them until I started (ab)using precommit
> > hooks for spot checking of inode/dquot/buffer log items.
> 
> You give little time for the review process.

I don't really think that is true.  But if you feel you need more time
please clearly ask for it.  I've done that in the past and most of the
time the relevant people acted on it (not always).

> > 2) Most of the metadir/rtgroups fixes are for things that hch reworked
> > towards the end of the six years the patchset has been under
> > development, and that introduced bugs.  Did it make things easier for a
> > second person to understand?  Yes.
> 
> No.

So you speak for other people here?

> I call bullshit. You guys are fast and loose with your patches. Giving
> little time for review and soaking.

I'm not sure who "you" is, but please say what is going wrong and what
you'd like to do better.

> > > becoming rather dodgy these days. Do things need to be this
> > > complicated?
> > 
> > Yeah, they do.  We left behind the kindly old world where people didn't
> > feed computers fuzzed datafiles and nobody got fired for a computer
> > crashing periodically.  Nowadays it seems that everything has to be
> > bulletproofed AND fast. :(
> 
> Cop-out answer.

What Darrick wrote feels a little snarky, but he has a very valid
point.  A lot of recent bug fixes come from better test coverage, where
better test coverage is mostly two new fuzzers hitting things, or
people using existing code for different things that weren't tested
much before.  And Darrick is single handedly responsible for a large
part of the better test coverage, both due to fuzzing and specific
xfstests.  As someone who's done a fair amount of new development
recently I'm extremely glad about all this extra coverage.

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Bill O'Donnell]

On Wed, Dec 04, 2024 at 10:58:33PM -0800, Christoph Hellwig wrote:
> On Thu, Dec 05, 2024 at 12:52:25AM -0600, Bill O'Donnell wrote:
> > > 1) Our vaunted^Wshitty review process didn't catch various coding bugs,
> > > and testing didn't trip over them until I started (ab)using precommit
> > > hooks for spot checking of inode/dquot/buffer log items.
> > 
> > You give little time for the review process.
> 
> I don't really think that is true.  But if you feel you need more time
> please clearly ask for it.  I've done that in the past and most of the
> time the relevant people acted on it (not always).
> 
> > > 2) Most of the metadir/rtgroups fixes are for things that hch reworked
> > > towards the end of the six years the patchset has been under
> > > development, and that introduced bugs.  Did it make things easier for a
> > > second person to understand?  Yes.
> > 
> > No.
> 
> So you speak for other people here?

No. I speak for myself. A lowly downstream developer.

> 
> > I call bullshit. You guys are fast and loose with your patches. Giving
> > little time for review and soaking.
> 
> I'm not sure who "you" is, but please say what is going wrong and what
> you'd like to do better.

You and Darrick. Can I be much clearer?

> 
> > > > becoming rather dodgy these days. Do things need to be this
> > > > complicated?
> > > 
> > > Yeah, they do.  We left behind the kindly old world where people didn't
> > > feed computers fuzzed datafiles and nobody got fired for a computer
> > > crashing periodically.  Nowadays it seems that everything has to be
> > > bulletproofed AND fast. :(
> > 
> > Cop-out answer.
> 
> What Darrick wrote feels a little snarky, but he has a very valid
> point.  A lot of recent bug fixes come from better test coverage, where
> better test coverage is mostly two new fuzzers hitting things, or
> people using existing code for different things that weren't tested
> much before.  And Darrick is single handedly responsible for a large
> part of the better test coverage, both due to fuzzing and specific
> xfstests.  As someone who's done a fair amount of new development
> recently I'm extremely glad about all this extra coverage.
> 
I think you are killing xfs with your fast and loose patches. Downstreamers
like me are having to clean up the mess you make of things.


> 

Re: [PATCH 1/6] xfs: don't move nondir/nonreg temporary repair files to the metadir namespace

[Darrick J. Wong]

On Wed, Dec 04, 2024 at 10:46:23PM -0800, Christoph Hellwig wrote:
> On Wed, Dec 04, 2024 at 10:14:50PM -0800, Darrick J. Wong wrote:
> > The function opportunistically moves sc->tempip from the regular
> > directory tree to the metadata directory tree if sc->ip is part of the
> > metadata directory tree.  However, the scrub setup functions grab sc->ip
> > and create sc->tempip before we actually get around to checking if the
> > file is the right type for the scrubber.
> > 
> > IOWs, you can invoke the symlink scrubber with the file handle of a
> > subdirectory in the metadir.  xrep_setup_symlink will create a temporary
> > symlink file, xrep_tempfile_adjust_directory_tree will foolishly try to
> > set the METADATA flag on the temp symlink, which trips the inode
> > verifier in the inode item precommit, which shuts down the filesystem
> > when expensive checks are turned on.  If they're /not/ turned on, then
> > xchk_symlink will return ENOENT when it sees that it's been passed a
> > symlink.
> 
> Maybe just write this down in a big fat comment?

Will do.

--D

Re: [PATCH 4/6] xfs: fix zero byte checking in the superblock scrubber

[Darrick J. Wong]

On Wed, Dec 04, 2024 at 10:48:39PM -0800, Christoph Hellwig wrote:
> On Wed, Dec 04, 2024 at 09:54:51PM -0800, Darrick J. Wong wrote:
> > > This really should be libxfs so tht it can be shared with
> > > secondary_sb_whack in xfsrepair.  The comment at the end of
> > > the xfs_dsb definition should also be changed to point to this
> > > libxfs version.
> > 
> > The xfs_repair version of this is subtlely different -- given a
> > secondary ondisk superblock, it figures out the size of the ondisk
> > superblock given the features set *in that alleged superblock*.  From
> > there it validates the secondary superblock.  The featureset in the
> > alleged superblock doesn't even have to match the primary super, but
> > it'll go zero things all the same before copying the incore super back
> > to disk:
> > 
> > 	if (xfs_sb_version_hasmetadir(sb))
> > 		size = offsetofend(struct xfs_dsb, sb_pad);
> > 	else if (xfs_sb_version_hasmetauuid(sb))
> > 		size = offsetofend(struct xfs_dsb, sb_meta_uuid);
> > 
> > This version in online computes the size of the secondary ondisk
> > superblock object given the features set in the *primary* superblock
> > that we used to mount the filesystem.
> 
> Well, it considers the size for the passed in superblock.  Where the
> passed in one happens to be the primary one and the usage is for the
> second.
> 
> > Also if I did that we'd have to recopy the xfs_sb_version_hasXXXX
> > functions back into libxfs after ripping most of them out.  Or we'd have
> > to encode the logic manually.  But even then, the xfs_repair and
> > xfs_scrub functions are /not quite/ switching on the same thing.
> 
> We don't really need the helpers and could just check the flag vs
> the field directly.
> 
> I'd personally prefer to share this code, but I also don't want to
> hold off the fix for it.  So if you prefer to stick to this
> version maybe just clearly document why these two are different
> with a comment that has the above information?

Ok.  I was thinking this hoist is a reasonable cleanup for 6.14 anyway,
not a bugfix to apply to 6.13.

--D

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Bill O'Donnell]

On Thu, Dec 05, 2024 at 01:04:21AM -0600, Bill O'Donnell wrote:
> On Wed, Dec 04, 2024 at 10:58:33PM -0800, Christoph Hellwig wrote:
> > On Thu, Dec 05, 2024 at 12:52:25AM -0600, Bill O'Donnell wrote:
> > > > 1) Our vaunted^Wshitty review process didn't catch various coding bugs,
> > > > and testing didn't trip over them until I started (ab)using precommit
> > > > hooks for spot checking of inode/dquot/buffer log items.
> > > 
> > > You give little time for the review process.
> > 
> > I don't really think that is true.  But if you feel you need more time
> > please clearly ask for it.  I've done that in the past and most of the
> > time the relevant people acted on it (not always).
> > 
> > > > 2) Most of the metadir/rtgroups fixes are for things that hch reworked
> > > > towards the end of the six years the patchset has been under
> > > > development, and that introduced bugs.  Did it make things easier for a
> > > > second person to understand?  Yes.
> > > 
> > > No.
> > 
> > So you speak for other people here?
> 
> No. I speak for myself. A lowly downstream developer.
> 
scrub is the worst offender. What the hell is it, and why do you insist its imortance?

> > 
> > > I call bullshit. You guys are fast and loose with your patches. Giving
> > > little time for review and soaking.
> > 
> > I'm not sure who "you" is, but please say what is going wrong and what
> > you'd like to do better.
> 
> You and Darrick. Can I be much clearer?
> 
> > 
> > > > > becoming rather dodgy these days. Do things need to be this
> > > > > complicated?
> > > > 
> > > > Yeah, they do.  We left behind the kindly old world where people didn't
> > > > feed computers fuzzed datafiles and nobody got fired for a computer
> > > > crashing periodically.  Nowadays it seems that everything has to be
> > > > bulletproofed AND fast. :(
> > > 
> > > Cop-out answer.
> > 
> > What Darrick wrote feels a little snarky, but he has a very valid
> > point.  A lot of recent bug fixes come from better test coverage, where
> > better test coverage is mostly two new fuzzers hitting things, or
> > people using existing code for different things that weren't tested
> > much before.  And Darrick is single handedly responsible for a large
> > part of the better test coverage, both due to fuzzing and specific
> > xfstests.  As someone who's done a fair amount of new development
> > recently I'm extremely glad about all this extra coverage.
> > 
> I think you are killing xfs with your fast and loose patches. Downstreamers
> like me are having to clean up the mess you make of things.
> 
> 
> > 
> 
> 

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Darrick J. Wong]

On Thu, Dec 05, 2024 at 01:04:21AM -0600, Bill O'Donnell wrote:
> On Wed, Dec 04, 2024 at 10:58:33PM -0800, Christoph Hellwig wrote:
> > On Thu, Dec 05, 2024 at 12:52:25AM -0600, Bill O'Donnell wrote:
> > > > 1) Our vaunted^Wshitty review process didn't catch various coding bugs,
> > > > and testing didn't trip over them until I started (ab)using precommit
> > > > hooks for spot checking of inode/dquot/buffer log items.
> > > 
> > > You give little time for the review process.

Seriously?!

Metadir has been out for review in some form or another since January
2019[1].  If five years and eleven months is not sufficient for you to
review a patchset or even to make enough noise that I'm aware that
you're even reading my code, then I don't want you ever to touch any of
my patchsets ever again.

> > I don't really think that is true.  But if you feel you need more time
> > please clearly ask for it.  I've done that in the past and most of the
> > time the relevant people acted on it (not always).
> > 
> > > > 2) Most of the metadir/rtgroups fixes are for things that hch reworked
> > > > towards the end of the six years the patchset has been under
> > > > development, and that introduced bugs.  Did it make things easier for a
> > > > second person to understand?  Yes.
> > > 
> > > No.
> > 
> > So you speak for other people here?
> 
> No. I speak for myself. A lowly downstream developer.
> 
> > 
> > > I call bullshit. You guys are fast and loose with your patches. Giving
> > > little time for review and soaking.
> > 
> > I'm not sure who "you" is, but please say what is going wrong and what
> > you'd like to do better.
> 
> You and Darrick. Can I be much clearer?
> 
> > 
> > > > > becoming rather dodgy these days. Do things need to be this
> > > > > complicated?
> > > > 
> > > > Yeah, they do.  We left behind the kindly old world where people didn't
> > > > feed computers fuzzed datafiles and nobody got fired for a computer
> > > > crashing periodically.  Nowadays it seems that everything has to be
> > > > bulletproofed AND fast. :(
> > > 
> > > Cop-out answer.
> > 
> > What Darrick wrote feels a little snarky, but he has a very valid
> > point.  A lot of recent bug fixes come from better test coverage, where
> > better test coverage is mostly two new fuzzers hitting things, or
> > people using existing code for different things that weren't tested
> > much before.  And Darrick is single handedly responsible for a large
> > part of the better test coverage, both due to fuzzing and specific
> > xfstests.  As someone who's done a fair amount of new development
> > recently I'm extremely glad about all this extra coverage.
> > 
> I think you are killing xfs with your fast and loose patches.

Go work on the maintenance mode filesystems like JFS then.  Shaggy would
probably love it if someone took on some of that.

> Downstreamers like me are having to clean up the mess you make of
> things.

What are you doing downstream these days, exactly?  You don't
participate in the LTS process at all, and your employer boasts about
ignoring that community process.  If your employer chooses to perform
independent forklift upgrades of the XFS codebase in its product every
three months and you don't like that, take it up with them, not
upstream.

--D

[1] https://lore.kernel.org/linux-xfs/154630934595.21716.17416691804044507782.stgit@magnolia/


> 
> > 
> 
> 

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Darrick J. Wong]

On Thu, Dec 05, 2024 at 01:30:42AM -0600, Bill O'Donnell wrote:
> On Thu, Dec 05, 2024 at 01:04:21AM -0600, Bill O'Donnell wrote:
> > On Wed, Dec 04, 2024 at 10:58:33PM -0800, Christoph Hellwig wrote:
> > > On Thu, Dec 05, 2024 at 12:52:25AM -0600, Bill O'Donnell wrote:
> > > > > 1) Our vaunted^Wshitty review process didn't catch various coding bugs,
> > > > > and testing didn't trip over them until I started (ab)using precommit
> > > > > hooks for spot checking of inode/dquot/buffer log items.
> > > > 
> > > > You give little time for the review process.
> > > 
> > > I don't really think that is true.  But if you feel you need more time
> > > please clearly ask for it.  I've done that in the past and most of the
> > > time the relevant people acted on it (not always).
> > > 
> > > > > 2) Most of the metadir/rtgroups fixes are for things that hch reworked
> > > > > towards the end of the six years the patchset has been under
> > > > > development, and that introduced bugs.  Did it make things easier for a
> > > > > second person to understand?  Yes.
> > > > 
> > > > No.
> > > 
> > > So you speak for other people here?
> > 
> > No. I speak for myself. A lowly downstream developer.
> > 
> scrub is the worst offender. What the hell is it, and why do you insist its imortance?

Online fsck, so you can check and repair metadata errors without needing
to incur downtime for xfs_repair.  This is information that was posted
in the design document review that was started in June 2022[1] and
merged in the kernel[2] last year before the code was merged.

[1] https://lore.kernel.org/linux-xfs/165456652256.167418.912764930038710353.stgit@magnolia/
[2] https://docs.kernel.org/filesystems/xfs/xfs-online-fsck-design.html

--D

> > > 
> > > > I call bullshit. You guys are fast and loose with your patches. Giving
> > > > little time for review and soaking.
> > > 
> > > I'm not sure who "you" is, but please say what is going wrong and what
> > > you'd like to do better.
> > 
> > You and Darrick. Can I be much clearer?
> > 
> > > 
> > > > > > becoming rather dodgy these days. Do things need to be this
> > > > > > complicated?
> > > > > 
> > > > > Yeah, they do.  We left behind the kindly old world where people didn't
> > > > > feed computers fuzzed datafiles and nobody got fired for a computer
> > > > > crashing periodically.  Nowadays it seems that everything has to be
> > > > > bulletproofed AND fast. :(
> > > > 
> > > > Cop-out answer.
> > > 
> > > What Darrick wrote feels a little snarky, but he has a very valid
> > > point.  A lot of recent bug fixes come from better test coverage, where
> > > better test coverage is mostly two new fuzzers hitting things, or
> > > people using existing code for different things that weren't tested
> > > much before.  And Darrick is single handedly responsible for a large
> > > part of the better test coverage, both due to fuzzing and specific
> > > xfstests.  As someone who's done a fair amount of new development
> > > recently I'm extremely glad about all this extra coverage.
> > > 
> > I think you are killing xfs with your fast and loose patches. Downstreamers
> > like me are having to clean up the mess you make of things.
> > 
> > 
> > > 
> > 
> > 
> 
> 

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Bill O'Donnell]

On Wed, Dec 04, 2024 at 11:33:21PM -0800, Darrick J. Wong wrote:
> On Thu, Dec 05, 2024 at 01:04:21AM -0600, Bill O'Donnell wrote:
> > On Wed, Dec 04, 2024 at 10:58:33PM -0800, Christoph Hellwig wrote:
> > > On Thu, Dec 05, 2024 at 12:52:25AM -0600, Bill O'Donnell wrote:
> > > > > 1) Our vaunted^Wshitty review process didn't catch various coding bugs,
> > > > > and testing didn't trip over them until I started (ab)using precommit
> > > > > hooks for spot checking of inode/dquot/buffer log items.
> > > > 
> > > > You give little time for the review process.
> 
> Seriously?!
> 
> Metadir has been out for review in some form or another since January
> 2019[1].  If five years and eleven months is not sufficient for you to
> review a patchset or even to make enough noise that I'm aware that
> you're even reading my code, then I don't want you ever to touch any of
> my patchsets ever again.
> 
> > > I don't really think that is true.  But if you feel you need more time
> > > please clearly ask for it.  I've done that in the past and most of the
> > > time the relevant people acted on it (not always).
> > > 
> > > > > 2) Most of the metadir/rtgroups fixes are for things that hch reworked
> > > > > towards the end of the six years the patchset has been under
> > > > > development, and that introduced bugs.  Did it make things easier for a
> > > > > second person to understand?  Yes.
> > > > 
> > > > No.
> > > 
> > > So you speak for other people here?
> > 
> > No. I speak for myself. A lowly downstream developer.
> > 
> > > 
> > > > I call bullshit. You guys are fast and loose with your patches. Giving
> > > > little time for review and soaking.
> > > 
> > > I'm not sure who "you" is, but please say what is going wrong and what
> > > you'd like to do better.
> > 
> > You and Darrick. Can I be much clearer?
> > 
> > > 
> > > > > > becoming rather dodgy these days. Do things need to be this
> > > > > > complicated?
> > > > > 
> > > > > Yeah, they do.  We left behind the kindly old world where people didn't
> > > > > feed computers fuzzed datafiles and nobody got fired for a computer
> > > > > crashing periodically.  Nowadays it seems that everything has to be
> > > > > bulletproofed AND fast. :(
> > > > 
> > > > Cop-out answer.
> > > 
> > > What Darrick wrote feels a little snarky, but he has a very valid
> > > point.  A lot of recent bug fixes come from better test coverage, where
> > > better test coverage is mostly two new fuzzers hitting things, or
> > > people using existing code for different things that weren't tested
> > > much before.  And Darrick is single handedly responsible for a large
> > > part of the better test coverage, both due to fuzzing and specific
> > > xfstests.  As someone who's done a fair amount of new development
> > > recently I'm extremely glad about all this extra coverage.
> > > 
> > I think you are killing xfs with your fast and loose patches.
> 
> Go work on the maintenance mode filesystems like JFS then.  Shaggy would
> probably love it if someone took on some of that.
> 
> > Downstreamers like me are having to clean up the mess you make of
> > things.
> 
> What are you doing downstream these days, exactly?  You don't
> participate in the LTS process at all, and your employer boasts about
> ignoring that community process.  If your employer chooses to perform
> independent forklift upgrades of the XFS codebase in its product every
> three months and you don't like that, take it up with them, not
> upstream.

Thanks for your reply. You win.

> 
> --D
> 
> [1] https://lore.kernel.org/linux-xfs/154630934595.21716.17416691804044507782.stgit@magnolia/
> 
> 
> > 
> > > 
> > 
> > 
> 

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Bill O'Donnell]

On Wed, Dec 04, 2024 at 11:33:21PM -0800, Darrick J. Wong wrote:
> On Thu, Dec 05, 2024 at 01:04:21AM -0600, Bill O'Donnell wrote:
> > On Wed, Dec 04, 2024 at 10:58:33PM -0800, Christoph Hellwig wrote:
> > > On Thu, Dec 05, 2024 at 12:52:25AM -0600, Bill O'Donnell wrote:
> > > > > 1) Our vaunted^Wshitty review process didn't catch various coding bugs,
> > > > > and testing didn't trip over them until I started (ab)using precommit
> > > > > hooks for spot checking of inode/dquot/buffer log items.
> > > > 
> > > > You give little time for the review process.
> 
> Seriously?!
> 
> Metadir has been out for review in some form or another since January
> 2019[1].  If five years and eleven months is not sufficient for you to
> review a patchset or even to make enough noise that I'm aware that
> you're even reading my code, then I don't want you ever to touch any of
> my patchsets ever again.
> 
> > > I don't really think that is true.  But if you feel you need more time
> > > please clearly ask for it.  I've done that in the past and most of the
> > > time the relevant people acted on it (not always).
> > > 
> > > > > 2) Most of the metadir/rtgroups fixes are for things that hch reworked
> > > > > towards the end of the six years the patchset has been under
> > > > > development, and that introduced bugs.  Did it make things easier for a
> > > > > second person to understand?  Yes.
> > > > 
> > > > No.
> > > 
> > > So you speak for other people here?
> > 
> > No. I speak for myself. A lowly downstream developer.
> > 
> > > 
> > > > I call bullshit. You guys are fast and loose with your patches. Giving
> > > > little time for review and soaking.
> > > 
> > > I'm not sure who "you" is, but please say what is going wrong and what
> > > you'd like to do better.
> > 
> > You and Darrick. Can I be much clearer?
> > 
> > > 
> > > > > > becoming rather dodgy these days. Do things need to be this
> > > > > > complicated?
> > > > > 
> > > > > Yeah, they do.  We left behind the kindly old world where people didn't
> > > > > feed computers fuzzed datafiles and nobody got fired for a computer
> > > > > crashing periodically.  Nowadays it seems that everything has to be
> > > > > bulletproofed AND fast. :(
> > > > 
> > > > Cop-out answer.
> > > 
> > > What Darrick wrote feels a little snarky, but he has a very valid
> > > point.  A lot of recent bug fixes come from better test coverage, where
> > > better test coverage is mostly two new fuzzers hitting things, or
> > > people using existing code for different things that weren't tested
> > > much before.  And Darrick is single handedly responsible for a large
> > > part of the better test coverage, both due to fuzzing and specific
> > > xfstests.  As someone who's done a fair amount of new development
> > > recently I'm extremely glad about all this extra coverage.
> > > 
> > I think you are killing xfs with your fast and loose patches.
> 
> Go work on the maintenance mode filesystems like JFS then.  Shaggy would
> probably love it if someone took on some of that.

No idea who "Shaggy" is. Nor do I care.	   
> 
> > Downstreamers like me are having to clean up the mess you make of
> > things.
> 
> What are you doing downstream these days, exactly?  You don't
> participate in the LTS process at all, and your employer boasts about
> ignoring that community process.  If your employer chooses to perform
> independent forklift upgrades of the XFS codebase in its product every
> three months and you don't like that, take it up with them, not
> upstream.
> 
> --D
> 
> [1] https://lore.kernel.org/linux-xfs/154630934595.21716.17416691804044507782.stgit@magnolia/
> 
> 
> > 
> > > 
> > 
> > 
> 

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Darrick J. Wong]

On Wed, Dec 04, 2024 at 10:58:33PM -0800, Christoph Hellwig wrote:

<cut out all the hostility>

> What Darrick wrote feels a little snarky, but he has a very valid
> point.  A lot of recent bug fixes come from better test coverage, where
> better test coverage is mostly two new fuzzers hitting things, or
> people using existing code for different things that weren't tested
> much before.  And Darrick is single handedly responsible for a large
> part of the better test coverage, both due to fuzzing and specific
> xfstests.  As someone who's done a fair amount of new development
> recently I'm extremely glad about all this extra coverage.

Aww, thank you! :)

--D

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Bill O'Donnell]

On Thu, Dec 05, 2024 at 01:46:58AM -0600, Bill O'Donnell wrote:
> On Wed, Dec 04, 2024 at 11:33:21PM -0800, Darrick J. Wong wrote:
> > On Thu, Dec 05, 2024 at 01:04:21AM -0600, Bill O'Donnell wrote:
> > > On Wed, Dec 04, 2024 at 10:58:33PM -0800, Christoph Hellwig wrote:
> > > > On Thu, Dec 05, 2024 at 12:52:25AM -0600, Bill O'Donnell wrote:
> > > > > > 1) Our vaunted^Wshitty review process didn't catch various coding bugs,
> > > > > > and testing didn't trip over them until I started (ab)using precommit
> > > > > > hooks for spot checking of inode/dquot/buffer log items.
> > > > > 
> > > > > You give little time for the review process.
> > 
> > Seriously?!
> > 
> > Metadir has been out for review in some form or another since January
> > 2019[1].  If five years and eleven months is not sufficient for you to
> > review a patchset or even to make enough noise that I'm aware that
> > you're even reading my code, then I don't want you ever to touch any of
> > my patchsets ever again.
> > 
> > > > I don't really think that is true.  But if you feel you need more time
> > > > please clearly ask for it.  I've done that in the past and most of the
> > > > time the relevant people acted on it (not always).
> > > > 
> > > > > > 2) Most of the metadir/rtgroups fixes are for things that hch reworked
> > > > > > towards the end of the six years the patchset has been under
> > > > > > development, and that introduced bugs.  Did it make things easier for a
> > > > > > second person to understand?  Yes.
> > > > > 
> > > > > No.
> > > > 
> > > > So you speak for other people here?
> > > 
> > > No. I speak for myself. A lowly downstream developer.
> > > 
> > > > 
> > > > > I call bullshit. You guys are fast and loose with your patches. Giving
> > > > > little time for review and soaking.
> > > > 
> > > > I'm not sure who "you" is, but please say what is going wrong and what
> > > > you'd like to do better.
> > > 
> > > You and Darrick. Can I be much clearer?
> > > 
> > > > 
> > > > > > > becoming rather dodgy these days. Do things need to be this
> > > > > > > complicated?
> > > > > > 
> > > > > > Yeah, they do.  We left behind the kindly old world where people didn't
> > > > > > feed computers fuzzed datafiles and nobody got fired for a computer
> > > > > > crashing periodically.  Nowadays it seems that everything has to be
> > > > > > bulletproofed AND fast. :(
> > > > > 
> > > > > Cop-out answer.
> > > > 
> > > > What Darrick wrote feels a little snarky, but he has a very valid
> > > > point.  A lot of recent bug fixes come from better test coverage, where
> > > > better test coverage is mostly two new fuzzers hitting things, or
> > > > people using existing code for different things that weren't tested
> > > > much before.  And Darrick is single handedly responsible for a large
> > > > part of the better test coverage, both due to fuzzing and specific
> > > > xfstests.  As someone who's done a fair amount of new development
> > > > recently I'm extremely glad about all this extra coverage.
> > > > 
> > > I think you are killing xfs with your fast and loose patches.
> > 
> > Go work on the maintenance mode filesystems like JFS then.  Shaggy would
> > probably love it if someone took on some of that.
> 
> No idea who "Shaggy" is. Nor do I care.	   
> > 
> > > Downstreamers like me are having to clean up the mess you make of
> > > things.
> > 
> > What are you doing downstream these days, exactly?  You don't
> > participate in the LTS process at all, and your employer boasts about
> > ignoring that community process.  If your employer chooses to perform
> > independent forklift upgrades of the XFS codebase in its product every
> > three months and you don't like that, take it up with them, not
> > upstream.

Why are you such a nasty person? I try to get along with people, but you're
impossible. I've been an engineer for 40+ years, and I've never encountered such
an arrogant one as you.


> > 
> > --D
> > 
> > [1] https://lore.kernel.org/linux-xfs/154630934595.21716.17416691804044507782.stgit@magnolia/
> > 
> > 
> > > 
> > > > 
> > > 
> > > 
> > 
> 
> 

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Greg KH]

On Thu, Dec 05, 2024 at 02:02:00AM -0600, Bill O'Donnell wrote:
> On Thu, Dec 05, 2024 at 01:46:58AM -0600, Bill O'Donnell wrote:
> > On Wed, Dec 04, 2024 at 11:33:21PM -0800, Darrick J. Wong wrote:
> > > On Thu, Dec 05, 2024 at 01:04:21AM -0600, Bill O'Donnell wrote:
> > > > On Wed, Dec 04, 2024 at 10:58:33PM -0800, Christoph Hellwig wrote:
> > > > > On Thu, Dec 05, 2024 at 12:52:25AM -0600, Bill O'Donnell wrote:
> > > > > > > 1) Our vaunted^Wshitty review process didn't catch various coding bugs,
> > > > > > > and testing didn't trip over them until I started (ab)using precommit
> > > > > > > hooks for spot checking of inode/dquot/buffer log items.
> > > > > > 
> > > > > > You give little time for the review process.
> > > 
> > > Seriously?!
> > > 
> > > Metadir has been out for review in some form or another since January
> > > 2019[1].  If five years and eleven months is not sufficient for you to
> > > review a patchset or even to make enough noise that I'm aware that
> > > you're even reading my code, then I don't want you ever to touch any of
> > > my patchsets ever again.
> > > 
> > > > > I don't really think that is true.  But if you feel you need more time
> > > > > please clearly ask for it.  I've done that in the past and most of the
> > > > > time the relevant people acted on it (not always).
> > > > > 
> > > > > > > 2) Most of the metadir/rtgroups fixes are for things that hch reworked
> > > > > > > towards the end of the six years the patchset has been under
> > > > > > > development, and that introduced bugs.  Did it make things easier for a
> > > > > > > second person to understand?  Yes.
> > > > > > 
> > > > > > No.
> > > > > 
> > > > > So you speak for other people here?
> > > > 
> > > > No. I speak for myself. A lowly downstream developer.
> > > > 
> > > > > 
> > > > > > I call bullshit. You guys are fast and loose with your patches. Giving
> > > > > > little time for review and soaking.
> > > > > 
> > > > > I'm not sure who "you" is, but please say what is going wrong and what
> > > > > you'd like to do better.
> > > > 
> > > > You and Darrick. Can I be much clearer?
> > > > 
> > > > > 
> > > > > > > > becoming rather dodgy these days. Do things need to be this
> > > > > > > > complicated?
> > > > > > > 
> > > > > > > Yeah, they do.  We left behind the kindly old world where people didn't
> > > > > > > feed computers fuzzed datafiles and nobody got fired for a computer
> > > > > > > crashing periodically.  Nowadays it seems that everything has to be
> > > > > > > bulletproofed AND fast. :(
> > > > > > 
> > > > > > Cop-out answer.
> > > > > 
> > > > > What Darrick wrote feels a little snarky, but he has a very valid
> > > > > point.  A lot of recent bug fixes come from better test coverage, where
> > > > > better test coverage is mostly two new fuzzers hitting things, or
> > > > > people using existing code for different things that weren't tested
> > > > > much before.  And Darrick is single handedly responsible for a large
> > > > > part of the better test coverage, both due to fuzzing and specific
> > > > > xfstests.  As someone who's done a fair amount of new development
> > > > > recently I'm extremely glad about all this extra coverage.
> > > > > 
> > > > I think you are killing xfs with your fast and loose patches.
> > > 
> > > Go work on the maintenance mode filesystems like JFS then.  Shaggy would
> > > probably love it if someone took on some of that.
> > 
> > No idea who "Shaggy" is. Nor do I care.	   
> > > 
> > > > Downstreamers like me are having to clean up the mess you make of
> > > > things.
> > > 
> > > What are you doing downstream these days, exactly?  You don't
> > > participate in the LTS process at all, and your employer boasts about
> > > ignoring that community process.  If your employer chooses to perform
> > > independent forklift upgrades of the XFS codebase in its product every
> > > three months and you don't like that, take it up with them, not
> > > upstream.
> 
> Why are you such a nasty person? I try to get along with people, but you're
> impossible. I've been an engineer for 40+ years, and I've never encountered such
> an arrogant one as you.

I have to step in here, sorry.

Please take a beat and relax and maybe get some sleep before you respond
again.  Darrick is not being "nasty" here at all, but reiterating the
fact that your company does do huge fork-lifts of code into their kernel
tree.  If that development model doesn't work for you, please work with
your company to change it.

And if you wish to help out here, please do so by reviewing and even
better yet, testing, the proposed changes.  If you can't just suck down
a patch series and put it into your test framework with a few
keystrokes, perhaps that needs to be worked on to make it simpler to do
from your side (i.e. that's what most of us do here with our development
systems.)

By critisizing the mere posting of bugfixes, you aren't helping anything
out at all, sorry.  Bugfixes are good, I don't know why you don't want
even more, that means that people are testing and finding issues to fix!
Surely you don't want the people finding the issues to be your users,
right?

thanks,

greg k-h

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Bill O'Donnell]

On Thu, Dec 05, 2024 at 09:39:42AM +0100, Greg KH wrote:
> On Thu, Dec 05, 2024 at 02:02:00AM -0600, Bill O'Donnell wrote:
> > On Thu, Dec 05, 2024 at 01:46:58AM -0600, Bill O'Donnell wrote:
> > > On Wed, Dec 04, 2024 at 11:33:21PM -0800, Darrick J. Wong wrote:
> > > > On Thu, Dec 05, 2024 at 01:04:21AM -0600, Bill O'Donnell wrote:
> > > > > On Wed, Dec 04, 2024 at 10:58:33PM -0800, Christoph Hellwig wrote:
> > > > > > On Thu, Dec 05, 2024 at 12:52:25AM -0600, Bill O'Donnell wrote:
> > > > > > > > 1) Our vaunted^Wshitty review process didn't catch various coding bugs,
> > > > > > > > and testing didn't trip over them until I started (ab)using precommit
> > > > > > > > hooks for spot checking of inode/dquot/buffer log items.
> > > > > > > 
> > > > > > > You give little time for the review process.
> > > > 
> > > > Seriously?!
> > > > 
> > > > Metadir has been out for review in some form or another since January
> > > > 2019[1].  If five years and eleven months is not sufficient for you to
> > > > review a patchset or even to make enough noise that I'm aware that
> > > > you're even reading my code, then I don't want you ever to touch any of
> > > > my patchsets ever again.
> > > > 
> > > > > > I don't really think that is true.  But if you feel you need more time
> > > > > > please clearly ask for it.  I've done that in the past and most of the
> > > > > > time the relevant people acted on it (not always).
> > > > > > 
> > > > > > > > 2) Most of the metadir/rtgroups fixes are for things that hch reworked
> > > > > > > > towards the end of the six years the patchset has been under
> > > > > > > > development, and that introduced bugs.  Did it make things easier for a
> > > > > > > > second person to understand?  Yes.
> > > > > > > 
> > > > > > > No.
> > > > > > 
> > > > > > So you speak for other people here?
> > > > > 
> > > > > No. I speak for myself. A lowly downstream developer.
> > > > > 
> > > > > > 
> > > > > > > I call bullshit. You guys are fast and loose with your patches. Giving
> > > > > > > little time for review and soaking.
> > > > > > 
> > > > > > I'm not sure who "you" is, but please say what is going wrong and what
> > > > > > you'd like to do better.
> > > > > 
> > > > > You and Darrick. Can I be much clearer?
> > > > > 
> > > > > > 
> > > > > > > > > becoming rather dodgy these days. Do things need to be this
> > > > > > > > > complicated?
> > > > > > > > 
> > > > > > > > Yeah, they do.  We left behind the kindly old world where people didn't
> > > > > > > > feed computers fuzzed datafiles and nobody got fired for a computer
> > > > > > > > crashing periodically.  Nowadays it seems that everything has to be
> > > > > > > > bulletproofed AND fast. :(
> > > > > > > 
> > > > > > > Cop-out answer.
> > > > > > 
> > > > > > What Darrick wrote feels a little snarky, but he has a very valid
> > > > > > point.  A lot of recent bug fixes come from better test coverage, where
> > > > > > better test coverage is mostly two new fuzzers hitting things, or
> > > > > > people using existing code for different things that weren't tested
> > > > > > much before.  And Darrick is single handedly responsible for a large
> > > > > > part of the better test coverage, both due to fuzzing and specific
> > > > > > xfstests.  As someone who's done a fair amount of new development
> > > > > > recently I'm extremely glad about all this extra coverage.
> > > > > > 
> > > > > I think you are killing xfs with your fast and loose patches.
> > > > 
> > > > Go work on the maintenance mode filesystems like JFS then.  Shaggy would
> > > > probably love it if someone took on some of that.
> > > 
> > > No idea who "Shaggy" is. Nor do I care.	   
> > > > 
> > > > > Downstreamers like me are having to clean up the mess you make of
> > > > > things.
> > > > 
> > > > What are you doing downstream these days, exactly?  You don't
> > > > participate in the LTS process at all, and your employer boasts about
> > > > ignoring that community process.  If your employer chooses to perform
> > > > independent forklift upgrades of the XFS codebase in its product every
> > > > three months and you don't like that, take it up with them, not
> > > > upstream.
> > 
> > Why are you such a nasty person? I try to get along with people, but you're
> > impossible. I've been an engineer for 40+ years, and I've never encountered such
> > an arrogant one as you.
> 
> I have to step in here, sorry.
> 
> Please take a beat and relax and maybe get some sleep before you respond
> again.  Darrick is not being "nasty" here at all, but reiterating the
> fact that your company does do huge fork-lifts of code into their kernel
> tree.  If that development model doesn't work for you, please work with
> your company to change it.
> 
> And if you wish to help out here, please do so by reviewing and even
> better yet, testing, the proposed changes.  If you can't just suck down
> a patch series and put it into your test framework with a few
> keystrokes, perhaps that needs to be worked on to make it simpler to do
> from your side (i.e. that's what most of us do here with our development
> systems.)
> 
> By critisizing the mere posting of bugfixes, you aren't helping anything
> out at all, sorry.  Bugfixes are good, I don't know why you don't want
> even more, that means that people are testing and finding issues to fix!
> Surely you don't want the people finding the issues to be your users,
> right?
> 
> thanks,

Thank you for putting this in a better perspective.
-Bill

> 
> greg k-h
> 

Re: [PATCHSET v2] xfs: proposed bug fixes for 6.13

[Bill O'Donnell]

On Wed, Dec 04, 2024 at 10:42:43PM -0800, Darrick J. Wong wrote:
> On Wed, Dec 04, 2024 at 07:26:54PM -0600, Bill O'Donnell wrote:
> > On Tue, Dec 03, 2024 at 07:02:23PM -0800, Darrick J. Wong wrote:
> > > Hi all,
> > > 
> > > Here are even more bugfixes for 6.13 that have been accumulating since
> > > 6.12 was released.
> > > 
> > > 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
> > 
> > Hi Darrick-
> > 
> > I must ask, why are these constant bug fixes and fixes for fixes, and
> > fixes for fixes for fixes often appearing? It's worrying that xfs is
> 
> Roughly speaking, the ~35 bugfixes can be split into three categories:
> 
> 1) Our vaunted^Wshitty review process didn't catch various coding bugs,
> and testing didn't trip over them until I started (ab)using precommit
> hooks for spot checking of inode/dquot/buffer log items.
> 
> 2) Most of the metadir/rtgroups fixes are for things that hch reworked
> towards the end of the six years the patchset has been under
> development, and that introduced bugs.  Did it make things easier for a
> second person to understand?  Yes.
> 
> 3) The rest are mostly cases of the authors not fully understanding the
> subtleties of that which they were constructing (myself included!) and
> lucking out that the errors cancelled each other out until someone
> started wanting to use that code for a slightly different purpose, which
> wouldn't be possible until the bug finally got fixed.
> 
> 4) The dquot buffer changes have been a known problem since dchinner
> decided that RMW cycles in the AIL with inode buffers was having very
> bad effects on reclaim performance.  Nobody stepped up to convert dquots
> (even though I noted this at the time) so here I am years later because
> the mm got pissy at us in 6.12.
> 
> 5) XFS lit up a lot of new functionality this year, which means the code
> is ripe with bugfixing opportunities where cognitive friction comes into
> play.
> 
> > becoming rather dodgy these days. Do things need to be this
> > complicated?
> 
> Yeah, they do.  We left behind the kindly old world where people didn't
> feed computers fuzzed datafiles and nobody got fired for a computer
> crashing periodically.  Nowadays it seems that everything has to be
> bulletproofed AND fast. :(
> 

My apologies to you, Darrick and to all on this thread. I'm in over my head
and barely treading water.

Thanks-
Bill



> --D
> 
> > -Bill
> > 
> > 
> > > 
> > > kernel git tree:
> > > https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=proposed-fixes-6.13
> > > ---
> > > Commits in this patchset:
> > >  * xfs: don't move nondir/nonreg temporary repair files to the metadir namespace
> > >  * xfs: don't crash on corrupt /quotas dirent
> > >  * xfs: check pre-metadir fields correctly
> > >  * xfs: fix zero byte checking in the superblock scrubber
> > >  * xfs: return from xfs_symlink_verify early on V4 filesystems
> > >  * xfs: port xfs_ioc_start_commit to multigrain timestamps
> > > ---
> > >  fs/xfs/libxfs/xfs_symlink_remote.c |    4 ++
> > >  fs/xfs/scrub/agheader.c            |   66 ++++++++++++++++++++++++++++--------
> > >  fs/xfs/scrub/tempfile.c            |    3 ++
> > >  fs/xfs/xfs_exchrange.c             |   14 ++++----
> > >  fs/xfs/xfs_qm.c                    |    7 ++++
> > >  5 files changed, 71 insertions(+), 23 deletions(-)
> > > 
> > > 
> > 
> > 
> 

[PATCH 1/6] xfs: don't move nondir/nonreg temporary repair files to the metadir namespace

[Darrick J. Wong]

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

Only directories or regular files are allowed in the metadata directory
tree.  Don't move the repair tempfile to the metadir namespace if this
is not true; this will cause the inode verifiers to trip.

xrep_tempfile_adjust_directory_tree opportunistically moves sc->tempip
from the regular directory tree to the metadata directory tree if sc->ip
is part of the metadata directory tree.  However, the scrub setup
functions grab sc->ip and create sc->tempip before we actually get
around to checking if the file mode is the right type for the scrubber.

IOWs, you can invoke the symlink scrubber with the file handle of a
subdirectory in the metadir.  xrep_setup_symlink will create a temporary
symlink file, xrep_tempfile_adjust_directory_tree will foolishly try to
set the METADATA flag on the temp symlink, which trips the inode
verifier in the inode item precommit, which shuts down the filesystem
when expensive checks are turned on.  If they're /not/ turned on, then
xchk_symlink will return ENOENT when it sees that it's been passed a
symlink, but the invalid inode could still get flushed to disk.  We
don't want that.

Cc: <stable@vger.kernel.org> # v6.13-rc1
Fixes: 9dc31acb01a1c7 ("xfs: move repair temporary files to the metadata directory tree")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/scrub/tempfile.c |    3 +++
 1 file changed, 3 insertions(+)


diff --git a/fs/xfs/scrub/tempfile.c b/fs/xfs/scrub/tempfile.c
index dc3802c7f678ce..82ecbb654fbb39 100644
--- a/fs/xfs/scrub/tempfile.c
+++ b/fs/xfs/scrub/tempfile.c
@@ -204,6 +204,9 @@ xrep_tempfile_adjust_directory_tree(
 
 	if (!sc->ip || !xfs_is_metadir_inode(sc->ip))
 		return 0;
+	if (!S_ISDIR(VFS_I(sc->tempip)->i_mode) &&
+	    !S_ISREG(VFS_I(sc->tempip)->i_mode))
+		return 0;
 
 	xfs_ilock(sc->tempip, XFS_IOLOCK_EXCL);
 	sc->temp_ilock_flags |= XFS_IOLOCK_EXCL;

[PATCH 1/6] xfs: don't move nondir/nonreg temporary repair files to the metadir namespace

[Darrick J. Wong]

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

Only directories or regular files are allowed in the metadata directory
tree.  Don't move the repair tempfile to the metadir namespace if this
is not true; this will cause the inode verifiers to trip.

xrep_tempfile_adjust_directory_tree opportunistically moves sc->tempip
from the regular directory tree to the metadata directory tree if sc->ip
is part of the metadata directory tree.  However, the scrub setup
functions grab sc->ip and create sc->tempip before we actually get
around to checking if the file mode is the right type for the scrubber.

IOWs, you can invoke the symlink scrubber with the file handle of a
subdirectory in the metadir.  xrep_setup_symlink will create a temporary
symlink file, xrep_tempfile_adjust_directory_tree will foolishly try to
set the METADATA flag on the temp symlink, which trips the inode
verifier in the inode item precommit, which shuts down the filesystem
when expensive checks are turned on.  If they're /not/ turned on, then
xchk_symlink will return ENOENT when it sees that it's been passed a
symlink, but the invalid inode could still get flushed to disk.  We
don't want that.

Cc: <stable@vger.kernel.org> # v6.13-rc1
Fixes: 9dc31acb01a1c7 ("xfs: move repair temporary files to the metadata directory tree")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/scrub/tempfile.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/scrub/tempfile.c b/fs/xfs/scrub/tempfile.c
index dc3802c7f678ce..2d7ca7e1bbca0f 100644
--- a/fs/xfs/scrub/tempfile.c
+++ b/fs/xfs/scrub/tempfile.c
@@ -184,11 +184,18 @@ xrep_tempfile_create(
 }
 
 /*
+ * Move sc->tempip from the regular directory tree to the metadata directory
+ * tree if sc->ip is part of the metadata directory tree and tempip has an
+ * eligible file mode.
+ *
  * Temporary files have to be created before we even know which inode we're
  * going to scrub, so we assume that they will be part of the regular directory
  * tree.  If it turns out that we're actually scrubbing a file from the
  * metadata directory tree, we have to subtract the temp file from the root
- * dquots and detach the dquots.
+ * dquots and detach the dquots prior to setting the METADATA iflag.  However,
+ * the scrub setup functions grab sc->ip and create sc->tempip before we
+ * actually get around to checking if the file mode is the right type for the
+ * scrubber.
  */
 int
 xrep_tempfile_adjust_directory_tree(
@@ -204,6 +211,9 @@ xrep_tempfile_adjust_directory_tree(
 
 	if (!sc->ip || !xfs_is_metadir_inode(sc->ip))
 		return 0;
+	if (!S_ISDIR(VFS_I(sc->tempip)->i_mode) &&
+	    !S_ISREG(VFS_I(sc->tempip)->i_mode))
+		return 0;
 
 	xfs_ilock(sc->tempip, XFS_IOLOCK_EXCL);
 	sc->temp_ilock_flags |= XFS_IOLOCK_EXCL;