diff mbox series

[v1.1,3/3] xfs: make inode unlinked bucket recovery work with quotacheck

Message ID 20230905163303.GU28186@frogsfrogsfrogs (mailing list archive)
State Superseded, archived
Headers show
Series None | expand

Commit Message

Darrick J. Wong Sept. 5, 2023, 4:33 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Teach quotacheck to reload the unlinked inode lists when walking the
inode table.  This requires extra state handling, since it's possible
that a reloaded inode will get inactivated before quotacheck tries to
scan it; in this case, we need to ensure that the reloaded inode does
not have dquots attached when it is freed.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v1.1: s/CONFIG_QUOTA/CONFIG_XFS_QUOTA/ and fix tracepoint flags decoding
---
 fs/xfs/xfs_inode.c |   12 +++++++++---
 fs/xfs/xfs_inode.h |    5 ++++-
 fs/xfs/xfs_mount.h |   10 +++++++++-
 fs/xfs/xfs_qm.c    |    7 +++++++
 4 files changed, 29 insertions(+), 5 deletions(-)

Comments

Dave Chinner Sept. 7, 2023, 7:11 a.m. UTC | #1 
On Tue, Sep 05, 2023 at 09:33:03AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Teach quotacheck to reload the unlinked inode lists when walking the
> inode table.  This requires extra state handling, since it's possible
> that a reloaded inode will get inactivated before quotacheck tries to
> scan it; in this case, we need to ensure that the reloaded inode does
> not have dquots attached when it is freed.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> v1.1: s/CONFIG_QUOTA/CONFIG_XFS_QUOTA/ and fix tracepoint flags decoding
> ---
>  fs/xfs/xfs_inode.c |   12 +++++++++---
>  fs/xfs/xfs_inode.h |    5 ++++-
>  fs/xfs/xfs_mount.h |   10 +++++++++-
>  fs/xfs/xfs_qm.c    |    7 +++++++
>  4 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 56f6bde6001b..22af7268169b 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1743,9 +1743,13 @@ xfs_inactive(
>  	     ip->i_df.if_nextents > 0 || ip->i_delayed_blks > 0))
>  		truncate = 1;
>  
> -	error = xfs_qm_dqattach(ip);
> -	if (error)
> -		goto out;
> +	if (xfs_iflags_test(ip, XFS_IQUOTAUNCHECKED)) {
> +		xfs_qm_dqdetach(ip);
> +	} else {
> +		error = xfs_qm_dqattach(ip);
> +		if (error)
> +			goto out;
> +	}

That needs a comment - I'm not going to remember why sometimes we
detatch dquots instead of attach them here....


....
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 6abcc34fafd8..7256090c3895 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1160,6 +1160,10 @@ xfs_qm_dqusage_adjust(
>  	if (error)
>  		return error;
>  
> +	error = xfs_inode_reload_unlinked(ip);
> +	if (error)
> +		goto error0;

Same comment here about doing millions of transaction create/cancel
for inodes that have non-zero link counts....

Also, same comment here about shutting down on reload error because
the irele() call will inactivate the inode and try to remove it from
the unlinked list....

Cheers,

Dave.
Darrick J. Wong Sept. 7, 2023, 6:34 p.m. UTC | #2 
On Thu, Sep 07, 2023 at 05:11:53PM +1000, Dave Chinner wrote:
> On Tue, Sep 05, 2023 at 09:33:03AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Teach quotacheck to reload the unlinked inode lists when walking the
> > inode table.  This requires extra state handling, since it's possible
> > that a reloaded inode will get inactivated before quotacheck tries to
> > scan it; in this case, we need to ensure that the reloaded inode does
> > not have dquots attached when it is freed.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> > v1.1: s/CONFIG_QUOTA/CONFIG_XFS_QUOTA/ and fix tracepoint flags decoding
> > ---
> >  fs/xfs/xfs_inode.c |   12 +++++++++---
> >  fs/xfs/xfs_inode.h |    5 ++++-
> >  fs/xfs/xfs_mount.h |   10 +++++++++-
> >  fs/xfs/xfs_qm.c    |    7 +++++++
> >  4 files changed, 29 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 56f6bde6001b..22af7268169b 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -1743,9 +1743,13 @@ xfs_inactive(
> >  	     ip->i_df.if_nextents > 0 || ip->i_delayed_blks > 0))
> >  		truncate = 1;
> >  
> > -	error = xfs_qm_dqattach(ip);
> > -	if (error)
> > -		goto out;
> > +	if (xfs_iflags_test(ip, XFS_IQUOTAUNCHECKED)) {
> > +		xfs_qm_dqdetach(ip);
> > +	} else {
> > +		error = xfs_qm_dqattach(ip);
> > +		if (error)
> > +			goto out;
> > +	}
> 
> That needs a comment - I'm not going to remember why sometimes we
> detatch dquots instead of attach them here....

	/*
	 * If this inode is being inactivated during a quotacheck and
	 * has not yet been scanned by quotacheck, we /must/ remove the
	 * dquots from the inode before inactivation changes the block
	 * and inode counts.  Most probably this is a result of
	 * reloading the incore iunlinked list to purge unrecovered
	 * unlinked inodes.
	 */

