delalloc and reflink fixes & tweaks V3

delalloc and reflink fixes & tweaks V3

[Christoph Hellwig]

Hi all,

this series contains the basic reflink and delalloc fixups intended
for 4.20.  All the bits that need more work are left out for now.

Changes since v2:
  - add a new comment
  - drop the patch to use unwritten extents for delalloc by default,
    it didn't actually do what it was intended to do, because there
    was no test to check for that, and actually changing that caused
    a few regressions that need more time looking into
  - add two new unwritten extent related cleanup patches

Changes since v1:
  - drop various patches
  - print the inode number for dangling delalloc extents

[PATCH 1/8] xfs: remove XFS_IO_INVALID

[Christoph Hellwig]

The invalid state isn't any different from a hole, so merge the two
states.  Use the more descriptive hole name, but keep it as the first
value of the enum to catch uninitialized fields.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_aops.c |  4 ++--
 fs/xfs/xfs_aops.h | 14 ++++++--------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 49f5f5896a43..338b9d9984e0 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -917,7 +917,7 @@ xfs_vm_writepage(
 	struct writeback_control *wbc)
 {
 	struct xfs_writepage_ctx wpc = {
-		.io_type = XFS_IO_INVALID,
+		.io_type = XFS_IO_HOLE,
 	};
 	int			ret;
 
@@ -933,7 +933,7 @@ xfs_vm_writepages(
 	struct writeback_control *wbc)
 {
 	struct xfs_writepage_ctx wpc = {
-		.io_type = XFS_IO_INVALID,
+		.io_type = XFS_IO_HOLE,
 	};
 	int			ret;
 
diff --git a/fs/xfs/xfs_aops.h b/fs/xfs/xfs_aops.h
index 9af867951a10..494b4338446e 100644
--- a/fs/xfs/xfs_aops.h
+++ b/fs/xfs/xfs_aops.h
@@ -12,21 +12,19 @@ extern struct bio_set xfs_ioend_bioset;
  * Types of I/O for bmap clustering and I/O completion tracking.
  */
 enum {
-	XFS_IO_INVALID,		/* initial state */
+	XFS_IO_HOLE,		/* covers region without any block allocation */
 	XFS_IO_DELALLOC,	/* covers delalloc region */
 	XFS_IO_UNWRITTEN,	/* covers allocated but uninitialized data */
 	XFS_IO_OVERWRITE,	/* covers already allocated extent */
 	XFS_IO_COW,		/* covers copy-on-write extent */
-	XFS_IO_HOLE,		/* covers region without any block allocation */
 };
 
 #define XFS_IO_TYPES \
-	{ XFS_IO_INVALID,		"invalid" }, \
-	{ XFS_IO_DELALLOC,		"delalloc" }, \
-	{ XFS_IO_UNWRITTEN,		"unwritten" }, \
-	{ XFS_IO_OVERWRITE,		"overwrite" }, \
-	{ XFS_IO_COW,			"CoW" }, \
-	{ XFS_IO_HOLE,			"hole" }
+	{ XFS_IO_HOLE,			"hole" },	\
+	{ XFS_IO_DELALLOC,		"delalloc" },	\
+	{ XFS_IO_UNWRITTEN,		"unwritten" },	\
+	{ XFS_IO_OVERWRITE,		"overwrite" },	\
+	{ XFS_IO_COW,			"CoW" }
 
 /*
  * Structure for buffered I/O completions.
-- 
2.19.0

[PATCH 2/8] xfs: remove suport for filesystems without unwritten extent flag

[Christoph Hellwig]

The option to enable unwritten extents was made default in 2003, removed
from mkfs in 2007, and cannot be disabled in v5.  We also rely on it for
a lot of common functionality, so filesystems without it will run a
completely untested and buggy code path.  Enabling the support also is
a simple bit flip using xfs_db, so legacy file systems can still be
brought forward.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c   | 21 ++++++----------
 fs/xfs/libxfs/xfs_format.h |  8 ++----
 fs/xfs/libxfs/xfs_sb.c     |  5 ++--
 fs/xfs/scrub/scrub.c       | 13 ----------
 fs/xfs/xfs_bmap_util.c     | 51 +-------------------------------------
 fs/xfs/xfs_ioctl.c         |  8 ------
 6 files changed, 12 insertions(+), 94 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index a47670332326..da6b768664e3 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4081,8 +4081,7 @@ xfs_bmapi_allocate(
 	 * extents to real extents when we're about to write the data.
 	 */
 	if ((!bma->wasdel || (bma->flags & XFS_BMAPI_COWFORK)) &&
-	    (bma->flags & XFS_BMAPI_PREALLOC) &&
-	    xfs_sb_version_hasextflgbit(&mp->m_sb))
+	    (bma->flags & XFS_BMAPI_PREALLOC))
 		bma->got.br_state = XFS_EXT_UNWRITTEN;
 
 	if (bma->wasdel)
@@ -5245,8 +5244,7 @@ __xfs_bunmapi(
 			 * unmapping part of it.  But we can't really
 			 * get rid of part of a realtime extent.
 			 */
-			if (del.br_state == XFS_EXT_UNWRITTEN ||
-			    !xfs_sb_version_hasextflgbit(&mp->m_sb)) {
+			if (del.br_state == XFS_EXT_UNWRITTEN) {
 				/*
 				 * This piece is unwritten, or we're not
 				 * using unwritten extents.  Skip over it.
@@ -5296,10 +5294,9 @@ __xfs_bunmapi(
 				del.br_blockcount -= mod;
 				del.br_startoff += mod;
 				del.br_startblock += mod;
-			} else if ((del.br_startoff == start &&
-				    (del.br_state == XFS_EXT_UNWRITTEN ||
-				     tp->t_blk_res == 0)) ||
-				   !xfs_sb_version_hasextflgbit(&mp->m_sb)) {
+			} else if (del.br_startoff == start &&
+				   (del.br_state == XFS_EXT_UNWRITTEN ||
+				    tp->t_blk_res == 0)) {
 				/*
 				 * Can't make it unwritten.  There isn't
 				 * a full extent here so just skip it.
@@ -6114,11 +6111,7 @@ xfs_bmap_validate_extent(
 		    XFS_FSB_TO_AGNO(mp, endfsb))
 			return __this_address;
 	}
-	if (irec->br_state != XFS_EXT_NORM) {
-		if (whichfork != XFS_DATA_FORK)
-			return __this_address;
-		if (!xfs_sb_version_hasextflgbit(&mp->m_sb))
-			return __this_address;
-	}
+	if (irec->br_state != XFS_EXT_NORM && whichfork != XFS_DATA_FORK)
+		return __this_address;
 	return NULL;
 }
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index afbe336600e1..9995d5ae380b 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -287,6 +287,8 @@ static inline bool xfs_sb_good_v4_features(struct xfs_sb *sbp)
 {
 	if (!(sbp->sb_versionnum & XFS_SB_VERSION_DIRV2BIT))
 		return false;
+	if (!(sbp->sb_versionnum & XFS_SB_VERSION_EXTFLGBIT))
+		return false;
 
 	/* check for unknown features in the fs */
 	if ((sbp->sb_versionnum & ~XFS_SB_VERSION_OKBITS) ||
@@ -357,12 +359,6 @@ static inline bool xfs_sb_version_haslogv2(struct xfs_sb *sbp)
 	       (sbp->sb_versionnum & XFS_SB_VERSION_LOGV2BIT);
 }
 
-static inline bool xfs_sb_version_hasextflgbit(struct xfs_sb *sbp)
-{
-	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 ||
-	       (sbp->sb_versionnum & XFS_SB_VERSION_EXTFLGBIT);
-}
-
 static inline bool xfs_sb_version_hassector(struct xfs_sb *sbp)
 {
 	return (sbp->sb_versionnum & XFS_SB_VERSION_SECTORBIT);
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 081f46e30556..b5a82acd7dfe 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -1115,7 +1115,8 @@ xfs_fs_geometry(
 
 	geo->version = XFS_FSOP_GEOM_VERSION;
 	geo->flags = XFS_FSOP_GEOM_FLAGS_NLINK |
-		     XFS_FSOP_GEOM_FLAGS_DIRV2;
+		     XFS_FSOP_GEOM_FLAGS_DIRV2 |
+		     XFS_FSOP_GEOM_FLAGS_EXTFLG;
 	if (xfs_sb_version_hasattr(sbp))
 		geo->flags |= XFS_FSOP_GEOM_FLAGS_ATTR;
 	if (xfs_sb_version_hasquota(sbp))
@@ -1124,8 +1125,6 @@ xfs_fs_geometry(
 		geo->flags |= XFS_FSOP_GEOM_FLAGS_IALIGN;
 	if (xfs_sb_version_hasdalign(sbp))
 		geo->flags |= XFS_FSOP_GEOM_FLAGS_DALIGN;
-	if (xfs_sb_version_hasextflgbit(sbp))
-		geo->flags |= XFS_FSOP_GEOM_FLAGS_EXTFLG;
 	if (xfs_sb_version_hassector(sbp))
 		geo->flags |= XFS_FSOP_GEOM_FLAGS_SECTOR;
 	if (xfs_sb_version_hasasciici(sbp))
diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index 4bfae1e61d30..1b2344d00525 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -412,19 +412,6 @@ xchk_validate_inputs(
 		goto out;
 	}
 
-	error = -EOPNOTSUPP;
-	/*
-	 * We won't scrub any filesystem that doesn't have the ability
-	 * to record unwritten extents.  The option was made default in
-	 * 2003, removed from mkfs in 2007, and cannot be disabled in
-	 * v5, so if we find a filesystem without this flag it's either
-	 * really old or totally unsupported.  Avoid it either way.
-	 * We also don't support v1-v3 filesystems, which aren't
-	 * mountable.
-	 */
-	if (!xfs_sb_version_hasextflgbit(&mp->m_sb))
-		goto out;
-
 	/*
 	 * We only want to repair read-write v5+ filesystems.  Defer the check
 	 * for ops->repair until after our scrub confirms that we need to
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 6de8d90041ff..416524f6ba69 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1042,44 +1042,6 @@ xfs_unmap_extent(
 	goto out_unlock;
 }
 
-static int
-xfs_adjust_extent_unmap_boundaries(
-	struct xfs_inode	*ip,
-	xfs_fileoff_t		*startoffset_fsb,
-	xfs_fileoff_t		*endoffset_fsb)
-{
-	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_bmbt_irec	imap;
-	int			nimap, error;
-	xfs_extlen_t		mod = 0;
-
-	nimap = 1;
-	error = xfs_bmapi_read(ip, *startoffset_fsb, 1, &imap, &nimap, 0);
-	if (error)
-		return error;
-
-	if (nimap && imap.br_startblock != HOLESTARTBLOCK) {
-		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
-		div_u64_rem(imap.br_startblock, mp->m_sb.sb_rextsize, &mod);
-		if (mod)
-			*startoffset_fsb += mp->m_sb.sb_rextsize - mod;
-	}
-
-	nimap = 1;
-	error = xfs_bmapi_read(ip, *endoffset_fsb - 1, 1, &imap, &nimap, 0);
-	if (error)
-		return error;
-
-	if (nimap && imap.br_startblock != HOLESTARTBLOCK) {
-		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
-		mod++;
-		if (mod && mod != mp->m_sb.sb_rextsize)
-			*endoffset_fsb -= mod;
-	}
-
-	return 0;
-}
-
 static int
 xfs_flush_unmap_range(
 	struct xfs_inode	*ip,
@@ -1133,19 +1095,8 @@ xfs_free_file_space(
 	endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);
 
 	/*
-	 * Need to zero the stuff we're not freeing, on disk.  If it's a RT file
-	 * and we can't use unwritten extents then we actually need to ensure
-	 * to zero the whole extent, otherwise we just need to take of block
-	 * boundaries, and xfs_bunmapi will handle the rest.
+	 * Need to zero the stuff we're not freeing, on disk.
 	 */
-	if (XFS_IS_REALTIME_INODE(ip) &&
-	    !xfs_sb_version_hasextflgbit(&mp->m_sb)) {
-		error = xfs_adjust_extent_unmap_boundaries(ip, &startoffset_fsb,
-				&endoffset_fsb);
-		if (error)
-			return error;
-	}
-
 	if (endoffset_fsb > startoffset_fsb) {
 		while (!done) {
 			error = xfs_unmap_extent(ip, startoffset_fsb,
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 0ef5ece5634c..6e2c08f30f60 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -604,14 +604,6 @@ xfs_ioc_space(
 	uint			iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL;
 	int			error;
 
-	/*
-	 * Only allow the sys admin to reserve space unless
-	 * unwritten extents are enabled.
-	 */
-	if (!xfs_sb_version_hasextflgbit(&ip->i_mount->m_sb) &&
-	    !capable(CAP_SYS_ADMIN))
-		return -EPERM;
-
 	if (inode->i_flags & (S_IMMUTABLE|S_APPEND))
 		return -EPERM;
 
-- 
2.19.0

Re: [PATCH 2/8] xfs: remove suport for filesystems without unwritten extent flag

[Brian Foster]

On Tue, Oct 02, 2018 at 10:42:01AM -0700, Christoph Hellwig wrote:
> The option to enable unwritten extents was made default in 2003, removed
> from mkfs in 2007, and cannot be disabled in v5.  We also rely on it for
> a lot of common functionality, so filesystems without it will run a
> completely untested and buggy code path.  Enabling the support also is
> a simple bit flip using xfs_db, so legacy file systems can still be
> brought forward.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_bmap.c   | 21 ++++++----------
>  fs/xfs/libxfs/xfs_format.h |  8 ++----
>  fs/xfs/libxfs/xfs_sb.c     |  5 ++--
>  fs/xfs/scrub/scrub.c       | 13 ----------
>  fs/xfs/xfs_bmap_util.c     | 51 +-------------------------------------
>  fs/xfs/xfs_ioctl.c         |  8 ------
>  6 files changed, 12 insertions(+), 94 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index a47670332326..da6b768664e3 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4081,8 +4081,7 @@ xfs_bmapi_allocate(
>  	 * extents to real extents when we're about to write the data.
>  	 */
>  	if ((!bma->wasdel || (bma->flags & XFS_BMAPI_COWFORK)) &&
> -	    (bma->flags & XFS_BMAPI_PREALLOC) &&
> -	    xfs_sb_version_hasextflgbit(&mp->m_sb))
> +	    (bma->flags & XFS_BMAPI_PREALLOC))
>  		bma->got.br_state = XFS_EXT_UNWRITTEN;
>  
>  	if (bma->wasdel)
> @@ -5245,8 +5244,7 @@ __xfs_bunmapi(
>  			 * unmapping part of it.  But we can't really
>  			 * get rid of part of a realtime extent.
>  			 */
> -			if (del.br_state == XFS_EXT_UNWRITTEN ||
> -			    !xfs_sb_version_hasextflgbit(&mp->m_sb)) {
> +			if (del.br_state == XFS_EXT_UNWRITTEN) {
>  				/*
>  				 * This piece is unwritten, or we're not
>  				 * using unwritten extents.  Skip over it.
> @@ -5296,10 +5294,9 @@ __xfs_bunmapi(
>  				del.br_blockcount -= mod;
>  				del.br_startoff += mod;
>  				del.br_startblock += mod;
> -			} else if ((del.br_startoff == start &&
> -				    (del.br_state == XFS_EXT_UNWRITTEN ||
> -				     tp->t_blk_res == 0)) ||
> -				   !xfs_sb_version_hasextflgbit(&mp->m_sb)) {
> +			} else if (del.br_startoff == start &&
> +				   (del.br_state == XFS_EXT_UNWRITTEN ||
> +				    tp->t_blk_res == 0)) {
>  				/*
>  				 * Can't make it unwritten.  There isn't
>  				 * a full extent here so just skip it.
> @@ -6114,11 +6111,7 @@ xfs_bmap_validate_extent(
>  		    XFS_FSB_TO_AGNO(mp, endfsb))
>  			return __this_address;
>  	}
> -	if (irec->br_state != XFS_EXT_NORM) {
> -		if (whichfork != XFS_DATA_FORK)
> -			return __this_address;
> -		if (!xfs_sb_version_hasextflgbit(&mp->m_sb))
> -			return __this_address;
> -	}
> +	if (irec->br_state != XFS_EXT_NORM && whichfork != XFS_DATA_FORK)
> +		return __this_address;
>  	return NULL;
>  }
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index afbe336600e1..9995d5ae380b 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -287,6 +287,8 @@ static inline bool xfs_sb_good_v4_features(struct xfs_sb *sbp)
>  {
>  	if (!(sbp->sb_versionnum & XFS_SB_VERSION_DIRV2BIT))
>  		return false;
> +	if (!(sbp->sb_versionnum & XFS_SB_VERSION_EXTFLGBIT))
> +		return false;
>  
>  	/* check for unknown features in the fs */
>  	if ((sbp->sb_versionnum & ~XFS_SB_VERSION_OKBITS) ||
> @@ -357,12 +359,6 @@ static inline bool xfs_sb_version_haslogv2(struct xfs_sb *sbp)
>  	       (sbp->sb_versionnum & XFS_SB_VERSION_LOGV2BIT);
>  }
>  
> -static inline bool xfs_sb_version_hasextflgbit(struct xfs_sb *sbp)
> -{
> -	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 ||
> -	       (sbp->sb_versionnum & XFS_SB_VERSION_EXTFLGBIT);
> -}
> -
>  static inline bool xfs_sb_version_hassector(struct xfs_sb *sbp)
>  {
>  	return (sbp->sb_versionnum & XFS_SB_VERSION_SECTORBIT);
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 081f46e30556..b5a82acd7dfe 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -1115,7 +1115,8 @@ xfs_fs_geometry(
>  
>  	geo->version = XFS_FSOP_GEOM_VERSION;
>  	geo->flags = XFS_FSOP_GEOM_FLAGS_NLINK |
> -		     XFS_FSOP_GEOM_FLAGS_DIRV2;
> +		     XFS_FSOP_GEOM_FLAGS_DIRV2 |
> +		     XFS_FSOP_GEOM_FLAGS_EXTFLG;
>  	if (xfs_sb_version_hasattr(sbp))
>  		geo->flags |= XFS_FSOP_GEOM_FLAGS_ATTR;
>  	if (xfs_sb_version_hasquota(sbp))
> @@ -1124,8 +1125,6 @@ xfs_fs_geometry(
>  		geo->flags |= XFS_FSOP_GEOM_FLAGS_IALIGN;
>  	if (xfs_sb_version_hasdalign(sbp))
>  		geo->flags |= XFS_FSOP_GEOM_FLAGS_DALIGN;
> -	if (xfs_sb_version_hasextflgbit(sbp))
> -		geo->flags |= XFS_FSOP_GEOM_FLAGS_EXTFLG;
>  	if (xfs_sb_version_hassector(sbp))
>  		geo->flags |= XFS_FSOP_GEOM_FLAGS_SECTOR;
>  	if (xfs_sb_version_hasasciici(sbp))
> diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> index 4bfae1e61d30..1b2344d00525 100644
> --- a/fs/xfs/scrub/scrub.c
> +++ b/fs/xfs/scrub/scrub.c
> @@ -412,19 +412,6 @@ xchk_validate_inputs(
>  		goto out;
>  	}
>  
> -	error = -EOPNOTSUPP;
> -	/*
> -	 * We won't scrub any filesystem that doesn't have the ability
> -	 * to record unwritten extents.  The option was made default in
> -	 * 2003, removed from mkfs in 2007, and cannot be disabled in
> -	 * v5, so if we find a filesystem without this flag it's either
> -	 * really old or totally unsupported.  Avoid it either way.
> -	 * We also don't support v1-v3 filesystems, which aren't
> -	 * mountable.
> -	 */
> -	if (!xfs_sb_version_hasextflgbit(&mp->m_sb))
> -		goto out;
> -
>  	/*
>  	 * We only want to repair read-write v5+ filesystems.  Defer the check
>  	 * for ops->repair until after our scrub confirms that we need to
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 6de8d90041ff..416524f6ba69 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1042,44 +1042,6 @@ xfs_unmap_extent(
>  	goto out_unlock;
>  }
>  
> -static int
> -xfs_adjust_extent_unmap_boundaries(
> -	struct xfs_inode	*ip,
> -	xfs_fileoff_t		*startoffset_fsb,
> -	xfs_fileoff_t		*endoffset_fsb)
> -{
> -	struct xfs_mount	*mp = ip->i_mount;
> -	struct xfs_bmbt_irec	imap;
> -	int			nimap, error;
> -	xfs_extlen_t		mod = 0;
> -
> -	nimap = 1;
> -	error = xfs_bmapi_read(ip, *startoffset_fsb, 1, &imap, &nimap, 0);
> -	if (error)
> -		return error;
> -
> -	if (nimap && imap.br_startblock != HOLESTARTBLOCK) {
> -		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
> -		div_u64_rem(imap.br_startblock, mp->m_sb.sb_rextsize, &mod);
> -		if (mod)
> -			*startoffset_fsb += mp->m_sb.sb_rextsize - mod;
> -	}
> -
> -	nimap = 1;
> -	error = xfs_bmapi_read(ip, *endoffset_fsb - 1, 1, &imap, &nimap, 0);
> -	if (error)
> -		return error;
> -
> -	if (nimap && imap.br_startblock != HOLESTARTBLOCK) {
> -		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
> -		mod++;
> -		if (mod && mod != mp->m_sb.sb_rextsize)
> -			*endoffset_fsb -= mod;
> -	}
> -
> -	return 0;
> -}
> -
>  static int
>  xfs_flush_unmap_range(
>  	struct xfs_inode	*ip,
> @@ -1133,19 +1095,8 @@ xfs_free_file_space(
>  	endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);
>  
>  	/*
> -	 * Need to zero the stuff we're not freeing, on disk.  If it's a RT file
> -	 * and we can't use unwritten extents then we actually need to ensure
> -	 * to zero the whole extent, otherwise we just need to take of block
> -	 * boundaries, and xfs_bunmapi will handle the rest.
> +	 * Need to zero the stuff we're not freeing, on disk.
>  	 */
> -	if (XFS_IS_REALTIME_INODE(ip) &&
> -	    !xfs_sb_version_hasextflgbit(&mp->m_sb)) {
> -		error = xfs_adjust_extent_unmap_boundaries(ip, &startoffset_fsb,
> -				&endoffset_fsb);
> -		if (error)
> -			return error;
> -	}
> -
>  	if (endoffset_fsb > startoffset_fsb) {
>  		while (!done) {
>  			error = xfs_unmap_extent(ip, startoffset_fsb,
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 0ef5ece5634c..6e2c08f30f60 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -604,14 +604,6 @@ xfs_ioc_space(
>  	uint			iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL;
>  	int			error;
>  
> -	/*
> -	 * Only allow the sys admin to reserve space unless
> -	 * unwritten extents are enabled.
> -	 */
> -	if (!xfs_sb_version_hasextflgbit(&ip->i_mount->m_sb) &&
> -	    !capable(CAP_SYS_ADMIN))
> -		return -EPERM;
> -
>  	if (inode->i_flags & (S_IMMUTABLE|S_APPEND))
>  		return -EPERM;
>  
> -- 
> 2.19.0
> 

Re: [PATCH 2/8] xfs: remove suport for filesystems without unwritten extent flag

[Darrick J. Wong]

On Tue, Oct 02, 2018 at 10:42:01AM -0700, Christoph Hellwig wrote:
> The option to enable unwritten extents was made default in 2003, removed
> from mkfs in 2007, and cannot be disabled in v5.  We also rely on it for
> a lot of common functionality, so filesystems without it will run a
> completely untested and buggy code path.  Enabling the support also is
> a simple bit flip using xfs_db, so legacy file systems can still be
> brought forward.

Will there be a migration tool in xfsprogs to help administrators roll
their filesystems forward?  Generally we don't seem to support enabling
new features on old filesystems (ala ext4) though maybe we should for a
very small and controlled set of porting operations?

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c   | 21 ++++++----------
>  fs/xfs/libxfs/xfs_format.h |  8 ++----
>  fs/xfs/libxfs/xfs_sb.c     |  5 ++--
>  fs/xfs/scrub/scrub.c       | 13 ----------
>  fs/xfs/xfs_bmap_util.c     | 51 +-------------------------------------
>  fs/xfs/xfs_ioctl.c         |  8 ------
>  6 files changed, 12 insertions(+), 94 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index a47670332326..da6b768664e3 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4081,8 +4081,7 @@ xfs_bmapi_allocate(
>  	 * extents to real extents when we're about to write the data.
>  	 */
>  	if ((!bma->wasdel || (bma->flags & XFS_BMAPI_COWFORK)) &&
> -	    (bma->flags & XFS_BMAPI_PREALLOC) &&
> -	    xfs_sb_version_hasextflgbit(&mp->m_sb))
> +	    (bma->flags & XFS_BMAPI_PREALLOC))
>  		bma->got.br_state = XFS_EXT_UNWRITTEN;
>  
>  	if (bma->wasdel)
> @@ -5245,8 +5244,7 @@ __xfs_bunmapi(
>  			 * unmapping part of it.  But we can't really
>  			 * get rid of part of a realtime extent.
>  			 */
> -			if (del.br_state == XFS_EXT_UNWRITTEN ||
> -			    !xfs_sb_version_hasextflgbit(&mp->m_sb)) {
> +			if (del.br_state == XFS_EXT_UNWRITTEN) {
>  				/*
>  				 * This piece is unwritten, or we're not
>  				 * using unwritten extents.  Skip over it.
> @@ -5296,10 +5294,9 @@ __xfs_bunmapi(
>  				del.br_blockcount -= mod;
>  				del.br_startoff += mod;
>  				del.br_startblock += mod;
> -			} else if ((del.br_startoff == start &&
> -				    (del.br_state == XFS_EXT_UNWRITTEN ||
> -				     tp->t_blk_res == 0)) ||
> -				   !xfs_sb_version_hasextflgbit(&mp->m_sb)) {
> +			} else if (del.br_startoff == start &&
> +				   (del.br_state == XFS_EXT_UNWRITTEN ||
> +				    tp->t_blk_res == 0)) {
>  				/*
>  				 * Can't make it unwritten.  There isn't
>  				 * a full extent here so just skip it.
> @@ -6114,11 +6111,7 @@ xfs_bmap_validate_extent(
>  		    XFS_FSB_TO_AGNO(mp, endfsb))
>  			return __this_address;
>  	}
> -	if (irec->br_state != XFS_EXT_NORM) {
> -		if (whichfork != XFS_DATA_FORK)
> -			return __this_address;
> -		if (!xfs_sb_version_hasextflgbit(&mp->m_sb))
> -			return __this_address;
> -	}
> +	if (irec->br_state != XFS_EXT_NORM && whichfork != XFS_DATA_FORK)
> +		return __this_address;
>  	return NULL;
>  }
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index afbe336600e1..9995d5ae380b 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -287,6 +287,8 @@ static inline bool xfs_sb_good_v4_features(struct xfs_sb *sbp)
>  {
>  	if (!(sbp->sb_versionnum & XFS_SB_VERSION_DIRV2BIT))
>  		return false;
> +	if (!(sbp->sb_versionnum & XFS_SB_VERSION_EXTFLGBIT))
> +		return false;

The kernel only spits out "XFS(sda): bad version" in dmesg.  Maybe it'd
be helpful to steer admins towards whatever they need to do to make
their ancient fs work again?

--D

>  
>  	/* check for unknown features in the fs */
>  	if ((sbp->sb_versionnum & ~XFS_SB_VERSION_OKBITS) ||
> @@ -357,12 +359,6 @@ static inline bool xfs_sb_version_haslogv2(struct xfs_sb *sbp)
>  	       (sbp->sb_versionnum & XFS_SB_VERSION_LOGV2BIT);
>  }
>  
> -static inline bool xfs_sb_version_hasextflgbit(struct xfs_sb *sbp)
> -{
> -	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 ||
> -	       (sbp->sb_versionnum & XFS_SB_VERSION_EXTFLGBIT);
> -}
> -
>  static inline bool xfs_sb_version_hassector(struct xfs_sb *sbp)
>  {
>  	return (sbp->sb_versionnum & XFS_SB_VERSION_SECTORBIT);
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 081f46e30556..b5a82acd7dfe 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -1115,7 +1115,8 @@ xfs_fs_geometry(
>  
>  	geo->version = XFS_FSOP_GEOM_VERSION;
>  	geo->flags = XFS_FSOP_GEOM_FLAGS_NLINK |
> -		     XFS_FSOP_GEOM_FLAGS_DIRV2;
> +		     XFS_FSOP_GEOM_FLAGS_DIRV2 |
> +		     XFS_FSOP_GEOM_FLAGS_EXTFLG;
>  	if (xfs_sb_version_hasattr(sbp))
>  		geo->flags |= XFS_FSOP_GEOM_FLAGS_ATTR;
>  	if (xfs_sb_version_hasquota(sbp))
> @@ -1124,8 +1125,6 @@ xfs_fs_geometry(
>  		geo->flags |= XFS_FSOP_GEOM_FLAGS_IALIGN;
>  	if (xfs_sb_version_hasdalign(sbp))
>  		geo->flags |= XFS_FSOP_GEOM_FLAGS_DALIGN;
> -	if (xfs_sb_version_hasextflgbit(sbp))
> -		geo->flags |= XFS_FSOP_GEOM_FLAGS_EXTFLG;
>  	if (xfs_sb_version_hassector(sbp))
>  		geo->flags |= XFS_FSOP_GEOM_FLAGS_SECTOR;
>  	if (xfs_sb_version_hasasciici(sbp))
> diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> index 4bfae1e61d30..1b2344d00525 100644
> --- a/fs/xfs/scrub/scrub.c
> +++ b/fs/xfs/scrub/scrub.c
> @@ -412,19 +412,6 @@ xchk_validate_inputs(
>  		goto out;
>  	}
>  
> -	error = -EOPNOTSUPP;
> -	/*
> -	 * We won't scrub any filesystem that doesn't have the ability
> -	 * to record unwritten extents.  The option was made default in
> -	 * 2003, removed from mkfs in 2007, and cannot be disabled in
> -	 * v5, so if we find a filesystem without this flag it's either
> -	 * really old or totally unsupported.  Avoid it either way.
> -	 * We also don't support v1-v3 filesystems, which aren't
> -	 * mountable.
> -	 */
> -	if (!xfs_sb_version_hasextflgbit(&mp->m_sb))
> -		goto out;
> -
>  	/*
>  	 * We only want to repair read-write v5+ filesystems.  Defer the check
>  	 * for ops->repair until after our scrub confirms that we need to
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 6de8d90041ff..416524f6ba69 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1042,44 +1042,6 @@ xfs_unmap_extent(
>  	goto out_unlock;
>  }
>  
> -static int
> -xfs_adjust_extent_unmap_boundaries(
> -	struct xfs_inode	*ip,
> -	xfs_fileoff_t		*startoffset_fsb,
> -	xfs_fileoff_t		*endoffset_fsb)
> -{
> -	struct xfs_mount	*mp = ip->i_mount;
> -	struct xfs_bmbt_irec	imap;
> -	int			nimap, error;
> -	xfs_extlen_t		mod = 0;
> -
> -	nimap = 1;
> -	error = xfs_bmapi_read(ip, *startoffset_fsb, 1, &imap, &nimap, 0);
> -	if (error)
> -		return error;
> -
> -	if (nimap && imap.br_startblock != HOLESTARTBLOCK) {
> -		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
> -		div_u64_rem(imap.br_startblock, mp->m_sb.sb_rextsize, &mod);
> -		if (mod)
> -			*startoffset_fsb += mp->m_sb.sb_rextsize - mod;
> -	}
> -
> -	nimap = 1;
> -	error = xfs_bmapi_read(ip, *endoffset_fsb - 1, 1, &imap, &nimap, 0);
> -	if (error)
> -		return error;
> -
> -	if (nimap && imap.br_startblock != HOLESTARTBLOCK) {
> -		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
> -		mod++;
> -		if (mod && mod != mp->m_sb.sb_rextsize)
> -			*endoffset_fsb -= mod;
> -	}
> -
> -	return 0;
> -}
> -
>  static int
>  xfs_flush_unmap_range(
>  	struct xfs_inode	*ip,
> @@ -1133,19 +1095,8 @@ xfs_free_file_space(
>  	endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);
>  
>  	/*
> -	 * Need to zero the stuff we're not freeing, on disk.  If it's a RT file
> -	 * and we can't use unwritten extents then we actually need to ensure
> -	 * to zero the whole extent, otherwise we just need to take of block
> -	 * boundaries, and xfs_bunmapi will handle the rest.
> +	 * Need to zero the stuff we're not freeing, on disk.
>  	 */
> -	if (XFS_IS_REALTIME_INODE(ip) &&
> -	    !xfs_sb_version_hasextflgbit(&mp->m_sb)) {
> -		error = xfs_adjust_extent_unmap_boundaries(ip, &startoffset_fsb,
> -				&endoffset_fsb);
> -		if (error)
> -			return error;
> -	}
> -
>  	if (endoffset_fsb > startoffset_fsb) {
>  		while (!done) {
>  			error = xfs_unmap_extent(ip, startoffset_fsb,
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 0ef5ece5634c..6e2c08f30f60 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -604,14 +604,6 @@ xfs_ioc_space(
>  	uint			iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL;
>  	int			error;
>  
> -	/*
> -	 * Only allow the sys admin to reserve space unless
> -	 * unwritten extents are enabled.
> -	 */
> -	if (!xfs_sb_version_hasextflgbit(&ip->i_mount->m_sb) &&
> -	    !capable(CAP_SYS_ADMIN))
> -		return -EPERM;
> -
>  	if (inode->i_flags & (S_IMMUTABLE|S_APPEND))
>  		return -EPERM;
>  
> -- 
> 2.19.0
> 

Re: [PATCH 2/8] xfs: remove suport for filesystems without unwritten extent flag

[Christoph Hellwig]

On Wed, Oct 03, 2018 at 07:52:05AM -0700, Darrick J. Wong wrote:
> Will there be a migration tool in xfsprogs to help administrators roll
> their filesystems forward?  Generally we don't seem to support enabling
> new features on old filesystems (ala ext4) though maybe we should for a
> very small and controlled set of porting operations?

I wasn't planning to unless needed, mostly because I don't really think
we'll see such file systems in the wild.  They'd be very broken if
you are using the current code on them already.

[PATCH 3/8] xfs: remove magic handling of unwritten extents in xfs_bmapi_allocate

[Christoph Hellwig]

There is no real need to treat unwritten delalloc extent special in
any way here, so remove the special casing and related comment.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index da6b768664e3..3bb250ee6c7c 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4069,20 +4069,10 @@ xfs_bmapi_allocate(
 	bma->got.br_startoff = bma->offset;
 	bma->got.br_startblock = bma->blkno;
 	bma->got.br_blockcount = bma->length;
-	bma->got.br_state = XFS_EXT_NORM;
-
-	/*
-	 * In the data fork, a wasdelay extent has been initialized, so
-	 * shouldn't be flagged as unwritten.
-	 *
-	 * For the cow fork, however, we convert delalloc reservations
-	 * (extents allocated for speculative preallocation) to
-	 * allocated unwritten extents, and only convert the unwritten
-	 * extents to real extents when we're about to write the data.
-	 */
-	if ((!bma->wasdel || (bma->flags & XFS_BMAPI_COWFORK)) &&
-	    (bma->flags & XFS_BMAPI_PREALLOC))
+	if (bma->flags & XFS_BMAPI_PREALLOC)
 		bma->got.br_state = XFS_EXT_UNWRITTEN;
+	else
+		bma->got.br_state = XFS_EXT_NORM;
 
 	if (bma->wasdel)
 		error = xfs_bmap_add_extent_delay_real(bma, whichfork);
-- 
2.19.0

Re: [PATCH 3/8] xfs: remove magic handling of unwritten extents in xfs_bmapi_allocate

[Brian Foster]

On Tue, Oct 02, 2018 at 10:42:02AM -0700, Christoph Hellwig wrote:
> There is no real need to treat unwritten delalloc extent special in
> any way here, so remove the special casing and related comment.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_bmap.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index da6b768664e3..3bb250ee6c7c 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4069,20 +4069,10 @@ xfs_bmapi_allocate(
>  	bma->got.br_startoff = bma->offset;
>  	bma->got.br_startblock = bma->blkno;
>  	bma->got.br_blockcount = bma->length;
> -	bma->got.br_state = XFS_EXT_NORM;
> -
> -	/*
> -	 * In the data fork, a wasdelay extent has been initialized, so
> -	 * shouldn't be flagged as unwritten.
> -	 *
> -	 * For the cow fork, however, we convert delalloc reservations
> -	 * (extents allocated for speculative preallocation) to
> -	 * allocated unwritten extents, and only convert the unwritten
> -	 * extents to real extents when we're about to write the data.
> -	 */
> -	if ((!bma->wasdel || (bma->flags & XFS_BMAPI_COWFORK)) &&
> -	    (bma->flags & XFS_BMAPI_PREALLOC))
> +	if (bma->flags & XFS_BMAPI_PREALLOC)
>  		bma->got.br_state = XFS_EXT_UNWRITTEN;
> +	else
> +		bma->got.br_state = XFS_EXT_NORM;
>  
>  	if (bma->wasdel)
>  		error = xfs_bmap_add_extent_delay_real(bma, whichfork);
> -- 
> 2.19.0
> 

Re: [PATCH 3/8] xfs: remove magic handling of unwritten extents in xfs_bmapi_allocate

[Dave Chinner]

On Tue, Oct 02, 2018 at 10:42:02AM -0700, Christoph Hellwig wrote:
> There is no real need to treat unwritten delalloc extent special in
> any way here, so remove the special casing and related comment.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index da6b768664e3..3bb250ee6c7c 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4069,20 +4069,10 @@ xfs_bmapi_allocate(
>  	bma->got.br_startoff = bma->offset;
>  	bma->got.br_startblock = bma->blkno;
>  	bma->got.br_blockcount = bma->length;
> -	bma->got.br_state = XFS_EXT_NORM;
> -
> -	/*
> -	 * In the data fork, a wasdelay extent has been initialized, so
> -	 * shouldn't be flagged as unwritten.
> -	 *
> -	 * For the cow fork, however, we convert delalloc reservations
> -	 * (extents allocated for speculative preallocation) to
> -	 * allocated unwritten extents, and only convert the unwritten
> -	 * extents to real extents when we're about to write the data.
> -	 */
> -	if ((!bma->wasdel || (bma->flags & XFS_BMAPI_COWFORK)) &&
> -	    (bma->flags & XFS_BMAPI_PREALLOC))
> +	if (bma->flags & XFS_BMAPI_PREALLOC)
>  		bma->got.br_state = XFS_EXT_UNWRITTEN;
> +	else
> +		bma->got.br_state = XFS_EXT_NORM;

I bisected the generic/127 rmap corruption down to this patch:

[   36.523002] run fstests generic/127 at 2018-10-06 19:25:42
[   48.642735] XFS: Assertion failed: fs_is_ok, file: fs/xfs/libxfs/xfs_rmap.c, line: 418
[   48.647380] ------------ [ cut here ]------------
[   48.648322] kernel BUG at fs/xfs/xfs_message.c:102!
[   48.649347] invalid opcode: 0000
[#1] PREEMPT SMP
[   48.650303] CPU: 9 PID: 4468 Comm: fsx Not tainted 4.19.0-rc6-dgc+ #681
[   48.651874] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.1-1 04/01/2014
[   48.653562] RIP: 0010:assfail+0x28/0x30
[   48.654332] Code: c3 90 66 66 66 66 90 48 89 f1 41 89 d0 48 c7 c6 70 56 2e 82 48 89 fa 31 ff e8 64 f9 ff ff 80 3d 75 9f 0a 01 00 75 03 0f 0b c3 <0f> 0b 66 0f 1f 44 00 00 66 66 66 66 90 48 63 f6 49 89a
[   48.657980] RSP: 0018:ffffc900049a7b18 EFLAGS: 00010202
[   48.659015] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[   48.660423] RDX: 00000000ffffffc0 RSI: 000000000000000a RDI: ffffffff8227ab34
[   48.661817] RBP: ffffc900049a7c38 R08: 0000000000000000 R09: 0000000000000000
[   48.663209] R10: 00000000000000c0 R11: f000000000000000 R12: 00000000000000c0
[   48.664634] R13: 0000000000000004 R14: ffff880239789d20 R15: 0000000000000000
[   48.666033] FS:  00007fb5eed1db80(0000) GS:ffff88023fd00000(0000) knlGS:0000000000000000
[   48.667608] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   48.668752] CR2: 00007f5ce9a0e8e0 CR3: 0000000238f5c001 CR4: 0000000000060ee0
[   48.670168] Call Trace:
[   48.671250]  xfs_rmap_unmap+0x633/0x980
[   48.672037]  ? kmem_zone_alloc+0x61/0xe0
[   48.672842]  xfs_rmap_finish_one+0x2d0/0x340
[   48.673692]  xfs_trans_log_finish_rmap_update+0x2f/0x40
[   48.674737]  xfs_rmap_update_finish_item+0x2c/0x40
[   48.675710]  xfs_defer_finish_noroll+0x184/0x520
[   48.676614]  ? xfs_rmap_update_cancel_item+0x10/0x10
[   48.677615]  ? xfs_free_file_space+0x355/0x390
[   48.678512]  __xfs_trans_commit+0x189/0x370
[   48.679360]  xfs_free_file_space+0x355/0x390
[   48.680227]  xfs_file_fallocate+0x1b3/0x330
[   48.681077]  ? __sb_start_write+0x8d/0xc0
[   48.681889]  vfs_fallocate+0x13d/0x270
[   48.682637]  ksys_fallocate+0x3c/0x70
[   48.683382]  __x64_sys_fallocate+0x1a/0x20
[   48.684208]  do_syscall_64+0x5a/0x180
[   48.684953]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

The corruption check that is failing is this:

        /* Make sure the unwritten flag matches. */
	XFS_WANT_CORRUPTED_GOTO(mp, (flags & XFS_RMAP_UNWRITTEN) ==
			(rec->rm_flags & XFS_RMAP_UNWRITTEN), out);

So this patch does not appear to be doing the right thing with
unwritten extent flagging in some case.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/8] xfs: remove magic handling of unwritten extents in xfs_bmapi_allocate

[Christoph Hellwig]

On Sat, Oct 06, 2018 at 07:34:34PM +1000, Dave Chinner wrote:
> I bisected the generic/127 rmap corruption down to this patch:

Heh, I just did so too this morning.  

> The corruption check that is failing is this:
> 
>         /* Make sure the unwritten flag matches. */
> 	XFS_WANT_CORRUPTED_GOTO(mp, (flags & XFS_RMAP_UNWRITTEN) ==
> 			(rec->rm_flags & XFS_RMAP_UNWRITTEN), out);
> 
> So this patch does not appear to be doing the right thing with
> unwritten extent flagging in some case.

I suspect the asserts actually are what is incorrect.  But given
how late we are in the cycle I've just dropped the patch and kicked
off xfstests runs (now including rmap, sigh..).

Re: [PATCH 3/8] xfs: remove magic handling of unwritten extents in xfs_bmapi_allocate

[Christoph Hellwig]

On Sat, Oct 06, 2018 at 11:43:31AM +0200, Christoph Hellwig wrote:
> I suspect the asserts actually are what is incorrect.  But given
> how late we are in the cycle I've just dropped the patch and kicked
> off xfstests runs (now including rmap, sigh..).

Without this patch test runs including rmap succeed.  Do you want
me to resend with the patch dropped (thing should just apply without
it as-is).

Re: [PATCH 3/8] xfs: remove magic handling of unwritten extents in xfs_bmapi_allocate

[Dave Chinner]

On Sun, Oct 07, 2018 at 12:13:19PM +0200, Christoph Hellwig wrote:
> On Sat, Oct 06, 2018 at 11:43:31AM +0200, Christoph Hellwig wrote:
> > I suspect the asserts actually are what is incorrect.  But given
> > how late we are in the cycle I've just dropped the patch and kicked
> > off xfstests runs (now including rmap, sigh..).
> 
> Without this patch test runs including rmap succeed.  Do you want
> me to resend with the patch dropped (thing should just apply without
> it as-is).

I'll drop it and see what happens...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/8] xfs: remove magic handling of unwritten extents in xfs_bmapi_allocate

[Dave Chinner]

On Mon, Oct 08, 2018 at 09:02:58AM +1100, Dave Chinner wrote:
> On Sun, Oct 07, 2018 at 12:13:19PM +0200, Christoph Hellwig wrote:
> > On Sat, Oct 06, 2018 at 11:43:31AM +0200, Christoph Hellwig wrote:
> > > I suspect the asserts actually are what is incorrect.  But given
> > > how late we are in the cycle I've just dropped the patch and kicked
> > > off xfstests runs (now including rmap, sigh..).
> > 
> > Without this patch test runs including rmap succeed.  Do you want
> > me to resend with the patch dropped (thing should just apply without
> > it as-is).
> 
> I'll drop it and see what happens...

Different problems after dropping that patch and re-instating the
rest of the series. The shutdown/io error stress tests now hang
randomly in unmount reclaiming inodes. e.g. generic/388:

.....
[ 4028.393874] XFS (pmem1): Mounting V5 Filesystem
[ 4028.397627] XFS (pmem1): Starting recovery (logdev: internal)
[ 4028.400436] XFS (pmem1): Ending recovery (logdev: internal)
[ 4028.406885] XFS (pmem1): xfs_do_force_shutdown(0x8) called from line 435 of file fs/xfs/libxfs/xfs_defer.c.  Return address = ffffffff814fc998
[ 4028.406966] XFS (pmem1): xfs_imap_lookup: xfs_ialloc_read_agi() returned error -5, agno 0
[ 4028.500258] XFS (pmem1): Unmounting Filesystem
[ 4230.446548] INFO: task umount:16507 blocked for more than 120 seconds.
[ 4230.449820]       Not tainted 4.19.0-rc7-dgc+ #682
[ 4230.452695] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 4230.456142] umount          D14552 16507  14540 0x00000000
[ 4230.458206] Call Trace:
[ 4230.458980]  ? __schedule+0x2bf/0x8a0
[ 4230.459852]  ? rwsem_down_write_failed+0x235/0x440
[ 4230.460815]  ? rwsem_down_write_failed+0x230/0x440
[ 4230.461777]  schedule+0x45/0x110
[ 4230.462597]  rwsem_down_write_failed+0x235/0x440
[ 4230.463685]  ? xfs_reclaim_inode+0x107/0x430
[ 4230.464549]  ? call_rwsem_down_write_failed+0x13/0x20
[ 4230.465564]  call_rwsem_down_write_failed+0x13/0x20
[ 4230.466599]  down_write+0x37/0x50
[ 4230.467438]  xfs_ilock+0x119/0x200
[ 4230.468133]  xfs_reclaim_inode+0x107/0x430
[ 4230.468965]  xfs_reclaim_inodes_ag+0x1bd/0x330
[ 4230.469870]  xfs_reclaim_inodes+0x2b/0x50
[ 4230.470736]  xfs_unmountfs+0x92/0x180
[ 4230.471682]  xfs_fs_put_super+0x39/0xb0
[ 4230.472466]  generic_shutdown_super+0x64/0x110
[ 4230.473368]  kill_block_super+0x21/0x50
[ 4230.474149]  deactivate_locked_super+0x39/0x70
[ 4230.475100]  cleanup_mnt+0x3b/0x80
[ 4230.476004]  task_work_run+0x82/0xb0
[ 4230.476740]  exit_to_usermode_loop+0xd3/0xe0
[ 4230.477608]  do_syscall_64+0x170/0x180
[ 4230.478421]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

And on a different machine, generic/475:

[ 8001.777349] XFS (dm-0): Unmounting Filesystem
[ 8001.850325] XFS (dm-0): Mounting V5 Filesystem
[ 8002.234755] XFS (dm-0): Starting recovery (logdev: internal)
[ 8002.410851] XFS (dm-0): Ending recovery (logdev: internal)
[ 8002.438481] XFS (dm-0): metadata I/O error in "xfs_trans_read_buf_map" at daddr 0xd60 len 8 error 5
[ 8002.438542] XFS (dm-0): metadata I/O error in "xfs_trans_read_buf_map" at daddr 0xa00728 len 8 error 5
[ 8002.441590] XFS (dm-0): metadata I/O error in "xfs_trans_read_buf_map" at daddr 0x1406f80 len 32 error 5
[ 8002.441597] XFS (dm-0): xfs_imap_to_bp: xfs_trans_read_buf() returned error -5.
[ 8002.442325] Buffer I/O error on dev dm-0, logical block 5242864, async page read
[ 8002.442377] XFS (dm-0): xfs_do_force_shutdown(0x1) called from line 327 of file fs/xfs/xfs_trans_buf.c.  Return address = ffffffff8156e5e4
[ 8002.442422] XFS (dm-0): metadata I/O error in "xfs_trans_read_buf_map" at daddr 0x1e00080 len 32 error 5
[ 8002.442427] XFS (dm-0): xfs_imap_to_bp: xfs_trans_read_buf() returned error -5.
[ 8002.442443] XFS (dm-0): metadata I/O error in "xlog_iodone" at daddr 0x1402790 len 64 error 5
[ 8002.442448] XFS (dm-0): xfs_do_force_shutdown(0x2) called from line 1271 of file fs/xfs/xfs_log.c.  Return address = ffffffff81558d45
[ 8002.442459] XFS (dm-0): xfs_imap_to_bp: xfs_trans_read_buf() returned error -5.
[ 8002.442480] XFS (dm-0): Log I/O Error Detected.  Shutting down filesystem
[ 8002.442481] XFS (dm-0): Please umount the filesystem and rectify the problem(s)
[ 8002.525313] XFS (dm-0): Unmounting Filesystem
[ 8217.616288] INFO: task umount:11278 blocked for more than 120 seconds.
[ 8217.621420]       Tainted: G        W         4.19.0-rc7-dgc+ #682
[ 8217.622924] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 8217.625024] umount          D14640 11278   9205 0x00000080
[ 8217.626512] Call Trace:
[ 8217.627192]  ? __schedule+0x2bf/0x8a0
[ 8217.628213]  ? rwsem_down_write_failed+0x235/0x440
[ 8217.629512]  ? rwsem_down_write_failed+0x230/0x440
[ 8217.630804]  schedule+0x45/0x110
[ 8217.631673]  rwsem_down_write_failed+0x235/0x440
[ 8217.632593]  ? xfs_reclaim_inode+0x107/0x430
[ 8217.633429]  ? call_rwsem_down_write_failed+0x13/0x20
[ 8217.634419]  call_rwsem_down_write_failed+0x13/0x20
[ 8217.635361]  down_write+0x37/0x50
[ 8217.636010]  xfs_ilock+0x119/0x200
[ 8217.636702]  xfs_reclaim_inode+0x107/0x430
[ 8217.637510]  xfs_reclaim_inodes_ag+0x1bd/0x330
[ 8217.638391]  ? _raw_spin_lock_irqsave+0x32/0x40
[ 8217.639276]  ? __flush_work+0x194/0x1e0
[ 8217.640027]  ? preempt_count_sub+0x43/0x50
[ 8217.640847]  ? _raw_spin_unlock_irqrestore+0x2c/0x60
[ 8217.641815]  ? del_timer+0x53/0x80
[ 8217.642490]  ? __cancel_work_timer+0x13c/0x1e0
[ 8217.643354]  ? xfs_ail_push_all_sync+0xcc/0xf0
[ 8217.644238]  xfs_reclaim_inodes+0x2b/0x50
[ 8217.645025]  xfs_unmountfs+0x92/0x180
[ 8217.645742]  xfs_fs_put_super+0x39/0xb0
[ 8217.646490]  generic_shutdown_super+0x64/0x110
[ 8217.647360]  kill_block_super+0x21/0x50
[ 8217.648131]  deactivate_locked_super+0x39/0x70
[ 8217.649006]  cleanup_mnt+0x3b/0x80
[ 8217.649675]  task_work_run+0x82/0xb0
[ 8217.650389]  exit_to_usermode_loop+0xd3/0xe0
[ 8217.651223]  do_syscall_64+0x170/0x180

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/8] xfs: remove magic handling of unwritten extents in xfs_bmapi_allocate

[Dave Chinner]

On Mon, Oct 08, 2018 at 01:24:55PM +1100, Dave Chinner wrote:
> On Mon, Oct 08, 2018 at 09:02:58AM +1100, Dave Chinner wrote:
> > On Sun, Oct 07, 2018 at 12:13:19PM +0200, Christoph Hellwig wrote:
> > > On Sat, Oct 06, 2018 at 11:43:31AM +0200, Christoph Hellwig wrote:
> > > > I suspect the asserts actually are what is incorrect.  But given
> > > > how late we are in the cycle I've just dropped the patch and kicked
> > > > off xfstests runs (now including rmap, sigh..).
> > > 
> > > Without this patch test runs including rmap succeed.  Do you want
> > > me to resend with the patch dropped (thing should just apply without
> > > it as-is).
> > 
> > I'll drop it and see what happens...
> 
> Different problems after dropping that patch and re-instating the
> rest of the series. The shutdown/io error stress tests now hang
> randomly in unmount reclaiming inodes. e.g. generic/388:

Ah, my fault. False alarm.

I had a patch that I knew was broken at the end of the series that I
hadn't commented out. All prior testing was without it, but when I
reset branch without this patch I simply popped everything back in,
including the broken patch at the end. Retesting now.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 4/8] xfs: handle zeroing in xfs_file_iomap_begin_delay

[Christoph Hellwig]

We only need to allocate blocks for zeroing for reflink inodes,
and for we currently have a special case for reflink files in
the otherwise direct I/O path that I'd like to get rid of.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_iomap.c | 44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 6320aca39f39..9b572a1fbd42 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -62,6 +62,21 @@ xfs_bmbt_to_iomap(
 	iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip));
 }
 
+static void
+xfs_hole_to_iomap(
+	struct xfs_inode	*ip,
+	struct iomap		*iomap,
+	xfs_fileoff_t		offset_fsb,
+	xfs_fileoff_t		end_fsb)
+{
+	iomap->addr = IOMAP_NULL_ADDR;
+	iomap->type = IOMAP_HOLE;
+	iomap->offset = XFS_FSB_TO_B(ip->i_mount, offset_fsb);
+	iomap->length = XFS_FSB_TO_B(ip->i_mount, end_fsb - offset_fsb);
+	iomap->bdev = xfs_find_bdev_for_inode(VFS_I(ip));
+	iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip));
+}
+
 xfs_extlen_t
 xfs_eof_alignment(
 	struct xfs_inode	*ip,
@@ -502,6 +517,7 @@ xfs_file_iomap_begin_delay(
 	struct inode		*inode,
 	loff_t			offset,
 	loff_t			count,
+	unsigned		flags,
 	struct iomap		*iomap)
 {
 	struct xfs_inode	*ip = XFS_I(inode);
@@ -538,13 +554,23 @@ xfs_file_iomap_begin_delay(
 			goto out_unlock;
 	}
 
+	end_fsb = min(XFS_B_TO_FSB(mp, offset + count), maxbytes_fsb);
+
 	eof = !xfs_iext_lookup_extent(ip, ifp, offset_fsb, &icur, &got);
-	if (!eof && got.br_startoff <= offset_fsb) {
-		if (xfs_is_reflink_inode(ip)) {
+	if (eof)
+		got.br_startoff = end_fsb; /* fake hole until the end */
+
+	if (got.br_startoff <= offset_fsb) {
+		/*
+		 * For reflink files we may need a delalloc reservation when
+		 * overwriting shared extents.   This includes zeroing of
+		 * existing extents that contain data.
+		 */
+		if (xfs_is_reflink_inode(ip) &&
+		    ((flags & IOMAP_WRITE) ||
+		     got.br_state != XFS_EXT_UNWRITTEN)) {
 			bool		shared;
 
-			end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
-					maxbytes_fsb);
 			xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
 			error = xfs_reflink_reserve_cow(ip, &got, &shared);
 			if (error)
@@ -555,6 +581,11 @@ xfs_file_iomap_begin_delay(
 		goto done;
 	}
 
+	if (flags & IOMAP_ZERO) {
+		xfs_hole_to_iomap(ip, iomap, offset_fsb, got.br_startoff);
+		goto out_unlock;
+	}
+
 	error = xfs_qm_dqattach_locked(ip, false);
 	if (error)
 		goto out_unlock;
@@ -1009,10 +1040,11 @@ xfs_file_iomap_begin(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
-	if (((flags & (IOMAP_WRITE | IOMAP_DIRECT)) == IOMAP_WRITE) &&
+	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && !(flags & IOMAP_DIRECT) &&
 			!IS_DAX(inode) && !xfs_get_extsz_hint(ip)) {
 		/* Reserve delalloc blocks for regular writeback. */
-		return xfs_file_iomap_begin_delay(inode, offset, length, iomap);
+		return xfs_file_iomap_begin_delay(inode, offset, length, flags,
+				iomap);
 	}
 
 	/*
-- 
2.19.0

Re: [PATCH 4/8] xfs: handle zeroing in xfs_file_iomap_begin_delay

[Brian Foster]

On Tue, Oct 02, 2018 at 10:42:03AM -0700, Christoph Hellwig wrote:
> We only need to allocate blocks for zeroing for reflink inodes,
> and for we currently have a special case for reflink files in
> the otherwise direct I/O path that I'd like to get rid of.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_iomap.c | 44 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 6320aca39f39..9b572a1fbd42 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -62,6 +62,21 @@ xfs_bmbt_to_iomap(
>  	iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip));
>  }
>  
> +static void
> +xfs_hole_to_iomap(
> +	struct xfs_inode	*ip,
> +	struct iomap		*iomap,
> +	xfs_fileoff_t		offset_fsb,
> +	xfs_fileoff_t		end_fsb)
> +{
> +	iomap->addr = IOMAP_NULL_ADDR;
> +	iomap->type = IOMAP_HOLE;
> +	iomap->offset = XFS_FSB_TO_B(ip->i_mount, offset_fsb);
> +	iomap->length = XFS_FSB_TO_B(ip->i_mount, end_fsb - offset_fsb);
> +	iomap->bdev = xfs_find_bdev_for_inode(VFS_I(ip));
> +	iomap->dax_dev = xfs_find_daxdev_for_inode(VFS_I(ip));
> +}
> +
>  xfs_extlen_t
>  xfs_eof_alignment(
>  	struct xfs_inode	*ip,
> @@ -502,6 +517,7 @@ xfs_file_iomap_begin_delay(
>  	struct inode		*inode,
>  	loff_t			offset,
>  	loff_t			count,
> +	unsigned		flags,
>  	struct iomap		*iomap)
>  {
>  	struct xfs_inode	*ip = XFS_I(inode);
> @@ -538,13 +554,23 @@ xfs_file_iomap_begin_delay(
>  			goto out_unlock;
>  	}
>  
> +	end_fsb = min(XFS_B_TO_FSB(mp, offset + count), maxbytes_fsb);
> +
>  	eof = !xfs_iext_lookup_extent(ip, ifp, offset_fsb, &icur, &got);
> -	if (!eof && got.br_startoff <= offset_fsb) {
> -		if (xfs_is_reflink_inode(ip)) {
> +	if (eof)
> +		got.br_startoff = end_fsb; /* fake hole until the end */
> +
> +	if (got.br_startoff <= offset_fsb) {
> +		/*
> +		 * For reflink files we may need a delalloc reservation when
> +		 * overwriting shared extents.   This includes zeroing of
> +		 * existing extents that contain data.
> +		 */
> +		if (xfs_is_reflink_inode(ip) &&
> +		    ((flags & IOMAP_WRITE) ||
> +		     got.br_state != XFS_EXT_UNWRITTEN)) {
>  			bool		shared;
>  
> -			end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
> -					maxbytes_fsb);
>  			xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
>  			error = xfs_reflink_reserve_cow(ip, &got, &shared);
>  			if (error)
> @@ -555,6 +581,11 @@ xfs_file_iomap_begin_delay(
>  		goto done;
>  	}
>  
> +	if (flags & IOMAP_ZERO) {
> +		xfs_hole_to_iomap(ip, iomap, offset_fsb, got.br_startoff);
> +		goto out_unlock;
> +	}
> +
>  	error = xfs_qm_dqattach_locked(ip, false);
>  	if (error)
>  		goto out_unlock;
> @@ -1009,10 +1040,11 @@ xfs_file_iomap_begin(
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return -EIO;
>  
> -	if (((flags & (IOMAP_WRITE | IOMAP_DIRECT)) == IOMAP_WRITE) &&
> +	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && !(flags & IOMAP_DIRECT) &&
>  			!IS_DAX(inode) && !xfs_get_extsz_hint(ip)) {
>  		/* Reserve delalloc blocks for regular writeback. */
> -		return xfs_file_iomap_begin_delay(inode, offset, length, iomap);
> +		return xfs_file_iomap_begin_delay(inode, offset, length, flags,
> +				iomap);
>  	}
>  
>  	/*
> -- 
> 2.19.0
> 

[PATCH 5/8] xfs: remove the unused shared argument to xfs_reflink_reserve_cow

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_iomap.c   |  6 ++----
 fs/xfs/xfs_reflink.c | 12 +++++-------
 fs/xfs/xfs_reflink.h |  2 +-
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 9b572a1fbd42..bdba6b91598a 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -569,10 +569,8 @@ xfs_file_iomap_begin_delay(
 		if (xfs_is_reflink_inode(ip) &&
 		    ((flags & IOMAP_WRITE) ||
 		     got.br_state != XFS_EXT_UNWRITTEN)) {
-			bool		shared;
-
 			xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
-			error = xfs_reflink_reserve_cow(ip, &got, &shared);
+			error = xfs_reflink_reserve_cow(ip, &got);
 			if (error)
 				goto out_unlock;
 		}
@@ -1097,7 +1095,7 @@ xfs_file_iomap_begin(
 			if (error)
 				goto out_unlock;
 		} else {
-			error = xfs_reflink_reserve_cow(ip, &imap, &shared);
+			error = xfs_reflink_reserve_cow(ip, &imap);
 			if (error)
 				goto out_unlock;
 		}
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 5289e22cb081..06e38e88ddee 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -241,7 +241,7 @@ xfs_reflink_trim_around_shared(
 /*
  * Trim the passed in imap to the next shared/unshared extent boundary, and
  * if imap->br_startoff points to a shared extent reserve space for it in the
- * COW fork.  In this case *shared is set to true, else to false.
+ * COW fork.
  *
  * Note that imap will always contain the block numbers for the existing blocks
  * in the data fork, as the upper layers need them for read-modify-write
@@ -250,14 +250,14 @@ xfs_reflink_trim_around_shared(
 int
 xfs_reflink_reserve_cow(
 	struct xfs_inode	*ip,
-	struct xfs_bmbt_irec	*imap,
-	bool			*shared)
+	struct xfs_bmbt_irec	*imap)
 {
 	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
 	struct xfs_bmbt_irec	got;
 	int			error = 0;
 	bool			eof = false, trimmed;
 	struct xfs_iext_cursor	icur;
+	bool			shared;
 
 	/*
 	 * Search the COW fork extent list first.  This serves two purposes:
@@ -273,18 +273,16 @@ xfs_reflink_reserve_cow(
 	if (!eof && got.br_startoff <= imap->br_startoff) {
 		trace_xfs_reflink_cow_found(ip, imap);
 		xfs_trim_extent(imap, got.br_startoff, got.br_blockcount);
-
-		*shared = true;
 		return 0;
 	}
 
 	/* Trim the mapping to the nearest shared extent boundary. */
-	error = xfs_reflink_trim_around_shared(ip, imap, shared, &trimmed);
+	error = xfs_reflink_trim_around_shared(ip, imap, &shared, &trimmed);
 	if (error)
 		return error;
 
 	/* Not shared?  Just report the (potentially capped) extent. */
-	if (!*shared)
+	if (!shared)
 		return 0;
 
 	/*
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index c585ad9552b2..b77f4079022a 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -13,7 +13,7 @@ extern int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
 		struct xfs_bmbt_irec *irec, bool *shared, bool *trimmed);
 
 extern int xfs_reflink_reserve_cow(struct xfs_inode *ip,
-		struct xfs_bmbt_irec *imap, bool *shared);
+		struct xfs_bmbt_irec *imap);
 extern int xfs_reflink_allocate_cow(struct xfs_inode *ip,
 		struct xfs_bmbt_irec *imap, bool *shared, uint *lockmode);
 extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
-- 
2.19.0

[PATCH 6/8] xfs: remove the unused trimmed argument from xfs_reflink_trim_around_shared

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_bmap_util.c |  4 ++--
 fs/xfs/xfs_iomap.c     |  5 ++---
 fs/xfs/xfs_reflink.c   | 15 +++++----------
 fs/xfs/xfs_reflink.h   |  2 +-
 4 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 416524f6ba69..7cfda25f1bb1 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -406,10 +406,10 @@ xfs_getbmap_report_one(
 	struct xfs_bmbt_irec	*got)
 {
 	struct kgetbmap		*p = out + bmv->bmv_entries;
-	bool			shared = false, trimmed = false;
+	bool			shared = false;
 	int			error;
 
-	error = xfs_reflink_trim_around_shared(ip, got, &shared, &trimmed);
+	error = xfs_reflink_trim_around_shared(ip, got, &shared);
 	if (error)
 		return error;
 
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index bdba6b91598a..27c93b5f029d 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1032,7 +1032,7 @@ xfs_file_iomap_begin(
 	struct xfs_bmbt_irec	imap;
 	xfs_fileoff_t		offset_fsb, end_fsb;
 	int			nimaps = 1, error = 0;
-	bool			shared = false, trimmed = false;
+	bool			shared = false;
 	unsigned		lockmode;
 
 	if (XFS_FORCED_SHUTDOWN(mp))
@@ -1068,8 +1068,7 @@ xfs_file_iomap_begin(
 
 	if (flags & IOMAP_REPORT) {
 		/* Trim the mapping to the nearest shared extent boundary. */
-		error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
-				&trimmed);
+		error = xfs_reflink_trim_around_shared(ip, &imap, &shared);
 		if (error)
 			goto out_unlock;
 	}
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 06e38e88ddee..1e39a7d21c7e 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -182,8 +182,7 @@ int
 xfs_reflink_trim_around_shared(
 	struct xfs_inode	*ip,
 	struct xfs_bmbt_irec	*irec,
-	bool			*shared,
-	bool			*trimmed)
+	bool			*shared)
 {
 	xfs_agnumber_t		agno;
 	xfs_agblock_t		agbno;
@@ -209,7 +208,7 @@ xfs_reflink_trim_around_shared(
 	if (error)
 		return error;
 
-	*shared = *trimmed = false;
+	*shared = false;
 	if (fbno == NULLAGBLOCK) {
 		/* No shared blocks at all. */
 		return 0;
@@ -222,8 +221,6 @@ xfs_reflink_trim_around_shared(
 		 */
 		irec->br_blockcount = flen;
 		*shared = true;
-		if (flen != aglen)
-			*trimmed = true;
 		return 0;
 	} else {
 		/*
@@ -233,7 +230,6 @@ xfs_reflink_trim_around_shared(
 		 * start of the shared region.
 		 */
 		irec->br_blockcount = fbno - agbno;
-		*trimmed = true;
 		return 0;
 	}
 }
@@ -255,7 +251,7 @@ xfs_reflink_reserve_cow(
 	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
 	struct xfs_bmbt_irec	got;
 	int			error = 0;
-	bool			eof = false, trimmed;
+	bool			eof = false;
 	struct xfs_iext_cursor	icur;
 	bool			shared;
 
@@ -277,7 +273,7 @@ xfs_reflink_reserve_cow(
 	}
 
 	/* Trim the mapping to the nearest shared extent boundary. */
-	error = xfs_reflink_trim_around_shared(ip, imap, &shared, &trimmed);
+	error = xfs_reflink_trim_around_shared(ip, imap, &shared);
 	if (error)
 		return error;
 
@@ -366,7 +362,6 @@ xfs_find_trim_cow_extent(
 	xfs_filblks_t		count_fsb = imap->br_blockcount;
 	struct xfs_iext_cursor	icur;
 	struct xfs_bmbt_irec	got;
-	bool			trimmed;
 
 	*found = false;
 
@@ -376,7 +371,7 @@ xfs_find_trim_cow_extent(
 	 */
 	if (!xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &icur, &got) ||
 	    got.br_startoff > offset_fsb)
-		return xfs_reflink_trim_around_shared(ip, imap, shared, &trimmed);
+		return xfs_reflink_trim_around_shared(ip, imap, shared);
 
 	*shared = true;
 	if (isnullstartblock(got.br_startblock)) {
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index b77f4079022a..7f47202b5639 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -10,7 +10,7 @@ extern int xfs_reflink_find_shared(struct xfs_mount *mp, struct xfs_trans *tp,
 		xfs_agnumber_t agno, xfs_agblock_t agbno, xfs_extlen_t aglen,
 		xfs_agblock_t *fbno, xfs_extlen_t *flen, bool find_maximal);
 extern int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
-		struct xfs_bmbt_irec *irec, bool *shared, bool *trimmed);
+		struct xfs_bmbt_irec *irec, bool *shared);
 
 extern int xfs_reflink_reserve_cow(struct xfs_inode *ip,
 		struct xfs_bmbt_irec *imap);
-- 
2.19.0

[PATCH 7/8] xfs: fix fork selection in xfs_find_trim_cow_extent

[Christoph Hellwig]

We should want to write directly into the data fork for blocks that don't
have an extent in the COW fork covering them yet.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_reflink.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 1e39a7d21c7e..ead35209ffae 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -369,9 +369,13 @@ xfs_find_trim_cow_extent(
 	 * If we don't find an overlapping extent, trim the range we need to
 	 * allocate to fit the hole we found.
 	 */
-	if (!xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &icur, &got) ||
-	    got.br_startoff > offset_fsb)
+	if (!xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &icur, &got))
+		got.br_startoff = offset_fsb + count_fsb;
+	if (got.br_startoff > offset_fsb) {
+		xfs_trim_extent(imap, imap->br_startoff,
+				got.br_startoff - imap->br_startoff);
 		return xfs_reflink_trim_around_shared(ip, imap, shared);
+	}
 
 	*shared = true;
 	if (isnullstartblock(got.br_startblock)) {
-- 
2.19.0

[PATCH 8/8] xfs: print dangling delalloc extents

[Christoph Hellwig]

Instead of just asserting that we have no delalloc space dangling
in an inode that gets freed print the actual offenders for debug
mode.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_super.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 207ee302b1bb..99250bcb65a7 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -933,6 +933,32 @@ xfs_fs_alloc_inode(
 	return NULL;
 }
 
+#ifdef DEBUG
+static void
+xfs_check_delalloc(
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
+	struct xfs_bmbt_irec	got;
+	struct xfs_iext_cursor	icur;
+
+	if (!ifp || !xfs_iext_lookup_extent(ip, ifp, 0, &icur, &got))
+		return;
+	do {
+		if (isnullstartblock(got.br_startblock)) {
+			xfs_warn(ip->i_mount,
+	"ino %llx %s fork has delalloc extent at [0x%llx:0x%llx]",
+				ip->i_ino,
+				whichfork == XFS_DATA_FORK ? "data" : "cow",
+				got.br_startoff, got.br_blockcount);
+		}
+	} while (xfs_iext_next_extent(ifp, &icur, &got));
+}
+#else
+#define xfs_check_delalloc(ip, whichfork)	do { } while (0)
+#endif
+
 /*
  * Now that the generic code is guaranteed not to be accessing
  * the linux inode, we can inactivate and reclaim the inode.
@@ -951,7 +977,12 @@ xfs_fs_destroy_inode(
 
 	xfs_inactive(ip);
 
-	ASSERT(XFS_FORCED_SHUTDOWN(ip->i_mount) || ip->i_delayed_blks == 0);
+	if (!XFS_FORCED_SHUTDOWN(ip->i_mount) && ip->i_delayed_blks) {
+		xfs_check_delalloc(ip, XFS_DATA_FORK);
+		xfs_check_delalloc(ip, XFS_COW_FORK);
+		ASSERT(0);
+	}
+
 	XFS_STATS_INC(ip->i_mount, vn_reclaim);
 
 	/*
-- 
2.19.0

Re: delalloc and reflink fixes & tweaks V3

[Dave Chinner]

On Tue, Oct 02, 2018 at 10:41:59AM -0700, Christoph Hellwig wrote:
> Hi all,
> 
> this series contains the basic reflink and delalloc fixups intended
> for 4.20.  All the bits that need more work are left out for now.
> 
> Changes since v2:
>   - add a new comment
>   - drop the patch to use unwritten extents for delalloc by default,
>     it didn't actually do what it was intended to do, because there
>     was no test to check for that, and actually changing that caused
>     a few regressions that need more time looking into
>   - add two new unwritten extent related cleanup patches
> 
> Changes since v1:
>   - drop various patches
>   - print the inode number for dangling delalloc extents

Something in this patchset causes generic/127 to corrupt the rmap
btree. 100% failure rate across all my test machines:

[  878.026760] run fstests generic/127 at 2018-10-05 17:31:13
[  888.752797] XFS: Assertion failed: fs_is_ok, file: fs/xfs/libxfs/xfs_rmap.c, line: 418
......
[  888.788156] Call Trace:
[  888.788683]  xfs_rmap_unmap+0x633/0x980
[  888.789461]  ? kmem_zone_alloc+0x61/0xe0
[  888.790245]  xfs_rmap_finish_one+0x2d0/0x340
[  888.791099]  xfs_trans_log_finish_rmap_update+0x2f/0x40
[  888.792146]  xfs_rmap_update_finish_item+0x2c/0x40
[  888.793093]  xfs_defer_finish_noroll+0x184/0x520
[  888.793996]  ? xfs_rmap_update_cancel_item+0x10/0x10
[  888.794965]  ? xfs_free_file_space+0x355/0x390
[  888.795850]  __xfs_trans_commit+0x189/0x370
[  888.796675]  xfs_free_file_space+0x355/0x390
[  888.797523]  xfs_file_fallocate+0x1b3/0x330
[  888.798356]  ? __sb_start_write+0x8d/0xc0
[  888.799153]  vfs_fallocate+0x13d/0x270
[  888.799908]  ksys_fallocate+0x3c/0x70
[  888.800634]  __x64_sys_fallocate+0x1a/0x20
[  888.801454]  do_syscall_64+0x5a/0x180
[  888.802186]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[

And the corruption is then tripped over again after reboot during
log recovery:

[  133.805375] XFS (sdb): Mounting V5 Filesystem
[  134.067122] XFS (sdb): Starting recovery (logdev: internal)
[  134.228779] XFS: Assertion failed: fs_is_ok, file: fs/xfs/libxfs/xfs_rmap.c, line: 545
.....
[  134.253087] Call Trace:
[  134.253630]  xfs_rmap_unmap+0x467/0x980
[  134.254363]  ? kmem_zone_alloc+0x61/0xe0
[  134.255108]  xfs_rmap_finish_one+0x2d0/0x340
[  134.255914]  xfs_trans_log_finish_rmap_update+0x2f/0x40
[  134.256899]  xfs_rui_recover+0x211/0x2d0
[  134.257667]  xlog_recover_process_rui+0x3c/0x60
[  134.258521]  ? xfs_trans_ail_cursor_first+0x17/0x80
[  134.259427]  xlog_recover_process_intents+0x283/0x310
[  134.260374]  ? xfs_iget+0x514/0xad0
[  134.261033]  ? xlog_recover_finish+0x18/0xa0
[  134.261864]  xlog_recover_finish+0x18/0xa0
[  134.262640]  xfs_log_mount_finish+0x6f/0x110
[  134.263430]  xfs_mountfs+0x66f/0x8e0
[  134.264096]  ? xfs_mru_cache_create+0x15a/0x1b0
[  134.264937]  xfs_fs_fill_super+0x4a5/0x650
[  134.265737]  ? xfs_test_remount_options+0x60/0x60
[  134.266625]  mount_bdev+0x174/0x1b0
[  134.267288]  mount_fs+0x30/0x150
[  134.267856]  vfs_kern_mount.part.30+0x54/0x120
[  134.268686]  do_mount+0x5dd/0xc90
[  134.269344]  ? memdup_user+0x3e/0x70
[  134.270491]  ksys_mount+0x80/0xd0
[  134.271555]  __x64_sys_mount+0x21/0x30
[  134.272739]  do_syscall_64+0x5a/0x180
[  134.273459]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com