Re: [PATCH v3 25/26] xfs: fix unit conversion error in xfs_log_calc_max_attrsetm_res

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

On Wed, Sep 21, 2022 at 10:44:57PM -0700, allison.henderson@oracle.com wrote:
> From: Allison Henderson <allison.henderson@oracle.com>

Er, did you change this patch much?

I was really hoping you'd *RVB* tag it and send it back out. :)

--D

> 
> Dave and I were discussing some recent test regressions as a result of
> me turning on nrext64=1 on realtime filesystems, when we noticed that
> the minimum log size of a 32M filesystem jumped from 954 blocks to 4287
> blocks.
> 
> Digging through xfs_log_calc_max_attrsetm_res, Dave noticed that @size
> contains the maximum estimated amount of space needed for a local format
> xattr, in bytes, but we feed this quantity to XFS_NEXTENTADD_SPACE_RES,
> which requires units of blocks.  This has resulted in an overestimation
> of the minimum log size over the years.
> 
> We should nominally correct this, but there's a backwards compatibility
> problem -- if we enable it now, the minimum log size will decrease.  If
> a corrected mkfs formats a filesystem with this new smaller log size, a
> user will encounter mount failures on an uncorrected kernel due to the
> larger minimum log size computations there.
> 
> However, the large extent counters feature is still EXPERIMENTAL, so we
> can gate the correction on that feature (or any features that get added
> after that) being enabled.  Any filesystem with nrext64 or any of the
> as-yet-undefined feature bits turned on will be rejected by old
> uncorrected kernels, so this should be safe even in the upgrade case.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_log_rlimit.c | 43 ++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
> index 9975b93a7412..e5c606fb7a6a 100644
> --- a/fs/xfs/libxfs/xfs_log_rlimit.c
> +++ b/fs/xfs/libxfs/xfs_log_rlimit.c
> @@ -16,6 +16,39 @@
>  #include "xfs_bmap_btree.h"
>  #include "xfs_trace.h"
>  
> +/*
> + * Decide if the filesystem has the parent pointer feature or any feature
> + * added after that.
> + */
> +static inline bool
> +xfs_has_parent_or_newer_feature(
> +	struct xfs_mount	*mp)
> +{
> +	if (!xfs_sb_is_v5(&mp->m_sb))
> +		return false;
> +
> +	if (xfs_sb_has_compat_feature(&mp->m_sb, ~0))
> +		return true;
> +
> +	if (xfs_sb_has_ro_compat_feature(&mp->m_sb,
> +				~(XFS_SB_FEAT_RO_COMPAT_FINOBT |
> +				 XFS_SB_FEAT_RO_COMPAT_RMAPBT |
> +				 XFS_SB_FEAT_RO_COMPAT_REFLINK |
> +				 XFS_SB_FEAT_RO_COMPAT_INOBTCNT)))
> +		return true;
> +
> +	if (xfs_sb_has_incompat_feature(&mp->m_sb,
> +				~(XFS_SB_FEAT_INCOMPAT_FTYPE |
> +				 XFS_SB_FEAT_INCOMPAT_SPINODES |
> +				 XFS_SB_FEAT_INCOMPAT_META_UUID |
> +				 XFS_SB_FEAT_INCOMPAT_BIGTIME |
> +				 XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR |
> +				 XFS_SB_FEAT_INCOMPAT_NREXT64)))
> +		return true;
> +
> +	return false;
> +}
> +
>  /*
>   * Calculate the maximum length in bytes that would be required for a local
>   * attribute value as large attributes out of line are not logged.
> @@ -31,6 +64,16 @@ xfs_log_calc_max_attrsetm_res(
>  	       MAXNAMELEN - 1;
>  	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
>  	nblks += XFS_B_TO_FSB(mp, size);
> +
> +	/*
> +	 * Starting with the parent pointer feature, every new fs feature
> +	 * corrects a unit conversion error in the xattr transaction
> +	 * reservation code that resulted in oversized minimum log size
> +	 * computations.
> +	 */
> +	if (xfs_has_parent_or_newer_feature(mp))
> +		size = XFS_B_TO_FSB(mp, size);
> +
>  	nblks += XFS_NEXTENTADD_SPACE_RES(mp, size, XFS_ATTR_FORK);
>  
>  	return  M_RES(mp)->tr_attrsetm.tr_logres +
> -- 
> 2.25.1
>