Re: [PATCH v6 10/13] xfs: iomap COW-based atomic write support

[John Garry]

On 20/03/2025 05:29, Christoph Hellwig wrote:
> On Wed, Mar 19, 2025 at 10:24:55AM +0000, John Garry wrote:
> > it seems to work ok, cheers
>
> Better test it very well, this was really just intended as a sketch..

Sure, I have been testing a lot so far.

I had been using fio in verify mode as a method to check racing threads 
reading and atomically writing the same file range, so I need to ensure 
that it covers the various paths in this function.

> > > +	count_fsb = end_fsb - offset_fsb;
> > > +	resaligned = xfs_aligned_fsb_count(offset_fsb, count_fsb,
> > > +			xfs_get_cowextsz_hint(ip));
> > > +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > > +
> > > +	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write,
> > > +			XFS_DIOSTRAT_SPACE_RES(mp, resaligned), 0, false, &tp);
> > >    	if (error)
> > >    		return error;
> > >    -	error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, &imap,
> > > -			&nimaps, 0);
> > > -	if (error)
> > > -		goto out_unlock;
> > > +	if (!xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &icur, &cmap))
> > > +		cmap.br_startoff = end_fsb;
> >
> > Do we really need this logic?
> >
> > offset_fsb does not change, and logically cmap.br_startoff == end_fsb
> > already, right?
>
> Afte unlocking and relocking the ilock the extent layout could have
> changed.

ok, understood. Maybe a comment will help understanding that.

>