How does that sound?

> ....
> > diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> > index 6abcc34fafd8..7256090c3895 100644
> > --- a/fs/xfs/xfs_qm.c
> > +++ b/fs/xfs/xfs_qm.c
> > @@ -1160,6 +1160,10 @@ xfs_qm_dqusage_adjust(
> >  	if (error)
> >  		return error;
> >  
> > +	error = xfs_inode_reload_unlinked(ip);
> > +	if (error)
> > +		goto error0;
> 
> Same comment here about doing millions of transaction create/cancel
> for inodes that have non-zero link counts....
> 
> Also, same comment here about shutting down on reload error because
> the irele() call will inactivate the inode and try to remove it from
> the unlinked list....

Ok, fixed this callsite too.

Thank you for looking at this series!

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
Dave Chinner Sept. 7, 2023, 9:40 p.m. UTC | #3 
On Thu, Sep 07, 2023 at 11:34:41AM -0700, Darrick J. Wong wrote:
> On Thu, Sep 07, 2023 at 05:11:53PM +1000, Dave Chinner wrote:
> > On Tue, Sep 05, 2023 at 09:33:03AM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Teach quotacheck to reload the unlinked inode lists when walking the
> > > inode table.  This requires extra state handling, since it's possible
> > > that a reloaded inode will get inactivated before quotacheck tries to
> > > scan it; in this case, we need to ensure that the reloaded inode does
> > > not have dquots attached when it is freed.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > > v1.1: s/CONFIG_QUOTA/CONFIG_XFS_QUOTA/ and fix tracepoint flags decoding
> > > ---
> > >  fs/xfs/xfs_inode.c |   12 +++++++++---
> > >  fs/xfs/xfs_inode.h |    5 ++++-
> > >  fs/xfs/xfs_mount.h |   10 +++++++++-
> > >  fs/xfs/xfs_qm.c    |    7 +++++++
> > >  4 files changed, 29 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index 56f6bde6001b..22af7268169b 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -1743,9 +1743,13 @@ xfs_inactive(
> > >  	     ip->i_df.if_nextents > 0 || ip->i_delayed_blks > 0))
> > >  		truncate = 1;
> > >  
> > > -	error = xfs_qm_dqattach(ip);
> > > -	if (error)
> > > -		goto out;
> > > +	if (xfs_iflags_test(ip, XFS_IQUOTAUNCHECKED)) {
> > > +		xfs_qm_dqdetach(ip);
> > > +	} else {
> > > +		error = xfs_qm_dqattach(ip);
> > > +		if (error)
> > > +			goto out;
> > > +	}
> > 
> > That needs a comment - I'm not going to remember why sometimes we
> > detatch dquots instead of attach them here....
> 
> 	/*
> 	 * If this inode is being inactivated during a quotacheck and
> 	 * has not yet been scanned by quotacheck, we /must/ remove the
> 	 * dquots from the inode before inactivation changes the block
> 	 * and inode counts.  Most probably this is a result of
> 	 * reloading the incore iunlinked list to purge unrecovered
> 	 * unlinked inodes.
> 	 */
> 
> How does that sound?

LGTM.

-Dave.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 56f6bde6001b..22af7268169b 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1743,9 +1743,13 @@  xfs_inactive(
 	     ip->i_df.if_nextents > 0 || ip->i_delayed_blks > 0))
 		truncate = 1;
 
-	error = xfs_qm_dqattach(ip);
-	if (error)
-		goto out;
+	if (xfs_iflags_test(ip, XFS_IQUOTAUNCHECKED)) {
+		xfs_qm_dqdetach(ip);
+	} else {
+		error = xfs_qm_dqattach(ip);
+		if (error)
+			goto out;
+	}
 
 	if (S_ISLNK(VFS_I(ip)->i_mode))
 		error = xfs_inactive_symlink(ip);
