Re: [PATCH v7.1 14/14] xfs: allow sysadmins to specify a maximum atomic write limit at mount time

[Darrick J. Wong]

On Wed, Apr 16, 2025 at 11:08:25AM +0100, John Garry wrote:
> On 15/04/2025 23:36, Darrick J. Wong wrote:
> 
> Thanks for this, but it still seems to be problematic for me.
> 
> In my test, I have agsize=22400, and when I attempt to mount with
> atomic_write_max=8M, it passes when it shouldn't. It should not because
> max_pow_of_two_factor(22400) = 128, and 8MB > 128 FSB.
> 
> How about these addition checks:
> 
> > +
> > +	if (new_max_bytes) {
> > +		xfs_extlen_t	max_write_fsbs =
> > +			rounddown_pow_of_two(XFS_B_TO_FSB(mp, MAX_RW_COUNT));
> > +		xfs_extlen_t	max_group_fsbs =
> > +			max(mp->m_groups[XG_TYPE_AG].blocks,
> > +			    mp->m_groups[XG_TYPE_RTG].blocks);
> > +
> > +		ASSERT(max_write_fsbs <= U32_MAX);
> 
> 		if (!is_power_of_2(new_max_bytes)) {
> 			xfs_warn(mp,
>  "max atomic write size of %llu bytes is not a power-of-2",
> 					new_max_bytes);
> 			return -EINVAL;
> 		}

Long-term I'm not convinced that we really need to have all these power
of two checks because the software fallback can remap just about
anything, but for now I see no harm in doing this because
generic_atomic_write_valid enforces that property on the IO length.

> > +
> > +		if (new_max_bytes % mp->m_sb.sb_blocksize > 0) {
> > +			xfs_warn(mp,
> > + "max atomic write size of %llu bytes not aligned with fsblock",
> > +					new_max_bytes);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (new_max_fsbs > max_write_fsbs) {
> > +			xfs_warn(mp,
> > + "max atomic write size of %lluk cannot be larger than max write size %lluk",
> > +					new_max_bytes >> 10,
> > +					XFS_FSB_TO_B(mp, max_write_fsbs) >> 10);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (new_max_fsbs > max_group_fsbs) {
> > +			xfs_warn(mp,
> > + "max atomic write size of %lluk cannot be larger than allocation group size %lluk",
> > +					new_max_bytes >> 10,
> > +					XFS_FSB_TO_B(mp, max_group_fsbs) >> 10);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> 
> 	if (new_max_fsbs > max_pow_of_two_factor(max_group_fsbs)) {
> 		xfs_warn(mp,
>  "max atomic write size of %lluk not aligned with allocation group size
> %lluk",
> 				new_max_bytes >> 10,
> 				XFS_FSB_TO_B(mp, max_group_fsbs) >> 10);
> 		return -EINVAL;

I think I'd rather clean up these bits:

	if (mp->m_ddev_targp->bt_bdev_awu_min > 0)
		max_agsize = max_pow_of_two_factor(mp->m_sb.sb_agblocks);
	else
		max_agsize = mp->m_ag_max_usable;

and

	if (mp->m_rtdev_targp && mp->m_rtdev_targp->bt_bdev_awu_min > 0)
		max_rgsize = max_pow_of_two_factor(rgs->blocks);
	else
		max_rgsize = rgs->blocks;

into a shared helper for xfs_compute_atomic_write_unit_max so that we
use the exact same logic in both places.  But I agree with the general
direction.

--D

> 	}
> 
> thanks,
> John
>