diff mbox series

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

Message ID 20200917070135.GV7955@magnolia
State New
Headers show
Series xfs: fix simple problems with log intent recovery | expand

Commit Message

Darrick J. Wong Sept. 17, 2020, 7:01 a.m. UTC
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(-)

Comments

Dave Chinner Sept. 17, 2020, 8:05 a.m. UTC | #1
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>
Christoph Hellwig Sept. 17, 2020, 9:06 a.m. UTC | #2
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.
Darrick J. Wong Sept. 18, 2020, 1:48 a.m. UTC | #3
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
diff mbox series

Patch

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++) {