Re: [PATCH 3/4] xfs: handle allocation failure in xfs_dquot_disk_alloc()

[Dave Chinner <david@fromorbit.com>]

On Wed, Apr 03, 2024 at 07:06:48AM -0700, Christoph Hellwig wrote:
> On Wed, Apr 03, 2024 at 08:38:18AM +1100, Dave Chinner wrote:
> > @@ -356,6 +356,23 @@ xfs_dquot_disk_alloc(
> >  	if (error)
> >  		goto err_cancel;
> >  
> > +	if (nmaps == 0) {
> > +		/*
> > +		 * Unexpected ENOSPC - the transaction reservation should have
> > +		 * guaranteed that this allocation will succeed. We don't know
> > +		 * why this happened, so just back out gracefully.
> 
> So looking at this code, xfs_dquot_disk_alloc allocates it's own
> transaction, and does so without a space reservation. 

It does have a valid space reservation:

	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_dqalloc,
                        XFS_QM_DQALLOC_SPACE_RES(mp), 0, 0, &tp);

The space reservation is XFS_QM_DQALLOC_SPACE_RES(mp):

#define XFS_QM_DQALLOC_SPACE_RES(mp)    \
        (XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK) + \
         XFS_DQUOT_CLUSTER_SIZE_FSB)

which is correct for allocating a single XFS_DQUOT_CLUSTER_SIZE_FSB
sized extent.

> In other words:
> an ENOSPC is entirely expected here in the current form.

I disagree with that assessment. :) 

> The code, just 
> like many other callers of xfs_bmapi_write, just fails to handle
> the weird 0 return value and zero nmaps convention properly.

Yes, that's exactly what this patch is fixing regardless of the
cause of the failure. It's the right thing to do - error handling by
assumption (i.e. ASSERT()) is simply poor code....

-Dave.
-- 
Dave Chinner
david@fromorbit.com