[PATCH v2 0/3] xfs: skip unnecessary discards

[PATCH v2 0/3] xfs: skip unnecessary discards

[Brian Foster]

Hi all,

Here's v2 of the skip discards series. The only difference from the
previous version is some factoring cleanups. This has since passed more
(online discard enabled) xfstests runs without exploding.

Thoughts, reviews, flames appreciated.

Brian

v2:
- Minor cleanups.
- Dropped the eofblocks unwritten extent rfc.
v1: https://marc.info/?l=linux-xfs&m=152571670220480&w=2
- Define and use bmapi flag for eofblocks trims.
- Separate patch to skip discards of unwritten extents.
- Various factoring cleanups.
rfc: https://marc.info/?l=linux-xfs&m=152476598825563&w=2

Brian Foster (3):
  xfs: add bmapi nodiscard flag
  xfs: skip online discard during eofblocks trims
  xfs: don't discard on free of unwritten extents

 fs/xfs/libxfs/xfs_alloc.c  | 10 +++++++---
 fs/xfs/libxfs/xfs_alloc.h  | 27 +++++++++++++++++++++++++--
 fs/xfs/libxfs/xfs_bmap.c   | 18 ++++++++++++++----
 fs/xfs/libxfs/xfs_bmap.h   | 30 ++++++++++++++++++++++++++++--
 fs/xfs/xfs_bmap_util.c     |  4 ++--
 fs/xfs/xfs_extfree_item.c  |  2 +-
 fs/xfs/xfs_inode.c         | 19 +++++++++++--------
 fs/xfs/xfs_inode.h         | 24 ++++++++++++++++++++++--
 fs/xfs/xfs_trans.h         |  3 ++-
 fs/xfs/xfs_trans_extfree.c | 13 +++++++++----
 10 files changed, 121 insertions(+), 29 deletions(-)

-- 
2.14.3

[PATCH v2 1/3] xfs: add bmapi nodiscard flag

[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 <bfoster@redhat.com>
---
 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(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 4bcc095fe44a..06e4b0c47c01 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -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:
diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
index cbf789ea5a4e..4a8e8dcaf352 100644
--- a/fs/xfs/libxfs/xfs_alloc.h
+++ b/fs/xfs/libxfs/xfs_alloc.h
@@ -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(
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 040eeda8426f..dfec27b10e7a 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -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);
+			}
+		}
 	}
 
 	/*
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 2b766b37096d..d8832e049636 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -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
 
+/* Skip online discard of freed extents */
+#define XFS_BMAPI_NODISCARD	0x1000
+
 #define XFS_BMAPI_FLAGS \
 	{ XFS_BMAPI_ENTIRE,	"ENTIRE" }, \
 	{ XFS_BMAPI_METADATA,	"METADATA" }, \
@@ -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,
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index b5b1e567b9f4..4735a31793b0 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -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;
 
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 9d542dfe0052..d5be3f6a3e8f 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -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 *);
diff --git a/fs/xfs/xfs_trans_extfree.c b/fs/xfs/xfs_trans_extfree.c
index ab438647592a..dab8b3b7a9b8 100644
--- a/fs/xfs/xfs_trans_extfree.c
+++ b/fs/xfs/xfs_trans_extfree.c
@@ -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;
 }
-- 
2.14.3

[PATCH v2 2/3] xfs: skip online discard during eofblocks trims

[Brian Foster]

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 <bfoster@redhat.com>
---
 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(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 8cd8c412f52d..696c3b6bd2c9 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -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
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 2b70c8b4cee2..d63ddd806074 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -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;
 
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 1eebc53df7d7..3dd3201f4409 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -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),
-- 
2.14.3

[PATCH v2 3/3] xfs: don't discard on free of unwritten extents

[Brian Foster]

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 <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index dfec27b10e7a..24f60ee810e4 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -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);
-- 
2.14.3

Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag

[Darrick J. Wong]

