| Message ID | e6b9090b-722a-c9d1-6c82-0dcb3f0be5a2@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFCRAP] xfs: handle ENOSPC quota return in xfs_file_buffered_aio_write | expand |
On Thu, May 14, 2020 at 04:43:36PM -0500, Eric Sandeen wrote: > + if (ret == -EDQUOT || > + (ret == -ENOSPC && ip->i_pdquot && > + XFS_IS_PQUOTA_ENFORCED(mp) && ip->i_pdquot)) { > + xfs_iunlock(ip, iolock); > + enospc |= xfs_inode_free_quota_eofblocks(ip); > + enospc |= xfs_inode_free_quota_cowblocks(ip); > + iolock = 0; Fun fact of the day: xfs_inode_free_quota_eofblocks and xfs_inode_free_quota_cowblocks don't actually do anything for project quotas. I've started to prepare a little cleanup series to help implementing what you want. Give me some time to do a quick test run and send it out.
On Thu, May 14, 2020 at 04:43:36PM -0500, Eric Sandeen wrote: > Since project quota returns ENOSPC rather than EDQUOT, the distinction > between those two error conditions in xfs_file_buffered_aio_write() is > incomplete. If project quota is on, and we get ENOSPC, it seems that > we have no good way to know if it was due to quota, or due to actual > out of space conditions, and we may need to run both remediation options... > > This is completely untested and not even built, because I'm really not sure > what the best way to go here is. True ENOSPC is hopefully rare, pquota > ENOSPC is probably less so, so I'm not sure how far we should go digging > to figure out what the root cause of the ENOSPC condition is, when pquota > is on (on this inode). > > If project quota is on on this inode and pquota enforced, should we look > to the super to make a determination about low free blocks in the fs? > > Should we crack open the dquot and see if it's over limit? > > Should we just run both conditions and hope for the best? > > Is this all best effort anyway, so we just simply care if we take the > improper action for pquota+ENOSPC? > > Probably-shouldn't-merge-this-sez: Eric Sandeen <sandeen@redhat.com> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index 4b8bdecc3863..8cec826046ce 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -647,27 +647,31 @@ xfs_file_buffered_aio_write( > * waits on dirty mappings. Since xfs_flush_inodes() is serialized, this > * also behaves as a filter to prevent too many eofblocks scans from > * running at the same time. > + * > + * Fun fact: Project quota returns -ENOSPC, so... complexity here. > */ > - if (ret == -EDQUOT && !enospc) { > - xfs_iunlock(ip, iolock); > - enospc = xfs_inode_free_quota_eofblocks(ip); > - if (enospc) > - goto write_retry; > - enospc = xfs_inode_free_quota_cowblocks(ip); > + if (!enospc) { > + if (ret == -ENOSPC) { > + struct xfs_eofblocks eofb = {0}; > + > + enospc = 1; > + xfs_flush_inodes(ip->i_mount); > + > + xfs_iunlock(ip, iolock); > + eofb.eof_flags = XFS_EOF_FLAGS_SYNC; > + xfs_icache_free_eofblocks(ip->i_mount, &eofb); > + xfs_icache_free_cowblocks(ip->i_mount, &eofb); > + } > + if (ret == -EDQUOT || > + (ret == -ENOSPC && ip->i_pdquot && > + XFS_IS_PQUOTA_ENFORCED(mp) && ip->i_pdquot)) { > + xfs_iunlock(ip, iolock); > + enospc |= xfs_inode_free_quota_eofblocks(ip); > + enospc |= xfs_inode_free_quota_cowblocks(ip); > + iolock = 0; > + } Christoph's comment aside, note that the quota helpers here are filtered scans based on the dquots attached to the inode. It's basically an optimized scan when we know the failure was due to quota, so I don't think there should ever be a need to run a quota scan after running the -ENOSPC handling above. For project quota, it might make more sense to check if a pdquot is attached, check xfs_dquot_lowsp() and conditionally update the eofb to do a filtered pquota scan if appropriate (since calling the quota helper above would also affect other dquots attached to the inode, which I don't think we want to do). Then we can fall back to the global scan if the pquota optimization is not relevant or still returns -ENOSPC on the subsequent retry. Brian > if (enospc) > goto write_retry; > - iolock = 0; > - } else if (ret == -ENOSPC && !enospc) { > - struct xfs_eofblocks eofb = {0}; > - > - enospc = 1; > - xfs_flush_inodes(ip->i_mount); > - > - xfs_iunlock(ip, iolock); > - eofb.eof_flags = XFS_EOF_FLAGS_SYNC; > - xfs_icache_free_eofblocks(ip->i_mount, &eofb); > - xfs_icache_free_cowblocks(ip->i_mount, &eofb); > - goto write_retry; > } > > current->backing_dev_info = NULL; >
On Mon, May 18, 2020 at 08:34:54AM -0400, Brian Foster wrote: > Christoph's comment aside, note that the quota helpers here are filtered > scans based on the dquots attached to the inode. It's basically an > optimized scan when we know the failure was due to quota, so I don't > think there should ever be a need to run a quota scan after running the > -ENOSPC handling above. For project quota, it might make more sense to > check if a pdquot is attached, check xfs_dquot_lowsp() and conditionally > update the eofb to do a filtered pquota scan if appropriate (since > calling the quota helper above would also affect other dquots attached > to the inode, which I don't think we want to do). Then we can fall back > to the global scan if the pquota optimization is not relevant or still > returns -ENOSPC on the subsequent retry. That's what I've implemented. But it turns out -ENOSPC can of course still mean a real -ENOSPC even with project quotas attached. So back to the drawing board - I think I basically need to replace the enospc with a tristate saying what kind of scan we've tried. Or we just ignore the issue and keep the current global scan after a potential project quota -ENOSPC, because all that cruft isn't worth it after all.
On Mon, May 18, 2020 at 10:01:12AM -0700, Christoph Hellwig wrote: > On Mon, May 18, 2020 at 08:34:54AM -0400, Brian Foster wrote: > > Christoph's comment aside, note that the quota helpers here are filtered > > scans based on the dquots attached to the inode. It's basically an > > optimized scan when we know the failure was due to quota, so I don't > > think there should ever be a need to run a quota scan after running the > > -ENOSPC handling above. For project quota, it might make more sense to > > check if a pdquot is attached, check xfs_dquot_lowsp() and conditionally > > update the eofb to do a filtered pquota scan if appropriate (since > > calling the quota helper above would also affect other dquots attached > > to the inode, which I don't think we want to do). Then we can fall back > > to the global scan if the pquota optimization is not relevant or still > > returns -ENOSPC on the subsequent retry. > > That's what I've implemented. But it turns out -ENOSPC can of course > still mean a real -ENOSPC even with project quotas attached. So back > to the drawing board - I think I basically need to replace the enospc > with a tristate saying what kind of scan we've tried. Or we just ignore > the issue and keep the current global scan after a potential project > quota -ENOSPC, because all that cruft isn't worth it after all. > Sure, that's why I was suggesting to check the quota for low free space conditions. One one hand, a quota scan might be worth it under low quota space conditions to avoid the heavy handed impact (i.e. flush everything) on an fs that otherwise might have plenty of free space. OTOH, it might be pointless if permanent -ENOSPC (due to project quota) is imminent and we always fall back to the global scan. Brian
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 4b8bdecc3863..8cec826046ce 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -647,27 +647,31 @@ xfs_file_buffered_aio_write( * waits on dirty mappings. Since xfs_flush_inodes() is serialized, this * also behaves as a filter to prevent too many eofblocks scans from * running at the same time. + * + * Fun fact: Project quota returns -ENOSPC, so... complexity here. */ - if (ret == -EDQUOT && !enospc) { - xfs_iunlock(ip, iolock); - enospc = xfs_inode_free_quota_eofblocks(ip); - if (enospc) - goto write_retry; - enospc = xfs_inode_free_quota_cowblocks(ip); + if (!enospc) { + if (ret == -ENOSPC) { + struct xfs_eofblocks eofb = {0}; + + enospc = 1; + xfs_flush_inodes(ip->i_mount); + + xfs_iunlock(ip, iolock); + eofb.eof_flags = XFS_EOF_FLAGS_SYNC; + xfs_icache_free_eofblocks(ip->i_mount, &eofb); + xfs_icache_free_cowblocks(ip->i_mount, &eofb); + } + if (ret == -EDQUOT || + (ret == -ENOSPC && ip->i_pdquot && + XFS_IS_PQUOTA_ENFORCED(mp) && ip->i_pdquot)) { + xfs_iunlock(ip, iolock); + enospc |= xfs_inode_free_quota_eofblocks(ip); + enospc |= xfs_inode_free_quota_cowblocks(ip); + iolock = 0; + } if (enospc) goto write_retry; - iolock = 0; - } else if (ret == -ENOSPC && !enospc) { - struct xfs_eofblocks eofb = {0}; - - enospc = 1; - xfs_flush_inodes(ip->i_mount); - - xfs_iunlock(ip, iolock); - eofb.eof_flags = XFS_EOF_FLAGS_SYNC; - xfs_icache_free_eofblocks(ip->i_mount, &eofb); - xfs_icache_free_cowblocks(ip->i_mount, &eofb); - goto write_retry; } current->backing_dev_info = NULL;
Since project quota returns ENOSPC rather than EDQUOT, the distinction between those two error conditions in xfs_file_buffered_aio_write() is incomplete. If project quota is on, and we get ENOSPC, it seems that we have no good way to know if it was due to quota, or due to actual out of space conditions, and we may need to run both remediation options... This is completely untested and not even built, because I'm really not sure what the best way to go here is. True ENOSPC is hopefully rare, pquota ENOSPC is probably less so, so I'm not sure how far we should go digging to figure out what the root cause of the ENOSPC condition is, when pquota is on (on this inode). If project quota is on on this inode and pquota enforced, should we look to the super to make a determination about low free blocks in the fs? Should we crack open the dquot and see if it's over limit? Should we just run both conditions and hope for the best? Is this all best effort anyway, so we just simply care if we take the improper action for pquota+ENOSPC? Probably-shouldn't-merge-this-sez: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Eric Sandeen <sandeen@redhat.com> ---