Re: [PATCH 1/2] xfs: rearrange code in xfs_inode_item_precommit

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Sep 18, 2025 at 08:12:53AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> There are similar extsize checks and updates done inside and outside
> the inode item lock, which could all be done under a single top
> level logic branch outside the ili_lock. The COW extsize fixup can
> potentially miss updating the XFS_ILOG_CORE in ili_fsync_fields, so
> moving this code up above the ili_fsync_fields update could also be
> considered a fix.
> 
> Further, to make the next change a bit cleaner, move where we
> calculate the on-disk flag mask to after we attach the cluster
> buffer to the the inode log item.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_inode_item.c | 65 ++++++++++++++++++-----------------------
>  1 file changed, 29 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index afb6cadf7793..318e7c68ec72 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -131,46 +131,28 @@ xfs_inode_item_precommit(
>  	}
>  
>  	/*
> -	 * Inode verifiers do not check that the extent size hint is an integer
> -	 * multiple of the rt extent size on a directory with both rtinherit
> -	 * and extszinherit flags set.  If we're logging a directory that is
> -	 * misconfigured in this way, clear the hint.
> +	 * Inode verifiers do not check that the extent size hints are an
> +	 * integer multiple of the rt extent size on a directory with
> +	 * rtinherit flags set.  If we're logging a directory that is
> +	 * misconfigured in this way, clear the bad hints.
>  	 */
> -	if ((ip->i_diflags & XFS_DIFLAG_RTINHERIT) &&
> -	    (ip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) &&
> -	    xfs_extlen_to_rtxmod(ip->i_mount, ip->i_extsize) > 0) {
> -		ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE |
> -				   XFS_DIFLAG_EXTSZINHERIT);
> -		ip->i_extsize = 0;
> -		flags |= XFS_ILOG_CORE;
> +	if (ip->i_diflags & XFS_DIFLAG_RTINHERIT) {
> +		if ((ip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) &&
> +		    xfs_extlen_to_rtxmod(ip->i_mount, ip->i_extsize) > 0) {
> +			ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE |
> +					   XFS_DIFLAG_EXTSZINHERIT);
> +			ip->i_extsize = 0;
> +			flags |= XFS_ILOG_CORE;
> +		}
> +		if ((ip->i_diflags2 & XFS_DIFLAG2_COWEXTSIZE) &&
> +		    xfs_extlen_to_rtxmod(ip->i_mount, ip->i_cowextsize) > 0) {
> +			ip->i_diflags2 &= ~XFS_DIFLAG2_COWEXTSIZE;
> +			ip->i_cowextsize = 0;
> +			flags |= XFS_ILOG_CORE;
> +		}
>  	}

Hrm, yeah, that cowextsize fixing code looks like it was merged in the
wrong place or something.  In theory it's a bug fix, but since we never
merged support for rtreflink and rtextsize > 1fsb this will never happen
in practice since rtextsize cannot change when reflink is enabled.

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D


>  
> -	/*
> -	 * Record the specific change for fdatasync optimisation. This allows
> -	 * fdatasync to skip log forces for inodes that are only timestamp
> -	 * dirty. Once we've processed the XFS_ILOG_IVERSION flag, convert it
> -	 * to XFS_ILOG_CORE so that the actual on-disk dirty tracking
> -	 * (ili_fields) correctly tracks that the version has changed.
> -	 */
>  	spin_lock(&iip->ili_lock);
> -	iip->ili_fsync_fields |= (flags & ~XFS_ILOG_IVERSION);
> -	if (flags & XFS_ILOG_IVERSION)
> -		flags = ((flags & ~XFS_ILOG_IVERSION) | XFS_ILOG_CORE);
> -
> -	/*
> -	 * Inode verifiers do not check that the CoW extent size hint is an
> -	 * integer multiple of the rt extent size on a directory with both
> -	 * rtinherit and cowextsize flags set.  If we're logging a directory
> -	 * that is misconfigured in this way, clear the hint.
> -	 */
> -	if ((ip->i_diflags & XFS_DIFLAG_RTINHERIT) &&
> -	    (ip->i_diflags2 & XFS_DIFLAG2_COWEXTSIZE) &&
> -	    xfs_extlen_to_rtxmod(ip->i_mount, ip->i_cowextsize) > 0) {
> -		ip->i_diflags2 &= ~XFS_DIFLAG2_COWEXTSIZE;
> -		ip->i_cowextsize = 0;
> -		flags |= XFS_ILOG_CORE;
> -	}
> -
>  	if (!iip->ili_item.li_buf) {
>  		struct xfs_buf	*bp;
>  		int		error;
> @@ -204,6 +186,17 @@ xfs_inode_item_precommit(
>  		xfs_trans_brelse(tp, bp);
>  	}
>  
> +	/*
> +	 * Record the specific change for fdatasync optimisation. This allows
> +	 * fdatasync to skip log forces for inodes that are only timestamp
> +	 * dirty. Once we've processed the XFS_ILOG_IVERSION flag, convert it
> +	 * to XFS_ILOG_CORE so that the actual on-disk dirty tracking
> +	 * (ili_fields) correctly tracks that the version has changed.
> +	 */
> +	iip->ili_fsync_fields |= (flags & ~XFS_ILOG_IVERSION);
> +	if (flags & XFS_ILOG_IVERSION)
> +		flags = ((flags & ~XFS_ILOG_IVERSION) | XFS_ILOG_CORE);
> +
>  	/*
>  	 * Always OR in the bits from the ili_last_fields field.  This is to
>  	 * coordinate with the xfs_iflush() and xfs_buf_inode_iodone() routines
> -- 
> 2.50.1
> 
>