xfs: skip discard of unwritten extents

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

---

Hi all,

What do folks think of something like this? The motivation here is that the VDO (dedup) devs had reported seeing online discards during write-only workloads. These turn out to be related to trimming post-eof preallocation blocks after large file copies. To my knowledge, this isn't really a prevalent or serious issue, but I think that technically these discards are unnecessary and so I was looking into how we could avoid them.

This behavior is of course not directly related to unwritten extents, but the immediate/obvious solution to bubble up a bmapi flag of some kind to xfs_free_eofblocks() seemed rather crude. From there, I figured that we technically don't need to discard any unwritten extents (within or beyond EOF) because they haven't been written to since being allocated. In fact, I'm not sure we have to even busy them, but it's roughly equivalent logic either way and I'm trying to avoid getting too clever.

I also recall that we've discussed using unwritten extents for delalloc -> real conversion to avoid the small stale data exposure window that exists in writeback. Without getting too deep into the reason we don't currently do an initial unwritten allocation [1], I don't think there's anything blocking us from converting any post-eof blocks that happen to be part of the resulting normal allocation. As it is, the imap is already trimmed to EOF by the writeback code for coherency reasons. If we were to convert post-eof blocks (not part of this patch) along with something like this patch, then we'd indirectly prevent discards for eofblocks trims.

Beyond the whole discard thing, conversion of post-eof blocks may have a couple other advantages. First, we eliminate the aforementioned writeback stale data exposure problem for writes over preallocated blocks (which doesn't solve the fundamental problem, but closes the gap). Second, the zeroing required for post-eof writes that jump over eofblocks (see xfs_file_aio_write_checks()) becomes a much lighter weight operation. Normal blocks are zeroed using buffered writes whereas this is essentially a no-op for unwritten extents.

Thoughts? Flames? Other ideas?

Brian

[1] I think I've actually attempted this change in the past, but I haven't dug through my old git branches as of yet to completely jog my memory. IIRC, this may have been held up by the remnants of buffer_heads being used to track state for the writeback code.

 fs/xfs/libxfs/xfs_alloc.c          | 8 ++++++--
 fs/xfs/libxfs/xfs_alloc.h          | 3 ++-
 fs/xfs/libxfs/xfs_bmap.c           | 9 ++++++---
 fs/xfs/libxfs/xfs_bmap.h           | 3 ++-
 fs/xfs/libxfs/xfs_bmap_btree.c     | 2 +-
 fs/xfs/libxfs/xfs_ialloc.c         | 4 ++--
 fs/xfs/libxfs/xfs_ialloc_btree.c   | 2 +-
 fs/xfs/libxfs/xfs_refcount.c       | 7 ++++---
 fs/xfs/libxfs/xfs_refcount_btree.c | 2 +-
 fs/xfs/xfs_extfree_item.c          | 2 +-
 fs/xfs/xfs_fsops.c                 | 2 +-
 fs/xfs/xfs_reflink.c               | 2 +-
 fs/xfs/xfs_trans.h                 | 3 ++-
 fs/xfs/xfs_trans_extfree.c         | 7 ++++---
 14 files changed, 34 insertions(+), 22 deletions(-)

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

index 4bcc095fe44a..942c90ec6747 100644
@@ -2954,13 +2954,15 @@ xfs_free_extent(
 	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 +2 -1

index cbf789ea5a4e..5c7d8391edc4 100644
@@ -196,7 +196,8 @@ xfs_free_extent(
 	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);
 
 int				/* error */
 xfs_alloc_lookup_le(

fs/xfs/libxfs/xfs_bmap.c +6 -3

index 040eeda8426f..a5a37803f589 100644
@@ -547,7 +547,8 @@ xfs_bmap_add_free(
 	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);

@@ -632,7 +634,7 @@ xfs_bmap_btree_to_extents(
 	if ((error = xfs_btree_check_block(cur, cblock, 0, cbp)))
 		return error;
 	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, whichfork);
-	xfs_bmap_add_free(mp, cur->bc_private.b.dfops, cbno, 1, &oinfo);
+	xfs_bmap_add_free(mp, cur->bc_private.b.dfops, cbno, 1, &oinfo, false);
 	ip->i_d.di_nblocks--;
 	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, -1L);
 	xfs_trans_binval(tp, cbp);

@@ -5106,7 +5108,8 @@ xfs_bmap_del_extent_real(
 				goto done;
 		} else
 			xfs_bmap_add_free(mp, dfops, del->br_startblock,
-					del->br_blockcount, NULL);
+					del->br_blockcount, NULL,
+					del->br_state == XFS_EXT_UNWRITTEN);
 	}
 
 	/*

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

index 2b766b37096d..0d2de22d143e 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

@@ -194,7 +195,7 @@ 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,
 			  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);

fs/xfs/libxfs/xfs_bmap_btree.c +1 -1

index d89d06bea6e3..cecddfcbe11e 100644
@@ -305,7 +305,7 @@ xfs_bmbt_free_block(
 	struct xfs_owner_info	oinfo;
 
 	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, cur->bc_private.b.whichfork);
-	xfs_bmap_add_free(mp, cur->bc_private.b.dfops, fsbno, 1, &oinfo);
+	xfs_bmap_add_free(mp, cur->bc_private.b.dfops, fsbno, 1, &oinfo, false);
 	ip->i_d.di_nblocks--;
 
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

fs/xfs/libxfs/xfs_ialloc.c +2 -2

index de627fa19168..854ebe04c86f 100644
@@ -1837,7 +1837,7 @@ xfs_difree_inode_chunk(
 	if (!xfs_inobt_issparse(rec->ir_holemask)) {
 		/* not sparse, calculate extent info directly */
 		xfs_bmap_add_free(mp, dfops, XFS_AGB_TO_FSB(mp, agno, sagbno),
-				  mp->m_ialloc_blks, &oinfo);
+				  mp->m_ialloc_blks, &oinfo, false);
 		return;
 	}
 

@@ -1881,7 +1881,7 @@ xfs_difree_inode_chunk(
 		ASSERT(agbno % mp->m_sb.sb_spino_align == 0);
 		ASSERT(contigblk % mp->m_sb.sb_spino_align == 0);
 		xfs_bmap_add_free(mp, dfops, XFS_AGB_TO_FSB(mp, agno, agbno),
-				  contigblk, &oinfo);
+				  contigblk, &oinfo, false);
 
 		/* reset range to current bit and carry on... */
 		startidx = endidx = nextbit;

fs/xfs/libxfs/xfs_ialloc_btree.c +1 -1

index 367e9a0726e6..977a33cc60d3 100644
@@ -153,7 +153,7 @@ __xfs_inobt_free_block(
 	xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_INOBT);
 	return xfs_free_extent(cur->bc_tp,
 			XFS_DADDR_TO_FSB(cur->bc_mp, XFS_BUF_ADDR(bp)), 1,
-			&oinfo, resv);
+			&oinfo, resv, false);
 }
 
 STATIC int

fs/xfs/libxfs/xfs_refcount.c +4 -3

