Re: [bug report] fstests generic/774 hang again

[Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

On Sep 22, 2026 / 08:05, Dave Chinner wrote:
> On Sat, Sep 19, 2026 at 08:53:39PM +0900, Shin'ichiro Kawasaki wrote:
[...]
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index 426a67b813a..8d10ae22884 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -829,7 +829,7 @@ xfs_file_dio_write_atomic(
> >  	struct kiocb		*iocb,
> >  	struct iov_iter		*from)
> >  {
> > -	unsigned int		iolock = XFS_IOLOCK_SHARED;
> > +	unsigned int		iolock;
> >  	ssize_t			ret, ocount = iov_iter_count(from);
> >  	unsigned int		dio_flags = 0;
> >  	const struct iomap_ops	*dops;
> > @@ -844,6 +844,17 @@ xfs_file_dio_write_atomic(
> >  		dops = &xfs_direct_write_iomap_ops;
> >  
> >  retry:
> > +	/*
> > +	 * Concurrent atomic write COW submissions under ILOCK can run out
> > +	 * journal reservation resource and can deadlock with the IO
> > +	 * completions. To avoid the deadlock, serialize submissions of
> > +	 * atomic write COW by taking IOLOCK exclusively.
> > +	 */
> > +	if (dops == &xfs_atomic_write_cow_iomap_ops)
> > +		iolock = XFS_IOLOCK_EXCL;
> > +	else
> > +		iolock = XFS_IOLOCK_SHARED;
> > +
> >  	ret = xfs_ilock_iocb_for_write(iocb, &iolock);
> >  	if (ret)
> >  		return ret;
> 
> Urk, that's pretty nasty.
> 
> > @@ -853,7 +864,8 @@ xfs_file_dio_write_atomic(
> >  		goto out_unlock;
> >  
> >  	/* Demote similar to xfs_file_dio_write_aligned() */
> > -	if (iolock == XFS_IOLOCK_EXCL) {
> > +	if (iolock == XFS_IOLOCK_EXCL &&
> > +	    dops != &xfs_atomic_write_cow_iomap_ops) {
> >  		xfs_ilock_demote(ip, XFS_IOLOCK_EXCL);
> >  		iolock = XFS_IOLOCK_SHARED;
> >  	}
> 
> And at this point, we now have several atomic write cow ops specific
> operations in this function (locking, the retry loop, etc).
> 
> This feels much more like there should be a separate function for
> the software cow path, and the fast path simply calls it on
> ENOPROTOOPT from the dio submission. That gets rid of all the
> conditionals and looping from the fast path. i.e
> 
> 	if (ocount > xfs_inode_buftarg(ip)->bt_awu_maxocount)
> 		return xfs_file_dio_write_atomic_cow();
> 
> 	/* do normal DIO write */
> 
> 	if (error == -ENOPROTOOPT)
> 		return xfs_file_dio_write_atomic_cow();
> 	return error;
> 
> 
> That essentially makes xfs_file_dio_write_atomic() and
> xfs_file_dio_write_aligned() the same code, except for the above two
> checks, hence they could easily be collapsed back into a common
> implementation is:
> 
> 	if ((iocb->ki_flags & IOCB_ATOMIC) &&
> 	    ocount > xfs_inode_buftarg(ip)->bt_awu_maxocount)
> 		return xfs_file_dio_write_atomic_cow();
> 
> 	/* do normal DIO write */
> 
> 	if (error == -ENOPROTOOPT)
> 		return xfs_file_dio_write_atomic_cow();
> 	return error;
> 
> That seems like a much more natural breakdown that the current
> duplication of the DIO write submission code...

Thanks for the comment. I'll try to factor out the duplication based
on your idea.

On the other hand, Darrick suggested antoher solution approach to allocate
enough space before taking ILOCK by increasing tr_logcount. I wonder which
way is the better: "allocate enough space" or "take exclusive IOLOCK". I'll
do some experiment for the "allocate enough space" approach before working
on the dupliaction clean up.