diff mbox series

[07/12] xfs: flush eof/cowblocks if we can't reserve quota for file blocks

Message ID 161214516600.140945.4401509001858536727.stgit@magnolia (mailing list archive)
State Accepted
Headers show
Series xfs: try harder to reclaim space when we run out | expand

Commit Message

Darrick J. Wong Feb. 1, 2021, 2:06 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

If a fs modification (data write, reflink, xattr set, fallocate, etc.)
is unable to reserve enough quota to handle the modification, try
clearing whatever space the filesystem might have been hanging onto in
the hopes of speeding up the filesystem.  The flushing behavior will
become particularly important when we add deferred inode inactivation
because that will increase the amount of space that isn't actively tied
to user data.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_reflink.c |    5 +++++
 fs/xfs/xfs_trans.c   |   10 ++++++++++
 2 files changed, 15 insertions(+)

Comments

Christoph Hellwig Feb. 1, 2021, 12:32 p.m. UTC | #1 
On Sun, Jan 31, 2021 at 06:06:06PM -0800, Darrick J. Wong wrote:
> @@ -1046,8 +1047,10 @@ xfs_trans_alloc_inode(
>  {
>  	struct xfs_trans	*tp;
>  	struct xfs_mount	*mp = ip->i_mount;
> +	bool			retried = false;
>  	int			error;
>  
> +retry:
>  	error = xfs_trans_alloc(mp, resv, dblocks,
>  			rblocks / mp->m_sb.sb_rextsize,
>  			force ? XFS_TRANS_RESERVE : 0, &tp);
> @@ -1065,6 +1068,13 @@ xfs_trans_alloc_inode(
>  	}
>  
>  	error = xfs_trans_reserve_quota_nblks(tp, ip, dblocks, rblocks, force);
> +	if (!retried && (error == -EDQUOT || error == -ENOSPC)) {

Nit: writing this as

	if ((error == -EDQUOT || error == -ENOSPC) && !retried) {

would make reading the line a little bit easier at least to me because
it checks the variable assigned in the line above first.

Otherwise this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong Feb. 1, 2021, 7:01 p.m. UTC | #2 
On Mon, Feb 01, 2021 at 12:32:45PM +0000, Christoph Hellwig wrote:
> On Sun, Jan 31, 2021 at 06:06:06PM -0800, Darrick J. Wong wrote:
> > @@ -1046,8 +1047,10 @@ xfs_trans_alloc_inode(
> >  {
> >  	struct xfs_trans	*tp;
> >  	struct xfs_mount	*mp = ip->i_mount;
> > +	bool			retried = false;
> >  	int			error;
> >  
> > +retry:
> >  	error = xfs_trans_alloc(mp, resv, dblocks,
> >  			rblocks / mp->m_sb.sb_rextsize,
> >  			force ? XFS_TRANS_RESERVE : 0, &tp);
> > @@ -1065,6 +1068,13 @@ xfs_trans_alloc_inode(
> >  	}
> >  
> >  	error = xfs_trans_reserve_quota_nblks(tp, ip, dblocks, rblocks, force);
> > +	if (!retried && (error == -EDQUOT || error == -ENOSPC)) {
> 
> Nit: writing this as
> 
> 	if ((error == -EDQUOT || error == -ENOSPC) && !retried) {
> 
> would make reading the line a little bit easier at least to me because
> it checks the variable assigned in the line above first.

Will fix in all three.

--D

> Otherwise this looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Brian Foster Feb. 2, 2021, 3:38 p.m. UTC | #3 
On Sun, Jan 31, 2021 at 06:06:06PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If a fs modification (data write, reflink, xattr set, fallocate, etc.)
> is unable to reserve enough quota to handle the modification, try
> clearing whatever space the filesystem might have been hanging onto in
> the hopes of speeding up the filesystem.  The flushing behavior will
> become particularly important when we add deferred inode inactivation
> because that will increase the amount of space that isn't actively tied
> to user data.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

(FWIW, I'm reviewing the patches from your reclaim-space-harder-5.12
branch as of this morning, which look like they have some deltas from
the posted versions based on Christoph's feedback.)

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_reflink.c |    5 +++++
>  fs/xfs/xfs_trans.c   |   10 ++++++++++
>  2 files changed, 15 insertions(+)
> 
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 086866f6e71f..725c7d8e4438 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1092,6 +1092,11 @@ xfs_reflink_remap_extent(
>  	 * count.  This is suboptimal, but the VFS flushed the dest range
>  	 * before we started.  That should have removed all the delalloc
>  	 * reservations, but we code defensively.
> +	 *
> +	 * xfs_trans_alloc_inode above already tried to grab an even larger
> +	 * quota reservation, and kicked off a blockgc scan if it couldn't.
> +	 * If we can't get a potentially smaller quota reservation now, we're
> +	 * done.
>  	 */
>  	if (!quota_reserved && !smap_real && dmap_written) {
>  		error = xfs_trans_reserve_quota_nblks(tp, ip,
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 466e1c86767f..f62c1c5f210f 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -23,6 +23,7 @@
>  #include "xfs_inode.h"
>  #include "xfs_dquot_item.h"
>  #include "xfs_dquot.h"
> +#include "xfs_icache.h"
>  
>  kmem_zone_t	*xfs_trans_zone;
>  
> @@ -1046,8 +1047,10 @@ xfs_trans_alloc_inode(
>  {
>  	struct xfs_trans	*tp;
>  	struct xfs_mount	*mp = ip->i_mount;
> +	bool			retried = false;
>  	int			error;
>  
> +retry:
>  	error = xfs_trans_alloc(mp, resv, dblocks,
>  			rblocks / mp->m_sb.sb_rextsize,
>  			force ? XFS_TRANS_RESERVE : 0, &tp);
> @@ -1065,6 +1068,13 @@ xfs_trans_alloc_inode(
>  	}
>  
>  	error = xfs_trans_reserve_quota_nblks(tp, ip, dblocks, rblocks, force);
> +	if (!retried && (error == -EDQUOT || error == -ENOSPC)) {
> +		xfs_trans_cancel(tp);
> +		xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +		xfs_blockgc_free_quota(ip, 0);
> +		retried = true;
> +		goto retry;
> +	}
>  	if (error)
>  		goto out_cancel;
>  
>
Darrick J. Wong Feb. 2, 2021, 4:53 p.m. UTC | #4 
On Tue, Feb 02, 2021 at 10:38:59AM -0500, Brian Foster wrote:
> On Sun, Jan 31, 2021 at 06:06:06PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > If a fs modification (data write, reflink, xattr set, fallocate, etc.)
> > is unable to reserve enough quota to handle the modification, try
> > clearing whatever space the filesystem might have been hanging onto in
> > the hopes of speeding up the filesystem.  The flushing behavior will
> > become particularly important when we add deferred inode inactivation
> > because that will increase the amount of space that isn't actively tied
> > to user data.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> 
> (FWIW, I'm reviewing the patches from your reclaim-space-harder-5.12
> branch as of this morning, which look like they have some deltas from
> the posted versions based on Christoph's feedback.)

Yes, it does. :(

I pushed the branches and had sent the first of the two patchsets
last night, and then the power went out so I gave up and went to bed.

But thanks for the review. :)

--D

> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> >  fs/xfs/xfs_reflink.c |    5 +++++
> >  fs/xfs/xfs_trans.c   |   10 ++++++++++
> >  2 files changed, 15 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index 086866f6e71f..725c7d8e4438 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -1092,6 +1092,11 @@ xfs_reflink_remap_extent(
> >  	 * count.  This is suboptimal, but the VFS flushed the dest range
> >  	 * before we started.  That should have removed all the delalloc
> >  	 * reservations, but we code defensively.
> > +	 *
> > +	 * xfs_trans_alloc_inode above already tried to grab an even larger
> > +	 * quota reservation, and kicked off a blockgc scan if it couldn't.
> > +	 * If we can't get a potentially smaller quota reservation now, we're
> > +	 * done.
> >  	 */
> >  	if (!quota_reserved && !smap_real && dmap_written) {
> >  		error = xfs_trans_reserve_quota_nblks(tp, ip,
> > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> > index 466e1c86767f..f62c1c5f210f 100644
> > --- a/fs/xfs/xfs_trans.c
> > +++ b/fs/xfs/xfs_trans.c
> > @@ -23,6 +23,7 @@
> >  #include "xfs_inode.h"
> >  #include "xfs_dquot_item.h"
> >  #include "xfs_dquot.h"
> > +#include "xfs_icache.h"
> >  
> >  kmem_zone_t	*xfs_trans_zone;
> >  
> > @@ -1046,8 +1047,10 @@ xfs_trans_alloc_inode(
> >  {
> >  	struct xfs_trans	*tp;
> >  	struct xfs_mount	*mp = ip->i_mount;
> > +	bool			retried = false;
> >  	int			error;
> >  
> > +retry:
> >  	error = xfs_trans_alloc(mp, resv, dblocks,
> >  			rblocks / mp->m_sb.sb_rextsize,
> >  			force ? XFS_TRANS_RESERVE : 0, &tp);
> > @@ -1065,6 +1068,13 @@ xfs_trans_alloc_inode(
> >  	}
> >  
> >  	error = xfs_trans_reserve_quota_nblks(tp, ip, dblocks, rblocks, force);
> > +	if (!retried && (error == -EDQUOT || error == -ENOSPC)) {
> > +		xfs_trans_cancel(tp);
> > +		xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > +		xfs_blockgc_free_quota(ip, 0);
> > +		retried = true;
> > +		goto retry;
> > +	}
> >  	if (error)
> >  		goto out_cancel;
> >  
> > 
>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 086866f6e71f..725c7d8e4438 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1092,6 +1092,11 @@  xfs_reflink_remap_extent(
 	 * count.  This is suboptimal, but the VFS flushed the dest range
 	 * before we started.  That should have removed all the delalloc
 	 * reservations, but we code defensively.
+	 *
+	 * xfs_trans_alloc_inode above already tried to grab an even larger
+	 * quota reservation, and kicked off a blockgc scan if it couldn't.
+	 * If we can't get a potentially smaller quota reservation now, we're
+	 * done.
 	 */
 	if (!quota_reserved && !smap_real && dmap_written) {
 		error = xfs_trans_reserve_quota_nblks(tp, ip,
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 466e1c86767f..f62c1c5f210f 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -23,6 +23,7 @@ 
 #include "xfs_inode.h"
 #include "xfs_dquot_item.h"
 #include "xfs_dquot.h"
+#include "xfs_icache.h"
 
 kmem_zone_t	*xfs_trans_zone;
 
@@ -1046,8 +1047,10 @@  xfs_trans_alloc_inode(
 {
 	struct xfs_trans	*tp;
 	struct xfs_mount	*mp = ip->i_mount;
+	bool			retried = false;
 	int			error;
 
+retry:
 	error = xfs_trans_alloc(mp, resv, dblocks,
 			rblocks / mp->m_sb.sb_rextsize,
 			force ? XFS_TRANS_RESERVE : 0, &tp);
@@ -1065,6 +1068,13 @@  xfs_trans_alloc_inode(
 	}
 
 	error = xfs_trans_reserve_quota_nblks(tp, ip, dblocks, rblocks, force);
+	if (!retried && (error == -EDQUOT || error == -ENOSPC)) {
+		xfs_trans_cancel(tp);
+		xfs_iunlock(ip, XFS_ILOCK_EXCL);
+		xfs_blockgc_free_quota(ip, 0);
+		retried = true;
+		goto retry;
+	}
 	if (error)
 		goto out_cancel;