[PATCH 0/2] xfs: fix simple problems with log intent recovery

[PATCH 0/2] xfs: fix simple problems with log intent recovery

[Darrick J. Wong]

Hi all,

Dave and I were talking about various issues that he had discovered
while wandering through the log recovery code, and then I started taking
a closer look and found bugs aplenty.  This first series is a very short
one that cleans up some low hanging fruit I found -- the most serious is
that we don't log new intent items created in the process of recovering
intent items that we found in the log, which breaks the recoverability
of log chains.  The other thing I found was that bmap intent recovery
didn't attach dquots, which could lead to incorrect quota accounting if
the BUI recovery caused a bmap btree shape change.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=fix-log-intent-recovery
---
 fs/xfs/libxfs/xfs_defer.c  |   26 ++++++++++++++++++++++++--
 fs/xfs/libxfs/xfs_defer.h  |    6 ++++++
 fs/xfs/xfs_bmap_item.c     |    7 ++++++-
 fs/xfs/xfs_refcount_item.c |    2 +-
 4 files changed, 37 insertions(+), 4 deletions(-)

[PATCH 1/2] xfs: log new intent items created as part of finishing recovered intent items

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

During a code inspection, I found a serious bug in the log intent item
recovery code when an intent item cannot complete all the work and
decides to requeue itself to get that done.  When this happens, the
item recovery creates a new incore deferred op representing the
remaining work and attaches it to the transaction that it allocated.  At
the end of _item_recover, it moves the entire chain of deferred ops to
the dummy parent_tp that xlog_recover_process_intents passed to it, but
fail to log a new intent item for the remaining work before committing
the transaction for the single unit of work.

xlog_finish_defer_ops logs those new intent items once recovery has
finished dealing with the intent items that it recovered, but this isn't
sufficient.  If the log is forced to disk after a recovered log item
decides to requeue itself and the system goes down before we call
xlog_finish_defer_ops, the second log recovery will never see the new
intent item and therefore has no idea that there was more work to do.
It will finish recovery leaving the filesystem in a corrupted state.