On Tue, May 08, 2018 at 01:22:29PM -0400, Brian Foster wrote:
> 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 <bfoster@redhat.com>
> ---
>  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(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 4bcc095fe44a..06e4b0c47c01 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -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:
> diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
> index cbf789ea5a4e..4a8e8dcaf352 100644
> --- a/fs/xfs/libxfs/xfs_alloc.h
> +++ b/fs/xfs/libxfs/xfs_alloc.h
> @@ -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(
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 040eeda8426f..dfec27b10e7a 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -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);
> +			}
> +		}
>  	}
>  
>  	/*
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index 2b766b37096d..d8832e049636 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -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
>  
> +/* 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 <darrick.wong@oracle.com>

--D

> @@ -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,
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index b5b1e567b9f4..4735a31793b0 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -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;
>  
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 9d542dfe0052..d5be3f6a3e8f 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -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 *);
> diff --git a/fs/xfs/xfs_trans_extfree.c b/fs/xfs/xfs_trans_extfree.c
> index ab438647592a..dab8b3b7a9b8 100644
> --- a/fs/xfs/xfs_trans_extfree.c
> +++ b/fs/xfs/xfs_trans_extfree.c
> @@ -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;
>  }
> -- 
> 2.14.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] xfs: skip online discard during eofblocks trims

[Darrick J. Wong]

On Tue, May 08, 2018 at 01:22:30PM -0400, Brian Foster wrote:
> 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 <bfoster@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  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(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 8cd8c412f52d..696c3b6bd2c9 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -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
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 2b70c8b4cee2..d63ddd806074 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -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;
>  
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 1eebc53df7d7..3dd3201f4409 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -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),
> -- 
> 2.14.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] xfs: don't discard on free of unwritten extents

[Darrick J. Wong]

On Tue, May 08, 2018 at 01:22:31PM -0400, Brian Foster wrote:
> 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 <bfoster@redhat.com>

Looks ok pending testing,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index dfec27b10e7a..24f60ee810e4 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -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);
> -- 
> 2.14.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag

[Darrick J. Wong]

On Tue, May 08, 2018 at 11:12:42AM -0700, Darrick J. Wong wrote:
> On Tue, May 08, 2018 at 01:22:29PM -0400, Brian Foster wrote:
> > 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 <bfoster@redhat.com>
> > ---
> >  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(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index 4bcc095fe44a..06e4b0c47c01 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -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:
> > diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
> > index cbf789ea5a4e..4a8e8dcaf352 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.h
> > +++ b/fs/xfs/libxfs/xfs_alloc.h
> > @@ -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(
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 040eeda8426f..dfec27b10e7a 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -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);
> > +			}
> > +		}
> >  	}
> >  
> >  	/*
> > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> > index 2b766b37096d..d8832e049636 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.h
> > +++ b/fs/xfs/libxfs/xfs_bmap.h
> > @@ -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
> >  
> > +/* 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

> 
> Looks ok otherwise,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> --D
> 
> > @@ -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,
> > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> > index b5b1e567b9f4..4735a31793b0 100644
> > --- a/fs/xfs/xfs_extfree_item.c
> > +++ b/fs/xfs/xfs_extfree_item.c
> > @@ -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;
> >  
> > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> > index 9d542dfe0052..d5be3f6a3e8f 100644
> > --- a/fs/xfs/xfs_trans.h
> > +++ b/fs/xfs/xfs_trans.h
> > @@ -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 *);
> > diff --git a/fs/xfs/xfs_trans_extfree.c b/fs/xfs/xfs_trans_extfree.c
> > index ab438647592a..dab8b3b7a9b8 100644
> > --- a/fs/xfs/xfs_trans_extfree.c
> > +++ b/fs/xfs/xfs_trans_extfree.c
> > @@ -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;
> >  }
> > -- 
> > 2.14.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] xfs: skip online discard during eofblocks trims

[Christoph Hellwig]

On Tue, May 08, 2018 at 01:22:30PM -0400, Brian Foster wrote:
> 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 <bfoster@redhat.com>
> ---
>  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(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 8cd8c412f52d..696c3b6bd2c9 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -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
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 2b70c8b4cee2..d63ddd806074 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -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;

Please just replace the whichfork argument to xfs_itruncate_extents
with a bmapi_flags one, and e'll get a much simpler interface.

Re: [PATCH v2 3/3] xfs: don't discard on free of unwritten extents

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag

[Christoph Hellwig]

On Tue, May 08, 2018 at 01:22:29PM -0400, Brian Foster wrote:
> 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 <bfoster@redhat.com>
> ---
>  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(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 4bcc095fe44a..06e4b0c47c01 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -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;

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.

>  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;

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.

Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag

[Brian Foster]

On Tue, May 08, 2018 at 06:28:08PM -0700, Darrick J. Wong wrote:
> On Tue, May 08, 2018 at 11:12:42AM -0700, Darrick J. Wong wrote:
> > On Tue, May 08, 2018 at 01:22:29PM -0400, Brian Foster wrote:
> > > 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 <bfoster@redhat.com>
> > > ---
> > >  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(-)
> > > 
...
> > > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> > > index 2b766b37096d..d8832e049636 100644
> > > --- a/fs/xfs/libxfs/xfs_bmap.h
> > > +++ b/fs/xfs/libxfs/xfs_bmap.h
> > > @@ -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
> > >  
> > > +/* 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.
> 

Yep, sorry. I should have caught that given it's right beneath the flag
definition. :P

> Should I just add:
> 
> 	{ XFS_BMAPI_NODISCARD,	"NODISCARD" }, \
> 
> at the end of XFS_BMAPI_FLAGS?
> 

Please do, I've made the change locally as well.

> 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...
> 

FWIW, in general I find these string translations useful when sifting
through a large enough pile of related trace data. It's much easier to
quickly and correctly grok the state/flags. I suppose it would be nice
if we could somehow centralize the data structure that sources the
strings, but on a quick look it seems that the trace infrastructure
expects this macro thing in order to create its own internal structure.
:/

Brian

> --D
> 
> > 
> > Looks ok otherwise,
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > --D
> > 
> > > @@ -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,
> > > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> > > index b5b1e567b9f4..4735a31793b0 100644
> > > --- a/fs/xfs/xfs_extfree_item.c
> > > +++ b/fs/xfs/xfs_extfree_item.c
> > > @@ -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;
> > >  
> > > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> > > index 9d542dfe0052..d5be3f6a3e8f 100644
> > > --- a/fs/xfs/xfs_trans.h
> > > +++ b/fs/xfs/xfs_trans.h
> > > @@ -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 *);
> > > diff --git a/fs/xfs/xfs_trans_extfree.c b/fs/xfs/xfs_trans_extfree.c
> > > index ab438647592a..dab8b3b7a9b8 100644
> > > --- a/fs/xfs/xfs_trans_extfree.c
> > > +++ b/fs/xfs/xfs_trans_extfree.c
> > > @@ -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;
> > >  }
> > > -- 
> > > 2.14.3
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag

[Brian Foster]

On Wed, May 09, 2018 at 12:46:29AM -0700, Christoph Hellwig wrote:
> On Tue, May 08, 2018 at 01:22:29PM -0400, Brian Foster wrote:
> > 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 <bfoster@redhat.com>
> > ---
> >  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(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index 4bcc095fe44a..06e4b0c47c01 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -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;
> 
> 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.
> 

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.

Brian

> >  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;
> 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag

[Christoph Hellwig]

> 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.

> 
> 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.

Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag

[Brian Foster]

On Wed, May 09, 2018 at 04:39:42AM -0700, Christoph Hellwig wrote:
> > 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.
> 
> > 
> > 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.
> 

Reasonable argument, for sure...

> 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.

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.

Brian

Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag

[Christoph Hellwig]

On Wed, May 09, 2018 at 08:01:51AM -0400, Brian Foster wrote:
> > 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.

Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag

[Brian Foster]

On Wed, May 09, 2018 at 05:07:38AM -0700, Christoph Hellwig wrote:
> On Wed, May 09, 2018 at 08:01:51AM -0400, Brian Foster wrote:
> > > 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.

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.

Brian

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

Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag

[Christoph Hellwig]

On Wed, May 09, 2018 at 08:47:56AM -0400, Brian Foster wrote:
> > 
> > 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.

I'd store flags in xfs_extent_free_item.  But most importantly
I think the _nodiscard wrappers need to go away.