Re: [PATCH 2/2] xfs: validate extsz hints against rt extent size when rtinherit is set

[Christoph Hellwig <hch@infradead.org>]

On Thu, May 20, 2021 at 09:42:27AM -0700, Darrick J. Wong wrote:
> 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.