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=pro... --- 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(-)
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;
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.
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
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?
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
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) {
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
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. */
Looks good:
Reviewed-by: Christoph Hellwig hch@lst.de
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);
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
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
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?
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
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))
Looks good:
Reviewed-by: Christoph Hellwig hch@lst.de
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;
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
Looks good:
Reviewed-by: Christoph Hellwig hch@lst.de
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=pro...
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(-)
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=pro...
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(-)
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:
- 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.
- 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.
- 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.
- 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.
- 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=pro...
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(-)
On Thu, Dec 05, 2024 at 12:52:25AM -0600, Bill O'Donnell wrote:
- 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).
- 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.
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:
- 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).
- 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.
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:
- 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).
- 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.
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:
- 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).
- 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.stg... [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.
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:
- 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).
- 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.st...
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:
- 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).
- 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.st...
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:
- 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).
- 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.st...
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:
- 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).
- 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.st...
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
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
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
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:
- 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.
- 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.
- 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.
- 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.
- 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=pro...
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(-)
linux-stable-mirror@lists.linaro.org
-
Bill O'Donnell -
Christoph Hellwig -
Darrick J. Wong -
Greg KH -
Jeff Layton