Re: [PATCH v6 13/13] xfs: update atomic write max size

[John Garry]

On 17/03/2025 07:25, Christoph Hellwig wrote:
> On Thu, Mar 13, 2025 at 05:13:10PM +0000, John Garry wrote:
> > For simplicity, limit at the max of what the mounted bdev can support in
> > terms of atomic write limits. Maybe in future we will have a better way
> > to advertise this optimised limit.
>
> You'll still need to cover limit this by the amount that can
> be commited in a single transactions. 

yeah ... I'll revisit that

> And handle the case where there
> is no hardware support at all.

So xfs_get_atomic_write_max_attr() -> xfs_inode_can_atomicwrite() covers 
no HW support.

The point of this function is just to calc atomic write limits according 
to mount point geometry and features.

Do you think that it is necessary to call xfs_inode_can_atomicwrite() 
here also? [And remove the xfs_get_atomic_write_max_attr() -> 
xfs_inode_can_atomicwrite()?]

> >   xfs_get_atomic_write_max_attr(
>
> I missed it in the previous version, but can be drop the
> pointless _attr for these two helpers?

ok, fine

> > +static inline void
> > +xfs_compute_awu_max(
>
> And use a more descriptive name than AWU, wich really just is a
> nvme field name.

I am just trying to be concise to limit spilling lines.

Maybe atomicwrite_unit_max is preferred

> > +	awu_max = 1;
> > +	while (1) {
> > +		if (agsize % (awu_max * 2))
> > +			break;
>
> 	while ((agsize % (awu_max * 2) == 0)) {
>
> ?
>
> > +	xfs_extlen_t		m_awu_max;	/* data device max atomic write */
>
> overly long line.

there are a few overly long lines here (so following that example), but 
since there is a request to change the name, I'll be definitely using a 
newline for the comment

Thanks,
John

>