index 560e28473024..e5cfbe2534b1 100644
@@ -883,7 +883,8 @@ xfs_refcount_adjust_extents(
 						cur->bc_private.a.agno,
 						tmp.rc_startblock);
 				xfs_bmap_add_free(cur->bc_mp, dfops, fsbno,
-						tmp.rc_blockcount, oinfo);
+						tmp.rc_blockcount, oinfo,
+						false);
 			}
 
 			(*agbno) += tmp.rc_blockcount;

@@ -926,7 +927,7 @@ xfs_refcount_adjust_extents(
 					cur->bc_private.a.agno,
 					ext.rc_startblock);
 			xfs_bmap_add_free(cur->bc_mp, dfops, fsbno,
-					ext.rc_blockcount, oinfo);
+					ext.rc_blockcount, oinfo, false);
 		}
 
 skip:

@@ -1658,7 +1659,7 @@ xfs_refcount_recover_cow_leftovers(
 
 		/* Free the block. */
 		xfs_bmap_add_free(mp, &dfops, fsb,
-				rr->rr_rrec.rc_blockcount, NULL);
+				rr->rr_rrec.rc_blockcount, NULL, false);
 
 		error = xfs_defer_finish(&tp, &dfops);
 		if (error)

fs/xfs/libxfs/xfs_refcount_btree.c +1 -1

index 375abfeb6267..bb0bdc6d97b5 100644
@@ -131,7 +131,7 @@ xfs_refcountbt_free_block(
 	be32_add_cpu(&agf->agf_refcount_blocks, -1);
 	xfs_alloc_log_agf(cur->bc_tp, agbp, XFS_AGF_REFCOUNT_BLOCKS);
 	error = xfs_free_extent(cur->bc_tp, fsbno, 1, &oinfo,
-			XFS_AG_RESV_METADATA);
+			XFS_AG_RESV_METADATA, false);
 	if (error)
 		return error;
 

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_fsops.c +1 -1

index 523792768080..9c555f81431e 100644
@@ -502,7 +502,7 @@ xfs_growfs_data_private(
 		error = xfs_free_extent(tp,
 				XFS_AGB_TO_FSB(mp, agno,
 					be32_to_cpu(agf->agf_length) - new),
-				new, &oinfo, XFS_AG_RESV_NONE);
+				new, &oinfo, XFS_AG_RESV_NONE, false);
 		if (error)
 			goto error0;
 	}

fs/xfs/xfs_reflink.c +1 -1

index cdbd342a5249..08381266ad85 100644
@@ -604,7 +604,7 @@ xfs_reflink_cancel_cow_blocks(
 
 			xfs_bmap_add_free(ip->i_mount, &dfops,
 					del.br_startblock, del.br_blockcount,
-					NULL);
+					NULL, false);
 
 			/* Roll the transaction */
 			xfs_defer_ijoin(&dfops, ip);

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 +4 -3

index ab438647592a..cd2acfa4e562 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;

@@ -80,7 +81,7 @@ 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);
+			XFS_AG_RESV_NONE, skip_discard);
 
 	/*
 	 * Mark the transaction dirty, even on error. This ensures the

@@ -195,7 +196,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.13.6

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

Hi,

Quoted 13 lines · Brian Foster

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

---

Hi all,

What do folks think of something like this? The motivation here is that the VDO (dedup) devs had reported seeing online discards during write-only workloads. These turn out to be related to trimming post-eof preallocation blocks after large file copies. To my knowledge, this isn't really a prevalent or serious issue, but I think that technically these discards are unnecessary and so I was looking into how we could avoid them.

Quoted 9 lines

This behavior is of course not directly related to unwritten extents, but the immediate/obvious solution to bubble up a bmapi flag of some kind to xfs_free_eofblocks() seemed rather crude. From there, I figured that we technically don't need to discard any unwritten extents (within or beyond EOF) because they haven't been written to since being allocated. In fact, I'm not sure we have to even busy them, but it's roughly equivalent logic either way and I'm trying to avoid getting too clever.

There is one thing I'm wondering here, pardon me if my assumptions are wrong.

Regarding the discard of post-eof blocks, one thing that comes to my mind, is what would happen after let's say, a failed fallocate (I've run into this issued a few months ago, and it just came to my mind while looking into this patch). Currently, after a failed fallocate (issued beyond EOF), we do not rollback any successfully allocated extents, keeping such extents allocated to the file, consuming filesystem's space.

Today, such extents are freed when xfs_free_eofblocks() is called, freeing up the space partially reserved by the failed fallocate.

I honestly do not remember if in such situation, we might already have issued any write to the underlying device or not, if we did, issuing a discard here is still useful.

I do *think* no writes have been issued to the block device so skipping discards is not an issue, but I thought it might be worth to bring up such case.

The right approach from user is still to rollback its failed fallocate, but I still think post eof block trim is useful in this case. Though I'm not sure if the discard itself is useful or not.

Thoughts?

Cheers

Quoted 338 lines

I also recall that we've discussed using unwritten extents for delalloc -> real conversion to avoid the small stale data exposure window that exists in writeback. Without getting too deep into the reason we don't currently do an initial unwritten allocation [1], I don't think there's anything blocking us from converting any post-eof blocks that happen to be part of the resulting normal allocation. As it is, the imap is already trimmed to EOF by the writeback code for coherency reasons. If we were to convert post-eof blocks (not part of this patch) along with something like this patch, then we'd indirectly prevent discards for eofblocks trims.

Beyond the whole discard thing, conversion of post-eof blocks may have a couple other advantages. First, we eliminate the aforementioned writeback stale data exposure problem for writes over preallocated blocks (which doesn't solve the fundamental problem, but closes the gap). Second, the zeroing required for post-eof writes that jump over eofblocks (see xfs_file_aio_write_checks()) becomes a much lighter weight operation. Normal blocks are zeroed using buffered writes whereas this is essentially a no-op for unwritten extents.

Thoughts? Flames? Other ideas?

Brian

[1] I think I've actually attempted this change in the past, but I haven't dug through my old git branches as of yet to completely jog my memory. IIRC, this may have been held up by the remnants of buffer_heads being used to track state for the writeback code.

 fs/xfs/libxfs/xfs_alloc.c          | 8 ++++++--
 fs/xfs/libxfs/xfs_alloc.h          | 3 ++-
 fs/xfs/libxfs/xfs_bmap.c           | 9 ++++++---
 fs/xfs/libxfs/xfs_bmap.h           | 3 ++-
 fs/xfs/libxfs/xfs_bmap_btree.c     | 2 +-
 fs/xfs/libxfs/xfs_ialloc.c         | 4 ++--
 fs/xfs/libxfs/xfs_ialloc_btree.c   | 2 +-
 fs/xfs/libxfs/xfs_refcount.c       | 7 ++++---
 fs/xfs/libxfs/xfs_refcount_btree.c | 2 +-
 fs/xfs/xfs_extfree_item.c          | 2 +-
 fs/xfs/xfs_fsops.c                 | 2 +-
 fs/xfs/xfs_reflink.c               | 2 +-
 fs/xfs/xfs_trans.h                 | 3 ++-
 fs/xfs/xfs_trans_extfree.c         | 7 ++++---
 14 files changed, 34 insertions(+), 22 deletions(-)

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

index 4bcc095fe44a..942c90ec6747 100644
@@ -2954,13 +2954,15 @@ xfs_free_extent(
 	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 +2 -1

index cbf789ea5a4e..5c7d8391edc4 100644
@@ -196,7 +196,8 @@ xfs_free_extent(
 	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);
 
 int				/* error */
 xfs_alloc_lookup_le(

fs/xfs/libxfs/xfs_bmap.c +6 -3

index 040eeda8426f..a5a37803f589 100644
@@ -547,7 +547,8 @@ xfs_bmap_add_free(
 	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);

@@ -632,7 +634,7 @@ xfs_bmap_btree_to_extents(
 	if ((error = xfs_btree_check_block(cur, cblock, 0, cbp)))
 		return error;
 	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, whichfork);
-	xfs_bmap_add_free(mp, cur->bc_private.b.dfops, cbno, 1, &oinfo);
+	xfs_bmap_add_free(mp, cur->bc_private.b.dfops, cbno, 1, &oinfo, false);
 	ip->i_d.di_nblocks--;
 	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, -1L);
 	xfs_trans_binval(tp, cbp);

