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

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Sep 28, 2026 at 07:30:24AM +1000, Dave Chinner wrote:
> 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.

Wellp, I'll let you and your LLM toy figure out the correct fix for
this, then.

--D