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

[Dave Chinner <dgc@kernel.org>]

On Fri, Sep 25, 2026 at 10:32:44AM +0900, Shin'ichiro Kawasaki wrote:
> 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.

No, that doesn't fix anything. tr_logcount is an optimisation to
minimise the number of blocking regrants a -typical- rolling
transaction will take. It trades off an increase in initial write
grant reservation space (i.e. they use more log space) to enable
more transaction rolls without needing to refresh the write grant
for the next operation in the transaction.

IOWs, it will reduce the number of concurrent transactions that can
be running at any given time, but if the deferops intent processing
can still run out of pre-reservation space and block waiting for a
write grant.

Hence increasing tr_logcount just kicks the can down
the road, making it slightly harder to trigger the deadlock at the
cost of lowering modification concurrency in the filesystem.


> 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.

"allocate enough space" if not a fix - it's a bandaid. "take
exclusive IOLOCK" reflects the fact that COW operations are
inherently single threaded due to their reliance on holding the
ILOCK_EXCL for long periods of time on both IO submission and IO
completion.

Cheers,

Dave.
-- 
Dave Chinner
dgc@kernel.org