@@ -5106,7 +5108,8 @@ xfs_bmap_del_extent_real(
 				goto done;
 		} else
 			xfs_bmap_add_free(mp, dfops, del->br_startblock,
-					del->br_blockcount, NULL);
+					del->br_blockcount, NULL,
+					del->br_state == XFS_EXT_UNWRITTEN);
 	}
 
 	/*

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

index 2b766b37096d..0d2de22d143e 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

@@ -194,7 +195,7 @@ 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,
 			  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);

fs/xfs/libxfs/xfs_bmap_btree.c +1 -1

index d89d06bea6e3..cecddfcbe11e 100644
@@ -305,7 +305,7 @@ xfs_bmbt_free_block(
 	struct xfs_owner_info	oinfo;
 
 	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, cur->bc_private.b.whichfork);
-	xfs_bmap_add_free(mp, cur->bc_private.b.dfops, fsbno, 1, &oinfo);
+	xfs_bmap_add_free(mp, cur->bc_private.b.dfops, fsbno, 1, &oinfo, false);
 	ip->i_d.di_nblocks--;
 
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

fs/xfs/libxfs/xfs_ialloc.c +2 -2

index de627fa19168..854ebe04c86f 100644
@@ -1837,7 +1837,7 @@ xfs_difree_inode_chunk(
 	if (!xfs_inobt_issparse(rec->ir_holemask)) {
 		/* not sparse, calculate extent info directly */
 		xfs_bmap_add_free(mp, dfops, XFS_AGB_TO_FSB(mp, agno, sagbno),
-				  mp->m_ialloc_blks, &oinfo);
+				  mp->m_ialloc_blks, &oinfo, false);
 		return;
 	}
 

@@ -1881,7 +1881,7 @@ xfs_difree_inode_chunk(
 		ASSERT(agbno % mp->m_sb.sb_spino_align == 0);
 		ASSERT(contigblk % mp->m_sb.sb_spino_align == 0);
 		xfs_bmap_add_free(mp, dfops, XFS_AGB_TO_FSB(mp, agno, agbno),
-				  contigblk, &oinfo);
+				  contigblk, &oinfo, false);
 
 		/* reset range to current bit and carry on... */
 		startidx = endidx = nextbit;

fs/xfs/libxfs/xfs_ialloc_btree.c +1 -1

index 367e9a0726e6..977a33cc60d3 100644
@@ -153,7 +153,7 @@ __xfs_inobt_free_block(
 	xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_INOBT);
 	return xfs_free_extent(cur->bc_tp,
 			XFS_DADDR_TO_FSB(cur->bc_mp, XFS_BUF_ADDR(bp)), 1,
-			&oinfo, resv);
+			&oinfo, resv, false);
 }
 
 STATIC int

fs/xfs/libxfs/xfs_refcount.c +4 -3

index 560e28473024..e5cfbe2534b1 100644
@@ -883,7 +883,8 @@ xfs_refcount_adjust_extents(
 						cur->bc_private.a.agno,
 						tmp.rc_startblock);
 				xfs_bmap_add_free(cur->bc_mp, dfops, fsbno,
-						tmp.rc_blockcount, oinfo);
+						tmp.rc_blockcount, oinfo,
+						false);
 			}
 
 			(*agbno) += tmp.rc_blockcount;

@@ -926,7 +927,7 @@ xfs_refcount_adjust_extents(
 					cur->bc_private.a.agno,
 					ext.rc_startblock);
 			xfs_bmap_add_free(cur->bc_mp, dfops, fsbno,
-					ext.rc_blockcount, oinfo);
+					ext.rc_blockcount, oinfo, false);
 		}
 
 skip:

@@ -1658,7 +1659,7 @@ xfs_refcount_recover_cow_leftovers(
 
 		/* Free the block. */
 		xfs_bmap_add_free(mp, &dfops, fsb,
-				rr->rr_rrec.rc_blockcount, NULL);
+				rr->rr_rrec.rc_blockcount, NULL, false);
 
 		error = xfs_defer_finish(&tp, &dfops);
 		if (error)

fs/xfs/libxfs/xfs_refcount_btree.c +1 -1

index 375abfeb6267..bb0bdc6d97b5 100644
@@ -131,7 +131,7 @@ xfs_refcountbt_free_block(
 	be32_add_cpu(&agf->agf_refcount_blocks, -1);
 	xfs_alloc_log_agf(cur->bc_tp, agbp, XFS_AGF_REFCOUNT_BLOCKS);
 	error = xfs_free_extent(cur->bc_tp, fsbno, 1, &oinfo,
-			XFS_AG_RESV_METADATA);
+			XFS_AG_RESV_METADATA, false);
 	if (error)
 		return error;
 

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_fsops.c +1 -1

index 523792768080..9c555f81431e 100644
@@ -502,7 +502,7 @@ xfs_growfs_data_private(
 		error = xfs_free_extent(tp,
 				XFS_AGB_TO_FSB(mp, agno,
 					be32_to_cpu(agf->agf_length) - new),
-				new, &oinfo, XFS_AG_RESV_NONE);
+				new, &oinfo, XFS_AG_RESV_NONE, false);
 		if (error)
 			goto error0;
 	}

fs/xfs/xfs_reflink.c +1 -1

index cdbd342a5249..08381266ad85 100644
@@ -604,7 +604,7 @@ xfs_reflink_cancel_cow_blocks(
 
 			xfs_bmap_add_free(ip->i_mount, &dfops,
 					del.br_startblock, del.br_blockcount,
-					NULL);
+					NULL, false);
 
 			/* Roll the transaction */
 			xfs_defer_ijoin(&dfops, ip);

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 +4 -3

index ab438647592a..cd2acfa4e562 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;

