diff mbox

[01/21] xfs: don't assume a left rmap when allocating a new rmap

Message ID 152986821631.3155.13981921703707802906.stgit@magnolia (mailing list archive)
State Accepted
Headers show

Commit Message

Darrick J. Wong June 24, 2018, 7:23 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

The original rmap code assumed that there would always be at least one
rmap in the rmapbt (the AG sb/agf/agi) and so errored out if it didn't
find one.  This assumption isn't true for the rmapbt repair function
(and it won't be true for realtime rmap either), so remove the check and
just deal with the situation.

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



--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Dave Chinner June 27, 2018, 12:54 a.m. UTC | #1 
On Sun, Jun 24, 2018 at 12:23:36PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The original rmap code assumed that there would always be at least one
> rmap in the rmapbt (the AG sb/agf/agi) and so errored out if it didn't
> find one.  This assumption isn't true for the rmapbt repair function
> (and it won't be true for realtime rmap either), so remove the check and
> just deal with the situation.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks reasonable.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
Allison Henderson June 28, 2018, 9:11 p.m. UTC | #2 
On 06/24/2018 12:23 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The original rmap code assumed that there would always be at least one
> rmap in the rmapbt (the AG sb/agf/agi) and so errored out if it didn't
> find one.  This assumption isn't true for the rmapbt repair function
> (and it won't be true for realtime rmap either), so remove the check and
> just deal with the situation.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/libxfs/xfs_rmap.c |   24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
> index d4460b0d2d81..8b2a2f81d110 100644
> --- a/fs/xfs/libxfs/xfs_rmap.c
> +++ b/fs/xfs/libxfs/xfs_rmap.c
> @@ -753,19 +753,19 @@ xfs_rmap_map(
>   			&have_lt);
>   	if (error)
>   		goto out_error;
> -	XFS_WANT_CORRUPTED_GOTO(mp, have_lt == 1, out_error);
> -
> -	error = xfs_rmap_get_rec(cur, &ltrec, &have_lt);
> -	if (error)
> -		goto out_error;
> -	XFS_WANT_CORRUPTED_GOTO(mp, have_lt == 1, out_error);
> -	trace_xfs_rmap_lookup_le_range_result(cur->bc_mp,
> -			cur->bc_private.a.agno, ltrec.rm_startblock,
> -			ltrec.rm_blockcount, ltrec.rm_owner,
> -			ltrec.rm_offset, ltrec.rm_flags);
> +	if (have_lt) {
> +		error = xfs_rmap_get_rec(cur, &ltrec, &have_lt);
> +		if (error)
> +			goto out_error;
> +		XFS_WANT_CORRUPTED_GOTO(mp, have_lt == 1, out_error);
> +		trace_xfs_rmap_lookup_le_range_result(cur->bc_mp,
> +				cur->bc_private.a.agno, ltrec.rm_startblock,
> +				ltrec.rm_blockcount, ltrec.rm_owner,
> +				ltrec.rm_offset, ltrec.rm_flags);
>   
> -	if (!xfs_rmap_is_mergeable(&ltrec, owner, flags))
> -		have_lt = 0;
> +		if (!xfs_rmap_is_mergeable(&ltrec, owner, flags))
> +			have_lt = 0;
> +	}
>   
>   	XFS_WANT_CORRUPTED_GOTO(mp,
>   		have_lt == 0 ||
> 

Alrighty, looks ok after some digging around. I'm still a little puzzled 
as to why the original code raised the assert without checking to see 
whats on the other side of the cursor?  Assuming the error condition
was supposed to be the case when the tree was empty.  In any case, it 
looks correct now.

Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=Q2PdVMOGp7_huNLFbP6xty0mgocZk65leUyLVRvSsSY&s=6-FSoklyIhTEtg811gAG43N9-7Z-sYFsm7zv33EadgQ&e=
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong June 29, 2018, 2:39 p.m. UTC | #3 
On Thu, Jun 28, 2018 at 02:11:38PM -0700, Allison Henderson wrote:
> On 06/24/2018 12:23 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > The original rmap code assumed that there would always be at least one
> > rmap in the rmapbt (the AG sb/agf/agi) and so errored out if it didn't
> > find one.  This assumption isn't true for the rmapbt repair function
> > (and it won't be true for realtime rmap either), so remove the check and
> > just deal with the situation.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >   fs/xfs/libxfs/xfs_rmap.c |   24 ++++++++++++------------
> >   1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
> > index d4460b0d2d81..8b2a2f81d110 100644
> > --- a/fs/xfs/libxfs/xfs_rmap.c
> > +++ b/fs/xfs/libxfs/xfs_rmap.c
> > @@ -753,19 +753,19 @@ xfs_rmap_map(
> >   			&have_lt);
> >   	if (error)
> >   		goto out_error;
> > -	XFS_WANT_CORRUPTED_GOTO(mp, have_lt == 1, out_error);
> > -
> > -	error = xfs_rmap_get_rec(cur, &ltrec, &have_lt);
> > -	if (error)
> > -		goto out_error;
> > -	XFS_WANT_CORRUPTED_GOTO(mp, have_lt == 1, out_error);
> > -	trace_xfs_rmap_lookup_le_range_result(cur->bc_mp,
> > -			cur->bc_private.a.agno, ltrec.rm_startblock,
> > -			ltrec.rm_blockcount, ltrec.rm_owner,
> > -			ltrec.rm_offset, ltrec.rm_flags);
> > +	if (have_lt) {
> > +		error = xfs_rmap_get_rec(cur, &ltrec, &have_lt);
> > +		if (error)
> > +			goto out_error;
> > +		XFS_WANT_CORRUPTED_GOTO(mp, have_lt == 1, out_error);
> > +		trace_xfs_rmap_lookup_le_range_result(cur->bc_mp,
> > +				cur->bc_private.a.agno, ltrec.rm_startblock,
> > +				ltrec.rm_blockcount, ltrec.rm_owner,
> > +				ltrec.rm_offset, ltrec.rm_flags);
> > -	if (!xfs_rmap_is_mergeable(&ltrec, owner, flags))
> > -		have_lt = 0;
> > +		if (!xfs_rmap_is_mergeable(&ltrec, owner, flags))
> > +			have_lt = 0;
> > +	}
> >   	XFS_WANT_CORRUPTED_GOTO(mp,
> >   		have_lt == 0 ||
> > 
> 
> Alrighty, looks ok after some digging around. I'm still a little puzzled as
> to why the original code raised the assert without checking to see whats on
> the other side of the cursor?  Assuming the error condition
> was supposed to be the case when the tree was empty.  In any case, it looks
> correct now.

At the time (~2014?) I don't think either Dave or I were thinking about
rmapbt being extended into the realtime device, so we thought that
assumption was a reasonable one to make.  That was, of course, long
before I got far enough along in designing online check to realize that
"hey, maybe we should be able to rebuild things from scratch too"... :)

Anyway, thank you both for the review.

--D

> Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=Q2PdVMOGp7_huNLFbP6xty0mgocZk65leUyLVRvSsSY&s=6-FSoklyIhTEtg811gAG43N9-7Z-sYFsm7zv33EadgQ&e=
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
index d4460b0d2d81..8b2a2f81d110 100644
--- a/fs/xfs/libxfs/xfs_rmap.c
+++ b/fs/xfs/libxfs/xfs_rmap.c
@@ -753,19 +753,19 @@  xfs_rmap_map(
 			&have_lt);
 	if (error)
 		goto out_error;
-	XFS_WANT_CORRUPTED_GOTO(mp, have_lt == 1, out_error);
-
-	error = xfs_rmap_get_rec(cur, &ltrec, &have_lt);
-	if (error)
-		goto out_error;
-	XFS_WANT_CORRUPTED_GOTO(mp, have_lt == 1, out_error);
-	trace_xfs_rmap_lookup_le_range_result(cur->bc_mp,
-			cur->bc_private.a.agno, ltrec.rm_startblock,
-			ltrec.rm_blockcount, ltrec.rm_owner,
-			ltrec.rm_offset, ltrec.rm_flags);
+	if (have_lt) {
+		error = xfs_rmap_get_rec(cur, &ltrec, &have_lt);
+		if (error)
+			goto out_error;
+		XFS_WANT_CORRUPTED_GOTO(mp, have_lt == 1, out_error);
+		trace_xfs_rmap_lookup_le_range_result(cur->bc_mp,
+				cur->bc_private.a.agno, ltrec.rm_startblock,
+				ltrec.rm_blockcount, ltrec.rm_owner,
+				ltrec.rm_offset, ltrec.rm_flags);
 
-	if (!xfs_rmap_is_mergeable(&ltrec, owner, flags))
-		have_lt = 0;
+		if (!xfs_rmap_is_mergeable(&ltrec, owner, flags))
+			have_lt = 0;
+	}
 
 	XFS_WANT_CORRUPTED_GOTO(mp,
 		have_lt == 0 ||