The same logic applies to /any/ deferred ops added during intent item
recovery, not just the one handling the remaining work.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_defer.c  |   26 ++++++++++++++++++++++++--
 fs/xfs/libxfs/xfs_defer.h  |    6 ++++++
 fs/xfs/xfs_bmap_item.c     |    2 +-
 fs/xfs/xfs_refcount_item.c |    2 +-
 4 files changed, 32 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index d8f586256add..29e9762f3b77 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -186,8 +186,9 @@ xfs_defer_create_intent(
 {
 	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
 
-	dfp->dfp_intent = ops->create_intent(tp, &dfp->dfp_work,
-			dfp->dfp_count, sort);
+	if (!dfp->dfp_intent)
+		dfp->dfp_intent = ops->create_intent(tp, &dfp->dfp_work,
+						     dfp->dfp_count, sort);
 }
 
 /*
@@ -390,6 +391,7 @@ xfs_defer_finish_one(
 			list_add(li, &dfp->dfp_work);
 			dfp->dfp_count++;
 			dfp->dfp_done = NULL;
+			dfp->dfp_intent = NULL;
 			xfs_defer_create_intent(tp, dfp, false);
 		}
 
@@ -552,3 +554,23 @@ xfs_defer_move(
 
 	xfs_defer_reset(stp);
 }
+
+/*
+ * Prepare a chain of fresh deferred ops work items to be completed later.  Log
+ * recovery requires the ability to put off until later the actual finishing
+ * work so that it can process unfinished items recovered from the log in
+ * correct order.
+ *
+ * Create and log intent items for all the work that we're capturing so that we
+ * can be assured that the items will get replayed if the system goes down
+ * before log recovery gets a chance to finish the work it put off.  Then we
+ * move the chain from stp to dtp.
+ */
+void
+xfs_defer_capture(
+	struct xfs_trans	*dtp,
+	struct xfs_trans	*stp)
+{
+	xfs_defer_create_intents(stp);
+	xfs_defer_move(dtp, stp);
+}
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index 6b2ca580f2b0..3164199162b6 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -63,4 +63,10 @@ extern const struct xfs_defer_op_type xfs_rmap_update_defer_type;
 extern const struct xfs_defer_op_type xfs_extent_free_defer_type;
 extern const struct xfs_defer_op_type xfs_agfl_free_defer_type;
 
+/*
+ * Functions to capture a chain of deferred operations and continue them later.
+ * This doesn't normally happen except log recovery.
+ */
+void xfs_defer_capture(struct xfs_trans *dtp, struct xfs_trans *stp);
+
 #endif /* __XFS_DEFER_H__ */
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index ec3691372e7c..815a0563288f 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -534,7 +534,7 @@ xfs_bui_item_recover(
 		xfs_bmap_unmap_extent(tp, ip, &irec);
 	}
 
-	xfs_defer_move(parent_tp, tp);
+	xfs_defer_capture(parent_tp, tp);
 	error = xfs_trans_commit(tp);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	xfs_irele(ip);
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index ca93b6488377..492d80a0b406 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -555,7 +555,7 @@ xfs_cui_item_recover(
 	}
 
 	xfs_refcount_finish_one_cleanup(tp, rcur, error);
-	xfs_defer_move(parent_tp, tp);
+	xfs_defer_capture(parent_tp, tp);
 	error = xfs_trans_commit(tp);
 	return error;
 

[PATCH 2/2] xfs: attach inode to dquot in xfs_bui_item_recover

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In the bmap intent item recovery code, we must be careful to attach the
inode to its dquots (if quotas are enabled) so that a change in the
shape of the bmap btree doesn't cause the quota counters to be
incorrect.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_bmap_item.c |    5 +++++
 1 file changed, 5 insertions(+)


diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 815a0563288f..598f713831c9 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -24,6 +24,7 @@
 #include "xfs_error.h"
 #include "xfs_log_priv.h"
 #include "xfs_log_recover.h"
+#include "xfs_quota.h"
 
 kmem_zone_t	*xfs_bui_zone;
 kmem_zone_t	*xfs_bud_zone;
@@ -498,6 +499,10 @@ xfs_bui_item_recover(
 	if (error)
 		goto err_inode;
 
+	error = xfs_qm_dqattach(ip);
+	if (error)
+		goto err_inode;
+
 	if (VFS_I(ip)->i_nlink == 0)
 		xfs_iflags_set(ip, XFS_IRECOVERY);
 

Re: [PATCH 2/2] xfs: attach inode to dquot in xfs_bui_item_recover

[Dave Chinner]

On Wed, Sep 16, 2020 at 08:28:56PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In the bmap intent item recovery code, we must be careful to attach the
> inode to its dquots (if quotas are enabled) so that a change in the
> shape of the bmap btree doesn't cause the quota counters to be
> incorrect.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_bmap_item.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> 
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 815a0563288f..598f713831c9 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -24,6 +24,7 @@
>  #include "xfs_error.h"
>  #include "xfs_log_priv.h"
>  #include "xfs_log_recover.h"
> +#include "xfs_quota.h"
>  
>  kmem_zone_t	*xfs_bui_zone;
>  kmem_zone_t	*xfs_bud_zone;
> @@ -498,6 +499,10 @@ xfs_bui_item_recover(
>  	if (error)
>  		goto err_inode;
>  
> +	error = xfs_qm_dqattach(ip);
> +	if (error)
> +		goto err_inode;

Won't this deadlock as the inode is already locked when it is
returned by xfs_iget()?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] xfs: log new intent items created as part of finishing recovered intent items

[Dave Chinner]

On Wed, Sep 16, 2020 at 08:28:49PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> During a code inspection, I found a serious bug in the log intent item
> recovery code when an intent item cannot complete all the work and
> decides to requeue itself to get that done.  When this happens, the
> item recovery creates a new incore deferred op representing the
> remaining work and attaches it to the transaction that it allocated.  At
> the end of _item_recover, it moves the entire chain of deferred ops to
> the dummy parent_tp that xlog_recover_process_intents passed to it, but
> fail to log a new intent item for the remaining work before committing
> the transaction for the single unit of work.
> 
> xlog_finish_defer_ops logs those new intent items once recovery has
> finished dealing with the intent items that it recovered, but this isn't
> sufficient.  If the log is forced to disk after a recovered log item
> decides to requeue itself and the system goes down before we call
> xlog_finish_defer_ops, the second log recovery will never see the new
> intent item and therefore has no idea that there was more work to do.
> It will finish recovery leaving the filesystem in a corrupted state.
> 
> The same logic applies to /any/ deferred ops added during intent item
> recovery, not just the one handling the remaining work.

Yup, that looks like a problem.

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_defer.c  |   26 ++++++++++++++++++++++++--
>  fs/xfs/libxfs/xfs_defer.h  |    6 ++++++
>  fs/xfs/xfs_bmap_item.c     |    2 +-
>  fs/xfs/xfs_refcount_item.c |    2 +-
>  4 files changed, 32 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index d8f586256add..29e9762f3b77 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -186,8 +186,9 @@ xfs_defer_create_intent(
>  {
>  	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
>  
> -	dfp->dfp_intent = ops->create_intent(tp, &dfp->dfp_work,
> -			dfp->dfp_count, sort);
> +	if (!dfp->dfp_intent)
> +		dfp->dfp_intent = ops->create_intent(tp, &dfp->dfp_work,
> +						     dfp->dfp_count, sort);
>  }
>  
>  /*
> @@ -390,6 +391,7 @@ xfs_defer_finish_one(
>  			list_add(li, &dfp->dfp_work);
>  			dfp->dfp_count++;
>  			dfp->dfp_done = NULL;
> +			dfp->dfp_intent = NULL;
>  			xfs_defer_create_intent(tp, dfp, false);
>  		}
>  
> @@ -552,3 +554,23 @@ xfs_defer_move(
>  
>  	xfs_defer_reset(stp);
>  }
> +
> +/*
> + * Prepare a chain of fresh deferred ops work items to be completed later.  Log
> + * recovery requires the ability to put off until later the actual finishing
> + * work so that it can process unfinished items recovered from the log in
> + * correct order.
> + *
> + * Create and log intent items for all the work that we're capturing so that we
> + * can be assured that the items will get replayed if the system goes down
> + * before log recovery gets a chance to finish the work it put off.  Then we
> + * move the chain from stp to dtp.
> + */
> +void
> +xfs_defer_capture(
> +	struct xfs_trans	*dtp,
> +	struct xfs_trans	*stp)
> +{
> +	xfs_defer_create_intents(stp);
> +	xfs_defer_move(dtp, stp);
> +}

Not sold on the "capture" name, but it'll do for now.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: attach inode to dquot in xfs_bui_item_recover

[Darrick J. Wong]

On Thu, Sep 17, 2020 at 02:54:56PM +1000, Dave Chinner wrote:
> On Wed, Sep 16, 2020 at 08:28:56PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In the bmap intent item recovery code, we must be careful to attach the
> > inode to its dquots (if quotas are enabled) so that a change in the
> > shape of the bmap btree doesn't cause the quota counters to be
> > incorrect.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_bmap_item.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> > index 815a0563288f..598f713831c9 100644
> > --- a/fs/xfs/xfs_bmap_item.c
> > +++ b/fs/xfs/xfs_bmap_item.c
> > @@ -24,6 +24,7 @@
> >  #include "xfs_error.h"
> >  #include "xfs_log_priv.h"
> >  #include "xfs_log_recover.h"
> > +#include "xfs_quota.h"
> >  
> >  kmem_zone_t	*xfs_bui_zone;
> >  kmem_zone_t	*xfs_bud_zone;
> > @@ -498,6 +499,10 @@ xfs_bui_item_recover(
> >  	if (error)
> >  		goto err_inode;
> >  
> > +	error = xfs_qm_dqattach(ip);
> > +	if (error)
> > +		goto err_inode;
> 
> Won't this deadlock as the inode is already locked when it is
> returned by xfs_iget()?

DOH, yes.  The patch "xfs: clean up xfs_bui_item_recover
iget/trans_alloc/ilock ordering" obscures that...

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

[PATCH v2 2/2] xfs: attach inode to dquot in xfs_bui_item_recover

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In the bmap intent item recovery code, we must be careful to attach the
inode to its dquots (if quotas are enabled) so that a change in the
shape of the bmap btree doesn't cause the quota counters to be
incorrect.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: use dqattach_locked
---
 fs/xfs/xfs_bmap_item.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 815a0563288f..2b1cf3ed8172 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -24,6 +24,7 @@
 #include "xfs_error.h"
 #include "xfs_log_priv.h"
 #include "xfs_log_recover.h"
+#include "xfs_quota.h"
 
 kmem_zone_t	*xfs_bui_zone;
 kmem_zone_t	*xfs_bud_zone;
@@ -498,6 +499,10 @@ xfs_bui_item_recover(
 	if (error)
 		goto err_inode;
 
+	error = xfs_qm_dqattach_locked(ip, false);
+	if (error)
+		goto err_inode;
+
 	if (VFS_I(ip)->i_nlink == 0)
 		xfs_iflags_set(ip, XFS_IRECOVERY);
 

[PATCH 3/2] xfs: free the intent item when allocating recovery transaction fails

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The recovery functions of all four log intent items fail to free the
intent item if the transaction allocation fails.  Fix this.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_bmap_item.c     |    5 ++++-
 fs/xfs/xfs_extfree_item.c  |    5 ++++-
 fs/xfs/xfs_refcount_item.c |    5 ++++-
 fs/xfs/xfs_rmap_item.c     |    5 ++++-
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 2b1cf3ed8172..85d18cd708ba 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -484,8 +484,11 @@ xfs_bui_item_recover(
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate,
 			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK), 0, 0, &tp);
-	if (error)
+	if (error) {
+		xfs_bui_release(buip);
 		return error;
+	}
+
 	/*
 	 * Recovery stashes all deferred ops during intent processing and
 	 * finishes them on completion. Transfer current dfops state to this
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 6cb8cd11072a..9ceac1a0a39f 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -619,8 +619,11 @@ xfs_efi_item_recover(
 	}
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
-	if (error)
+	if (error) {
+		xfs_efi_release(efip);
 		return error;
+	}
+
 	efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents);
 
 	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 492d80a0b406..aae2a6ec00d3 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -491,8 +491,11 @@ xfs_cui_item_recover(
 	 */
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate,
 			mp->m_refc_maxlevels * 2, 0, XFS_TRANS_RESERVE, &tp);
-	if (error)
+	if (error) {
+		xfs_cui_release(cuip);
 		return error;
+	}
+
 	/*
 	 * Recovery stashes all deferred ops during intent processing and
 	 * finishes them on completion. Transfer current dfops state to this
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index dc5b0753cd51..9e7fabb54ff1 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -523,8 +523,11 @@ xfs_rui_item_recover(
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate,
 			mp->m_rmap_maxlevels, 0, XFS_TRANS_RESERVE, &tp);
-	if (error)
+	if (error) {
+		xfs_rui_release(ruip);
 		return error;
+	}
+
 	rudp = xfs_trans_get_rud(tp, ruip);
 
 	for (i = 0; i < ruip->rui_format.rui_nextents; i++) {

Re: [PATCH v2 2/2] xfs: attach inode to dquot in xfs_bui_item_recover

[Dave Chinner]

On Thu, Sep 17, 2020 at 12:01:03AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In the bmap intent item recovery code, we must be careful to attach the
> inode to its dquots (if quotas are enabled) so that a change in the
> shape of the bmap btree doesn't cause the quota counters to be
> incorrect.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: use dqattach_locked
> ---
>  fs/xfs/xfs_bmap_item.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 815a0563288f..2b1cf3ed8172 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -24,6 +24,7 @@
>  #include "xfs_error.h"
>  #include "xfs_log_priv.h"
>  #include "xfs_log_recover.h"
> +#include "xfs_quota.h"
>  
>  kmem_zone_t	*xfs_bui_zone;
>  kmem_zone_t	*xfs_bud_zone;
> @@ -498,6 +499,10 @@ xfs_bui_item_recover(
>  	if (error)
>  		goto err_inode;
>  
> +	error = xfs_qm_dqattach_locked(ip, false);
> +	if (error)
> +		goto err_inode;
> +
>  	if (VFS_I(ip)->i_nlink == 0)
>  		xfs_iflags_set(ip, XFS_IRECOVERY);

Looks good now.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/2] xfs: free the intent item when allocating recovery transaction fails

[Dave Chinner]

On Thu, Sep 17, 2020 at 12:01:35AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The recovery functions of all four log intent items fail to free the
> intent item if the transaction allocation fails.  Fix this.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Good catch :)

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 2/2] xfs: attach inode to dquot in xfs_bui_item_recover

[Christoph Hellwig]

On Thu, Sep 17, 2020 at 12:01:03AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In the bmap intent item recovery code, we must be careful to attach the
> inode to its dquots (if quotas are enabled) so that a change in the
> shape of the bmap btree doesn't cause the quota counters to be
> incorrect.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/2] xfs: free the intent item when allocating recovery transaction fails

[Christoph Hellwig]

On Thu, Sep 17, 2020 at 12:01:35AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The recovery functions of all four log intent items fail to free the
> intent item if the transaction allocation fails.  Fix this.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_bmap_item.c     |    5 ++++-
>  fs/xfs/xfs_extfree_item.c  |    5 ++++-
>  fs/xfs/xfs_refcount_item.c |    5 ++++-
>  fs/xfs/xfs_rmap_item.c     |    5 ++++-
>  4 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 2b1cf3ed8172..85d18cd708ba 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -484,8 +484,11 @@ xfs_bui_item_recover(
>  
>  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate,
>  			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK), 0, 0, &tp);
> -	if (error)
> +	if (error) {
> +		xfs_bui_release(buip);
>  		return error;
> +	}

This should probably use a common label instead of duplicating the
release three times.

That beind said I don't think we need either the existing or newly
added calls.  At the end of log recovery we always call
xlog_recover_cancel_intents, which will release all intents remaining
in the AIL.

Re: [PATCH 1/2] xfs: log new intent items created as part of finishing recovered intent items

[Christoph Hellwig]

On Wed, Sep 16, 2020 at 08:28:49PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> During a code inspection, I found a serious bug in the log intent item
> recovery code when an intent item cannot complete all the work and
> decides to requeue itself to get that done.  When this happens, the
> item recovery creates a new incore deferred op representing the
> remaining work and attaches it to the transaction that it allocated.  At
> the end of _item_recover, it moves the entire chain of deferred ops to
> the dummy parent_tp that xlog_recover_process_intents passed to it, but
> fail to log a new intent item for the remaining work before committing
> the transaction for the single unit of work.
> 
> xlog_finish_defer_ops logs those new intent items once recovery has
> finished dealing with the intent items that it recovered, but this isn't
> sufficient.  If the log is forced to disk after a recovered log item
> decides to requeue itself and the system goes down before we call
> xlog_finish_defer_ops, the second log recovery will never see the new
> intent item and therefore has no idea that there was more work to do.
> It will finish recovery leaving the filesystem in a corrupted state.
> 
> The same logic applies to /any/ deferred ops added during intent item
> recovery, not just the one handling the remaining work.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

I wonder how we could come up with a reliable reproducer for this,
though..

Re: [PATCH 1/2] xfs: log new intent items created as part of finishing recovered intent items

[Darrick J. Wong]

On Thu, Sep 17, 2020 at 10:07:42AM +0100, Christoph Hellwig wrote:
> On Wed, Sep 16, 2020 at 08:28:49PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > During a code inspection, I found a serious bug in the log intent item
> > recovery code when an intent item cannot complete all the work and
> > decides to requeue itself to get that done.  When this happens, the
> > item recovery creates a new incore deferred op representing the
> > remaining work and attaches it to the transaction that it allocated.  At
> > the end of _item_recover, it moves the entire chain of deferred ops to
> > the dummy parent_tp that xlog_recover_process_intents passed to it, but
> > fail to log a new intent item for the remaining work before committing
> > the transaction for the single unit of work.
> > 
> > xlog_finish_defer_ops logs those new intent items once recovery has
> > finished dealing with the intent items that it recovered, but this isn't
> > sufficient.  If the log is forced to disk after a recovered log item
> > decides to requeue itself and the system goes down before we call
> > xlog_finish_defer_ops, the second log recovery will never see the new
> > intent item and therefore has no idea that there was more work to do.
> > It will finish recovery leaving the filesystem in a corrupted state.
> > 
> > The same logic applies to /any/ deferred ops added during intent item
> > recovery, not just the one handling the remaining work.
> 
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> I wonder how we could come up with a reliable reproducer for this,
> though..

Yeah, I've never actually seen this trip in practice.  I suppose we
could add an error injection point to force the log and bail out midway
through recovery, but that won't help much on unfixed kernels.

--D

Re: [PATCH 3/2] xfs: free the intent item when allocating recovery transaction fails

[Darrick J. Wong]

On Thu, Sep 17, 2020 at 10:06:45AM +0100, Christoph Hellwig wrote:
> On Thu, Sep 17, 2020 at 12:01:35AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > The recovery functions of all four log intent items fail to free the
> > intent item if the transaction allocation fails.  Fix this.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_bmap_item.c     |    5 ++++-
> >  fs/xfs/xfs_extfree_item.c  |    5 ++++-
> >  fs/xfs/xfs_refcount_item.c |    5 ++++-
> >  fs/xfs/xfs_rmap_item.c     |    5 ++++-
> >  4 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> > index 2b1cf3ed8172..85d18cd708ba 100644
> > --- a/fs/xfs/xfs_bmap_item.c
> > +++ b/fs/xfs/xfs_bmap_item.c
> > @@ -484,8 +484,11 @@ xfs_bui_item_recover(
> >  
> >  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate,
> >  			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK), 0, 0, &tp);
> > -	if (error)
> > +	if (error) {
> > +		xfs_bui_release(buip);
> >  		return error;
> > +	}
> 
> This should probably use a common label instead of duplicating the
> release three times.
> 
> That beind said I don't think we need either the existing or newly
> added calls.  At the end of log recovery we always call
> xlog_recover_cancel_intents, which will release all intents remaining
> in the AIL.

You know, that's right, recovery will clean up all the intents for us if
we fail.  Ok, new patch. :)

--D

[PATCH v2 3/2] xfs: fix simple problems with log intent recovery

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Nowadays, log recovery will call ->release on the recovered intent items
if recovery fails.  Therefore, it's redundant to release them from
inside the ->recover functions when they're about to return an error.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: log recovery already cancels the intents for us, so don't free them
---
 fs/xfs/xfs_bmap_item.c     |   12 ++----------
 fs/xfs/xfs_extfree_item.c  |    8 +-------
 fs/xfs/xfs_refcount_item.c |    8 +-------
 fs/xfs/xfs_rmap_item.c     |    8 +-------
 4 files changed, 5 insertions(+), 31 deletions(-)

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 2b1cf3ed8172..b04ebcd78316 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -444,10 +444,8 @@ xfs_bui_item_recover(
 	int				error = 0;
 
 	/* Only one mapping operation per BUI... */
-	if (buip->bui_format.bui_nextents != XFS_BUI_MAX_FAST_EXTENTS) {
-		xfs_bui_release(buip);
+	if (buip->bui_format.bui_nextents != XFS_BUI_MAX_FAST_EXTENTS)
 		return -EFSCORRUPTED;
-	}
 
 	/*
 	 * First check the validity of the extent described by the
@@ -473,14 +471,8 @@ xfs_bui_item_recover(
 	    startblock_fsb >= mp->m_sb.sb_dblocks ||
 	    bmap->me_len >= mp->m_sb.sb_agblocks ||
 	    inode_fsb >= mp->m_sb.sb_dblocks ||
-	    (bmap->me_flags & ~XFS_BMAP_EXTENT_FLAGS)) {
-		/*
-		 * This will pull the BUI from the AIL and
-		 * free the memory associated with it.
-		 */
-		xfs_bui_release(buip);
+	    (bmap->me_flags & ~XFS_BMAP_EXTENT_FLAGS))
 		return -EFSCORRUPTED;
-	}
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate,
 			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK), 0, 0, &tp);
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 6cb8cd11072a..9093d2e7afdf 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -608,14 +608,8 @@ xfs_efi_item_recover(
 		if (startblock_fsb == 0 ||
 		    extp->ext_len == 0 ||
 		    startblock_fsb >= mp->m_sb.sb_dblocks ||
-		    extp->ext_len >= mp->m_sb.sb_agblocks) {
-			/*
-			 * This will pull the EFI from the AIL and
-			 * free the memory associated with it.
-			 */
-			xfs_efi_release(efip);
+		    extp->ext_len >= mp->m_sb.sb_agblocks)
 			return -EFSCORRUPTED;
-		}
 	}
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 492d80a0b406..3e34b7662361 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -467,14 +467,8 @@ xfs_cui_item_recover(
 		    refc->pe_len == 0 ||
 		    startblock_fsb >= mp->m_sb.sb_dblocks ||
 		    refc->pe_len >= mp->m_sb.sb_agblocks ||
-		    (refc->pe_flags & ~XFS_REFCOUNT_EXTENT_FLAGS)) {
-			/*
-			 * This will pull the CUI from the AIL and
-			 * free the memory associated with it.
-			 */
-			xfs_cui_release(cuip);
+		    (refc->pe_flags & ~XFS_REFCOUNT_EXTENT_FLAGS))
 			return -EFSCORRUPTED;
