diff mbox series

[2/3] xfs: clean up the metadata validation in xfs_swap_extent_rmap

Message ID 158864102271.182577.2059355876586003107.stgit@magnolia (mailing list archive)
State New, archived
Headers show
Series xfs: random SWAPEXT fixes | expand

Commit Message

Darrick J. Wong May 5, 2020, 1:10 a.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Bail out if there's something not right with either file's fork
mappings.

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

Comments

Christoph Hellwig May 6, 2020, 2:56 p.m. UTC | #1 
On Mon, May 04, 2020 at 06:10:22PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Bail out if there's something not right with either file's fork
> mappings.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_bmap_util.c |   31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index cc23a3e23e2d..2774939e176d 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1342,8 +1342,16 @@ xfs_swap_extent_rmap(
>  				&nimaps, 0);
>  		if (error)
>  			goto out;
> -		ASSERT(nimaps == 1);
> -		ASSERT(tirec.br_startblock != DELAYSTARTBLOCK);
> +		if (nimaps != 1 || tirec.br_startblock == DELAYSTARTBLOCK) {
> +			/*
> +			 * We should never get no mapping or a delalloc extent
> +			 * since the donor file should have been flushed by the
> +			 * caller.
> +			 */
> +			ASSERT(0);
> +			error = -EINVAL;
> +			goto out;
> +		}

I'm not even sure the !nimaps case still exists.  Usually this will
return a hole extent, which we don't seem to handle here?

In general I think this code would be improved quite a bit by using
xfs_iext_lookup_extent instead of xfs_bmapi_read.

Same for the second hunk.
Darrick J. Wong May 6, 2020, 4:45 p.m. UTC | #2 
On Wed, May 06, 2020 at 07:56:33AM -0700, Christoph Hellwig wrote:
> On Mon, May 04, 2020 at 06:10:22PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Bail out if there's something not right with either file's fork
> > mappings.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_bmap_util.c |   31 +++++++++++++++++++++++--------
> >  1 file changed, 23 insertions(+), 8 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index cc23a3e23e2d..2774939e176d 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -1342,8 +1342,16 @@ xfs_swap_extent_rmap(
> >  				&nimaps, 0);
> >  		if (error)
> >  			goto out;
> > -		ASSERT(nimaps == 1);
> > -		ASSERT(tirec.br_startblock != DELAYSTARTBLOCK);
> > +		if (nimaps != 1 || tirec.br_startblock == DELAYSTARTBLOCK) {
> > +			/*
> > +			 * We should never get no mapping or a delalloc extent
> > +			 * since the donor file should have been flushed by the
> > +			 * caller.
> > +			 */
> > +			ASSERT(0);
> > +			error = -EINVAL;
> > +			goto out;
> > +		}
> 
> I'm not even sure the !nimaps case still exists.  Usually this will
> return a hole extent, which we don't seem to handle here?

xfs_bmap_unmap_extent and xfs_bmap_map_extent are NOPs if you pass
them a hole.

But yeah, the !nimaps case doesn't seem to exist anymore.

> In general I think this code would be improved quite a bit by using
> xfs_iext_lookup_extent instead of xfs_bmapi_read.
> 
> Same for the second hunk.

I'd rather not make major changes to this code because my long range
plan is to replace all this with a much cleaner implementation in the
atomic file update series[1].  This patchset bandages the wtfs that I
found while writing that series, projecting that review of atomic file
updates is going to take a while...

--D

[1] https://lwn.net/ml/linux-fsdevel/158812825316.168506.932540609191384366.stgit@magnolia/
diff mbox series

Patch

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index cc23a3e23e2d..2774939e176d 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1342,8 +1342,16 @@  xfs_swap_extent_rmap(
 				&nimaps, 0);
 		if (error)
 			goto out;
-		ASSERT(nimaps == 1);
-		ASSERT(tirec.br_startblock != DELAYSTARTBLOCK);
+		if (nimaps != 1 || tirec.br_startblock == DELAYSTARTBLOCK) {
+			/*
+			 * We should never get no mapping or a delalloc extent
+			 * since the donor file should have been flushed by the
+			 * caller.
+			 */
+			ASSERT(0);
+			error = -EINVAL;
+			goto out;
+		}
 
 		trace_xfs_swap_extent_rmap_remap(tip, &tirec);
 		ilen = tirec.br_blockcount;
@@ -1360,8 +1368,17 @@  xfs_swap_extent_rmap(
 					&nimaps, 0);
 			if (error)
 				goto out;
-			ASSERT(nimaps == 1);
-			ASSERT(tirec.br_startoff == irec.br_startoff);
+			if (nimaps != 1 ||
+			    tirec.br_startoff != irec.br_startoff) {
+				/*
+				 * We should never get no mapping or a mapping
+				 * for another offset, but bail out if that
+				 * ever does.
+				 */
+				ASSERT(0);
+				error = -EFSCORRUPTED;
+				goto out;
+			}
 			trace_xfs_swap_extent_rmap_remap_piece(ip, &irec);
 
 			/* Trim the extent. */
@@ -1400,11 +1417,9 @@  xfs_swap_extent_rmap(
 		offset_fsb += ilen;
 	}
 
-	tip->i_d.di_flags2 = tip_flags2;
-	return 0;
-
 out:
-	trace_xfs_swap_extent_rmap_error(ip, error, _RET_IP_);
+	if (error)
+		trace_xfs_swap_extent_rmap_error(ip, error, _RET_IP_);
 	tip->i_d.di_flags2 = tip_flags2;
 	return error;
 }