@@ -80,7 +81,7 @@ 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);
+			XFS_AG_RESV_NONE, skip_discard);
 
 	/*
 	 * Mark the transaction dirty, even on error. This ensures the

@@ -195,7 +196,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.13.6

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

Signature · 5 lines

Carlos
--
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 10 lines

Quoted 9 lines

This behavior is of course not directly related to unwritten extents, but the immediate/obvious solution to bubble up a bmapi flag of some kind to xfs_free_eofblocks() seemed rather crude. From there, I figured that we technically don't need to discard any unwritten extents (within or beyond EOF) because they haven't been written to since being allocated. In fact, I'm not sure we have to even busy them, but it's roughly equivalent logic either way and I'm trying to avoid getting too clever.

Quoted 9 lines

There is one thing I'm wondering here, pardon me if my assumptions are wrong.

Regarding the discard of post-eof blocks, one thing that comes to my mind, is what would happen after let's say, a failed fallocate (I've run into this issued a few months ago, and it just came to my mind while looking into this patch). Currently, after a failed fallocate (issued beyond EOF), we do not rollback any successfully allocated extents, keeping such extents allocated to the file, consuming filesystem's space.

Right.. I think this is fairly generic behavior, regardless of whether some of the extents extend or run past eof. IOW, a failed falloc is simply a partial falloc operation.

Quoted 3 lines

Today, such extents are freed when xfs_free_eofblocks() is called, freeing up the space partially reserved by the failed fallocate.

Hmm, I suspect this is more of an implementation detail than a hard and fast rule. The eofblocks trimming should not occur on any inode that has user-driven preallocation, which is marked via XFS_DIFLAG_PREALLOC. I haven't tested behavior, but on a quick look it appears we set this flag only after successful completion of xfs_alloc_file_space() in the fallocate path.

I think that means it's possible for a first fallocate on an inode to partially succeed allocating blocks and fail to set the flag. This means an eofblocks trim could affect such blocks. I also think that means it's possible for a subsequent fallocate to partially succeed where the flag may have already been set on the inode..?

If so, that means a subsequent
eofblocks trim would not affect such blocks.

FWIW, I'm not terribly concerned with error handling behavior in this case. Ultimately the user will need to fix up the file one way or another and this patch is more concerned about discard of the resulting blocks if they are freed (independent of whether they should have been freed in the first place).

Quoted 4 lines

I honestly do not remember if in such situation, we might already have issued any write to the underlying device or not, if we did, issuing a discard here is still useful.

eofblocks aside, I think it's always possible for the fs to send writes and then crash after some of those writes have completed but before we've converted unwritten extents. If the latter transaction is lost, the blocks will have been written by an underlying thin device (for e.g.) but not tracked as such in the fs. Failure to discard such blocks if they are eventually freed means the underlying device would not release them.

Quoted 3 lines

I do *think* no writes have been issued to the block device so skipping discards is not an issue, but I thought it might be worth to bring up such case.

I think it's a good point wrt to the discard. I'm not totally sure what the right answer is. On one hand, I think the discards are technically advisory so it might be a worthwhile tradeoff for a rare situation that can already be introduced in other ways (such as a discard being ignored or an intermediate mount without online discard enabled). We could always recommend an fstrim after a log recovery, for example.

On the other hand, others might think that it's more appropriate to be aggressive with online discards in the fs as we are today to provide the device with as much information as possible.

Brian

Quoted 341 lines

The right approach from user is still to rollback its failed fallocate, but I still think post eof block trim is useful in this case. Though I'm not sure if the discard itself is useful or not.

Thoughts?

Cheers

Quoted 334 lines

I also recall that we've discussed using unwritten extents for delalloc -> real conversion to avoid the small stale data exposure window that exists in writeback. Without getting too deep into the reason we don't currently do an initial unwritten allocation [1], I don't think there's anything blocking us from converting any post-eof blocks that happen to be part of the resulting normal allocation. As it is, the imap is already trimmed to EOF by the writeback code for coherency reasons. If we were to convert post-eof blocks (not part of this patch) along with something like this patch, then we'd indirectly prevent discards for eofblocks trims.

Beyond the whole discard thing, conversion of post-eof blocks may have a couple other advantages. First, we eliminate the aforementioned writeback stale data exposure problem for writes over preallocated blocks (which doesn't solve the fundamental problem, but closes the gap). Second, the zeroing required for post-eof writes that jump over eofblocks (see xfs_file_aio_write_checks()) becomes a much lighter weight operation. Normal blocks are zeroed using buffered writes whereas this is essentially a no-op for unwritten extents.

Thoughts? Flames? Other ideas?

Brian

[1] I think I've actually attempted this change in the past, but I haven't dug through my old git branches as of yet to completely jog my memory. IIRC, this may have been held up by the remnants of buffer_heads being used to track state for the writeback code.

 fs/xfs/libxfs/xfs_alloc.c          | 8 ++++++--
 fs/xfs/libxfs/xfs_alloc.h          | 3 ++-
 fs/xfs/libxfs/xfs_bmap.c           | 9 ++++++---
 fs/xfs/libxfs/xfs_bmap.h           | 3 ++-
 fs/xfs/libxfs/xfs_bmap_btree.c     | 2 +-
 fs/xfs/libxfs/xfs_ialloc.c         | 4 ++--
 fs/xfs/libxfs/xfs_ialloc_btree.c   | 2 +-
 fs/xfs/libxfs/xfs_refcount.c       | 7 ++++---
 fs/xfs/libxfs/xfs_refcount_btree.c | 2 +-
 fs/xfs/xfs_extfree_item.c          | 2 +-
 fs/xfs/xfs_fsops.c                 | 2 +-
 fs/xfs/xfs_reflink.c               | 2 +-
 fs/xfs/xfs_trans.h                 | 3 ++-
 fs/xfs/xfs_trans_extfree.c         | 7 ++++---
 14 files changed, 34 insertions(+), 22 deletions(-)

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

index 4bcc095fe44a..942c90ec6747 100644
@@ -2954,13 +2954,15 @@ xfs_free_extent(
 	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 +2 -1

index cbf789ea5a4e..5c7d8391edc4 100644
@@ -196,7 +196,8 @@ xfs_free_extent(
 	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);
 
 int				/* error */
 xfs_alloc_lookup_le(

fs/xfs/libxfs/xfs_bmap.c +6 -3

index 040eeda8426f..a5a37803f589 100644
@@ -547,7 +547,8 @@ xfs_bmap_add_free(
 	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);

@@ -632,7 +634,7 @@ xfs_bmap_btree_to_extents(
 	if ((error = xfs_btree_check_block(cur, cblock, 0, cbp)))
 		return error;
 	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, whichfork);
-	xfs_bmap_add_free(mp, cur->bc_private.b.dfops, cbno, 1, &oinfo);
+	xfs_bmap_add_free(mp, cur->bc_private.b.dfops, cbno, 1, &oinfo, false);
 	ip->i_d.di_nblocks--;
 	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, -1L);
 	xfs_trans_binval(tp, cbp);

@@ -5106,7 +5108,8 @@ xfs_bmap_del_extent_real(
 				goto done;
 		} else
 			xfs_bmap_add_free(mp, dfops, del->br_startblock,
-					del->br_blockcount, NULL);
+					del->br_blockcount, NULL,
+					del->br_state == XFS_EXT_UNWRITTEN);
 	}
 
 	/*

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

index 2b766b37096d..0d2de22d143e 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

@@ -194,7 +195,7 @@ 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,
 			  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);

fs/xfs/libxfs/xfs_bmap_btree.c +1 -1

index d89d06bea6e3..cecddfcbe11e 100644
@@ -305,7 +305,7 @@ xfs_bmbt_free_block(
 	struct xfs_owner_info	oinfo;
 
 	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, cur->bc_private.b.whichfork);
-	xfs_bmap_add_free(mp, cur->bc_private.b.dfops, fsbno, 1, &oinfo);
+	xfs_bmap_add_free(mp, cur->bc_private.b.dfops, fsbno, 1, &oinfo, false);
 	ip->i_d.di_nblocks--;
 
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

fs/xfs/libxfs/xfs_ialloc.c +2 -2

index de627fa19168..854ebe04c86f 100644
@@ -1837,7 +1837,7 @@ xfs_difree_inode_chunk(
 	if (!xfs_inobt_issparse(rec->ir_holemask)) {
 		/* not sparse, calculate extent info directly */
 		xfs_bmap_add_free(mp, dfops, XFS_AGB_TO_FSB(mp, agno, sagbno),
-				  mp->m_ialloc_blks, &oinfo);
+				  mp->m_ialloc_blks, &oinfo, false);
 		return;
 	}
 

