| Message ID | 20240402221127.1200501-4-david@fromorbit.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xfs: fixes for 6.9-rcX | expand |
On Wed, Apr 03, 2024 at 08:38:18AM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > If free space accounting is screwed up, then dquot allocation may go > ahead when there is no space available. xfs_dquot_disk_alloc() does > not handle allocation failure - it expects that it will not get > called when there isn't space available to allocate dquots. > > Because fuzzers have been screwing up the free space accounting, we > are seeing failures in dquot allocation, and they aren't being > caught on produciton kernels. Debug kernels will assert fail in this > case, so turn that assert fail into more robust error handling to > avoid these issues in future. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> Sounds fine to me! It'll be interesting to see what happens the next time one of my VMs trips this. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/xfs_dquot.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > index c98cb468c357..a2652e3d5164 100644 > --- a/fs/xfs/xfs_dquot.c > +++ b/fs/xfs/xfs_dquot.c > @@ -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. > + * > + * We commit the transaction instead of cancelling it as it may > + * be dirty due to extent count upgrade. This avoids a potential > + * filesystem shutdown when this happens. We ignore any error > + * from the transaction commit - we always return -ENOSPC to the > + * caller here so we really don't care if the commit fails for > + * some unknown reason... > + */ > + xfs_trans_commit(tp); > + return -ENOSPC; > + } > + > ASSERT(map.br_blockcount == XFS_DQUOT_CLUSTER_SIZE_FSB); > ASSERT(nmaps == 1); > ASSERT((map.br_startblock != DELAYSTARTBLOCK) && > -- > 2.43.0 > >
> + 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. > + * > + * We commit the transaction instead of cancelling it as it may > + * be dirty due to extent count upgrade. This avoids a potential > + * filesystem shutdown when this happens. We ignore any error > + * from the transaction commit - we always return -ENOSPC to the > + * caller here so we really don't care if the commit fails for > + * some unknown reason... > + */ > + xfs_trans_commit(tp); > + return -ENOSPC; A cancel and thus shutdown does seem like the right behavior for a trap for an unknown bug..
On Tue, Apr 02, 2024 at 09:41:56PM -0700, Christoph Hellwig wrote: > > + 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. > > + * > > + * We commit the transaction instead of cancelling it as it may > > + * be dirty due to extent count upgrade. This avoids a potential > > + * filesystem shutdown when this happens. We ignore any error > > + * from the transaction commit - we always return -ENOSPC to the > > + * caller here so we really don't care if the commit fails for > > + * some unknown reason... > > + */ > > + xfs_trans_commit(tp); > > + return -ENOSPC; > > A cancel and thus shutdown does seem like the right behavior for a trap > for an unknown bug.. Usually this will result in the file write erroring out, right? --D
On Tue, Apr 02, 2024 at 09:54:56PM -0700, Darrick J. Wong wrote:
> Usually this will result in the file write erroring out, right?
quota file allocations usually come originally from a file write or
inode creation. But I'm not entirely sure if that was the question..
On Tue, Apr 02, 2024 at 09:56:24PM -0700, Christoph Hellwig wrote: > On Tue, Apr 02, 2024 at 09:54:56PM -0700, Darrick J. Wong wrote: > > Usually this will result in the file write erroring out, right? > > quota file allocations usually come originally from a file write or > inode creation. But I'm not entirely sure if that was the question.. Heh, and the question was based on a misreading of your comment. 8-) AFAICT this can result in dqattach erroring out, which seems mostly benign. --D
On Tue, Apr 02, 2024 at 10:04:30PM -0700, Darrick J. Wong wrote: > On Tue, Apr 02, 2024 at 09:56:24PM -0700, Christoph Hellwig wrote: > > On Tue, Apr 02, 2024 at 09:54:56PM -0700, Darrick J. Wong wrote: > > > Usually this will result in the file write erroring out, right? > > > > quota file allocations usually come originally from a file write or > > inode creation. But I'm not entirely sure if that was the question.. > > Heh, and the question was based on a misreading of your comment. 8-) > > AFAICT this can result in dqattach erroring out, which seems mostly > benign. Right - this propagates the ENOSPC error back to the caller without a shutdown being required. If we get a corruption detected, then the allocation will return an error, not nmaps == 0. That error will cause a corruption. But an unexpected allocation failure right at ENOSPC can occur without there being corruption because of, say, one of the many, many near ENOSPC accounting bugs we've had to fix over the past 20 years, and if the allocation fails we should just clean up and return -ENOSPC without shutting down the filesystem. We're right at ENOSPC, so there's every chance that the next operation after the dquot was attached would fail with ENOSPC anyway.... So, yeah, I don't see any reason to shut the filesytsem down because we ended up with a transient ENOSPC error or an off-by one in the free space accounting somewhere... -Dave.
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. In other words: an ENOSPC is entirely expected here in the current form. 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.
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.
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c index c98cb468c357..a2652e3d5164 100644 --- a/fs/xfs/xfs_dquot.c +++ b/fs/xfs/xfs_dquot.c @@ -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. + * + * We commit the transaction instead of cancelling it as it may + * be dirty due to extent count upgrade. This avoids a potential + * filesystem shutdown when this happens. We ignore any error + * from the transaction commit - we always return -ENOSPC to the + * caller here so we really don't care if the commit fails for + * some unknown reason... + */ + xfs_trans_commit(tp); + return -ENOSPC; + } + ASSERT(map.br_blockcount == XFS_DQUOT_CLUSTER_SIZE_FSB); ASSERT(nmaps == 1); ASSERT((map.br_startblock != DELAYSTARTBLOCK) &&