xfs: skip unnecessary discards

Freed extents are unconditionally discarded when online discard is enabled. Define XFS_BMAPI_NODISCARD to allow callers to bypass discards when unnecessary. For example, this will be useful for eofblocks trimming.

This patch does not change behavior.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_alloc.c  | 10 +++++++---
 fs/xfs/libxfs/xfs_alloc.h  | 27 +++++++++++++++++++++++++--
 fs/xfs/libxfs/xfs_bmap.c   | 17 +++++++++++++----
 fs/xfs/libxfs/xfs_bmap.h   | 30 ++++++++++++++++++++++++++++--
 fs/xfs/xfs_extfree_item.c  |  2 +-
 fs/xfs/xfs_trans.h         |  3 ++-
 fs/xfs/xfs_trans_extfree.c | 13 +++++++++----
 7 files changed, 85 insertions(+), 17 deletions(-)

fs/xfs/libxfs/xfs_alloc.c +7 -3

index 4bcc095fe44a..06e4b0c47c01 100644
@@ -2949,18 +2949,20 @@ xfs_free_extent_fix_freelist(
  * after fixing up the freelist.
  */
 int				/* error */
-xfs_free_extent(
+__xfs_free_extent(
 	struct xfs_trans	*tp,	/* transaction pointer */
 	xfs_fsblock_t		bno,	/* starting block number of extent */
 	xfs_extlen_t		len,	/* length of extent */
 	struct xfs_owner_info	*oinfo,	/* extent owner */
-	enum xfs_ag_resv_type	type)	/* block reservation type */
+	enum xfs_ag_resv_type	type,	/* block reservation type */
+	bool			skip_discard)
 {
 	struct xfs_mount	*mp = tp->t_mountp;
 	struct xfs_buf		*agbp;
 	xfs_agnumber_t		agno = XFS_FSB_TO_AGNO(mp, bno);
 	xfs_agblock_t		agbno = XFS_FSB_TO_AGBNO(mp, bno);
 	int			error;
+	unsigned int		busy_flags = 0;
 
 	ASSERT(len != 0);
 	ASSERT(type != XFS_AG_RESV_AGFL);

@@ -2984,7 +2986,9 @@ xfs_free_extent(
 	if (error)
 		goto err;
 
-	xfs_extent_busy_insert(tp, agno, agbno, len, 0);
+	if (skip_discard)
+		busy_flags |= XFS_EXTENT_BUSY_SKIP_DISCARD;
+	xfs_extent_busy_insert(tp, agno, agbno, len, busy_flags);
 	return 0;
 
 err:

fs/xfs/libxfs/xfs_alloc.h +25 -2

index cbf789ea5a4e..4a8e8dcaf352 100644
@@ -191,12 +191,35 @@ xfs_alloc_vextent(
  * Free an extent.
  */
 int				/* error */
-xfs_free_extent(
+__xfs_free_extent(
 	struct xfs_trans	*tp,	/* transaction pointer */
 	xfs_fsblock_t		bno,	/* starting block number of extent */
 	xfs_extlen_t		len,	/* length of extent */
 	struct xfs_owner_info	*oinfo,	/* extent owner */
-	enum xfs_ag_resv_type	type);	/* block reservation type */
+	enum xfs_ag_resv_type	type,	/* block reservation type */
+	bool			skip_discard);
+
+static inline int
+xfs_free_extent(
+	struct xfs_trans	*tp,
+	xfs_fsblock_t		bno,
+	xfs_extlen_t		len,
+	struct xfs_owner_info	*oinfo,
+	enum xfs_ag_resv_type	type)
+{
+	return __xfs_free_extent(tp, bno, len, oinfo, type, false);
+}
+
+static inline int
+xfs_free_extent_nodiscard(
+	struct xfs_trans	*tp,
+	xfs_fsblock_t		bno,
+	xfs_extlen_t		len,
+	struct xfs_owner_info	*oinfo,
+	enum xfs_ag_resv_type	type)
+{
+	return __xfs_free_extent(tp, bno, len, oinfo, type, true);
+}
 
 int				/* error */
 xfs_alloc_lookup_le(

fs/xfs/libxfs/xfs_bmap.c +13 -4

index 040eeda8426f..dfec27b10e7a 100644
@@ -542,12 +542,13 @@ xfs_bmap_validate_ret(
  * The list is maintained sorted (by block number).
  */
 void
-xfs_bmap_add_free(
+__xfs_bmap_add_free(
 	struct xfs_mount		*mp,
 	struct xfs_defer_ops		*dfops,
 	xfs_fsblock_t			bno,
 	xfs_filblks_t			len,
-	struct xfs_owner_info		*oinfo)
+	struct xfs_owner_info		*oinfo,
+	bool				skip_discard)
 {
 	struct xfs_extent_free_item	*new;		/* new element */
 #ifdef DEBUG

@@ -574,6 +575,7 @@ xfs_bmap_add_free(
 		new->xefi_oinfo = *oinfo;
 	else
 		xfs_rmap_skip_owner_update(&new->xefi_oinfo);
+	new->xefi_skip_discard = skip_discard;
 	trace_xfs_bmap_free_defer(mp, XFS_FSB_TO_AGNO(mp, bno), 0,
 			XFS_FSB_TO_AGBNO(mp, bno), len);
 	xfs_defer_add(dfops, XFS_DEFER_OPS_TYPE_FREE, &new->xefi_list);

@@ -5104,9 +5106,16 @@ xfs_bmap_del_extent_real(
 			error = xfs_refcount_decrease_extent(mp, dfops, del);
 			if (error)
 				goto done;
-		} else
-			xfs_bmap_add_free(mp, dfops, del->br_startblock,
+		} else {
+			if (bflags & XFS_BMAPI_NODISCARD) {
+				xfs_bmap_add_free_nodiscard(mp, dfops,
+					del->br_startblock, del->br_blockcount,
+					NULL);
+			} else {
+				xfs_bmap_add_free(mp, dfops, del->br_startblock,
 					del->br_blockcount, NULL);
+			}
+		}
 	}
 
 	/*

fs/xfs/libxfs/xfs_bmap.h +1 -0

index 2b766b37096d..d8832e049636 100644
@@ -68,6 +68,7 @@ struct xfs_extent_free_item
 	xfs_extlen_t		xefi_blockcount;/* number of blocks in extent */
 	struct list_head	xefi_list;
 	struct xfs_owner_info	xefi_oinfo;	/* extent owner */
+	bool			xefi_skip_discard;
 };
 
 #define	XFS_BMAP_MAX_NMAP	4

fs/xfs/libxfs/xfs_bmap.h +24 -2

@@ -192,9 +196,9 @@ void	xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno,
 void	xfs_trim_extent_eof(struct xfs_bmbt_irec *, struct xfs_inode *);
 int	xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd);
 void	xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork);
-void	xfs_bmap_add_free(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
+void	__xfs_bmap_add_free(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
 			  xfs_fsblock_t bno, xfs_filblks_t len,
-			  struct xfs_owner_info *oinfo);
+			  struct xfs_owner_info *oinfo, bool skip_discard);
 void	xfs_bmap_compute_maxlevels(struct xfs_mount *mp, int whichfork);
 int	xfs_bmap_first_unused(struct xfs_trans *tp, struct xfs_inode *ip,
 		xfs_extlen_t len, xfs_fileoff_t *unused, int whichfork);

@@ -240,6 +244,28 @@ int	xfs_bmapi_reserve_delalloc(struct xfs_inode *ip, int whichfork,
 		struct xfs_bmbt_irec *got, struct xfs_iext_cursor *cur,
 		int eof);
 
+static inline void
+xfs_bmap_add_free(
+	struct xfs_mount		*mp,
+	struct xfs_defer_ops		*dfops,
+	xfs_fsblock_t			bno,
+	xfs_filblks_t			len,
+	struct xfs_owner_info		*oinfo)
+{
+	__xfs_bmap_add_free(mp, dfops, bno, len, oinfo, false);
+}
+
+static inline void
+xfs_bmap_add_free_nodiscard(
+	struct xfs_mount		*mp,
+	struct xfs_defer_ops		*dfops,
+	xfs_fsblock_t			bno,
+	xfs_filblks_t			len,
+	struct xfs_owner_info		*oinfo)
+{
+	__xfs_bmap_add_free(mp, dfops, bno, len, oinfo, true);
+}
+
 enum xfs_bmap_intent_type {
 	XFS_BMAP_MAP = 1,
 	XFS_BMAP_UNMAP,

fs/xfs/xfs_extfree_item.c +1 -1

index b5b1e567b9f4..4735a31793b0 100644
@@ -542,7 +542,7 @@ xfs_efi_recover(
 	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
 		extp = &efip->efi_format.efi_extents[i];
 		error = xfs_trans_free_extent(tp, efdp, extp->ext_start,
-					      extp->ext_len, &oinfo);
+					      extp->ext_len, &oinfo, false);
 		if (error)
 			goto abort_error;
 

fs/xfs/xfs_trans.h +2 -1

index 9d542dfe0052..d5be3f6a3e8f 100644
@@ -228,7 +228,8 @@ struct xfs_efd_log_item	*xfs_trans_get_efd(struct xfs_trans *,
 				  uint);
 int		xfs_trans_free_extent(struct xfs_trans *,
 				      struct xfs_efd_log_item *, xfs_fsblock_t,
-				      xfs_extlen_t, struct xfs_owner_info *);
+				      xfs_extlen_t, struct xfs_owner_info *,
+				      bool);
 int		xfs_trans_commit(struct xfs_trans *);
 int		xfs_trans_roll(struct xfs_trans **);
 int		xfs_trans_roll_inode(struct xfs_trans **, struct xfs_inode *);

fs/xfs/xfs_trans_extfree.c +9 -4

index ab438647592a..dab8b3b7a9b8 100644
@@ -68,7 +68,8 @@ xfs_trans_free_extent(
 	struct xfs_efd_log_item	*efdp,
 	xfs_fsblock_t		start_block,
 	xfs_extlen_t		ext_len,
-	struct xfs_owner_info	*oinfo)
+	struct xfs_owner_info	*oinfo,
+	bool			skip_discard)
 {
 	struct xfs_mount	*mp = tp->t_mountp;
 	uint			next_extent;

@@ -79,8 +80,12 @@ xfs_trans_free_extent(
 
 	trace_xfs_bmap_free_deferred(tp->t_mountp, agno, 0, agbno, ext_len);
 
-	error = xfs_free_extent(tp, start_block, ext_len, oinfo,
-			XFS_AG_RESV_NONE);
+	if (skip_discard)
+		error = xfs_free_extent_nodiscard(tp, start_block, ext_len,
+						  oinfo, XFS_AG_RESV_NONE);
+	else
+		error = xfs_free_extent(tp, start_block, ext_len, oinfo,
+					XFS_AG_RESV_NONE);
 
 	/*
 	 * Mark the transaction dirty, even on error. This ensures the

@@ -195,7 +200,7 @@ xfs_extent_free_finish_item(
 	error = xfs_trans_free_extent(tp, done_item,
 			free->xefi_startblock,
 			free->xefi_blockcount,
-			&free->xefi_oinfo);
+			&free->xefi_oinfo, free->xefi_skip_discard);
 	kmem_free(free);
 	return error;
 }

Signature · 6 lines

2.14.3

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

Quoted 161 lines · Brian Foster

Freed extents are unconditionally discarded when online discard is enabled. Define XFS_BMAPI_NODISCARD to allow callers to bypass discards when unnecessary. For example, this will be useful for eofblocks trimming.

This patch does not change behavior.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_alloc.c  | 10 +++++++---
 fs/xfs/libxfs/xfs_alloc.h  | 27 +++++++++++++++++++++++++--
 fs/xfs/libxfs/xfs_bmap.c   | 17 +++++++++++++----
 fs/xfs/libxfs/xfs_bmap.h   | 30 ++++++++++++++++++++++++++++--
 fs/xfs/xfs_extfree_item.c  |  2 +-
 fs/xfs/xfs_trans.h         |  3 ++-
 fs/xfs/xfs_trans_extfree.c | 13 +++++++++----
 7 files changed, 85 insertions(+), 17 deletions(-)

fs/xfs/libxfs/xfs_alloc.c +7 -3

index 4bcc095fe44a..06e4b0c47c01 100644
@@ -2949,18 +2949,20 @@ xfs_free_extent_fix_freelist(
  * after fixing up the freelist.
  */
 int				/* error */
-xfs_free_extent(
+__xfs_free_extent(
 	struct xfs_trans	*tp,	/* transaction pointer */
 	xfs_fsblock_t		bno,	/* starting block number of extent */
 	xfs_extlen_t		len,	/* length of extent */
 	struct xfs_owner_info	*oinfo,	/* extent owner */
-	enum xfs_ag_resv_type	type)	/* block reservation type */
+	enum xfs_ag_resv_type	type,	/* block reservation type */
+	bool			skip_discard)
 {
 	struct xfs_mount	*mp = tp->t_mountp;
 	struct xfs_buf		*agbp;
 	xfs_agnumber_t		agno = XFS_FSB_TO_AGNO(mp, bno);
 	xfs_agblock_t		agbno = XFS_FSB_TO_AGBNO(mp, bno);
 	int			error;
+	unsigned int		busy_flags = 0;
 
 	ASSERT(len != 0);
 	ASSERT(type != XFS_AG_RESV_AGFL);

@@ -2984,7 +2986,9 @@ xfs_free_extent(
 	if (error)
 		goto err;
 
-	xfs_extent_busy_insert(tp, agno, agbno, len, 0);
+	if (skip_discard)
+		busy_flags |= XFS_EXTENT_BUSY_SKIP_DISCARD;
+	xfs_extent_busy_insert(tp, agno, agbno, len, busy_flags);
 	return 0;
 
 err:

fs/xfs/libxfs/xfs_alloc.h +25 -2

index cbf789ea5a4e..4a8e8dcaf352 100644
@@ -191,12 +191,35 @@ xfs_alloc_vextent(
  * Free an extent.
  */
 int				/* error */
-xfs_free_extent(
+__xfs_free_extent(
 	struct xfs_trans	*tp,	/* transaction pointer */
 	xfs_fsblock_t		bno,	/* starting block number of extent */
 	xfs_extlen_t		len,	/* length of extent */
 	struct xfs_owner_info	*oinfo,	/* extent owner */
-	enum xfs_ag_resv_type	type);	/* block reservation type */
+	enum xfs_ag_resv_type	type,	/* block reservation type */
+	bool			skip_discard);
+
+static inline int
+xfs_free_extent(
+	struct xfs_trans	*tp,
+	xfs_fsblock_t		bno,
+	xfs_extlen_t		len,
+	struct xfs_owner_info	*oinfo,
+	enum xfs_ag_resv_type	type)
+{
+	return __xfs_free_extent(tp, bno, len, oinfo, type, false);
+}
+
+static inline int
+xfs_free_extent_nodiscard(
+	struct xfs_trans	*tp,
+	xfs_fsblock_t		bno,
+	xfs_extlen_t		len,
+	struct xfs_owner_info	*oinfo,
+	enum xfs_ag_resv_type	type)
+{
+	return __xfs_free_extent(tp, bno, len, oinfo, type, true);
+}
 
 int				/* error */
 xfs_alloc_lookup_le(

fs/xfs/libxfs/xfs_bmap.c +13 -4

index 040eeda8426f..dfec27b10e7a 100644
@@ -542,12 +542,13 @@ xfs_bmap_validate_ret(
  * The list is maintained sorted (by block number).
  */
 void
-xfs_bmap_add_free(
+__xfs_bmap_add_free(
 	struct xfs_mount		*mp,
 	struct xfs_defer_ops		*dfops,
 	xfs_fsblock_t			bno,
 	xfs_filblks_t			len,
-	struct xfs_owner_info		*oinfo)
+	struct xfs_owner_info		*oinfo,
+	bool				skip_discard)
 {
 	struct xfs_extent_free_item	*new;		/* new element */
 #ifdef DEBUG

@@ -574,6 +575,7 @@ xfs_bmap_add_free(
 		new->xefi_oinfo = *oinfo;
 	else
 		xfs_rmap_skip_owner_update(&new->xefi_oinfo);
+	new->xefi_skip_discard = skip_discard;
 	trace_xfs_bmap_free_defer(mp, XFS_FSB_TO_AGNO(mp, bno), 0,
 			XFS_FSB_TO_AGBNO(mp, bno), len);
 	xfs_defer_add(dfops, XFS_DEFER_OPS_TYPE_FREE, &new->xefi_list);

@@ -5104,9 +5106,16 @@ xfs_bmap_del_extent_real(
 			error = xfs_refcount_decrease_extent(mp, dfops, del);
 			if (error)
 				goto done;
-		} else
-			xfs_bmap_add_free(mp, dfops, del->br_startblock,
+		} else {
+			if (bflags & XFS_BMAPI_NODISCARD) {
+				xfs_bmap_add_free_nodiscard(mp, dfops,
+					del->br_startblock, del->br_blockcount,
+					NULL);
+			} else {
+				xfs_bmap_add_free(mp, dfops, del->br_startblock,
 					del->br_blockcount, NULL);
+			}
+		}
 	}
 
 	/*

fs/xfs/libxfs/xfs_bmap.h +1 -0

index 2b766b37096d..d8832e049636 100644
@@ -68,6 +68,7 @@ struct xfs_extent_free_item
 	xfs_extlen_t		xefi_blockcount;/* number of blocks in extent */
 	struct list_head	xefi_list;
 	struct xfs_owner_info	xefi_oinfo;	/* extent owner */
+	bool			xefi_skip_discard;
 };
 
 #define	XFS_BMAP_MAX_NMAP	4

@@ -116,6 +117,9 @@ struct xfs_extent_free_item
 /* Only convert unwritten extents, don't allocate new blocks */
 #define XFS_BMAPI_CONVERT_ONLY	0x800
 

fs/xfs/libxfs/xfs_bmap.h struct xfs_extent_free_item · line 125

Quoted 6 lines

fs/xfs/libxfs/xfs_bmap.h +3 -0

+/* Skip online discard of freed extents */
+#define XFS_BMAPI_NODISCARD	0x1000
+
 #define XFS_BMAPI_FLAGS \
 	{ XFS_BMAPI_ENTIRE,	"ENTIRE" }, \
 	{ XFS_BMAPI_METADATA,	"METADATA" }, \

XFS_BMAPI_FLAGS needs to be updated ot have XFS_BMAPI_NODISCARD.

Looks ok otherwise,

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 113 lines

fs/xfs/libxfs/xfs_bmap.h +24 -2

@@ -192,9 +196,9 @@ void	xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno,
 void	xfs_trim_extent_eof(struct xfs_bmbt_irec *, struct xfs_inode *);
 int	xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd);
 void	xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork);
-void	xfs_bmap_add_free(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
+void	__xfs_bmap_add_free(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
 			  xfs_fsblock_t bno, xfs_filblks_t len,
-			  struct xfs_owner_info *oinfo);
+			  struct xfs_owner_info *oinfo, bool skip_discard);
 void	xfs_bmap_compute_maxlevels(struct xfs_mount *mp, int whichfork);
 int	xfs_bmap_first_unused(struct xfs_trans *tp, struct xfs_inode *ip,
 		xfs_extlen_t len, xfs_fileoff_t *unused, int whichfork);

@@ -240,6 +244,28 @@ int	xfs_bmapi_reserve_delalloc(struct xfs_inode *ip, int whichfork,
 		struct xfs_bmbt_irec *got, struct xfs_iext_cursor *cur,
 		int eof);
 
+static inline void
+xfs_bmap_add_free(
+	struct xfs_mount		*mp,
+	struct xfs_defer_ops		*dfops,
+	xfs_fsblock_t			bno,
+	xfs_filblks_t			len,
+	struct xfs_owner_info		*oinfo)
+{
+	__xfs_bmap_add_free(mp, dfops, bno, len, oinfo, false);
+}
+
+static inline void
+xfs_bmap_add_free_nodiscard(
+	struct xfs_mount		*mp,
+	struct xfs_defer_ops		*dfops,
+	xfs_fsblock_t			bno,
+	xfs_filblks_t			len,
+	struct xfs_owner_info		*oinfo)
+{
+	__xfs_bmap_add_free(mp, dfops, bno, len, oinfo, true);
+}
+
 enum xfs_bmap_intent_type {
 	XFS_BMAP_MAP = 1,
 	XFS_BMAP_UNMAP,

fs/xfs/xfs_extfree_item.c +1 -1

index b5b1e567b9f4..4735a31793b0 100644
@@ -542,7 +542,7 @@ xfs_efi_recover(
 	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
 		extp = &efip->efi_format.efi_extents[i];
 		error = xfs_trans_free_extent(tp, efdp, extp->ext_start,
-					      extp->ext_len, &oinfo);
+					      extp->ext_len, &oinfo, false);
 		if (error)
 			goto abort_error;
 

fs/xfs/xfs_trans.h +2 -1

index 9d542dfe0052..d5be3f6a3e8f 100644
@@ -228,7 +228,8 @@ struct xfs_efd_log_item	*xfs_trans_get_efd(struct xfs_trans *,
 				  uint);
 int		xfs_trans_free_extent(struct xfs_trans *,
 				      struct xfs_efd_log_item *, xfs_fsblock_t,
-				      xfs_extlen_t, struct xfs_owner_info *);
+				      xfs_extlen_t, struct xfs_owner_info *,
+				      bool);
 int		xfs_trans_commit(struct xfs_trans *);
 int		xfs_trans_roll(struct xfs_trans **);
 int		xfs_trans_roll_inode(struct xfs_trans **, struct xfs_inode *);

fs/xfs/xfs_trans_extfree.c +9 -4

index ab438647592a..dab8b3b7a9b8 100644
@@ -68,7 +68,8 @@ xfs_trans_free_extent(
 	struct xfs_efd_log_item	*efdp,
 	xfs_fsblock_t		start_block,
 	xfs_extlen_t		ext_len,
-	struct xfs_owner_info	*oinfo)
+	struct xfs_owner_info	*oinfo,
+	bool			skip_discard)
 {
 	struct xfs_mount	*mp = tp->t_mountp;
 	uint			next_extent;

@@ -79,8 +80,12 @@ xfs_trans_free_extent(
 
 	trace_xfs_bmap_free_deferred(tp->t_mountp, agno, 0, agbno, ext_len);
 
-	error = xfs_free_extent(tp, start_block, ext_len, oinfo,
-			XFS_AG_RESV_NONE);
+	if (skip_discard)
+		error = xfs_free_extent_nodiscard(tp, start_block, ext_len,
+						  oinfo, XFS_AG_RESV_NONE);
+	else
+		error = xfs_free_extent(tp, start_block, ext_len, oinfo,
+					XFS_AG_RESV_NONE);
 
 	/*
 	 * Mark the transaction dirty, even on error. This ensures the

@@ -195,7 +200,7 @@ xfs_extent_free_finish_item(
 	error = xfs_trans_free_extent(tp, done_item,
 			free->xefi_startblock,
 			free->xefi_blockcount,
-			&free->xefi_oinfo);
+			&free->xefi_oinfo, free->xefi_skip_discard);
 	kmem_free(free);
 	return error;
 }

Signature · 6 lines

2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 19 lines · Darrick J. Wong

Quoted 18 lines · Brian Foster

Freed extents are unconditionally discarded when online discard is enabled. Define XFS_BMAPI_NODISCARD to allow callers to bypass discards when unnecessary. For example, this will be useful for eofblocks trimming.

This patch does not change behavior.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_alloc.c  | 10 +++++++---
 fs/xfs/libxfs/xfs_alloc.h  | 27 +++++++++++++++++++++++++--
 fs/xfs/libxfs/xfs_bmap.c   | 17 +++++++++++++----
 fs/xfs/libxfs/xfs_bmap.h   | 30 ++++++++++++++++++++++++++++--
 fs/xfs/xfs_extfree_item.c  |  2 +-
 fs/xfs/xfs_trans.h         |  3 ++-
 fs/xfs/xfs_trans_extfree.c | 13 +++++++++----
 7 files changed, 85 insertions(+), 17 deletions(-)

Quoted 16 lines

Quoted 16 lines

fs/xfs/libxfs/xfs_bmap.h +1 -0

index 2b766b37096d..d8832e049636 100644
@@ -68,6 +68,7 @@ struct xfs_extent_free_item
 	xfs_extlen_t		xefi_blockcount;/* number of blocks in extent */
 	struct list_head	xefi_list;
 	struct xfs_owner_info	xefi_oinfo;	/* extent owner */
+	bool			xefi_skip_discard;
 };
 
 #define	XFS_BMAP_MAX_NMAP	4

@@ -116,6 +117,9 @@ struct xfs_extent_free_item
 /* Only convert unwritten extents, don't allocate new blocks */
 #define XFS_BMAPI_CONVERT_ONLY	0x800
 

Quoted 8 lines

fs/xfs/libxfs/xfs_bmap.h struct xfs_extent_free_item · line 125

Quoted 6 lines

fs/xfs/libxfs/xfs_bmap.h +3 -0

+/* Skip online discard of freed extents */
+#define XFS_BMAPI_NODISCARD	0x1000
+
 #define XFS_BMAPI_FLAGS \
 	{ XFS_BMAPI_ENTIRE,	"ENTIRE" }, \
 	{ XFS_BMAPI_METADATA,	"METADATA" }, \

XFS_BMAPI_FLAGS needs to be updated ot have XFS_BMAPI_NODISCARD.

Ok, truncated thought there...

XFS_BMAPI_FLAGS needs to be updated to include a string for XFS_BMAPI_NODISCARD.

Should I just add:

	{ XFS_BMAPI_NODISCARD,	"NODISCARD" }, \

at the end of XFS_BMAPI_FLAGS?

Granted I kinda hate these, because the C definition is emitted as a string as part of every tracepoint definition in the xfs module and again for every trace point definition in the ftrace binary stream, but I guess people find the flags translation useful? I ripped all of mine out prior to submitting reflink/rmap/scrub...

--D

Quoted 115 lines

Looks ok otherwise,

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 109 lines

fs/xfs/libxfs/xfs_bmap.h +24 -2

@@ -192,9 +196,9 @@ void	xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno,
 void	xfs_trim_extent_eof(struct xfs_bmbt_irec *, struct xfs_inode *);
 int	xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd);
 void	xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork);
-void	xfs_bmap_add_free(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
+void	__xfs_bmap_add_free(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
 			  xfs_fsblock_t bno, xfs_filblks_t len,
-			  struct xfs_owner_info *oinfo);
+			  struct xfs_owner_info *oinfo, bool skip_discard);
 void	xfs_bmap_compute_maxlevels(struct xfs_mount *mp, int whichfork);
 int	xfs_bmap_first_unused(struct xfs_trans *tp, struct xfs_inode *ip,
 		xfs_extlen_t len, xfs_fileoff_t *unused, int whichfork);

@@ -240,6 +244,28 @@ int	xfs_bmapi_reserve_delalloc(struct xfs_inode *ip, int whichfork,
 		struct xfs_bmbt_irec *got, struct xfs_iext_cursor *cur,
 		int eof);
 
+static inline void
+xfs_bmap_add_free(
+	struct xfs_mount		*mp,
+	struct xfs_defer_ops		*dfops,
+	xfs_fsblock_t			bno,
+	xfs_filblks_t			len,
+	struct xfs_owner_info		*oinfo)
+{
+	__xfs_bmap_add_free(mp, dfops, bno, len, oinfo, false);
+}
+
+static inline void
+xfs_bmap_add_free_nodiscard(
+	struct xfs_mount		*mp,
+	struct xfs_defer_ops		*dfops,
+	xfs_fsblock_t			bno,
+	xfs_filblks_t			len,
+	struct xfs_owner_info		*oinfo)
+{
+	__xfs_bmap_add_free(mp, dfops, bno, len, oinfo, true);
+}
+
 enum xfs_bmap_intent_type {
 	XFS_BMAP_MAP = 1,
 	XFS_BMAP_UNMAP,

fs/xfs/xfs_extfree_item.c +1 -1

index b5b1e567b9f4..4735a31793b0 100644
@@ -542,7 +542,7 @@ xfs_efi_recover(
 	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
 		extp = &efip->efi_format.efi_extents[i];
 		error = xfs_trans_free_extent(tp, efdp, extp->ext_start,
-					      extp->ext_len, &oinfo);
+					      extp->ext_len, &oinfo, false);
 		if (error)
 			goto abort_error;
 

fs/xfs/xfs_trans.h +2 -1

index 9d542dfe0052..d5be3f6a3e8f 100644
@@ -228,7 +228,8 @@ struct xfs_efd_log_item	*xfs_trans_get_efd(struct xfs_trans *,
 				  uint);
 int		xfs_trans_free_extent(struct xfs_trans *,
 				      struct xfs_efd_log_item *, xfs_fsblock_t,
-				      xfs_extlen_t, struct xfs_owner_info *);
+				      xfs_extlen_t, struct xfs_owner_info *,
+				      bool);
 int		xfs_trans_commit(struct xfs_trans *);
 int		xfs_trans_roll(struct xfs_trans **);
 int		xfs_trans_roll_inode(struct xfs_trans **, struct xfs_inode *);

fs/xfs/xfs_trans_extfree.c +9 -4

index ab438647592a..dab8b3b7a9b8 100644
@@ -68,7 +68,8 @@ xfs_trans_free_extent(
 	struct xfs_efd_log_item	*efdp,
 	xfs_fsblock_t		start_block,
 	xfs_extlen_t		ext_len,
-	struct xfs_owner_info	*oinfo)
+	struct xfs_owner_info	*oinfo,
+	bool			skip_discard)
 {
 	struct xfs_mount	*mp = tp->t_mountp;
 	uint			next_extent;

@@ -79,8 +80,12 @@ xfs_trans_free_extent(
 
 	trace_xfs_bmap_free_deferred(tp->t_mountp, agno, 0, agbno, ext_len);
 
-	error = xfs_free_extent(tp, start_block, ext_len, oinfo,
-			XFS_AG_RESV_NONE);
+	if (skip_discard)
+		error = xfs_free_extent_nodiscard(tp, start_block, ext_len,
+						  oinfo, XFS_AG_RESV_NONE);
+	else
+		error = xfs_free_extent(tp, start_block, ext_len, oinfo,
+					XFS_AG_RESV_NONE);
 
 	/*
 	 * Mark the transaction dirty, even on error. This ensures the

@@ -195,7 +200,7 @@ xfs_extent_free_finish_item(
 	error = xfs_trans_free_extent(tp, done_item,
 			free->xefi_startblock,
 			free->xefi_blockcount,
-			&free->xefi_oinfo);
+			&free->xefi_oinfo, free->xefi_skip_discard);
 	kmem_free(free);
 	return error;
 }

Signature · 1 line

2.14.3

Quoted 8 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Freed extents are unconditionally discarded when online discard is enabled. Define XFS_BMAPI_NODISCARD to allow callers to bypass discards when unnecessary. For example, this will be useful for eofblocks trimming.

This patch does not change behavior.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_alloc.c  | 10 +++++++---
 fs/xfs/libxfs/xfs_alloc.h  | 27 +++++++++++++++++++++++++--
 fs/xfs/libxfs/xfs_bmap.c   | 17 +++++++++++++----
 fs/xfs/libxfs/xfs_bmap.h   | 30 ++++++++++++++++++++++++++++--
 fs/xfs/xfs_extfree_item.c  |  2 +-
 fs/xfs/xfs_trans.h         |  3 ++-
 fs/xfs/xfs_trans_extfree.c | 13 +++++++++----
 7 files changed, 85 insertions(+), 17 deletions(-)

fs/xfs/libxfs/xfs_alloc.c +4 -2

index 4bcc095fe44a..06e4b0c47c01 100644
@@ -2949,18 +2949,20 @@ xfs_free_extent_fix_freelist(
  * after fixing up the freelist.
  */
 int				/* error */
-xfs_free_extent(
+__xfs_free_extent(
 	struct xfs_trans	*tp,	/* transaction pointer */
 	xfs_fsblock_t		bno,	/* starting block number of extent */
 	xfs_extlen_t		len,	/* length of extent */
 	struct xfs_owner_info	*oinfo,	/* extent owner */
-	enum xfs_ag_resv_type	type)	/* block reservation type */
+	enum xfs_ag_resv_type	type,	/* block reservation type */
+	bool			skip_discard)
 {
 	struct xfs_mount	*mp = tp->t_mountp;
 	struct xfs_buf		*agbp;
 	xfs_agnumber_t		agno = XFS_FSB_TO_AGNO(mp, bno);
 	xfs_agblock_t		agbno = XFS_FSB_TO_AGBNO(mp, bno);
 	int			error;
+	unsigned int		busy_flags = 0;
 
 	ASSERT(len != 0);
 	ASSERT(type != XFS_AG_RESV_AGFL);

fs/xfs/libxfs/xfs_alloc.c +3 -2

 void
-xfs_bmap_add_free(
+__xfs_bmap_add_free(
 	struct xfs_mount		*mp,
 	struct xfs_defer_ops		*dfops,
 	xfs_fsblock_t			bno,
 	xfs_filblks_t			len,
-	struct xfs_owner_info		*oinfo)
+	struct xfs_owner_info		*oinfo,
+	bool				skip_discard)
 {
 	struct xfs_extent_free_item	*new;		/* new element */
 #ifdef DEBUG

Quoted 49 lines · Brian Foster

Freed extents are unconditionally discarded when online discard is enabled. Define XFS_BMAPI_NODISCARD to allow callers to bypass discards when unnecessary. For example, this will be useful for eofblocks trimming.

This patch does not change behavior.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_alloc.c  | 10 +++++++---
 fs/xfs/libxfs/xfs_alloc.h  | 27 +++++++++++++++++++++++++--
 fs/xfs/libxfs/xfs_bmap.c   | 17 +++++++++++++----
 fs/xfs/libxfs/xfs_bmap.h   | 30 ++++++++++++++++++++++++++++--
 fs/xfs/xfs_extfree_item.c  |  2 +-
 fs/xfs/xfs_trans.h         |  3 ++-
 fs/xfs/xfs_trans_extfree.c | 13 +++++++++----
 7 files changed, 85 insertions(+), 17 deletions(-)

fs/xfs/libxfs/xfs_alloc.c +4 -2

index 4bcc095fe44a..06e4b0c47c01 100644
@@ -2949,18 +2949,20 @@ xfs_free_extent_fix_freelist(
  * after fixing up the freelist.
  */
 int				/* error */
-xfs_free_extent(
+__xfs_free_extent(
 	struct xfs_trans	*tp,	/* transaction pointer */
 	xfs_fsblock_t		bno,	/* starting block number of extent */
 	xfs_extlen_t		len,	/* length of extent */
 	struct xfs_owner_info	*oinfo,	/* extent owner */
-	enum xfs_ag_resv_type	type)	/* block reservation type */
+	enum xfs_ag_resv_type	type,	/* block reservation type */
+	bool			skip_discard)
 {
 	struct xfs_mount	*mp = tp->t_mountp;
 	struct xfs_buf		*agbp;
 	xfs_agnumber_t		agno = XFS_FSB_TO_AGNO(mp, bno);
 	xfs_agblock_t		agbno = XFS_FSB_TO_AGBNO(mp, bno);
 	int			error;
+	unsigned int		busy_flags = 0;
 
 	ASSERT(len != 0);
 	ASSERT(type != XFS_AG_RESV_AGFL);

@@ -2984,7 +2986,9 @@ xfs_free_extent(
 	if (error)
 		goto err;
 

fs/xfs/libxfs/xfs_alloc.c xfs_free_extent( · line 2990

Quoted 3 lines

fs/xfs/libxfs/xfs_alloc.c +2 -1

-	xfs_extent_busy_insert(tp, agno, agbno, len, 0);
+	if (skip_discard)
+		busy_flags |= XFS_EXTENT_BUSY_SKIP_DISCARD;

Please just add an busy_flags argument to xfs_free_extent, and pass 0 in the oter callers. That removes a lot of the boilerplate code. Then just add an assert to xfs_free_extent that no flag other than XFS_EXTENT_BUSY_SKIP_DISCARD is passed for now.

Quoted 14 lines

fs/xfs/libxfs/xfs_alloc.c +3 -2

 void
-xfs_bmap_add_free(
+__xfs_bmap_add_free(
 	struct xfs_mount		*mp,
 	struct xfs_defer_ops		*dfops,
 	xfs_fsblock_t			bno,
 	xfs_filblks_t			len,
-	struct xfs_owner_info		*oinfo)
+	struct xfs_owner_info		*oinfo,
+	bool				skip_discard)
 {
 	struct xfs_extent_free_item	*new;		/* new element */
 #ifdef DEBUG

fs/xfs/libxfs/xfs_alloc.c xfs_bmap_add_free( · line 578

Quoted 4 lines

fs/xfs/libxfs/xfs_alloc.c +1 -0

@@ -574,6 +575,7 @@ xfs_bmap_add_free(
 		new->xefi_oinfo = *oinfo;
 	else
 		xfs_rmap_skip_owner_update(&new->xefi_oinfo);
+	new->xefi_skip_discard = skip_discard;

Similarly here, I think we should just pass through the bmapi flags. But given the number of callers I think just adding a xfs_bmap_add_free_flags as the low-level interface with a higher level xfs_bmap_add_free wrapper for all other callers is fine. --
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I deliberately went away from that approach based on Dave's feedback (similarly with the xfs_itruncate_extents() comment in the subsequent patch). I used the bool that is used here rather than pass the flag directly, but it's essentially the same idea in terms of exposing the function signature change to callers.

TBH, I don't much care which of the N possible variants of code factoring we use here, but I think we're officially bikeshedding once the feedback starts to go in circles as such. ;) So unless I see some broader/mutual agreement (or a strong preference from the maintainer) on a change back to something like the original factoring (perhaps using a flag instead of a bool), I'm going to assume changing it back will simply reintroduce the previous feedback and leave this as is.

Quoted 5 lines

I deliberately went away from that approach based on Dave's feedback (similarly with the xfs_itruncate_extents() comment in the subsequent patch). I used the bool that is used here rather than pass the flag directly, but it's essentially the same idea in terms of exposing the function signature change to callers.

I haven't had time to catch up on the previous iterations, sorry.

Quoted 8 lines

TBH, I don't much care which of the N possible variants of code factoring we use here, but I think we're officially bikeshedding once the feedback starts to go in circles as such. ;) So unless I see some broader/mutual agreement (or a strong preference from the maintainer) on a change back to something like the original factoring (perhaps using a flag instead of a bool), I'm going to assume changing it back will simply reintroduce the previous feedback and leave this as is.

FYI, I really hate trivial rappers that just hide a single argument. There are a few use cases for them, mostly because the API has a lot of pre-existing callers that don't care and it would create too much churn. But creating wrappers for both possible arguments of a bool is just silly.

Also in general bools can be ver confusing, especially once we grow more than one in a given prototype. If we already only use it to set a flag deeper down I much, much prefer to expose the flag as it is self-documenting, very much unlike a 'true' or 'false' argument.

Quoted 1 line

self-documenting, very much unlike a 'true' or 'false' argument.

I'm fine with replacing the bool argument(s) with flags where applicable if we do eliminate the wrappers. I'm just hesitant to change it given the previous feedback to move away from something very close..

Dave, care to chime in here? As mentioned, I'll do a refactored v3 if there's some kind of consensus/agreement on a final approach.

Quoted 8 lines · Brian Foster

Quoted 1 line

self-documenting, very much unlike a 'true' or 'false' argument.

I'm fine with replacing the bool argument(s) with flags where applicable if we do eliminate the wrappers. I'm just hesitant to change it given the previous feedback to move away from something very close..

Dave, care to chime in here? As mentioned, I'll do a refactored v3 if there's some kind of consensus/agreement on a final approach.

I've read the thread on the original patch now. While not my preference I'm fine with doing an xfs_itruncate_extents_flags with a single xfs_itruncate_extents wrapper and the same for bmapi, as long as we pass flags instead of the bool, and don't add pointless wrappers for the nodiscard case - those are just trickle down flags in general, so we should keep things as simple as possible.

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

Quoted 7 lines

I've read the thread on the original patch now. While not my preference I'm fine with doing an xfs_itruncate_extents_flags with a single xfs_itruncate_extents wrapper and the same for bmapi, as long as we pass flags instead of the bool, and don't add pointless wrappers for the nodiscard case - those are just trickle down flags in general, so we should keep things as simple as possible.

Ok, do you mean to include xfs_free_extent() in that as well? E.g., xfs_free_extent_flags(..., XFS_EXTENT_BUSY_SKIP_DISCARD) vs. a single wrapper without _flags()? Note that that flag is still sourced from a boolean unless we also change the xfs_extent_free_item field, which I'm not sure makes sense. Alternatively, I could just kill the xfs_free_extent_nodiscard() wrapper and call the internal variant from the one place that wants to toggle discard behavior.

We've had reports of online discard operations being sent from XFS on write-only workloads. These discards occur as a result of eofblocks trims that can occur after a large file copy completes.

These discards are slightly confusing for users who might be paying close attention to online discards (i.e., vdo) due to performance sensitivity. They also happen to be spurious because freed post-eof blocks by definition have not been written to during the current allocation cycle.

Update xfs_free_eofblocks() to skip discards that are purely attributed to eofblocks trims. This cuts down the number of spurious discards that may occur on write-only workloads due to normal preallocation activity.

Note that discards of post-eof extents can still occur from other codepaths that do not isolate handling of post-eof blocks from those within eof. For example, file unlinks and truncates may still cause discards for any file blocks affected by the operation.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_bmap_util.c |  4 ++--
 fs/xfs/xfs_inode.c     | 19 +++++++++++--------
 fs/xfs/xfs_inode.h     | 24 ++++++++++++++++++++++--
 3 files changed, 35 insertions(+), 12 deletions(-)

fs/xfs/xfs_bmap_util.c +2 -2

index 8cd8c412f52d..696c3b6bd2c9 100644
@@ -871,8 +871,8 @@ xfs_free_eofblocks(
 		 * contents of the file are flushed to disk then the files
 		 * may be full of holes (ie NULL files bug).
 		 */
-		error = xfs_itruncate_extents(&tp, ip, XFS_DATA_FORK,
-					      XFS_ISIZE(ip));
+		error = xfs_itruncate_extents_nodiscard(&tp, ip, XFS_DATA_FORK,
+							XFS_ISIZE(ip));
 		if (error) {
 			/*
 			 * If we get an error at this point we simply don't

fs/xfs/xfs_inode.c +11 -8

index 2b70c8b4cee2..d63ddd806074 100644
@@ -1534,11 +1534,12 @@ xfs_itruncate_clear_reflink_flags(
  * dirty on error so that transactions can be easily aborted if possible.
  */
 int
-xfs_itruncate_extents(
+__xfs_itruncate_extents(
 	struct xfs_trans	**tpp,
 	struct xfs_inode	*ip,
 	int			whichfork,
-	xfs_fsize_t		new_size)
+	xfs_fsize_t		new_size,
+	bool			skip_discard)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp = *tpp;

@@ -1549,6 +1550,7 @@ xfs_itruncate_extents(
 	xfs_filblks_t		unmap_len;
 	int			error = 0;
 	int			done = 0;
+	int			flags;
 
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(!atomic_read(&VFS_I(ip)->i_count) ||

@@ -1561,6 +1563,10 @@ xfs_itruncate_extents(
 
 	trace_xfs_itruncate_extents_start(ip, new_size);
 
+	flags = xfs_bmapi_aflag(whichfork);
+	if (skip_discard)
+		flags |= XFS_BMAPI_NODISCARD;
+
 	/*
 	 * Since it is possible for space to become allocated beyond
 	 * the end of the file (in a crash where the space is allocated

@@ -1579,12 +1585,9 @@ xfs_itruncate_extents(
 	unmap_len = last_block - first_unmap_block + 1;
 	while (!done) {
 		xfs_defer_init(&dfops, &first_block);
-		error = xfs_bunmapi(tp, ip,
-				    first_unmap_block, unmap_len,
-				    xfs_bmapi_aflag(whichfork),
-				    XFS_ITRUNC_MAX_EXTENTS,
-				    &first_block, &dfops,
-				    &done);
+		error = xfs_bunmapi(tp, ip, first_unmap_block, unmap_len, flags,
+				    XFS_ITRUNC_MAX_EXTENTS, &first_block,
+				    &dfops, &done);
 		if (error)
 			goto out_bmap_cancel;
 

fs/xfs/xfs_inode.h +22 -2

index 1eebc53df7d7..3dd3201f4409 100644
@@ -415,8 +415,8 @@ uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
 uint		xfs_ip2xflags(struct xfs_inode *);
 int		xfs_ifree(struct xfs_trans *, xfs_inode_t *,
 			   struct xfs_defer_ops *);
-int		xfs_itruncate_extents(struct xfs_trans **, struct xfs_inode *,
-				      int, xfs_fsize_t);
+int		__xfs_itruncate_extents(struct xfs_trans **, struct xfs_inode *,
+				        int, xfs_fsize_t, bool);
 void		xfs_iext_realloc(xfs_inode_t *, int, int);
 
 void		xfs_iunpin_wait(xfs_inode_t *);

@@ -433,6 +433,26 @@ int		xfs_dir_ialloc(struct xfs_trans **, struct xfs_inode *, umode_t,
 			       xfs_nlink_t, dev_t, prid_t,
 			       struct xfs_inode **);
 
+static inline int
+xfs_itruncate_extents(
+	struct xfs_trans	**tpp,
+	struct xfs_inode	*ip,
+	int			whichfork,
+	xfs_fsize_t		new_size)
+{
+	return __xfs_itruncate_extents(tpp, ip, whichfork, new_size, false);
+}
+
+static inline int
+xfs_itruncate_extents_nodiscard(
+	struct xfs_trans	**tpp,
+	struct xfs_inode	*ip,
+	int			whichfork,
+	xfs_fsize_t		new_size)
+{
+	return __xfs_itruncate_extents(tpp, ip, whichfork, new_size, true);
+}
+
 /* from xfs_file.c */
 enum xfs_prealloc_flags {
 	XFS_PREALLOC_SET	= (1 << 1),

Signature · 6 lines

2.14.3

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

We've had reports of online discard operations being sent from XFS on write-only workloads. These discards occur as a result of eofblocks trims that can occur after a large file copy completes.

These discards are slightly confusing for users who might be paying close attention to online discards (i.e., vdo) due to performance sensitivity. They also happen to be spurious because freed post-eof blocks by definition have not been written to during the current allocation cycle.

Update xfs_free_eofblocks() to skip discards that are purely attributed to eofblocks trims. This cuts down the number of spurious discards that may occur on write-only workloads due to normal preallocation activity.

Note that discards of post-eof extents can still occur from other codepaths that do not isolate handling of post-eof blocks from those within eof. For example, file unlinks and truncates may still cause discards for any file blocks affected by the operation.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_bmap_util.c |  4 ++--
 fs/xfs/xfs_inode.c     | 19 +++++++++++--------
 fs/xfs/xfs_inode.h     | 24 ++++++++++++++++++++++--
 3 files changed, 35 insertions(+), 12 deletions(-)

fs/xfs/xfs_bmap_util.c +2 -2

index 8cd8c412f52d..696c3b6bd2c9 100644
@@ -871,8 +871,8 @@ xfs_free_eofblocks(
 		 * contents of the file are flushed to disk then the files
 		 * may be full of holes (ie NULL files bug).
 		 */
-		error = xfs_itruncate_extents(&tp, ip, XFS_DATA_FORK,
-					      XFS_ISIZE(ip));
+		error = xfs_itruncate_extents_nodiscard(&tp, ip, XFS_DATA_FORK,
+							XFS_ISIZE(ip));
 		if (error) {
 			/*
 			 * If we get an error at this point we simply don't

fs/xfs/xfs_inode.c +4 -2

index 2b70c8b4cee2..d63ddd806074 100644
@@ -1534,11 +1534,12 @@ xfs_itruncate_clear_reflink_flags(
  * dirty on error so that transactions can be easily aborted if possible.
  */
 int
-xfs_itruncate_extents(
+__xfs_itruncate_extents(
 	struct xfs_trans	**tpp,
 	struct xfs_inode	*ip,
 	int			whichfork,
-	xfs_fsize_t		new_size)
+	xfs_fsize_t		new_size,
+	bool			skip_discard)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp = *tpp;

@@ -1549,6 +1550,7 @@ xfs_itruncate_extents(
 	xfs_filblks_t		unmap_len;
 	int			error = 0;
 	int			done = 0;
+	int			flags;
 
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(!atomic_read(&VFS_I(ip)->i_count) ||

Unwritten extents by definition have not been written to until they are converted to normal written extents. If unwritten extents are freed from a file, it is therefore guaranteed that the blocks have not been written to since allocation (note that zero range punches and reallocates blocks).

To cut down on online discards generated from workloads that make use of preallocation, skip discards of extents if they are in the unwritten state when the extent is freed.

Note that this optimization does not apply to log recovery, during which all freed extents are discarded if online discard is enabled. Also note that it may be possible for a filesystem crash to occur after write completion of an unwritten extent but before unwritten conversion such that the extent remains unwritten after log recovery. Since this pseudo-inconsistency may already be possible after a crash (consider writing to recently allocated blocks where the allocation transaction is lost after a crash), this change shouldn't introduce any fundamental limitations that don't already exist. In short, on storage stacks where discards are important, it's good practice to run an occasional fstrim even with online discard enabled in the filesystem, particularly after a crash.

Signed-off-by Brian Foster <[email protected]>

---
 fs/xfs/libxfs/xfs_bmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/xfs/libxfs/xfs_bmap.c +2 -1

index dfec27b10e7a..24f60ee810e4 100644
@@ -5107,7 +5107,8 @@ xfs_bmap_del_extent_real(
 			if (error)
 				goto done;
 		} else {
-			if (bflags & XFS_BMAPI_NODISCARD) {
+			if ((bflags & XFS_BMAPI_NODISCARD) ||
+			    (del->br_state == XFS_EXT_UNWRITTEN)) {
 				xfs_bmap_add_free_nodiscard(mp, dfops,
 					del->br_startblock, del->br_blockcount,
 					NULL);

Signature · 6 lines

2.14.3

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