Re: [PATCH 2/2] xfs: validate extsz hints against rt extent size when rtinherit is set
From: Christoph Hellwig <hch@infradead.org>
Date: 2021-05-21 07:49:48
On Thu, May 20, 2021 at 09:42:27AM -0700, Darrick J. Wong wrote:
quoted hunk ↗ jump to hunk
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c index 045118c7bf78..dce267dbea5f 100644 --- a/fs/xfs/libxfs/xfs_inode_buf.c +++ b/fs/xfs/libxfs/xfs_inode_buf.c@@ -589,8 +589,21 @@ xfs_inode_validate_extsize( inherit_flag = (flags & XFS_DIFLAG_EXTSZINHERIT); extsize_bytes = XFS_FSB_TO_B(mp, extsize); + /* + * Historically, XFS didn't check that the extent size hint was an + * integer multiple of the rt extent size on a directory with both + * rtinherit and extszinherit flags set. This results in math errors + * in the rt allocator and inode verifier errors when the unaligned + * hint value propagates into new realtime files. Since there might + * be filesystems in the wild, the best we can do for now is to + * mitigate the harms by stopping the propagation. + * + * The next time we add a new inode feature, the if test below should + * also trigger if that new feature is enabled and (rtinherit_flag && + * inherit_flag). + */
I don't understand how this comment relates to the change below, and in fact I don't understand the last paragraph at all.
if (rt_flag) - blocksize_bytes = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog; + blocksize_bytes = XFS_FSB_TO_B(mp, mp->m_sb.sb_rextsize);
This just looks like a cleanup, that is replace the open coded version of XFS_FSB_TO_B with the actual helper.
+ /* + * Clear invalid extent size hints set on files with rtinherit and + * extszinherit set. See the comments in xfs_inode_validate_extsize + * for details. + */
Maybe that comment should be here as it makes a whole lot more sense where we do the actual clearing.
+ if ((ip->i_diflags & XFS_DIFLAG_RTINHERIT) &&
+ (ip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) &&
+ (ip->i_extsize % ip->i_mount->m_sb.sb_rextsize) > 0) {
+ ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE | XFS_DIFLAG_EXTSZINHERIT);Overly long line.
+ xfs_failaddr_t failaddr;
umode_t mode = VFS_I(ip)->i_mode;
if (S_ISDIR(mode)) {
if (pip->i_diflags & XFS_DIFLAG_RTINHERIT)
- di_flags |= XFS_DIFLAG_RTINHERIT;
+ ip->i_diflags |= XFS_DIFLAG_RTINHERIT;The removal of the di_flags seems like an unrelated (though nice) cleanup.