@@ -1963,6 +1967,8 @@  xfs_iunlink_reload_next(
 	trace_xfs_iunlink_reload_next(next_ip);
 rele:
 	ASSERT(!(VFS_I(next_ip)->i_state & I_DONTCACHE));
+	if (xfs_is_quotacheck_running(mp) && next_ip)
+		xfs_iflags_set(next_ip, XFS_IQUOTAUNCHECKED);
 	xfs_irele(next_ip);
 	return error;
 }
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index a111b5551ecd..0c5bdb91152e 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -344,6 +344,9 @@  static inline bool xfs_inode_has_large_extent_counts(struct xfs_inode *ip)
  */
 #define XFS_INACTIVATING	(1 << 13)
 
+/* Quotacheck is running but inode has not been added to quota counts. */
+#define XFS_IQUOTAUNCHECKED	(1 << 14)
+
 /* All inode state flags related to inode reclaim. */
 #define XFS_ALL_IRECLAIM_FLAGS	(XFS_IRECLAIMABLE | \
 				 XFS_IRECLAIM | \
@@ -358,7 +361,7 @@  static inline bool xfs_inode_has_large_extent_counts(struct xfs_inode *ip)
 #define XFS_IRECLAIM_RESET_FLAGS	\
 	(XFS_IRECLAIMABLE | XFS_IRECLAIM | \
 	 XFS_IDIRTY_RELEASE | XFS_ITRUNCATED | XFS_NEED_INACTIVE | \
-	 XFS_INACTIVATING)
+	 XFS_INACTIVATING | XFS_IQUOTAUNCHECKED)
 
 /*
  * Flags for inode locking.
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 6e2806654e94..d19cca099bc3 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -405,6 +405,8 @@  __XFS_HAS_FEAT(nouuid, NOUUID)
 #define XFS_OPSTATE_WARNED_SHRINK	8
 /* Kernel has logged a warning about logged xattr updates being used. */
 #define XFS_OPSTATE_WARNED_LARP		9
+/* Mount time quotacheck is running */
+#define XFS_OPSTATE_QUOTACHECK_RUNNING	10
 
 #define __XFS_IS_OPSTATE(name, NAME) \
 static inline bool xfs_is_ ## name (struct xfs_mount *mp) \
@@ -427,6 +429,11 @@  __XFS_IS_OPSTATE(inode32, INODE32)
 __XFS_IS_OPSTATE(readonly, READONLY)
 __XFS_IS_OPSTATE(inodegc_enabled, INODEGC_ENABLED)
 __XFS_IS_OPSTATE(blockgc_enabled, BLOCKGC_ENABLED)
+#ifdef CONFIG_XFS_QUOTA
+__XFS_IS_OPSTATE(quotacheck_running, QUOTACHECK_RUNNING)
+#else
+# define xfs_is_quotacheck_running(mp)	(false)
+#endif
 
 static inline bool
 xfs_should_warn(struct xfs_mount *mp, long nr)
@@ -444,7 +451,8 @@  xfs_should_warn(struct xfs_mount *mp, long nr)
 	{ (1UL << XFS_OPSTATE_BLOCKGC_ENABLED),		"blockgc" }, \
 	{ (1UL << XFS_OPSTATE_WARNED_SCRUB),		"wscrub" }, \
 	{ (1UL << XFS_OPSTATE_WARNED_SHRINK),		"wshrink" }, \
-	{ (1UL << XFS_OPSTATE_WARNED_LARP),		"wlarp" }
+	{ (1UL << XFS_OPSTATE_WARNED_LARP),		"wlarp" }, \
+	{ (1UL << XFS_OPSTATE_QUOTACHECK_RUNNING),	"quotacheck" }
 
 /*
  * Max and min values for mount-option defined I/O
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 6abcc34fafd8..7256090c3895 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1160,6 +1160,10 @@  xfs_qm_dqusage_adjust(
 	if (error)
 		return error;
 
+	error = xfs_inode_reload_unlinked(ip);
+	if (error)
+		goto error0;
+
 	ASSERT(ip->i_delayed_blks == 0);
 
 	if (XFS_IS_REALTIME_INODE(ip)) {
@@ -1173,6 +1177,7 @@  xfs_qm_dqusage_adjust(
 	}
 
 	nblks = (xfs_qcnt_t)ip->i_nblocks - rtblks;
+	xfs_iflags_clear(ip, XFS_IQUOTAUNCHECKED);
 
 	/*
 	 * Add the (disk blocks and inode) resources occupied by this
@@ -1319,8 +1324,10 @@  xfs_qm_quotacheck(
 		flags |= XFS_PQUOTA_CHKD;
 	}
 
+	xfs_set_quotacheck_running(mp);
 	error = xfs_iwalk_threaded(mp, 0, 0, xfs_qm_dqusage_adjust, 0, true,
 			NULL);
+	xfs_clear_quotacheck_running(mp);
 
 	/*
 	 * On error, the inode walk may have partially populated the dquot