-		}
 	}
 
 	/*
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index dc5b0753cd51..e38ec5d736be 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -511,14 +511,8 @@ xfs_rui_item_recover(
 		    rmap->me_len == 0 ||
 		    startblock_fsb >= mp->m_sb.sb_dblocks ||
 		    rmap->me_len >= mp->m_sb.sb_agblocks ||
-		    (rmap->me_flags & ~XFS_RMAP_EXTENT_FLAGS)) {
-			/*
-			 * This will pull the RUI from the AIL and
-			 * free the memory associated with it.
-			 */
-			xfs_rui_release(ruip);
+		    (rmap->me_flags & ~XFS_RMAP_EXTENT_FLAGS))
 			return -EFSCORRUPTED;
-		}
 	}
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate,

[PATCH v3 3/2] xfs: don't release log intent items when recovery fails

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Nowadays, log recovery will call ->release on the recovered intent items
if recovery fails.  Therefore, it's redundant to release them from
inside the ->recover functions when they're about to return an error.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v3: fix subject line
v2: log recovery frees unfinished intent items on failure, so remove
release calls
---
 fs/xfs/xfs_bmap_item.c     |   12 ++----------
 fs/xfs/xfs_extfree_item.c  |    8 +-------
 fs/xfs/xfs_refcount_item.c |    8 +-------
 fs/xfs/xfs_rmap_item.c     |    8 +-------
 4 files changed, 5 insertions(+), 31 deletions(-)

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 2b1cf3ed8172..b04ebcd78316 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -444,10 +444,8 @@ xfs_bui_item_recover(
 	int				error = 0;
 
 	/* Only one mapping operation per BUI... */
-	if (buip->bui_format.bui_nextents != XFS_BUI_MAX_FAST_EXTENTS) {
-		xfs_bui_release(buip);
+	if (buip->bui_format.bui_nextents != XFS_BUI_MAX_FAST_EXTENTS)
 		return -EFSCORRUPTED;
-	}
 
 	/*
 	 * First check the validity of the extent described by the
@@ -473,14 +471,8 @@ xfs_bui_item_recover(
 	    startblock_fsb >= mp->m_sb.sb_dblocks ||
 	    bmap->me_len >= mp->m_sb.sb_agblocks ||
 	    inode_fsb >= mp->m_sb.sb_dblocks ||
-	    (bmap->me_flags & ~XFS_BMAP_EXTENT_FLAGS)) {
-		/*
-		 * This will pull the BUI from the AIL and
-		 * free the memory associated with it.
-		 */
-		xfs_bui_release(buip);
+	    (bmap->me_flags & ~XFS_BMAP_EXTENT_FLAGS))
 		return -EFSCORRUPTED;
