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
>
