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@xxxxxxxxxxxxx