-	}
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate,
 			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK), 0, 0, &tp);
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 6cb8cd11072a..9093d2e7afdf 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -608,14 +608,8 @@ xfs_efi_item_recover(
 		if (startblock_fsb == 0 ||
 		    extp->ext_len == 0 ||
 		    startblock_fsb >= mp->m_sb.sb_dblocks ||
-		    extp->ext_len >= mp->m_sb.sb_agblocks) {
-			/*
-			 * This will pull the EFI from the AIL and
-			 * free the memory associated with it.
-			 */
-			xfs_efi_release(efip);
+		    extp->ext_len >= mp->m_sb.sb_agblocks)
 			return -EFSCORRUPTED;
-		}
 	}
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 492d80a0b406..3e34b7662361 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -467,14 +467,8 @@ xfs_cui_item_recover(
 		    refc->pe_len == 0 ||
 		    startblock_fsb >= mp->m_sb.sb_dblocks ||
 		    refc->pe_len >= mp->m_sb.sb_agblocks ||
-		    (refc->pe_flags & ~XFS_REFCOUNT_EXTENT_FLAGS)) {
-			/*
-			 * This will pull the CUI from the AIL and
-			 * free the memory associated with it.
-			 */
-			xfs_cui_release(cuip);
+		    (refc->pe_flags & ~XFS_REFCOUNT_EXTENT_FLAGS))
 			return -EFSCORRUPTED;
