* [PATCH v2 0/3] xfs: skip unnecessary discards
@ 2018-05-08 17:22 Brian Foster
2018-05-08 17:22 ` [PATCH v2 1/3] xfs: add bmapi nodiscard flag Brian Foster
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Brian Foster @ 2018-05-08 17:22 UTC (permalink / raw)
To: linux-xfs
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
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH v2 1/3] xfs: add bmapi nodiscard flag 2018-05-08 17:22 [PATCH v2 0/3] xfs: skip unnecessary discards Brian Foster @ 2018-05-08 17:22 ` Brian Foster 2018-05-08 18:12 ` Darrick J. Wong 2018-05-09 7:46 ` Christoph Hellwig 2018-05-08 17:22 ` [PATCH v2 2/3] xfs: skip online discard during eofblocks trims Brian Foster 2018-05-08 17:22 ` [PATCH v2 3/3] xfs: don't discard on free of unwritten extents Brian Foster 2 siblings, 2 replies; 18+ messages in thread From: Brian Foster @ 2018-05-08 17:22 UTC (permalink / raw) To: linux-xfs 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag 2018-05-08 17:22 ` [PATCH v2 1/3] xfs: add bmapi nodiscard flag Brian Foster @ 2018-05-08 18:12 ` Darrick J. Wong 2018-05-09 1:28 ` Darrick J. Wong 2018-05-09 7:46 ` Christoph Hellwig 1 sibling, 1 reply; 18+ messages in thread From: Darrick J. Wong @ 2018-05-08 18:12 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag 2018-05-08 18:12 ` Darrick J. Wong @ 2018-05-09 1:28 ` Darrick J. Wong 2018-05-09 10:56 ` Brian Foster 0 siblings, 1 reply; 18+ messages in thread From: Darrick J. Wong @ 2018-05-09 1:28 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag 2018-05-09 1:28 ` Darrick J. Wong @ 2018-05-09 10:56 ` Brian Foster 0 siblings, 0 replies; 18+ messages in thread From: Brian Foster @ 2018-05-09 10:56 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag 2018-05-08 17:22 ` [PATCH v2 1/3] xfs: add bmapi nodiscard flag Brian Foster 2018-05-08 18:12 ` Darrick J. Wong @ 2018-05-09 7:46 ` Christoph Hellwig 2018-05-09 10:58 ` Brian Foster 1 sibling, 1 reply; 18+ messages in thread From: Christoph Hellwig @ 2018-05-09 7:46 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag 2018-05-09 7:46 ` Christoph Hellwig @ 2018-05-09 10:58 ` Brian Foster 2018-05-09 11:39 ` Christoph Hellwig 0 siblings, 1 reply; 18+ messages in thread From: Brian Foster @ 2018-05-09 10:58 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag 2018-05-09 10:58 ` Brian Foster @ 2018-05-09 11:39 ` Christoph Hellwig 2018-05-09 12:01 ` Brian Foster 0 siblings, 1 reply; 18+ messages in thread From: Christoph Hellwig @ 2018-05-09 11:39 UTC (permalink / raw) To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs > 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag 2018-05-09 11:39 ` Christoph Hellwig @ 2018-05-09 12:01 ` Brian Foster 2018-05-09 12:07 ` Christoph Hellwig 0 siblings, 1 reply; 18+ messages in thread From: Brian Foster @ 2018-05-09 12:01 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs, Dave Chinner 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag 2018-05-09 12:01 ` Brian Foster @ 2018-05-09 12:07 ` Christoph Hellwig 2018-05-09 12:47 ` Brian Foster 0 siblings, 1 reply; 18+ messages in thread From: Christoph Hellwig @ 2018-05-09 12:07 UTC (permalink / raw) To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs, Dave Chinner 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag 2018-05-09 12:07 ` Christoph Hellwig @ 2018-05-09 12:47 ` Brian Foster 2018-05-10 8:25 ` Christoph Hellwig 0 siblings, 1 reply; 18+ messages in thread From: Brian Foster @ 2018-05-09 12:47 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs, Dave Chinner 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] xfs: add bmapi nodiscard flag 2018-05-09 12:47 ` Brian Foster @ 2018-05-10 8:25 ` Christoph Hellwig 0 siblings, 0 replies; 18+ messages in thread From: Christoph Hellwig @ 2018-05-10 8:25 UTC (permalink / raw) To: Brian Foster; +Cc: Christoph Hellwig, linux-xfs, Dave Chinner 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/3] xfs: skip online discard during eofblocks trims 2018-05-08 17:22 [PATCH v2 0/3] xfs: skip unnecessary discards Brian Foster 2018-05-08 17:22 ` [PATCH v2 1/3] xfs: add bmapi nodiscard flag Brian Foster @ 2018-05-08 17:22 ` Brian Foster 2018-05-08 18:14 ` Darrick J. Wong 2018-05-09 7:40 ` Christoph Hellwig 2018-05-08 17:22 ` [PATCH v2 3/3] xfs: don't discard on free of unwritten extents Brian Foster 2 siblings, 2 replies; 18+ messages in thread From: Brian Foster @ 2018-05-08 17:22 UTC (permalink / raw) To: linux-xfs 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] xfs: skip online discard during eofblocks trims 2018-05-08 17:22 ` [PATCH v2 2/3] xfs: skip online discard during eofblocks trims Brian Foster @ 2018-05-08 18:14 ` Darrick J. Wong 2018-05-09 7:40 ` Christoph Hellwig 1 sibling, 0 replies; 18+ messages in thread From: Darrick J. Wong @ 2018-05-08 18:14 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] xfs: skip online discard during eofblocks trims 2018-05-08 17:22 ` [PATCH v2 2/3] xfs: skip online discard during eofblocks trims Brian Foster 2018-05-08 18:14 ` Darrick J. Wong @ 2018-05-09 7:40 ` Christoph Hellwig 1 sibling, 0 replies; 18+ messages in thread From: Christoph Hellwig @ 2018-05-09 7:40 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 3/3] xfs: don't discard on free of unwritten extents 2018-05-08 17:22 [PATCH v2 0/3] xfs: skip unnecessary discards Brian Foster 2018-05-08 17:22 ` [PATCH v2 1/3] xfs: add bmapi nodiscard flag Brian Foster 2018-05-08 17:22 ` [PATCH v2 2/3] xfs: skip online discard during eofblocks trims Brian Foster @ 2018-05-08 17:22 ` Brian Foster 2018-05-08 18:14 ` Darrick J. Wong 2018-05-09 7:40 ` Christoph Hellwig 2 siblings, 2 replies; 18+ messages in thread From: Brian Foster @ 2018-05-08 17:22 UTC (permalink / raw) To: linux-xfs 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] xfs: don't discard on free of unwritten extents 2018-05-08 17:22 ` [PATCH v2 3/3] xfs: don't discard on free of unwritten extents Brian Foster @ 2018-05-08 18:14 ` Darrick J. Wong 2018-05-09 7:40 ` Christoph Hellwig 1 sibling, 0 replies; 18+ messages in thread From: Darrick J. Wong @ 2018-05-08 18:14 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] xfs: don't discard on free of unwritten extents 2018-05-08 17:22 ` [PATCH v2 3/3] xfs: don't discard on free of unwritten extents Brian Foster 2018-05-08 18:14 ` Darrick J. Wong @ 2018-05-09 7:40 ` Christoph Hellwig 1 sibling, 0 replies; 18+ messages in thread From: Christoph Hellwig @ 2018-05-09 7:40 UTC (permalink / raw) To: Brian Foster; +Cc: linux-xfs Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-05-10 8:25 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-08 17:22 [PATCH v2 0/3] xfs: skip unnecessary discards Brian Foster 2018-05-08 17:22 ` [PATCH v2 1/3] xfs: add bmapi nodiscard flag Brian Foster 2018-05-08 18:12 ` Darrick J. Wong 2018-05-09 1:28 ` Darrick J. Wong 2018-05-09 10:56 ` Brian Foster 2018-05-09 7:46 ` Christoph Hellwig 2018-05-09 10:58 ` Brian Foster 2018-05-09 11:39 ` Christoph Hellwig 2018-05-09 12:01 ` Brian Foster 2018-05-09 12:07 ` Christoph Hellwig 2018-05-09 12:47 ` Brian Foster 2018-05-10 8:25 ` Christoph Hellwig 2018-05-08 17:22 ` [PATCH v2 2/3] xfs: skip online discard during eofblocks trims Brian Foster 2018-05-08 18:14 ` Darrick J. Wong 2018-05-09 7:40 ` Christoph Hellwig 2018-05-08 17:22 ` [PATCH v2 3/3] xfs: don't discard on free of unwritten extents Brian Foster 2018-05-08 18:14 ` Darrick J. Wong 2018-05-09 7:40 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.