@@ -1881,7 +1881,7 @@ xfs_difree_inode_chunk(
 		ASSERT(agbno % mp->m_sb.sb_spino_align == 0);
 		ASSERT(contigblk % mp->m_sb.sb_spino_align == 0);
 		xfs_bmap_add_free(mp, dfops, XFS_AGB_TO_FSB(mp, agno, agbno),
-				  contigblk, &oinfo);
+				  contigblk, &oinfo, false);
 
 		/* reset range to current bit and carry on... */
 		startidx = endidx = nextbit;

fs/xfs/libxfs/xfs_ialloc_btree.c +1 -1

index 367e9a0726e6..977a33cc60d3 100644
@@ -153,7 +153,7 @@ __xfs_inobt_free_block(
 	xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_INOBT);
 	return xfs_free_extent(cur->bc_tp,
 			XFS_DADDR_TO_FSB(cur->bc_mp, XFS_BUF_ADDR(bp)), 1,
-			&oinfo, resv);
+			&oinfo, resv, false);
 }
 
 STATIC int

fs/xfs/libxfs/xfs_refcount.c +4 -3

index 560e28473024..e5cfbe2534b1 100644
@@ -883,7 +883,8 @@ xfs_refcount_adjust_extents(
 						cur->bc_private.a.agno,
 						tmp.rc_startblock);
 				xfs_bmap_add_free(cur->bc_mp, dfops, fsbno,
-						tmp.rc_blockcount, oinfo);
+						tmp.rc_blockcount, oinfo,
+						false);
 			}
 
 			(*agbno) += tmp.rc_blockcount;

@@ -926,7 +927,7 @@ xfs_refcount_adjust_extents(
 					cur->bc_private.a.agno,
 					ext.rc_startblock);
 			xfs_bmap_add_free(cur->bc_mp, dfops, fsbno,
-					ext.rc_blockcount, oinfo);
+					ext.rc_blockcount, oinfo, false);
 		}
 
 skip:

@@ -1658,7 +1659,7 @@ xfs_refcount_recover_cow_leftovers(
 
 		/* Free the block. */
 		xfs_bmap_add_free(mp, &dfops, fsb,
-				rr->rr_rrec.rc_blockcount, NULL);
+				rr->rr_rrec.rc_blockcount, NULL, false);
 
 		error = xfs_defer_finish(&tp, &dfops);
 		if (error)

fs/xfs/libxfs/xfs_refcount_btree.c +1 -1

index 375abfeb6267..bb0bdc6d97b5 100644
@@ -131,7 +131,7 @@ xfs_refcountbt_free_block(
 	be32_add_cpu(&agf->agf_refcount_blocks, -1);
 	xfs_alloc_log_agf(cur->bc_tp, agbp, XFS_AGF_REFCOUNT_BLOCKS);
 	error = xfs_free_extent(cur->bc_tp, fsbno, 1, &oinfo,
-			XFS_AG_RESV_METADATA);
+			XFS_AG_RESV_METADATA, false);
 	if (error)
 		return error;
 

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_fsops.c +1 -1

index 523792768080..9c555f81431e 100644
@@ -502,7 +502,7 @@ xfs_growfs_data_private(
 		error = xfs_free_extent(tp,
 				XFS_AGB_TO_FSB(mp, agno,
 					be32_to_cpu(agf->agf_length) - new),
-				new, &oinfo, XFS_AG_RESV_NONE);
+				new, &oinfo, XFS_AG_RESV_NONE, false);
 		if (error)
 			goto error0;
 	}

fs/xfs/xfs_reflink.c +1 -1

index cdbd342a5249..08381266ad85 100644
@@ -604,7 +604,7 @@ xfs_reflink_cancel_cow_blocks(
 
 			xfs_bmap_add_free(ip->i_mount, &dfops,
 					del.br_startblock, del.br_blockcount,
-					NULL);
+					NULL, false);
 
 			/* Roll the transaction */
 			xfs_defer_ijoin(&dfops, ip);

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 +4 -3

index ab438647592a..cd2acfa4e562 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;