-		}
 	}
 
 	/*
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index dc5b0753cd51..e38ec5d736be 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -511,14 +511,8 @@ xfs_rui_item_recover(
 		    rmap->me_len == 0 ||
 		    startblock_fsb >= mp->m_sb.sb_dblocks ||
 		    rmap->me_len >= mp->m_sb.sb_agblocks ||
-		    (rmap->me_flags & ~XFS_RMAP_EXTENT_FLAGS)) {
-			/*
-			 * This will pull the RUI from the AIL and
-			 * free the memory associated with it.
-			 */
-			xfs_rui_release(ruip);
+		    (rmap->me_flags & ~XFS_RMAP_EXTENT_FLAGS))
 			return -EFSCORRUPTED;
-		}
 	}
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate,

Re: [PATCH v3 3/2] xfs: don't release log intent items when recovery fails

[Christoph Hellwig]

On Thu, Sep 17, 2020 at 07:19:40PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Nowadays, log recovery will call ->release on the recovered intent items
> if recovery fails.  Therefore, it's redundant to release them from
> inside the ->recover functions when they're about to return an error.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v3: fix subject line
> v2: log recovery frees unfinished intent items on failure, so remove
> release calls

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

I'm actually pretty sure I have the same patch lingering in one my
unfinished branches somewhere..

Re: [PATCH v3 3/2] xfs: don't release log intent items when recovery fails

[Dave Chinner]

On Thu, Sep 17, 2020 at 07:19:40PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Nowadays, log recovery will call ->release on the recovered intent items
> if recovery fails.  Therefore, it's redundant to release them from
> inside the ->recover functions when they're about to return an error.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v3: fix subject line
> v2: log recovery frees unfinished intent items on failure, so remove
> release calls
> ---
>  fs/xfs/xfs_bmap_item.c     |   12 ++----------
>  fs/xfs/xfs_extfree_item.c  |    8 +-------
>  fs/xfs/xfs_refcount_item.c |    8 +-------
>  fs/xfs/xfs_rmap_item.c     |    8 +-------
>  4 files changed, 5 insertions(+), 31 deletions(-)

looks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com