@@ -80,7 +81,7 @@ 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);
+			XFS_AG_RESV_NONE, skip_discard);
 
 	/*
 	 * Mark the transaction dirty, even on error. This ensures the

@@ -195,7 +196,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.13.6

Quoted 11 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

Signature · 5 lines

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

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

---

Hi all,

What do folks think of something like this?

The motivation here is that
the VDO (dedup) devs had reported seeing online discards during write-only workloads. These turn out to be related to trimming post-eof preallocation blocks after large file copies. To my knowledge, this isn't really a prevalent or serious issue, but I think that technically these discards are unnecessary and so I was looking into how we could avoid them.

This behavior is of course not directly related to unwritten extents, but the immediate/obvious solution to bubble up a bmapi flag of some kind to xfs_free_eofblocks() seemed rather crude. From there, I figured that we technically don't need to discard any unwritten extents (within or beyond EOF) because they haven't been written to since being allocated. In fact, I'm not sure we have to even busy them, but it's roughly equivalent logic either way and I'm trying to avoid getting too clever.

I also recall that we've discussed using unwritten extents for delalloc -> real conversion to avoid the small stale data exposure window that exists in writeback. Without getting too deep into the reason we don't currently do an initial unwritten allocation [1], I don't think there's anything blocking us from converting any post-eof blocks that happen to be part of the resulting normal allocation. As it is, the imap is already trimmed to EOF by the writeback code for coherency reasons. If we were to convert post-eof blocks (not part of this patch) along with something like this patch, then we'd indirectly prevent discards for eofblocks trims.

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

index 4bcc095fe44a..942c90ec6747 100644
@@ -2954,13 +2954,15 @@ xfs_free_extent(
 	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);

Quoted 6 lines · Brian Foster

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

---

Hi all,

What do folks think of something like this?

Definitely sounds like something we need to address.

Quoted 7 lines

The motivation here is that
the VDO (dedup) devs had reported seeing online discards during write-only workloads. These turn out to be related to trimming post-eof preallocation blocks after large file copies. To my knowledge, this isn't really a prevalent or serious issue, but I think that technically these discards are unnecessary and so I was looking into how we could avoid them.

We simply trucate post-eof extents, right? So we know in xfs_itruncate_extents() if the inode size is changing, not to mention we know if the extent is beyond EOF? e.g. all calls to xfs_itruncate_extents() other than xfs_free_eofblocks() change the inode size and so directly indicate they are removing written blocks. Anything where the inode size is not changing is doing a post-eof removal, and so we can assume no data has been written?

So rather than converting everything to unwritten extents, the "skip discard flag" is simply triggered via extents being freed sitting beyond the current EOF (not the new EOF) and/or being unwritten?

Quoted 8 lines

This behavior is of course not directly related to unwritten extents, but the immediate/obvious solution to bubble up a bmapi flag of some kind to xfs_free_eofblocks() seemed rather crude. From there, I figured that we technically don't need to discard any unwritten extents (within or beyond EOF) because they haven't been written to since being allocated. In fact, I'm not sure we have to even busy them, but it's roughly equivalent logic either way and I'm trying to avoid getting too clever.

I think we still need to busy them to avoid re-allocating them in the same checkpoint, as data extent free/realloc in the same checkpoint could result in a failed recovery (i.e. partial checkpoint replay) leaving the extent linked into two separate files.

Quoted 10 lines

I also recall that we've discussed using unwritten extents for delalloc -> real conversion to avoid the small stale data exposure window that exists in writeback. Without getting too deep into the reason we don't currently do an initial unwritten allocation [1], I don't think there's anything blocking us from converting any post-eof blocks that happen to be part of the resulting normal allocation. As it is, the imap is already trimmed to EOF by the writeback code for coherency reasons. If we were to convert post-eof blocks (not part of this patch) along with something like this patch, then we'd indirectly prevent discards for eofblocks trims.

I think we should leave that as a separate problem, as writeback currently has issues with the way we manage bufferhead state. i.e. things don't work right if we put unwritten extents under delalloc buffers and vice versa. [ I have patches to address that I'm working on.] And there's also the issue that we need to change the delalloc reservations to take into account block allocations required by unwritten extent conversion needed by delalloc.

Hence I think we should address that as a separate problem, not as the solution to avoiding discard of post-eof extents.

Quoted 25 lines

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

index 4bcc095fe44a..942c90ec6747 100644
@@ -2954,13 +2954,15 @@ xfs_free_extent(
 	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 2992

Quoted 5 lines

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

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

Rather than changing xfs_free_extent(), how about adding a xfs_free_extent_nodiscard() wrapper, and only call it when processing an extent that doesn't need discard? This means none of the other code that frees extents needs to be changed. Similarly add a xfs_bmap_add_free_nodiscard() wrapper. That will cut down on the code churn caused by passing new parameters everywhere....

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
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 18 lines · Dave Chinner

Quoted 6 lines · Brian Foster

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

---

Hi all,

What do folks think of something like this?

Definitely sounds like something we need to address.

Quoted 7 lines

The motivation here is that
the VDO (dedup) devs had reported seeing online discards during write-only workloads. These turn out to be related to trimming post-eof preallocation blocks after large file copies. To my knowledge, this isn't really a prevalent or serious issue, but I think that technically these discards are unnecessary and so I was looking into how we could avoid them.

Quoted 8 lines

We simply trucate post-eof extents, right? So we know in xfs_itruncate_extents() if the inode size is changing, not to mention we know if the extent is beyond EOF? e.g. all calls to xfs_itruncate_extents() other than xfs_free_eofblocks() change the inode size and so directly indicate they are removing written blocks. Anything where the inode size is not changing is doing a post-eof removal, and so we can assume no data has been written?

Yes, xfs_free_eofblocks() is the only caller explicitly responsible for trimming post-eof extents.

Quoted 4 lines

So rather than converting everything to unwritten extents, the "skip discard flag" is simply triggered via extents being freed sitting beyond the current EOF (not the new EOF) and/or being unwritten?

That was pretty much my initial thought, but note that the extent free is ultimately deferred down in xfs_free_eofblocks() -> xfs_itruncate_extents() -> xfs_bunmapi() -> xfs_bmap_del_extent_real() -> xfs_bmap_add_free(). We can communicate this down to that point with an itruncate_extents() parameter and XFS_BMAPI_NODISCARD flag or some such, it just seemed a bit kludgy to pass that down through those layers when the unwritten state is known in the bunmapi code (but I'll take that approach if preferred).

Quoted 9 lines

Quoted 8 lines

This behavior is of course not directly related to unwritten extents, but the immediate/obvious solution to bubble up a bmapi flag of some kind to xfs_free_eofblocks() seemed rather crude. From there, I figured that we technically don't need to discard any unwritten extents (within or beyond EOF) because they haven't been written to since being allocated. In fact, I'm not sure we have to even busy them, but it's roughly equivalent logic either way and I'm trying to avoid getting too clever.

Quoted 6 lines

I think we still need to busy them to avoid re-allocating them in the same checkpoint, as data extent free/realloc in the same checkpoint could result in a failed recovery (i.e. partial checkpoint replay) leaving the extent linked into two separate files.

Ah, Ok.. I was only thinking about metadata/data reuse. Hm, isn't the filesystem essentially corrupted on a failed/partial recovery anyways? (Not that this matters much in this context, I wasn't planning to bypass the busy sequence...).

Quoted 11 lines

Quoted 10 lines

I also recall that we've discussed using unwritten extents for delalloc -> real conversion to avoid the small stale data exposure window that exists in writeback. Without getting too deep into the reason we don't currently do an initial unwritten allocation [1], I don't think there's anything blocking us from converting any post-eof blocks that happen to be part of the resulting normal allocation. As it is, the imap is already trimmed to EOF by the writeback code for coherency reasons. If we were to convert post-eof blocks (not part of this patch) along with something like this patch, then we'd indirectly prevent discards for eofblocks trims.

Quoted 8 lines

I think we should leave that as a separate problem, as writeback currently has issues with the way we manage bufferhead state. i.e. things don't work right if we put unwritten extents under delalloc buffers and vice versa. [ I have patches to address that I'm working on.] And there's also the issue that we need to change the delalloc reservations to take into account block allocations required by unwritten extent conversion needed by delalloc.

Right.. I wasn't planning to try and solve the whole buffer head state mismatch thing as a dependency to not discard eofblocks. I was only going to convert blocks that happened to be post-eof after the xfs_iomap_write_allocate() allocation because those blocks by definition don't have buffers. So it's essentially just another xfs_bmapi_write() call from xfs_iomap_write_allocate() to convert eofblocks if the just allocated mapping crosses eof.

Quoted 3 lines

Hence I think we should address that as a separate problem, not as the solution to avoiding discard of post-eof extents.

Fair enough, I'll look into tacking on a separate patch to also skip discards for unwritten extents (irrespective of eof).

Quoted 36 lines · Brian Foster

Quoted 18 lines · Dave Chinner

Quoted 6 lines · Brian Foster

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

---

Hi all,

What do folks think of something like this?

Definitely sounds like something we need to address.

Quoted 7 lines

The motivation here is that
the VDO (dedup) devs had reported seeing online discards during write-only workloads. These turn out to be related to trimming post-eof preallocation blocks after large file copies. To my knowledge, this isn't really a prevalent or serious issue, but I think that technically these discards are unnecessary and so I was looking into how we could avoid them.

Quoted 8 lines

We simply trucate post-eof extents, right? So we know in xfs_itruncate_extents() if the inode size is changing, not to mention we know if the extent is beyond EOF? e.g. all calls to xfs_itruncate_extents() other than xfs_free_eofblocks() change the inode size and so directly indicate they are removing written blocks. Anything where the inode size is not changing is doing a post-eof removal, and so we can assume no data has been written?

Yes, xfs_free_eofblocks() is the only caller explicitly responsible for trimming post-eof extents.

Quoted 4 lines

So rather than converting everything to unwritten extents, the "skip discard flag" is simply triggered via extents being freed sitting beyond the current EOF (not the new EOF) and/or being unwritten?

Quoted 8 lines

That was pretty much my initial thought, but note that the extent free is ultimately deferred down in xfs_free_eofblocks() -> xfs_itruncate_extents() -> xfs_bunmapi() -> xfs_bmap_del_extent_real() -> xfs_bmap_add_free(). We can communicate this down to that point with an itruncate_extents() parameter and XFS_BMAPI_NODISCARD flag or some such, it just seemed a bit kludgy to pass that down through those layers when the unwritten state is known in the bunmapi code (but I'll take that approach if preferred).

Hmmm - don't we already pass the XFS_BMAPI* flags to xfs_bmap_del_extent_real()? If so, I don't think there's anything extra that needs plumbing here. Conceptually it seems cleaner to me to direct extent freeing policy through the bmapi interface flags than it is add another flag interface elsewhere...

Quoted 16 lines

Quoted 9 lines

Quoted 8 lines

This behavior is of course not directly related to unwritten extents, but the immediate/obvious solution to bubble up a bmapi flag of some kind to xfs_free_eofblocks() seemed rather crude. From there, I figured that we technically don't need to discard any unwritten extents (within or beyond EOF) because they haven't been written to since being allocated. In fact, I'm not sure we have to even busy them, but it's roughly equivalent logic either way and I'm trying to avoid getting too clever.

Quoted 6 lines

I think we still need to busy them to avoid re-allocating them in the same checkpoint, as data extent free/realloc in the same checkpoint could result in a failed recovery (i.e. partial checkpoint replay) leaving the extent linked into two separate files.

Quoted 4 lines

Ah, Ok.. I was only thinking about metadata/data reuse. Hm, isn't the filesystem essentially corrupted on a failed/partial recovery anyways? (Not that this matters much in this context, I wasn't planning to bypass the busy sequence...).

Yeah, the filesystem will be metadata corrupt, but IIUC skipping the busy extent state and multiply linking data extents will make it worse because repair just throws away multiply linked data extents. i.e. partial log recovery could cause valid user data that should have been left at rest to be thrown away by repair.

Quoted 20 lines

Quoted 11 lines

Quoted 10 lines

I also recall that we've discussed using unwritten extents for delalloc -> real conversion to avoid the small stale data exposure window that exists in writeback. Without getting too deep into the reason we don't currently do an initial unwritten allocation [1], I don't think there's anything blocking us from converting any post-eof blocks that happen to be part of the resulting normal allocation. As it is, the imap is already trimmed to EOF by the writeback code for coherency reasons. If we were to convert post-eof blocks (not part of this patch) along with something like this patch, then we'd indirectly prevent discards for eofblocks trims.

Quoted 8 lines

I think we should leave that as a separate problem, as writeback currently has issues with the way we manage bufferhead state. i.e. things don't work right if we put unwritten extents under delalloc buffers and vice versa. [ I have patches to address that I'm working on.] And there's also the issue that we need to change the delalloc reservations to take into account block allocations required by unwritten extent conversion needed by delalloc.

Quoted 7 lines

Right.. I wasn't planning to try and solve the whole buffer head state mismatch thing as a dependency to not discard eofblocks. I was only going to convert blocks that happened to be post-eof after the xfs_iomap_write_allocate() allocation because those blocks by definition don't have buffers. So it's essentially just another xfs_bmapi_write() call from xfs_iomap_write_allocate() to convert eofblocks if the just allocated mapping crosses eof.

I think it's a bit more complex than that. We have delalloc buffers in memory beyond the on disk EOF (i.e. in-memory EOF is far beyond on disk EOF) but when we do the allocation we would have to mark the entire extent covering beyond the current IO past the on-disk EOF as unwritten. Hence the range between the on-disk EOF and the in-memory EOF ends up with unwritten extents under a delalloc range. Now our IO completion needs to be different for the next IO beyond EOF, because it needs to do unwritten completion, not just a file size update. This is currently problematic because we use the "buffer head is delalloc" state to determine the IO completion action, not the on disk extent state.

We could solve this with the current code, but it just gets complex (even more so than what we have now). I'd much prefer we change over to doing allocation based on the extent tree state (the patches I have) rather than bufferhead state first, and then the change to using unwritten extents for delalloc ranges is nice and simple...

Quoted 6 lines

Quoted 3 lines

Hence I think we should address that as a separate problem, not as the solution to avoiding discard of post-eof extents.

Fair enough, I'll look into tacking on a separate patch to also skip discards for unwritten extents (irrespective of eof).

Awesome!

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
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 46 lines · Dave Chinner

Quoted 36 lines · Brian Foster

Quoted 18 lines · Dave Chinner

Quoted 6 lines · Brian Foster

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

---

Hi all,

What do folks think of something like this?

Definitely sounds like something we need to address.

Quoted 7 lines

The motivation here is that
the VDO (dedup) devs had reported seeing online discards during write-only workloads. These turn out to be related to trimming post-eof preallocation blocks after large file copies. To my knowledge, this isn't really a prevalent or serious issue, but I think that technically these discards are unnecessary and so I was looking into how we could avoid them.

Quoted 8 lines

We simply trucate post-eof extents, right? So we know in xfs_itruncate_extents() if the inode size is changing, not to mention we know if the extent is beyond EOF? e.g. all calls to xfs_itruncate_extents() other than xfs_free_eofblocks() change the inode size and so directly indicate they are removing written blocks. Anything where the inode size is not changing is doing a post-eof removal, and so we can assume no data has been written?

Yes, xfs_free_eofblocks() is the only caller explicitly responsible for trimming post-eof extents.

Quoted 4 lines

So rather than converting everything to unwritten extents, the "skip discard flag" is simply triggered via extents being freed sitting beyond the current EOF (not the new EOF) and/or being unwritten?

Quoted 8 lines

That was pretty much my initial thought, but note that the extent free is ultimately deferred down in xfs_free_eofblocks() -> xfs_itruncate_extents() -> xfs_bunmapi() -> xfs_bmap_del_extent_real() -> xfs_bmap_add_free(). We can communicate this down to that point with an itruncate_extents() parameter and XFS_BMAPI_NODISCARD flag or some such, it just seemed a bit kludgy to pass that down through those layers when the unwritten state is known in the bunmapi code (but I'll take that approach if preferred).

Quoted 6 lines

Hmmm - don't we already pass the XFS_BMAPI* flags to xfs_bmap_del_extent_real()? If so, I don't think there's anything extra that needs plumbing here. Conceptually it seems cleaner to me to direct extent freeing policy through the bmapi interface flags than it is add another flag interface elsewhere...

Yeah, the bmapi flag will cover down through there. The params (or wrappers) are needed at the very top (xfs_itruncate_extents()) and bottom (xfs_bmap_add_free_nodiscard()) of the chain.

The point I was trying to make above is just that the interface for controlling discards as such for unwritten extents (eofblocks aside..) is much more simple. We reduce the need to the xfs_bmap_add_free_nodiscard() helper in that case.

Quoted 17 lines

Quoted 9 lines

Quoted 8 lines

I think we should leave that as a separate problem, as writeback currently has issues with the way we manage bufferhead state. i.e. things don't work right if we put unwritten extents under delalloc buffers and vice versa. [ I have patches to address that I'm working on.] And there's also the issue that we need to change the delalloc reservations to take into account block allocations required by unwritten extent conversion needed by delalloc.

Quoted 7 lines

Right.. I wasn't planning to try and solve the whole buffer head state mismatch thing as a dependency to not discard eofblocks. I was only going to convert blocks that happened to be post-eof after the xfs_iomap_write_allocate() allocation because those blocks by definition don't have buffers. So it's essentially just another xfs_bmapi_write() call from xfs_iomap_write_allocate() to convert eofblocks if the just allocated mapping crosses eof.

Quoted 12 lines

I think it's a bit more complex than that. We have delalloc buffers in memory beyond the on disk EOF (i.e. in-memory EOF is far beyond on disk EOF) but when we do the allocation we would have to mark the entire extent covering beyond the current IO past the on-disk EOF as unwritten. Hence the range between the on-disk EOF and the in-memory EOF ends up with unwritten extents under a delalloc range. Now our IO completion needs to be different for the next IO beyond EOF, because it needs to do unwritten completion, not just a file size update. This is currently problematic because we use the "buffer head is delalloc" state to determine the IO completion action, not the on disk extent state.

Right, but the conversion would be based on the in-core EOF so we don't end up with unwritten extents beneath delalloc buffer_head's. In fact, the intent is to only convert blocks that are guaranteed to not have any buffer_head coverage at all.

Hmm, I think you're thinking that I want to change some or all of the existing delalloc -> real allocation to a delalloc -> prealloc allocation somehow or another such that it affects within-eof state, and that's not really what I had in mind.

Quoted 54 lines · Brian Foster

Quoted 46 lines · Dave Chinner

Quoted 36 lines · Brian Foster

Quoted 18 lines · Dave Chinner

Quoted 6 lines · Brian Foster

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

---

Hi all,

What do folks think of something like this?

Definitely sounds like something we need to address.

Quoted 7 lines

The motivation here is that
the VDO (dedup) devs had reported seeing online discards during write-only workloads. These turn out to be related to trimming post-eof preallocation blocks after large file copies. To my knowledge, this isn't really a prevalent or serious issue, but I think that technically these discards are unnecessary and so I was looking into how we could avoid them.

Quoted 8 lines

We simply trucate post-eof extents, right? So we know in xfs_itruncate_extents() if the inode size is changing, not to mention we know if the extent is beyond EOF? e.g. all calls to xfs_itruncate_extents() other than xfs_free_eofblocks() change the inode size and so directly indicate they are removing written blocks. Anything where the inode size is not changing is doing a post-eof removal, and so we can assume no data has been written?

Yes, xfs_free_eofblocks() is the only caller explicitly responsible for trimming post-eof extents.

Quoted 4 lines

So rather than converting everything to unwritten extents, the "skip discard flag" is simply triggered via extents being freed sitting beyond the current EOF (not the new EOF) and/or being unwritten?

Quoted 8 lines

That was pretty much my initial thought, but note that the extent free is ultimately deferred down in xfs_free_eofblocks() -> xfs_itruncate_extents() -> xfs_bunmapi() -> xfs_bmap_del_extent_real() -> xfs_bmap_add_free(). We can communicate this down to that point with an itruncate_extents() parameter and XFS_BMAPI_NODISCARD flag or some such, it just seemed a bit kludgy to pass that down through those layers when the unwritten state is known in the bunmapi code (but I'll take that approach if preferred).

Quoted 6 lines

Hmmm - don't we already pass the XFS_BMAPI* flags to xfs_bmap_del_extent_real()? If so, I don't think there's anything extra that needs plumbing here. Conceptually it seems cleaner to me to direct extent freeing policy through the bmapi interface flags than it is add another flag interface elsewhere...

Quoted 8 lines

Yeah, the bmapi flag will cover down through there. The params (or wrappers) are needed at the very top (xfs_itruncate_extents()) and bottom (xfs_bmap_add_free_nodiscard()) of the chain.

The point I was trying to make above is just that the interface for controlling discards as such for unwritten extents (eofblocks aside..) is much more simple. We reduce the need to the xfs_bmap_add_free_nodiscard() helper in that case.

Yeah, I see that. I was really looking at whether it's a general optimisation we can make without having to add lots of plumbing,,,

Quoted 30 lines

Quoted 17 lines

Quoted 9 lines

Quoted 8 lines

I think we should leave that as a separate problem, as writeback currently has issues with the way we manage bufferhead state. i.e. things don't work right if we put unwritten extents under delalloc buffers and vice versa. [ I have patches to address that I'm working on.] And there's also the issue that we need to change the delalloc reservations to take into account block allocations required by unwritten extent conversion needed by delalloc.

Quoted 7 lines

Right.. I wasn't planning to try and solve the whole buffer head state mismatch thing as a dependency to not discard eofblocks. I was only going to convert blocks that happened to be post-eof after the xfs_iomap_write_allocate() allocation because those blocks by definition don't have buffers. So it's essentially just another xfs_bmapi_write() call from xfs_iomap_write_allocate() to convert eofblocks if the just allocated mapping crosses eof.

Quoted 12 lines

I think it's a bit more complex than that. We have delalloc buffers in memory beyond the on disk EOF (i.e. in-memory EOF is far beyond on disk EOF) but when we do the allocation we would have to mark the entire extent covering beyond the current IO past the on-disk EOF as unwritten. Hence the range between the on-disk EOF and the in-memory EOF ends up with unwritten extents under a delalloc range. Now our IO completion needs to be different for the next IO beyond EOF, because it needs to do unwritten completion, not just a file size update. This is currently problematic because we use the "buffer head is delalloc" state to determine the IO completion action, not the on disk extent state.

Quoted 4 lines

Right, but the conversion would be based on the in-core EOF so we don't end up with unwritten extents beneath delalloc buffer_head's. In fact, the intent is to only convert blocks that are guaranteed to not have any buffer_head coverage at all.

Which means if we truncate after the initial allocation, but before we write the in-core data, we still discard the range between the on-disk EOF and where the unwritten extent beyond EOF begins. And for large files, that means we might not be converting any of the extent allocated beyond EOF at all because we can hmore dirty data queued up in memory than we can allocate contiguously.

Quoted 4 lines

Hmm, I think you're thinking that I want to change some or all of the existing delalloc -> real allocation to a delalloc -> prealloc allocation somehow or another such that it affects within-eof state, and that's not really what I had in mind.

Not really, I was thinking about how the in memory EOF changes can affect the on-disk EOF changes and the action that we need to take when completing an IO and extending EOF. Some of the issues are the same as for allocations within EOF, but I think there are some different ones because of the way specualtive preallocation beyond EOF works....

I suspect we don't really want to change allocation time behaviour right now, just truncate time behaviour where we know that we've already locked out incoming IO and the in-memory size cannot change...

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
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