deferred operations cleanup

deferred operations cleanup

[Christoph Hellwig]

Hi all,

this series cleans up the deferred operations code by merging a few
methods, and using more type safe types.

[PATCH 01/11] xfs: remove the xfs_efi_log_item_t typedef

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_extfree_item.c |  2 +-
 fs/xfs/xfs_extfree_item.h | 10 +++++-----
 fs/xfs/xfs_log_recover.c  |  4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 6ea847f6e2980..00309b81607cd 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -161,7 +161,7 @@ xfs_efi_init(
 
 	ASSERT(nextents > 0);
 	if (nextents > XFS_EFI_MAX_FAST_EXTENTS) {
-		size = (uint)(sizeof(xfs_efi_log_item_t) +
+		size = (uint)(sizeof(struct xfs_efi_log_item) +
 			((nextents - 1) * sizeof(xfs_extent_t)));
 		efip = kmem_zalloc(size, 0);
 	} else {
diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index 16aaab06d4ecc..b9b567f355756 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -50,13 +50,13 @@ struct kmem_zone;
  * of commit failure or log I/O errors. Note that the EFD is not inserted in the
  * AIL, so at this point both the EFI and EFD are freed.
  */
-typedef struct xfs_efi_log_item {
+struct xfs_efi_log_item {
 	struct xfs_log_item	efi_item;
 	atomic_t		efi_refcount;
 	atomic_t		efi_next_extent;
 	unsigned long		efi_flags;	/* misc flags */
 	xfs_efi_log_format_t	efi_format;
-} xfs_efi_log_item_t;
+};
 
 /*
  * This is the "extent free done" log item.  It is used to log
@@ -65,7 +65,7 @@ typedef struct xfs_efi_log_item {
  */
 typedef struct xfs_efd_log_item {
 	struct xfs_log_item	efd_item;
-	xfs_efi_log_item_t	*efd_efip;
+	struct xfs_efi_log_item *efd_efip;
 	uint			efd_next_extent;
 	xfs_efd_log_format_t	efd_format;
 } xfs_efd_log_item_t;
@@ -78,10 +78,10 @@ typedef struct xfs_efd_log_item {
 extern struct kmem_zone	*xfs_efi_zone;
 extern struct kmem_zone	*xfs_efd_zone;
 
-xfs_efi_log_item_t	*xfs_efi_init(struct xfs_mount *, uint);
+struct xfs_efi_log_item	*xfs_efi_init(struct xfs_mount *, uint);
 int			xfs_efi_copy_format(xfs_log_iovec_t *buf,
 					    xfs_efi_log_format_t *dst_efi_fmt);
-void			xfs_efi_item_free(xfs_efi_log_item_t *);
+void			xfs_efi_item_free(struct xfs_efi_log_item *);
 void			xfs_efi_release(struct xfs_efi_log_item *);
 
 int			xfs_efi_recover(struct xfs_mount *mp,
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 11c3502b07b13..c81f71e2888cf 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3365,7 +3365,7 @@ xlog_recover_efd_pass2(
 	struct xlog_recover_item	*item)
 {
 	xfs_efd_log_format_t	*efd_formatp;
-	xfs_efi_log_item_t	*efip = NULL;
+	struct xfs_efi_log_item	*efip = NULL;
 	struct xfs_log_item	*lip;
 	uint64_t		efi_id;
 	struct xfs_ail_cursor	cur;
@@ -3386,7 +3386,7 @@ xlog_recover_efd_pass2(
 	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
 	while (lip != NULL) {
 		if (lip->li_type == XFS_LI_EFI) {
-			efip = (xfs_efi_log_item_t *)lip;
+			efip = (struct xfs_efi_log_item *)lip;
 			if (efip->efi_format.efi_id == efi_id) {
 				/*
 				 * Drop the EFD reference to the EFI. This
-- 
2.26.2

Re: [PATCH 01/11] xfs: remove the xfs_efi_log_item_t typedef

[Brian Foster]

On Wed, Apr 29, 2020 at 05:05:01PM +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_extfree_item.c |  2 +-
>  fs/xfs/xfs_extfree_item.h | 10 +++++-----
>  fs/xfs/xfs_log_recover.c  |  4 ++--
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 6ea847f6e2980..00309b81607cd 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -161,7 +161,7 @@ xfs_efi_init(
>  
>  	ASSERT(nextents > 0);
>  	if (nextents > XFS_EFI_MAX_FAST_EXTENTS) {
> -		size = (uint)(sizeof(xfs_efi_log_item_t) +
> +		size = (uint)(sizeof(struct xfs_efi_log_item) +
>  			((nextents - 1) * sizeof(xfs_extent_t)));
>  		efip = kmem_zalloc(size, 0);
>  	} else {
> diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
> index 16aaab06d4ecc..b9b567f355756 100644
> --- a/fs/xfs/xfs_extfree_item.h
> +++ b/fs/xfs/xfs_extfree_item.h
> @@ -50,13 +50,13 @@ struct kmem_zone;
>   * of commit failure or log I/O errors. Note that the EFD is not inserted in the
>   * AIL, so at this point both the EFI and EFD are freed.
>   */
> -typedef struct xfs_efi_log_item {
> +struct xfs_efi_log_item {
>  	struct xfs_log_item	efi_item;
>  	atomic_t		efi_refcount;
>  	atomic_t		efi_next_extent;
>  	unsigned long		efi_flags;	/* misc flags */
>  	xfs_efi_log_format_t	efi_format;
> -} xfs_efi_log_item_t;
> +};
>  
>  /*
>   * This is the "extent free done" log item.  It is used to log
> @@ -65,7 +65,7 @@ typedef struct xfs_efi_log_item {
>   */
>  typedef struct xfs_efd_log_item {
>  	struct xfs_log_item	efd_item;
> -	xfs_efi_log_item_t	*efd_efip;
> +	struct xfs_efi_log_item *efd_efip;
>  	uint			efd_next_extent;
>  	xfs_efd_log_format_t	efd_format;
>  } xfs_efd_log_item_t;
> @@ -78,10 +78,10 @@ typedef struct xfs_efd_log_item {
>  extern struct kmem_zone	*xfs_efi_zone;
>  extern struct kmem_zone	*xfs_efd_zone;
>  
> -xfs_efi_log_item_t	*xfs_efi_init(struct xfs_mount *, uint);
> +struct xfs_efi_log_item	*xfs_efi_init(struct xfs_mount *, uint);
>  int			xfs_efi_copy_format(xfs_log_iovec_t *buf,
>  					    xfs_efi_log_format_t *dst_efi_fmt);
> -void			xfs_efi_item_free(xfs_efi_log_item_t *);
> +void			xfs_efi_item_free(struct xfs_efi_log_item *);
>  void			xfs_efi_release(struct xfs_efi_log_item *);
>  
>  int			xfs_efi_recover(struct xfs_mount *mp,
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 11c3502b07b13..c81f71e2888cf 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3365,7 +3365,7 @@ xlog_recover_efd_pass2(
>  	struct xlog_recover_item	*item)
>  {
>  	xfs_efd_log_format_t	*efd_formatp;
> -	xfs_efi_log_item_t	*efip = NULL;
> +	struct xfs_efi_log_item	*efip = NULL;
>  	struct xfs_log_item	*lip;
>  	uint64_t		efi_id;
>  	struct xfs_ail_cursor	cur;
> @@ -3386,7 +3386,7 @@ xlog_recover_efd_pass2(
>  	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
>  	while (lip != NULL) {
>  		if (lip->li_type == XFS_LI_EFI) {
> -			efip = (xfs_efi_log_item_t *)lip;
> +			efip = (struct xfs_efi_log_item *)lip;
>  			if (efip->efi_format.efi_id == efi_id) {
>  				/*
>  				 * Drop the EFD reference to the EFI. This
> -- 
> 2.26.2
> 

[PATCH 02/11] xfs: remove the xfs_efd_log_item_t typedef

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_extfree_item.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index b9b567f355756..a2a736a77fa94 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -63,12 +63,12 @@ struct xfs_efi_log_item {
  * the fact that some extents earlier mentioned in an efi item
  * have been freed.
  */
-typedef struct xfs_efd_log_item {
+struct xfs_efd_log_item {
 	struct xfs_log_item	efd_item;
 	struct xfs_efi_log_item *efd_efip;
 	uint			efd_next_extent;
 	xfs_efd_log_format_t	efd_format;
-} xfs_efd_log_item_t;
+};
 
 /*
  * Max number of extents in fast allocation path.
-- 
2.26.2

Re: [PATCH 02/11] xfs: remove the xfs_efd_log_item_t typedef

[Brian Foster]

On Wed, Apr 29, 2020 at 05:05:02PM +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/xfs_extfree_item.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
> index b9b567f355756..a2a736a77fa94 100644
> --- a/fs/xfs/xfs_extfree_item.h
> +++ b/fs/xfs/xfs_extfree_item.h
> @@ -63,12 +63,12 @@ struct xfs_efi_log_item {
>   * the fact that some extents earlier mentioned in an efi item
>   * have been freed.
>   */
> -typedef struct xfs_efd_log_item {
> +struct xfs_efd_log_item {
>  	struct xfs_log_item	efd_item;
>  	struct xfs_efi_log_item *efd_efip;
>  	uint			efd_next_extent;
>  	xfs_efd_log_format_t	efd_format;
> -} xfs_efd_log_item_t;
> +};
>  
>  /*
>   * Max number of extents in fast allocation path.
> -- 
> 2.26.2
> 

[PATCH 03/11] xfs: remove the xfs_inode_log_item_t typedef

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_inode_fork.c  | 2 +-
 fs/xfs/libxfs/xfs_trans_inode.c | 2 +-
 fs/xfs/xfs_inode.c              | 4 ++--
 fs/xfs/xfs_inode_item.c         | 2 +-
 fs/xfs/xfs_inode_item.h         | 4 ++--
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 518c6f0ec3a61..3e9a42f1e23b9 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -592,7 +592,7 @@ void
 xfs_iflush_fork(
 	xfs_inode_t		*ip,
 	xfs_dinode_t		*dip,
-	xfs_inode_log_item_t	*iip,
+	struct xfs_inode_log_item *iip,
 	int			whichfork)
 {
 	char			*cp;
diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
index 2b8ccb5b975df..b5dfb66548422 100644
--- a/fs/xfs/libxfs/xfs_trans_inode.c
+++ b/fs/xfs/libxfs/xfs_trans_inode.c
@@ -27,7 +27,7 @@ xfs_trans_ijoin(
 	struct xfs_inode	*ip,
 	uint			lock_flags)
 {
-	xfs_inode_log_item_t	*iip;
+	struct xfs_inode_log_item *iip;
 
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 	if (ip->i_itemp == NULL)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index d1772786af29d..0e2ef3f56be41 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2602,7 +2602,7 @@ xfs_ifree_cluster(
 	xfs_daddr_t		blkno;
 	xfs_buf_t		*bp;
 	xfs_inode_t		*ip;
-	xfs_inode_log_item_t	*iip;
+	struct xfs_inode_log_item *iip;
 	struct xfs_log_item	*lip;
 	struct xfs_perag	*pag;
 	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
@@ -2662,7 +2662,7 @@ xfs_ifree_cluster(
 		 */
 		list_for_each_entry(lip, &bp->b_li_list, li_bio_list) {
 			if (lip->li_type == XFS_LI_INODE) {
-				iip = (xfs_inode_log_item_t *)lip;
+				iip = (struct xfs_inode_log_item *)lip;
 				ASSERT(iip->ili_logged == 1);
 				lip->li_cb = xfs_istale_done;
 				xfs_trans_ail_copy_lsn(mp->m_ail,
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index f779cca2346f3..75b74bbe38e43 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -780,7 +780,7 @@ xfs_iflush_abort(
 	xfs_inode_t		*ip,
 	bool			stale)
 {
-	xfs_inode_log_item_t	*iip = ip->i_itemp;
+	struct xfs_inode_log_item *iip = ip->i_itemp;
 
 	if (iip) {
 		if (test_bit(XFS_LI_IN_AIL, &iip->ili_item.li_flags)) {
diff --git a/fs/xfs/xfs_inode_item.h b/fs/xfs/xfs_inode_item.h
index 07a60e74c39c8..ad667fd4ae622 100644
--- a/fs/xfs/xfs_inode_item.h
+++ b/fs/xfs/xfs_inode_item.h
@@ -13,7 +13,7 @@ struct xfs_bmbt_rec;
 struct xfs_inode;
 struct xfs_mount;
 
-typedef struct xfs_inode_log_item {
+struct xfs_inode_log_item {
 	struct xfs_log_item	ili_item;	   /* common portion */
 	struct xfs_inode	*ili_inode;	   /* inode ptr */
 	xfs_lsn_t		ili_flush_lsn;	   /* lsn at last flush */
@@ -23,7 +23,7 @@ typedef struct xfs_inode_log_item {
 	unsigned int		ili_last_fields;   /* fields when flushed */
 	unsigned int		ili_fields;	   /* fields to be logged */
 	unsigned int		ili_fsync_fields;  /* logged since last fsync */
-} xfs_inode_log_item_t;
+};
 
 static inline int xfs_inode_clean(xfs_inode_t *ip)
 {
-- 
2.26.2

Re: [PATCH 03/11] xfs: remove the xfs_inode_log_item_t typedef

[Brian Foster]

On Wed, Apr 29, 2020 at 05:05:03PM +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/libxfs/xfs_inode_fork.c  | 2 +-
>  fs/xfs/libxfs/xfs_trans_inode.c | 2 +-
>  fs/xfs/xfs_inode.c              | 4 ++--
>  fs/xfs/xfs_inode_item.c         | 2 +-
>  fs/xfs/xfs_inode_item.h         | 4 ++--
>  5 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 518c6f0ec3a61..3e9a42f1e23b9 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -592,7 +592,7 @@ void
>  xfs_iflush_fork(
>  	xfs_inode_t		*ip,
>  	xfs_dinode_t		*dip,
> -	xfs_inode_log_item_t	*iip,
> +	struct xfs_inode_log_item *iip,
>  	int			whichfork)
>  {
>  	char			*cp;
> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index 2b8ccb5b975df..b5dfb66548422 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -27,7 +27,7 @@ xfs_trans_ijoin(
>  	struct xfs_inode	*ip,
>  	uint			lock_flags)
>  {
> -	xfs_inode_log_item_t	*iip;
> +	struct xfs_inode_log_item *iip;
>  
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  	if (ip->i_itemp == NULL)
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index d1772786af29d..0e2ef3f56be41 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2602,7 +2602,7 @@ xfs_ifree_cluster(
>  	xfs_daddr_t		blkno;
>  	xfs_buf_t		*bp;
>  	xfs_inode_t		*ip;
> -	xfs_inode_log_item_t	*iip;
> +	struct xfs_inode_log_item *iip;
>  	struct xfs_log_item	*lip;
>  	struct xfs_perag	*pag;
>  	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
> @@ -2662,7 +2662,7 @@ xfs_ifree_cluster(
>  		 */
>  		list_for_each_entry(lip, &bp->b_li_list, li_bio_list) {
>  			if (lip->li_type == XFS_LI_INODE) {
> -				iip = (xfs_inode_log_item_t *)lip;
> +				iip = (struct xfs_inode_log_item *)lip;
>  				ASSERT(iip->ili_logged == 1);
>  				lip->li_cb = xfs_istale_done;
>  				xfs_trans_ail_copy_lsn(mp->m_ail,
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index f779cca2346f3..75b74bbe38e43 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -780,7 +780,7 @@ xfs_iflush_abort(
>  	xfs_inode_t		*ip,
>  	bool			stale)
>  {
> -	xfs_inode_log_item_t	*iip = ip->i_itemp;
> +	struct xfs_inode_log_item *iip = ip->i_itemp;
>  
>  	if (iip) {
>  		if (test_bit(XFS_LI_IN_AIL, &iip->ili_item.li_flags)) {
> diff --git a/fs/xfs/xfs_inode_item.h b/fs/xfs/xfs_inode_item.h
> index 07a60e74c39c8..ad667fd4ae622 100644
> --- a/fs/xfs/xfs_inode_item.h
> +++ b/fs/xfs/xfs_inode_item.h
> @@ -13,7 +13,7 @@ struct xfs_bmbt_rec;
>  struct xfs_inode;
>  struct xfs_mount;
>  
> -typedef struct xfs_inode_log_item {
> +struct xfs_inode_log_item {
>  	struct xfs_log_item	ili_item;	   /* common portion */
>  	struct xfs_inode	*ili_inode;	   /* inode ptr */
>  	xfs_lsn_t		ili_flush_lsn;	   /* lsn at last flush */
> @@ -23,7 +23,7 @@ typedef struct xfs_inode_log_item {
>  	unsigned int		ili_last_fields;   /* fields when flushed */
>  	unsigned int		ili_fields;	   /* fields to be logged */
>  	unsigned int		ili_fsync_fields;  /* logged since last fsync */
> -} xfs_inode_log_item_t;
> +};
>  
>  static inline int xfs_inode_clean(xfs_inode_t *ip)
>  {
> -- 
> 2.26.2
> 

[PATCH 04/11] xfs: factor out a xfs_defer_create_intent helper

[Christoph Hellwig]

Create a helper that encapsulates the whole logic to create a defer
intent.  This reorders some of the work that was done, but none of
that has an affect on the operation as only fields that don't directly
interact are affected.

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

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 22557527cfdb6..8a38da602b7d9 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -178,6 +178,23 @@ static const struct xfs_defer_op_type *defer_op_types[] = {
 	[XFS_DEFER_OPS_TYPE_AGFL_FREE]	= &xfs_agfl_free_defer_type,
 };
 
+static void
+xfs_defer_create_intent(
+	struct xfs_trans		*tp,
+	struct xfs_defer_pending	*dfp,
+	bool				sort)
+{
+	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
+	struct list_head		*li;
+
+	if (sort)
+		list_sort(tp->t_mountp, &dfp->dfp_work, ops->diff_items);
+
+	dfp->dfp_intent = ops->create_intent(tp, dfp->dfp_count);
+	list_for_each(li, &dfp->dfp_work)
+		ops->log_item(tp, dfp->dfp_intent, li);
+}
+
 /*
  * For each pending item in the intake list, log its intent item and the
  * associated extents, then add the entire intake list to the end of
@@ -187,17 +204,11 @@ STATIC void
 xfs_defer_create_intents(
 	struct xfs_trans		*tp)
 {
-	struct list_head		*li;
 	struct xfs_defer_pending	*dfp;
-	const struct xfs_defer_op_type	*ops;
 
 	list_for_each_entry(dfp, &tp->t_dfops, dfp_list) {
-		ops = defer_op_types[dfp->dfp_type];
-		dfp->dfp_intent = ops->create_intent(tp, dfp->dfp_count);
 		trace_xfs_defer_create_intent(tp->t_mountp, dfp);
-		list_sort(tp->t_mountp, &dfp->dfp_work, ops->diff_items);
-		list_for_each(li, &dfp->dfp_work)
-			ops->log_item(tp, dfp->dfp_intent, li);
+		xfs_defer_create_intent(tp, dfp, true);
 	}
 }
 
@@ -419,17 +430,13 @@ xfs_defer_finish_noroll(
 		}
 		if (error == -EAGAIN) {
 			/*
-			 * Caller wants a fresh transaction, so log a
-			 * new log intent item to replace the old one
-			 * and roll the transaction.  See "Requesting
-			 * a Fresh Transaction while Finishing
-			 * Deferred Work" above.
+			 * Caller wants a fresh transaction, so log a new log
+			 * intent item to replace the old one and roll the
+			 * transaction.  See "Requesting a Fresh Transaction
+			 * while Finishing Deferred Work" above.
 			 */
-			dfp->dfp_intent = ops->create_intent(*tp,
-					dfp->dfp_count);
 			dfp->dfp_done = NULL;
-			list_for_each(li, &dfp->dfp_work)
-				ops->log_item(*tp, dfp->dfp_intent, li);
+			xfs_defer_create_intent(*tp, dfp, false);
 		} else {
 			/* Done with the dfp, free it. */
 			list_del(&dfp->dfp_list);
-- 
2.26.2

Re: [PATCH 04/11] xfs: factor out a xfs_defer_create_intent helper

[Brian Foster]

On Wed, Apr 29, 2020 at 05:05:04PM +0200, Christoph Hellwig wrote:
> Create a helper that encapsulates the whole logic to create a defer
> intent.  This reorders some of the work that was done, but none of
> that has an affect on the operation as only fields that don't directly
> interact are affected.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/libxfs/xfs_defer.c | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 22557527cfdb6..8a38da602b7d9 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -178,6 +178,23 @@ static const struct xfs_defer_op_type *defer_op_types[] = {
>  	[XFS_DEFER_OPS_TYPE_AGFL_FREE]	= &xfs_agfl_free_defer_type,
>  };
>  
> +static void
> +xfs_defer_create_intent(
> +	struct xfs_trans		*tp,
> +	struct xfs_defer_pending	*dfp,
> +	bool				sort)
> +{
> +	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
> +	struct list_head		*li;
> +
> +	if (sort)
> +		list_sort(tp->t_mountp, &dfp->dfp_work, ops->diff_items);
> +
> +	dfp->dfp_intent = ops->create_intent(tp, dfp->dfp_count);
> +	list_for_each(li, &dfp->dfp_work)
> +		ops->log_item(tp, dfp->dfp_intent, li);
> +}
> +
>  /*
>   * For each pending item in the intake list, log its intent item and the
>   * associated extents, then add the entire intake list to the end of
> @@ -187,17 +204,11 @@ STATIC void
>  xfs_defer_create_intents(
>  	struct xfs_trans		*tp)
>  {
> -	struct list_head		*li;
>  	struct xfs_defer_pending	*dfp;
> -	const struct xfs_defer_op_type	*ops;
>  
>  	list_for_each_entry(dfp, &tp->t_dfops, dfp_list) {
> -		ops = defer_op_types[dfp->dfp_type];
> -		dfp->dfp_intent = ops->create_intent(tp, dfp->dfp_count);
>  		trace_xfs_defer_create_intent(tp->t_mountp, dfp);
> -		list_sort(tp->t_mountp, &dfp->dfp_work, ops->diff_items);
> -		list_for_each(li, &dfp->dfp_work)
> -			ops->log_item(tp, dfp->dfp_intent, li);
> +		xfs_defer_create_intent(tp, dfp, true);
>  	}
>  }
>  
> @@ -419,17 +430,13 @@ xfs_defer_finish_noroll(
>  		}
>  		if (error == -EAGAIN) {
>  			/*
> -			 * Caller wants a fresh transaction, so log a
> -			 * new log intent item to replace the old one
> -			 * and roll the transaction.  See "Requesting
> -			 * a Fresh Transaction while Finishing
> -			 * Deferred Work" above.
> +			 * Caller wants a fresh transaction, so log a new log
> +			 * intent item to replace the old one and roll the
> +			 * transaction.  See "Requesting a Fresh Transaction
> +			 * while Finishing Deferred Work" above.
>  			 */
> -			dfp->dfp_intent = ops->create_intent(*tp,
> -					dfp->dfp_count);
>  			dfp->dfp_done = NULL;
> -			list_for_each(li, &dfp->dfp_work)
> -				ops->log_item(*tp, dfp->dfp_intent, li);
> +			xfs_defer_create_intent(*tp, dfp, false);
>  		} else {
>  			/* Done with the dfp, free it. */
>  			list_del(&dfp->dfp_list);
> -- 
> 2.26.2
> 

[PATCH 05/11] xfs: merge the ->log_item defer op into ->create_intent

[Christoph Hellwig]

These are aways called together, and my merging them we reduce the amount
of indirect calls, improve type safety and in general clean up the code
a bit.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_defer.c  |  6 ++---
 fs/xfs/libxfs/xfs_defer.h  |  4 ++--
 fs/xfs/xfs_bmap_item.c     | 47 +++++++++++++++---------------------
 fs/xfs/xfs_extfree_item.c  | 49 ++++++++++++++++----------------------
 fs/xfs/xfs_refcount_item.c | 48 ++++++++++++++++---------------------
 fs/xfs/xfs_rmap_item.c     | 48 ++++++++++++++++---------------------
 6 files changed, 83 insertions(+), 119 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 8a38da602b7d9..56d1357f9d137 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -185,14 +185,12 @@ xfs_defer_create_intent(
 	bool				sort)
 {
 	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
-	struct list_head		*li;
 
 	if (sort)
 		list_sort(tp->t_mountp, &dfp->dfp_work, ops->diff_items);
 
-	dfp->dfp_intent = ops->create_intent(tp, dfp->dfp_count);
-	list_for_each(li, &dfp->dfp_work)
-		ops->log_item(tp, dfp->dfp_intent, li);
+	dfp->dfp_intent = ops->create_intent(tp, &dfp->dfp_work,
+			dfp->dfp_count);
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index 7c28d7608ac62..d6a4577c25b05 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -50,8 +50,8 @@ struct xfs_defer_op_type {
 	void (*finish_cleanup)(struct xfs_trans *, void *, int);
 	void (*cancel_item)(struct list_head *);
 	int (*diff_items)(void *, struct list_head *, struct list_head *);
-	void *(*create_intent)(struct xfs_trans *, uint);
-	void (*log_item)(struct xfs_trans *, void *, struct list_head *);
+	void *(*create_intent)(struct xfs_trans *tp, struct list_head *items,
+			unsigned int count);
 	unsigned int		max_items;
 };
 
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index ee6f4229cebc4..dea97956d78d6 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -278,27 +278,6 @@ xfs_bmap_update_diff_items(
 	return ba->bi_owner->i_ino - bb->bi_owner->i_ino;
 }
 
-/* Get an BUI. */
-STATIC void *
-xfs_bmap_update_create_intent(
-	struct xfs_trans		*tp,
-	unsigned int			count)
-{
-	struct xfs_bui_log_item		*buip;
-
-	ASSERT(count == XFS_BUI_MAX_FAST_EXTENTS);
-	ASSERT(tp != NULL);
-
-	buip = xfs_bui_init(tp->t_mountp);
-	ASSERT(buip != NULL);
-
-	/*
-	 * Get a log_item_desc to point at the new item.
-	 */
-	xfs_trans_add_item(tp, &buip->bui_item);
-	return buip;
-}
-
 /* Set the map extent flags for this mapping. */
 static void
 xfs_trans_set_bmap_flags(
@@ -326,16 +305,12 @@ xfs_trans_set_bmap_flags(
 STATIC void
 xfs_bmap_update_log_item(
 	struct xfs_trans		*tp,
-	void				*intent,
-	struct list_head		*item)
+	struct xfs_bui_log_item		*buip,
+	struct xfs_bmap_intent		*bmap)
 {
-	struct xfs_bui_log_item		*buip = intent;
-	struct xfs_bmap_intent		*bmap;
 	uint				next_extent;
 	struct xfs_map_extent		*map;
 
-	bmap = container_of(item, struct xfs_bmap_intent, bi_list);
-
 	tp->t_flags |= XFS_TRANS_DIRTY;
 	set_bit(XFS_LI_DIRTY, &buip->bui_item.li_flags);
 
@@ -355,6 +330,23 @@ xfs_bmap_update_log_item(
 			bmap->bi_bmap.br_state);
 }
 
+STATIC void *
+xfs_bmap_update_create_intent(
+	struct xfs_trans		*tp,
+	struct list_head		*items,
+	unsigned int			count)
+{
+	struct xfs_bui_log_item		*buip = xfs_bui_init(tp->t_mountp);
+	struct xfs_bmap_intent		*bmap;
+
+	ASSERT(count == XFS_BUI_MAX_FAST_EXTENTS);
+
+	xfs_trans_add_item(tp, &buip->bui_item);
+	list_for_each_entry(bmap, items, bi_list)
+		xfs_bmap_update_log_item(tp, buip, bmap);
+	return buip;
+}
+
 /* Get an BUD so we can process all the deferred rmap updates. */
 STATIC void *
 xfs_bmap_update_create_done(
@@ -419,7 +411,6 @@ const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
 	.diff_items	= xfs_bmap_update_diff_items,
 	.create_intent	= xfs_bmap_update_create_intent,
 	.abort_intent	= xfs_bmap_update_abort_intent,
-	.log_item	= xfs_bmap_update_log_item,
 	.create_done	= xfs_bmap_update_create_done,
 	.finish_item	= xfs_bmap_update_finish_item,
 	.cancel_item	= xfs_bmap_update_cancel_item,
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 00309b81607cd..cb22c7ad31817 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -412,41 +412,16 @@ xfs_extent_free_diff_items(
 		XFS_FSB_TO_AGNO(mp, rb->xefi_startblock);
 }
 
-/* Get an EFI. */
-STATIC void *
-xfs_extent_free_create_intent(
-	struct xfs_trans		*tp,
-	unsigned int			count)
-{
-	struct xfs_efi_log_item		*efip;
-
-	ASSERT(tp != NULL);
-	ASSERT(count > 0);
-
-	efip = xfs_efi_init(tp->t_mountp, count);
-	ASSERT(efip != NULL);
-
-	/*
-	 * Get a log_item_desc to point at the new item.
-	 */
-	xfs_trans_add_item(tp, &efip->efi_item);
-	return efip;
-}
-
 /* Log a free extent to the intent item. */
 STATIC void
 xfs_extent_free_log_item(
 	struct xfs_trans		*tp,
-	void				*intent,
-	struct list_head		*item)
+	struct xfs_efi_log_item		*efip,
+	struct xfs_extent_free_item	*free)
 {
-	struct xfs_efi_log_item		*efip = intent;
-	struct xfs_extent_free_item	*free;
 	uint				next_extent;
 	struct xfs_extent		*extp;
 
-	free = container_of(item, struct xfs_extent_free_item, xefi_list);
-
 	tp->t_flags |= XFS_TRANS_DIRTY;
 	set_bit(XFS_LI_DIRTY, &efip->efi_item.li_flags);
 
@@ -462,6 +437,24 @@ xfs_extent_free_log_item(
 	extp->ext_len = free->xefi_blockcount;
 }
 
+STATIC void *
+xfs_extent_free_create_intent(
+	struct xfs_trans		*tp,
+	struct list_head		*items,
+	unsigned int			count)
+{
+	struct xfs_mount		*mp = tp->t_mountp;
+	struct xfs_efi_log_item		*efip = xfs_efi_init(mp, count);
+	struct xfs_extent_free_item	*free;
+
+	ASSERT(count > 0);
+
+	xfs_trans_add_item(tp, &efip->efi_item);
+	list_for_each_entry(free, items, xefi_list)
+		xfs_extent_free_log_item(tp, efip, free);
+	return efip;
+}
+
 /* Get an EFD so we can process all the free extents. */
 STATIC void *
 xfs_extent_free_create_done(
@@ -516,7 +509,6 @@ const struct xfs_defer_op_type xfs_extent_free_defer_type = {
 	.diff_items	= xfs_extent_free_diff_items,
 	.create_intent	= xfs_extent_free_create_intent,
 	.abort_intent	= xfs_extent_free_abort_intent,
-	.log_item	= xfs_extent_free_log_item,
 	.create_done	= xfs_extent_free_create_done,
 	.finish_item	= xfs_extent_free_finish_item,
 	.cancel_item	= xfs_extent_free_cancel_item,
@@ -582,7 +574,6 @@ const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
 	.diff_items	= xfs_extent_free_diff_items,
 	.create_intent	= xfs_extent_free_create_intent,
 	.abort_intent	= xfs_extent_free_abort_intent,
-	.log_item	= xfs_extent_free_log_item,
 	.create_done	= xfs_extent_free_create_done,
 	.finish_item	= xfs_agfl_free_finish_item,
 	.cancel_item	= xfs_extent_free_cancel_item,
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 8eeed73928cdf..325d49fc0406c 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -284,27 +284,6 @@ xfs_refcount_update_diff_items(
 		XFS_FSB_TO_AGNO(mp, rb->ri_startblock);
 }
 
-/* Get an CUI. */
-STATIC void *
-xfs_refcount_update_create_intent(
-	struct xfs_trans		*tp,
-	unsigned int			count)
-{
-	struct xfs_cui_log_item		*cuip;
-
-	ASSERT(tp != NULL);
-	ASSERT(count > 0);
-
-	cuip = xfs_cui_init(tp->t_mountp, count);
-	ASSERT(cuip != NULL);
-
-	/*
-	 * Get a log_item_desc to point at the new item.
-	 */
-	xfs_trans_add_item(tp, &cuip->cui_item);
-	return cuip;
-}
-
 /* Set the phys extent flags for this reverse mapping. */
 static void
 xfs_trans_set_refcount_flags(
@@ -328,16 +307,12 @@ xfs_trans_set_refcount_flags(
 STATIC void
 xfs_refcount_update_log_item(
 	struct xfs_trans		*tp,
-	void				*intent,
-	struct list_head		*item)
+	struct xfs_cui_log_item		*cuip,
+	struct xfs_refcount_intent	*refc)
 {
-	struct xfs_cui_log_item		*cuip = intent;
-	struct xfs_refcount_intent	*refc;
 	uint				next_extent;
 	struct xfs_phys_extent		*ext;
 
-	refc = container_of(item, struct xfs_refcount_intent, ri_list);
-
 	tp->t_flags |= XFS_TRANS_DIRTY;
 	set_bit(XFS_LI_DIRTY, &cuip->cui_item.li_flags);
 
@@ -354,6 +329,24 @@ xfs_refcount_update_log_item(
 	xfs_trans_set_refcount_flags(ext, refc->ri_type);
 }
 
+STATIC void *
+xfs_refcount_update_create_intent(
+	struct xfs_trans		*tp,
+	struct list_head		*items,
+	unsigned int			count)
+{
+	struct xfs_mount		*mp = tp->t_mountp;
+	struct xfs_cui_log_item		*cuip = xfs_cui_init(mp, count);
+	struct xfs_refcount_intent	*refc;
+
+	ASSERT(count > 0);
+
+	xfs_trans_add_item(tp, &cuip->cui_item);
+	list_for_each_entry(refc, items, ri_list)
+		xfs_refcount_update_log_item(tp, cuip, refc);
+	return cuip;
+}
+
 /* Get an CUD so we can process all the deferred refcount updates. */
 STATIC void *
 xfs_refcount_update_create_done(
@@ -432,7 +425,6 @@ const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
 	.diff_items	= xfs_refcount_update_diff_items,
 	.create_intent	= xfs_refcount_update_create_intent,
 	.abort_intent	= xfs_refcount_update_abort_intent,
-	.log_item	= xfs_refcount_update_log_item,
 	.create_done	= xfs_refcount_update_create_done,
 	.finish_item	= xfs_refcount_update_finish_item,
 	.finish_cleanup = xfs_refcount_update_finish_cleanup,
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 4911b68f95dda..842d817f5168c 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -352,41 +352,16 @@ xfs_rmap_update_diff_items(
 		XFS_FSB_TO_AGNO(mp, rb->ri_bmap.br_startblock);
 }
 
-/* Get an RUI. */
-STATIC void *
-xfs_rmap_update_create_intent(
-	struct xfs_trans		*tp,
-	unsigned int			count)
-{
-	struct xfs_rui_log_item		*ruip;
-
-	ASSERT(tp != NULL);
-	ASSERT(count > 0);
-
-	ruip = xfs_rui_init(tp->t_mountp, count);
-	ASSERT(ruip != NULL);
-
-	/*
-	 * Get a log_item_desc to point at the new item.
-	 */
-	xfs_trans_add_item(tp, &ruip->rui_item);
-	return ruip;
-}
-
 /* Log rmap updates in the intent item. */
 STATIC void
 xfs_rmap_update_log_item(
 	struct xfs_trans		*tp,
-	void				*intent,
-	struct list_head		*item)
+	struct xfs_rui_log_item		*ruip,
+	struct xfs_rmap_intent		*rmap)
 {
-	struct xfs_rui_log_item		*ruip = intent;
-	struct xfs_rmap_intent		*rmap;
 	uint				next_extent;
 	struct xfs_map_extent		*map;
 
-	rmap = container_of(item, struct xfs_rmap_intent, ri_list);
-
 	tp->t_flags |= XFS_TRANS_DIRTY;
 	set_bit(XFS_LI_DIRTY, &ruip->rui_item.li_flags);
 
@@ -406,6 +381,24 @@ xfs_rmap_update_log_item(
 			rmap->ri_bmap.br_state);
 }
 
+STATIC void *
+xfs_rmap_update_create_intent(
+	struct xfs_trans		*tp,
+	struct list_head		*items,
+	unsigned int			count)
+{
+	struct xfs_mount		*mp = tp->t_mountp;
+	struct xfs_rui_log_item		*ruip = xfs_rui_init(mp, count);
+	struct xfs_rmap_intent		*rmap;
+
+	ASSERT(count > 0);
+
+	xfs_trans_add_item(tp, &ruip->rui_item);
+	list_for_each_entry(rmap, items, ri_list)
+		xfs_rmap_update_log_item(tp, ruip, rmap);
+	return ruip;
+}
+
 /* Get an RUD so we can process all the deferred rmap updates. */
 STATIC void *
 xfs_rmap_update_create_done(
@@ -476,7 +469,6 @@ const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
 	.diff_items	= xfs_rmap_update_diff_items,
 	.create_intent	= xfs_rmap_update_create_intent,
 	.abort_intent	= xfs_rmap_update_abort_intent,
-	.log_item	= xfs_rmap_update_log_item,
 	.create_done	= xfs_rmap_update_create_done,
 	.finish_item	= xfs_rmap_update_finish_item,
 	.finish_cleanup = xfs_rmap_update_finish_cleanup,
-- 
2.26.2

Re: [PATCH 05/11] xfs: merge the ->log_item defer op into ->create_intent

[Brian Foster]

On Wed, Apr 29, 2020 at 05:05:05PM +0200, Christoph Hellwig wrote:
> These are aways called together, and my merging them we reduce the amount
> of indirect calls, improve type safety and in general clean up the code
> a bit.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_defer.c  |  6 ++---
>  fs/xfs/libxfs/xfs_defer.h  |  4 ++--
>  fs/xfs/xfs_bmap_item.c     | 47 +++++++++++++++---------------------
>  fs/xfs/xfs_extfree_item.c  | 49 ++++++++++++++++----------------------
>  fs/xfs/xfs_refcount_item.c | 48 ++++++++++++++++---------------------
>  fs/xfs/xfs_rmap_item.c     | 48 ++++++++++++++++---------------------
>  6 files changed, 83 insertions(+), 119 deletions(-)
> 
...
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index ee6f4229cebc4..dea97956d78d6 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
...
> @@ -355,6 +330,23 @@ xfs_bmap_update_log_item(
>  			bmap->bi_bmap.br_state);
>  }
>  
> +STATIC void *
> +xfs_bmap_update_create_intent(
> +	struct xfs_trans		*tp,
> +	struct list_head		*items,
> +	unsigned int			count)
> +{
> +	struct xfs_bui_log_item		*buip = xfs_bui_init(tp->t_mountp);

I'd prefer to see these _init() calls separate from the variable
declarations, but otherwise nice cleanup:

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

> +	struct xfs_bmap_intent		*bmap;
> +
> +	ASSERT(count == XFS_BUI_MAX_FAST_EXTENTS);
> +
> +	xfs_trans_add_item(tp, &buip->bui_item);
> +	list_for_each_entry(bmap, items, bi_list)
> +		xfs_bmap_update_log_item(tp, buip, bmap);
> +	return buip;
> +}
> +
>  /* Get an BUD so we can process all the deferred rmap updates. */
>  STATIC void *
>  xfs_bmap_update_create_done(
> @@ -419,7 +411,6 @@ const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
>  	.diff_items	= xfs_bmap_update_diff_items,
>  	.create_intent	= xfs_bmap_update_create_intent,
>  	.abort_intent	= xfs_bmap_update_abort_intent,
> -	.log_item	= xfs_bmap_update_log_item,
>  	.create_done	= xfs_bmap_update_create_done,
>  	.finish_item	= xfs_bmap_update_finish_item,
>  	.cancel_item	= xfs_bmap_update_cancel_item,
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 00309b81607cd..cb22c7ad31817 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -412,41 +412,16 @@ xfs_extent_free_diff_items(
>  		XFS_FSB_TO_AGNO(mp, rb->xefi_startblock);
>  }
>  
> -/* Get an EFI. */
> -STATIC void *
> -xfs_extent_free_create_intent(
> -	struct xfs_trans		*tp,
> -	unsigned int			count)
> -{
> -	struct xfs_efi_log_item		*efip;
> -
> -	ASSERT(tp != NULL);
> -	ASSERT(count > 0);
> -
> -	efip = xfs_efi_init(tp->t_mountp, count);
> -	ASSERT(efip != NULL);
> -
> -	/*
> -	 * Get a log_item_desc to point at the new item.
> -	 */
> -	xfs_trans_add_item(tp, &efip->efi_item);
> -	return efip;
> -}
> -
>  /* Log a free extent to the intent item. */
>  STATIC void
>  xfs_extent_free_log_item(
>  	struct xfs_trans		*tp,
> -	void				*intent,
> -	struct list_head		*item)
> +	struct xfs_efi_log_item		*efip,
> +	struct xfs_extent_free_item	*free)
>  {
> -	struct xfs_efi_log_item		*efip = intent;
> -	struct xfs_extent_free_item	*free;
>  	uint				next_extent;
>  	struct xfs_extent		*extp;
>  
> -	free = container_of(item, struct xfs_extent_free_item, xefi_list);
> -
>  	tp->t_flags |= XFS_TRANS_DIRTY;
>  	set_bit(XFS_LI_DIRTY, &efip->efi_item.li_flags);
>  
> @@ -462,6 +437,24 @@ xfs_extent_free_log_item(
>  	extp->ext_len = free->xefi_blockcount;
>  }
>  
> +STATIC void *
> +xfs_extent_free_create_intent(
> +	struct xfs_trans		*tp,
> +	struct list_head		*items,
> +	unsigned int			count)
> +{
> +	struct xfs_mount		*mp = tp->t_mountp;
> +	struct xfs_efi_log_item		*efip = xfs_efi_init(mp, count);
> +	struct xfs_extent_free_item	*free;
> +
> +	ASSERT(count > 0);
> +
> +	xfs_trans_add_item(tp, &efip->efi_item);
> +	list_for_each_entry(free, items, xefi_list)
> +		xfs_extent_free_log_item(tp, efip, free);
> +	return efip;
> +}
> +
>  /* Get an EFD so we can process all the free extents. */
>  STATIC void *
>  xfs_extent_free_create_done(
> @@ -516,7 +509,6 @@ const struct xfs_defer_op_type xfs_extent_free_defer_type = {
>  	.diff_items	= xfs_extent_free_diff_items,
>  	.create_intent	= xfs_extent_free_create_intent,
>  	.abort_intent	= xfs_extent_free_abort_intent,
> -	.log_item	= xfs_extent_free_log_item,
>  	.create_done	= xfs_extent_free_create_done,
>  	.finish_item	= xfs_extent_free_finish_item,
>  	.cancel_item	= xfs_extent_free_cancel_item,
> @@ -582,7 +574,6 @@ const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
>  	.diff_items	= xfs_extent_free_diff_items,
>  	.create_intent	= xfs_extent_free_create_intent,
>  	.abort_intent	= xfs_extent_free_abort_intent,
> -	.log_item	= xfs_extent_free_log_item,
>  	.create_done	= xfs_extent_free_create_done,
>  	.finish_item	= xfs_agfl_free_finish_item,
>  	.cancel_item	= xfs_extent_free_cancel_item,
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index 8eeed73928cdf..325d49fc0406c 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -284,27 +284,6 @@ xfs_refcount_update_diff_items(
>  		XFS_FSB_TO_AGNO(mp, rb->ri_startblock);
>  }
>  
> -/* Get an CUI. */
> -STATIC void *
> -xfs_refcount_update_create_intent(
> -	struct xfs_trans		*tp,
> -	unsigned int			count)
> -{
> -	struct xfs_cui_log_item		*cuip;
> -
> -	ASSERT(tp != NULL);
> -	ASSERT(count > 0);
> -
> -	cuip = xfs_cui_init(tp->t_mountp, count);
> -	ASSERT(cuip != NULL);
> -
> -	/*
> -	 * Get a log_item_desc to point at the new item.
> -	 */
> -	xfs_trans_add_item(tp, &cuip->cui_item);
> -	return cuip;
> -}
> -
>  /* Set the phys extent flags for this reverse mapping. */
>  static void
>  xfs_trans_set_refcount_flags(
> @@ -328,16 +307,12 @@ xfs_trans_set_refcount_flags(
>  STATIC void
>  xfs_refcount_update_log_item(
>  	struct xfs_trans		*tp,
> -	void				*intent,
> -	struct list_head		*item)
> +	struct xfs_cui_log_item		*cuip,
> +	struct xfs_refcount_intent	*refc)
>  {
> -	struct xfs_cui_log_item		*cuip = intent;
> -	struct xfs_refcount_intent	*refc;
>  	uint				next_extent;
>  	struct xfs_phys_extent		*ext;
>  
> -	refc = container_of(item, struct xfs_refcount_intent, ri_list);
> -
>  	tp->t_flags |= XFS_TRANS_DIRTY;
>  	set_bit(XFS_LI_DIRTY, &cuip->cui_item.li_flags);
>  
> @@ -354,6 +329,24 @@ xfs_refcount_update_log_item(
>  	xfs_trans_set_refcount_flags(ext, refc->ri_type);
>  }
>  
> +STATIC void *
> +xfs_refcount_update_create_intent(
> +	struct xfs_trans		*tp,
> +	struct list_head		*items,
> +	unsigned int			count)
> +{
> +	struct xfs_mount		*mp = tp->t_mountp;
> +	struct xfs_cui_log_item		*cuip = xfs_cui_init(mp, count);
> +	struct xfs_refcount_intent	*refc;
> +
> +	ASSERT(count > 0);
> +
> +	xfs_trans_add_item(tp, &cuip->cui_item);
> +	list_for_each_entry(refc, items, ri_list)
> +		xfs_refcount_update_log_item(tp, cuip, refc);
> +	return cuip;
> +}
> +
>  /* Get an CUD so we can process all the deferred refcount updates. */
>  STATIC void *
>  xfs_refcount_update_create_done(
> @@ -432,7 +425,6 @@ const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
>  	.diff_items	= xfs_refcount_update_diff_items,
>  	.create_intent	= xfs_refcount_update_create_intent,
>  	.abort_intent	= xfs_refcount_update_abort_intent,
> -	.log_item	= xfs_refcount_update_log_item,
>  	.create_done	= xfs_refcount_update_create_done,
>  	.finish_item	= xfs_refcount_update_finish_item,
>  	.finish_cleanup = xfs_refcount_update_finish_cleanup,
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index 4911b68f95dda..842d817f5168c 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -352,41 +352,16 @@ xfs_rmap_update_diff_items(
>  		XFS_FSB_TO_AGNO(mp, rb->ri_bmap.br_startblock);
>  }
>  
> -/* Get an RUI. */
> -STATIC void *
> -xfs_rmap_update_create_intent(
> -	struct xfs_trans		*tp,
> -	unsigned int			count)
> -{
> -	struct xfs_rui_log_item		*ruip;
> -
> -	ASSERT(tp != NULL);
> -	ASSERT(count > 0);
> -
> -	ruip = xfs_rui_init(tp->t_mountp, count);
> -	ASSERT(ruip != NULL);
> -
> -	/*
> -	 * Get a log_item_desc to point at the new item.
> -	 */
> -	xfs_trans_add_item(tp, &ruip->rui_item);
> -	return ruip;
> -}
> -
>  /* Log rmap updates in the intent item. */
>  STATIC void
>  xfs_rmap_update_log_item(
>  	struct xfs_trans		*tp,
> -	void				*intent,
> -	struct list_head		*item)
> +	struct xfs_rui_log_item		*ruip,
> +	struct xfs_rmap_intent		*rmap)
>  {
> -	struct xfs_rui_log_item		*ruip = intent;
> -	struct xfs_rmap_intent		*rmap;
>  	uint				next_extent;
>  	struct xfs_map_extent		*map;
>  
> -	rmap = container_of(item, struct xfs_rmap_intent, ri_list);
> -
>  	tp->t_flags |= XFS_TRANS_DIRTY;
>  	set_bit(XFS_LI_DIRTY, &ruip->rui_item.li_flags);
>  
> @@ -406,6 +381,24 @@ xfs_rmap_update_log_item(
>  			rmap->ri_bmap.br_state);
>  }
>  
> +STATIC void *
> +xfs_rmap_update_create_intent(
> +	struct xfs_trans		*tp,
> +	struct list_head		*items,
> +	unsigned int			count)
> +{
> +	struct xfs_mount		*mp = tp->t_mountp;
> +	struct xfs_rui_log_item		*ruip = xfs_rui_init(mp, count);
> +	struct xfs_rmap_intent		*rmap;
> +
> +	ASSERT(count > 0);
> +
> +	xfs_trans_add_item(tp, &ruip->rui_item);
> +	list_for_each_entry(rmap, items, ri_list)
> +		xfs_rmap_update_log_item(tp, ruip, rmap);
> +	return ruip;
> +}
> +
>  /* Get an RUD so we can process all the deferred rmap updates. */
>  STATIC void *
>  xfs_rmap_update_create_done(
> @@ -476,7 +469,6 @@ const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
>  	.diff_items	= xfs_rmap_update_diff_items,
>  	.create_intent	= xfs_rmap_update_create_intent,
>  	.abort_intent	= xfs_rmap_update_abort_intent,
> -	.log_item	= xfs_rmap_update_log_item,
>  	.create_done	= xfs_rmap_update_create_done,
>  	.finish_item	= xfs_rmap_update_finish_item,
>  	.finish_cleanup = xfs_rmap_update_finish_cleanup,
> -- 
> 2.26.2
> 

[PATCH 06/11] xfs: merge the ->diff_items defer op into ->create_intent

[Christoph Hellwig]

This avoids a per-item indirect call, and also simplifies the interface
a bit.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_defer.c  | 5 +----
 fs/xfs/libxfs/xfs_defer.h  | 3 +--
 fs/xfs/xfs_bmap_item.c     | 9 ++++++---
 fs/xfs/xfs_extfree_item.c  | 7 ++++---
 fs/xfs/xfs_refcount_item.c | 6 ++++--
 fs/xfs/xfs_rmap_item.c     | 6 ++++--
 6 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 56d1357f9d137..5402a7bf31108 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -186,11 +186,8 @@ xfs_defer_create_intent(
 {
 	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
 
-	if (sort)
-		list_sort(tp->t_mountp, &dfp->dfp_work, ops->diff_items);
-
 	dfp->dfp_intent = ops->create_intent(tp, &dfp->dfp_work,
-			dfp->dfp_count);
+			dfp->dfp_count, sort);
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index d6a4577c25b05..660f5c3821d6b 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -49,9 +49,8 @@ struct xfs_defer_op_type {
 			void **);
 	void (*finish_cleanup)(struct xfs_trans *, void *, int);
 	void (*cancel_item)(struct list_head *);
-	int (*diff_items)(void *, struct list_head *, struct list_head *);
 	void *(*create_intent)(struct xfs_trans *tp, struct list_head *items,
-			unsigned int count);
+			unsigned int count, bool sort);
 	unsigned int		max_items;
 };
 
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index dea97956d78d6..f9505c5873bd2 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -334,14 +334,18 @@ STATIC void *
 xfs_bmap_update_create_intent(
 	struct xfs_trans		*tp,
 	struct list_head		*items,
-	unsigned int			count)
+	unsigned int			count,
+	bool				sort)
 {
-	struct xfs_bui_log_item		*buip = xfs_bui_init(tp->t_mountp);
+	struct xfs_mount		*mp = tp->t_mountp;
+	struct xfs_bui_log_item		*buip = xfs_bui_init(mp);
 	struct xfs_bmap_intent		*bmap;
 
 	ASSERT(count == XFS_BUI_MAX_FAST_EXTENTS);
 
 	xfs_trans_add_item(tp, &buip->bui_item);
+	if (sort)
+		list_sort(mp, items, xfs_bmap_update_diff_items);
 	list_for_each_entry(bmap, items, bi_list)
 		xfs_bmap_update_log_item(tp, buip, bmap);
 	return buip;
@@ -408,7 +412,6 @@ xfs_bmap_update_cancel_item(
 
 const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
 	.max_items	= XFS_BUI_MAX_FAST_EXTENTS,
-	.diff_items	= xfs_bmap_update_diff_items,
 	.create_intent	= xfs_bmap_update_create_intent,
 	.abort_intent	= xfs_bmap_update_abort_intent,
 	.create_done	= xfs_bmap_update_create_done,
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index cb22c7ad31817..3e10eba9d22bd 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -441,7 +441,8 @@ STATIC void *
 xfs_extent_free_create_intent(
 	struct xfs_trans		*tp,
 	struct list_head		*items,
-	unsigned int			count)
+	unsigned int			count,
+	bool				sort)
 {
 	struct xfs_mount		*mp = tp->t_mountp;
 	struct xfs_efi_log_item		*efip = xfs_efi_init(mp, count);
@@ -450,6 +451,8 @@ xfs_extent_free_create_intent(
 	ASSERT(count > 0);
 
 	xfs_trans_add_item(tp, &efip->efi_item);
+	if (sort)
+		list_sort(mp, items, xfs_extent_free_diff_items);
 	list_for_each_entry(free, items, xefi_list)
 		xfs_extent_free_log_item(tp, efip, free);
 	return efip;
@@ -506,7 +509,6 @@ xfs_extent_free_cancel_item(
 
 const struct xfs_defer_op_type xfs_extent_free_defer_type = {
 	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
-	.diff_items	= xfs_extent_free_diff_items,
 	.create_intent	= xfs_extent_free_create_intent,
 	.abort_intent	= xfs_extent_free_abort_intent,
 	.create_done	= xfs_extent_free_create_done,
@@ -571,7 +573,6 @@ xfs_agfl_free_finish_item(
 /* sub-type with special handling for AGFL deferred frees */
 const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
 	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
-	.diff_items	= xfs_extent_free_diff_items,
 	.create_intent	= xfs_extent_free_create_intent,
 	.abort_intent	= xfs_extent_free_abort_intent,
 	.create_done	= xfs_extent_free_create_done,
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 325d49fc0406c..efc32ec55afdf 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -333,7 +333,8 @@ STATIC void *
 xfs_refcount_update_create_intent(
 	struct xfs_trans		*tp,
 	struct list_head		*items,
-	unsigned int			count)
+	unsigned int			count,
+	bool				sort)
 {
 	struct xfs_mount		*mp = tp->t_mountp;
 	struct xfs_cui_log_item		*cuip = xfs_cui_init(mp, count);
@@ -342,6 +343,8 @@ xfs_refcount_update_create_intent(
 	ASSERT(count > 0);
 
 	xfs_trans_add_item(tp, &cuip->cui_item);
+	if (sort)
+		list_sort(mp, items, xfs_refcount_update_diff_items);
 	list_for_each_entry(refc, items, ri_list)
 		xfs_refcount_update_log_item(tp, cuip, refc);
 	return cuip;
@@ -422,7 +425,6 @@ xfs_refcount_update_cancel_item(
 
 const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
 	.max_items	= XFS_CUI_MAX_FAST_EXTENTS,
-	.diff_items	= xfs_refcount_update_diff_items,
 	.create_intent	= xfs_refcount_update_create_intent,
 	.abort_intent	= xfs_refcount_update_abort_intent,
 	.create_done	= xfs_refcount_update_create_done,
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 842d817f5168c..40567cf0c216e 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -385,7 +385,8 @@ STATIC void *
 xfs_rmap_update_create_intent(
 	struct xfs_trans		*tp,
 	struct list_head		*items,
-	unsigned int			count)
+	unsigned int			count,
+	bool				sort)
 {
 	struct xfs_mount		*mp = tp->t_mountp;
 	struct xfs_rui_log_item		*ruip = xfs_rui_init(mp, count);
@@ -394,6 +395,8 @@ xfs_rmap_update_create_intent(
 	ASSERT(count > 0);
 
 	xfs_trans_add_item(tp, &ruip->rui_item);
+	if (sort)
+		list_sort(mp, items, xfs_rmap_update_diff_items);
 	list_for_each_entry(rmap, items, ri_list)
 		xfs_rmap_update_log_item(tp, ruip, rmap);
 	return ruip;
@@ -466,7 +469,6 @@ xfs_rmap_update_cancel_item(
 
 const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
 	.max_items	= XFS_RUI_MAX_FAST_EXTENTS,
-	.diff_items	= xfs_rmap_update_diff_items,
 	.create_intent	= xfs_rmap_update_create_intent,
 	.abort_intent	= xfs_rmap_update_abort_intent,
 	.create_done	= xfs_rmap_update_create_done,
-- 
2.26.2

Re: [PATCH 06/11] xfs: merge the ->diff_items defer op into ->create_intent

[Brian Foster]

On Wed, Apr 29, 2020 at 05:05:06PM +0200, Christoph Hellwig wrote:
> This avoids a per-item indirect call, and also simplifies the interface
> a bit.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/libxfs/xfs_defer.c  | 5 +----
>  fs/xfs/libxfs/xfs_defer.h  | 3 +--
>  fs/xfs/xfs_bmap_item.c     | 9 ++++++---
>  fs/xfs/xfs_extfree_item.c  | 7 ++++---
>  fs/xfs/xfs_refcount_item.c | 6 ++++--
>  fs/xfs/xfs_rmap_item.c     | 6 ++++--
>  6 files changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 56d1357f9d137..5402a7bf31108 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -186,11 +186,8 @@ xfs_defer_create_intent(
>  {
>  	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
>  
> -	if (sort)
> -		list_sort(tp->t_mountp, &dfp->dfp_work, ops->diff_items);
> -
>  	dfp->dfp_intent = ops->create_intent(tp, &dfp->dfp_work,
> -			dfp->dfp_count);
> +			dfp->dfp_count, sort);
>  }
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index d6a4577c25b05..660f5c3821d6b 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -49,9 +49,8 @@ struct xfs_defer_op_type {
>  			void **);
>  	void (*finish_cleanup)(struct xfs_trans *, void *, int);
>  	void (*cancel_item)(struct list_head *);
> -	int (*diff_items)(void *, struct list_head *, struct list_head *);
>  	void *(*create_intent)(struct xfs_trans *tp, struct list_head *items,
> -			unsigned int count);
> +			unsigned int count, bool sort);
>  	unsigned int		max_items;
>  };
>  
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index dea97956d78d6..f9505c5873bd2 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -334,14 +334,18 @@ STATIC void *
>  xfs_bmap_update_create_intent(
>  	struct xfs_trans		*tp,
>  	struct list_head		*items,
> -	unsigned int			count)
> +	unsigned int			count,
> +	bool				sort)
>  {
> -	struct xfs_bui_log_item		*buip = xfs_bui_init(tp->t_mountp);
> +	struct xfs_mount		*mp = tp->t_mountp;
> +	struct xfs_bui_log_item		*buip = xfs_bui_init(mp);
>  	struct xfs_bmap_intent		*bmap;
>  
>  	ASSERT(count == XFS_BUI_MAX_FAST_EXTENTS);
>  
>  	xfs_trans_add_item(tp, &buip->bui_item);
> +	if (sort)
> +		list_sort(mp, items, xfs_bmap_update_diff_items);
>  	list_for_each_entry(bmap, items, bi_list)
>  		xfs_bmap_update_log_item(tp, buip, bmap);
>  	return buip;
> @@ -408,7 +412,6 @@ xfs_bmap_update_cancel_item(
>  
>  const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
>  	.max_items	= XFS_BUI_MAX_FAST_EXTENTS,
> -	.diff_items	= xfs_bmap_update_diff_items,
>  	.create_intent	= xfs_bmap_update_create_intent,
>  	.abort_intent	= xfs_bmap_update_abort_intent,
>  	.create_done	= xfs_bmap_update_create_done,
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index cb22c7ad31817..3e10eba9d22bd 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -441,7 +441,8 @@ STATIC void *
>  xfs_extent_free_create_intent(
>  	struct xfs_trans		*tp,
>  	struct list_head		*items,
> -	unsigned int			count)
> +	unsigned int			count,
> +	bool				sort)
>  {
>  	struct xfs_mount		*mp = tp->t_mountp;
>  	struct xfs_efi_log_item		*efip = xfs_efi_init(mp, count);
> @@ -450,6 +451,8 @@ xfs_extent_free_create_intent(
>  	ASSERT(count > 0);
>  
>  	xfs_trans_add_item(tp, &efip->efi_item);
> +	if (sort)
> +		list_sort(mp, items, xfs_extent_free_diff_items);
>  	list_for_each_entry(free, items, xefi_list)
>  		xfs_extent_free_log_item(tp, efip, free);
>  	return efip;
> @@ -506,7 +509,6 @@ xfs_extent_free_cancel_item(
>  
>  const struct xfs_defer_op_type xfs_extent_free_defer_type = {
>  	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
> -	.diff_items	= xfs_extent_free_diff_items,
>  	.create_intent	= xfs_extent_free_create_intent,
>  	.abort_intent	= xfs_extent_free_abort_intent,
>  	.create_done	= xfs_extent_free_create_done,
> @@ -571,7 +573,6 @@ xfs_agfl_free_finish_item(
>  /* sub-type with special handling for AGFL deferred frees */
>  const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
>  	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
> -	.diff_items	= xfs_extent_free_diff_items,
>  	.create_intent	= xfs_extent_free_create_intent,
>  	.abort_intent	= xfs_extent_free_abort_intent,
>  	.create_done	= xfs_extent_free_create_done,
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index 325d49fc0406c..efc32ec55afdf 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -333,7 +333,8 @@ STATIC void *
>  xfs_refcount_update_create_intent(
>  	struct xfs_trans		*tp,
>  	struct list_head		*items,
> -	unsigned int			count)
> +	unsigned int			count,
> +	bool				sort)
>  {
>  	struct xfs_mount		*mp = tp->t_mountp;
>  	struct xfs_cui_log_item		*cuip = xfs_cui_init(mp, count);
> @@ -342,6 +343,8 @@ xfs_refcount_update_create_intent(
>  	ASSERT(count > 0);
>  
>  	xfs_trans_add_item(tp, &cuip->cui_item);
> +	if (sort)
> +		list_sort(mp, items, xfs_refcount_update_diff_items);
>  	list_for_each_entry(refc, items, ri_list)
>  		xfs_refcount_update_log_item(tp, cuip, refc);
>  	return cuip;
> @@ -422,7 +425,6 @@ xfs_refcount_update_cancel_item(
>  
>  const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
>  	.max_items	= XFS_CUI_MAX_FAST_EXTENTS,
> -	.diff_items	= xfs_refcount_update_diff_items,
>  	.create_intent	= xfs_refcount_update_create_intent,
>  	.abort_intent	= xfs_refcount_update_abort_intent,
>  	.create_done	= xfs_refcount_update_create_done,
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index 842d817f5168c..40567cf0c216e 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -385,7 +385,8 @@ STATIC void *
>  xfs_rmap_update_create_intent(
>  	struct xfs_trans		*tp,
>  	struct list_head		*items,
> -	unsigned int			count)
> +	unsigned int			count,
> +	bool				sort)
>  {
>  	struct xfs_mount		*mp = tp->t_mountp;
>  	struct xfs_rui_log_item		*ruip = xfs_rui_init(mp, count);
> @@ -394,6 +395,8 @@ xfs_rmap_update_create_intent(
>  	ASSERT(count > 0);
>  
>  	xfs_trans_add_item(tp, &ruip->rui_item);
> +	if (sort)
> +		list_sort(mp, items, xfs_rmap_update_diff_items);
>  	list_for_each_entry(rmap, items, ri_list)
>  		xfs_rmap_update_log_item(tp, ruip, rmap);
>  	return ruip;
> @@ -466,7 +469,6 @@ xfs_rmap_update_cancel_item(
>  
>  const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
>  	.max_items	= XFS_RUI_MAX_FAST_EXTENTS,
> -	.diff_items	= xfs_rmap_update_diff_items,
>  	.create_intent	= xfs_rmap_update_create_intent,
>  	.abort_intent	= xfs_rmap_update_abort_intent,
>  	.create_done	= xfs_rmap_update_create_done,
> -- 
> 2.26.2
> 

[PATCH 07/11] xfs: turn dfp_intent into a xfs_log_item

[Christoph Hellwig]

All defer op instance place their own extension of the log item into
the dfp_intent field.  Replace that with a xfs_log_item to improve type
safety and make the code easier to follow.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_defer.h  | 11 ++++++-----
 fs/xfs/xfs_bmap_item.c     | 12 ++++++------
 fs/xfs/xfs_extfree_item.c  | 12 ++++++------
 fs/xfs/xfs_refcount_item.c | 12 ++++++------
 fs/xfs/xfs_rmap_item.c     | 12 ++++++------
 5 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index 660f5c3821d6b..7b6cc3808a91b 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -28,7 +28,7 @@ enum xfs_defer_ops_type {
 struct xfs_defer_pending {
 	struct list_head		dfp_list;	/* pending items */
 	struct list_head		dfp_work;	/* work items */
-	void				*dfp_intent;	/* log intent item */
+	struct xfs_log_item		*dfp_intent;	/* log intent item */
 	void				*dfp_done;	/* log done item */
 	unsigned int			dfp_count;	/* # extent items */
 	enum xfs_defer_ops_type		dfp_type;
@@ -43,14 +43,15 @@ void xfs_defer_move(struct xfs_trans *dtp, struct xfs_trans *stp);
 
 /* Description of a deferred type. */
 struct xfs_defer_op_type {
-	void (*abort_intent)(void *);
-	void *(*create_done)(struct xfs_trans *, void *, unsigned int);
+	struct xfs_log_item *(*create_intent)(struct xfs_trans *tp,
+			struct list_head *items, unsigned int count, bool sort);
+	void (*abort_intent)(struct xfs_log_item *intent);
+	void *(*create_done)(struct xfs_trans *tp, struct xfs_log_item *intent,
+			unsigned int count);
 	int (*finish_item)(struct xfs_trans *, struct list_head *, void *,
 			void **);
 	void (*finish_cleanup)(struct xfs_trans *, void *, int);
 	void (*cancel_item)(struct list_head *);
-	void *(*create_intent)(struct xfs_trans *tp, struct list_head *items,
-			unsigned int count, bool sort);
 	unsigned int		max_items;
 };
 
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index f9505c5873bd2..7b2153fca2d9e 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -330,7 +330,7 @@ xfs_bmap_update_log_item(
 			bmap->bi_bmap.br_state);
 }
 
-STATIC void *
+static struct xfs_log_item *
 xfs_bmap_update_create_intent(
 	struct xfs_trans		*tp,
 	struct list_head		*items,
@@ -348,17 +348,17 @@ xfs_bmap_update_create_intent(
 		list_sort(mp, items, xfs_bmap_update_diff_items);
 	list_for_each_entry(bmap, items, bi_list)
 		xfs_bmap_update_log_item(tp, buip, bmap);
-	return buip;
+	return &buip->bui_item;
 }
 
 /* Get an BUD so we can process all the deferred rmap updates. */
 STATIC void *
 xfs_bmap_update_create_done(
 	struct xfs_trans		*tp,
-	void				*intent,
+	struct xfs_log_item		*intent,
 	unsigned int			count)
 {
-	return xfs_trans_get_bud(tp, intent);
+	return xfs_trans_get_bud(tp, BUI_ITEM(intent));
 }
 
 /* Process a deferred rmap update. */
@@ -394,9 +394,9 @@ xfs_bmap_update_finish_item(
 /* Abort all pending BUIs. */
 STATIC void
 xfs_bmap_update_abort_intent(
-	void				*intent)
+	struct xfs_log_item		*intent)
 {
-	xfs_bui_release(intent);
+	xfs_bui_release(BUI_ITEM(intent));
 }
 
 /* Cancel a deferred rmap update. */
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 3e10eba9d22bd..0453b6f2b1d69 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -437,7 +437,7 @@ xfs_extent_free_log_item(
 	extp->ext_len = free->xefi_blockcount;
 }
 
-STATIC void *
+static struct xfs_log_item *
 xfs_extent_free_create_intent(
 	struct xfs_trans		*tp,
 	struct list_head		*items,
@@ -455,17 +455,17 @@ xfs_extent_free_create_intent(
 		list_sort(mp, items, xfs_extent_free_diff_items);
 	list_for_each_entry(free, items, xefi_list)
 		xfs_extent_free_log_item(tp, efip, free);
-	return efip;
+	return &efip->efi_item;
 }
 
 /* Get an EFD so we can process all the free extents. */
 STATIC void *
 xfs_extent_free_create_done(
 	struct xfs_trans		*tp,
-	void				*intent,
+	struct xfs_log_item		*intent,
 	unsigned int			count)
 {
-	return xfs_trans_get_efd(tp, intent, count);
+	return xfs_trans_get_efd(tp, EFI_ITEM(intent), count);
 }
 
 /* Process a free extent. */
@@ -491,9 +491,9 @@ xfs_extent_free_finish_item(
 /* Abort all pending EFIs. */
 STATIC void
 xfs_extent_free_abort_intent(
-	void				*intent)
+	struct xfs_log_item		*intent)
 {
-	xfs_efi_release(intent);
+	xfs_efi_release(EFI_ITEM(intent));
 }
 
 /* Cancel a free extent. */
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index efc32ec55afdf..e8d3278e066e3 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -329,7 +329,7 @@ xfs_refcount_update_log_item(
 	xfs_trans_set_refcount_flags(ext, refc->ri_type);
 }
 
-STATIC void *
+static struct xfs_log_item *
 xfs_refcount_update_create_intent(
 	struct xfs_trans		*tp,
 	struct list_head		*items,
@@ -347,17 +347,17 @@ xfs_refcount_update_create_intent(
 		list_sort(mp, items, xfs_refcount_update_diff_items);
 	list_for_each_entry(refc, items, ri_list)
 		xfs_refcount_update_log_item(tp, cuip, refc);
-	return cuip;
+	return &cuip->cui_item;
 }
 
 /* Get an CUD so we can process all the deferred refcount updates. */
 STATIC void *
 xfs_refcount_update_create_done(
 	struct xfs_trans		*tp,
-	void				*intent,
+	struct xfs_log_item		*intent,
 	unsigned int			count)
 {
-	return xfs_trans_get_cud(tp, intent);
+	return xfs_trans_get_cud(tp, CUI_ITEM(intent));
 }
 
 /* Process a deferred refcount update. */
@@ -407,9 +407,9 @@ xfs_refcount_update_finish_cleanup(
 /* Abort all pending CUIs. */
 STATIC void
 xfs_refcount_update_abort_intent(
-	void				*intent)
+	struct xfs_log_item		*intent)
 {
-	xfs_cui_release(intent);
+	xfs_cui_release(CUI_ITEM(intent));
 }
 
 /* Cancel a deferred refcount update. */
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 40567cf0c216e..a417e15fd0ce4 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -381,7 +381,7 @@ xfs_rmap_update_log_item(
 			rmap->ri_bmap.br_state);
 }
 
-STATIC void *
+static struct xfs_log_item *
 xfs_rmap_update_create_intent(
 	struct xfs_trans		*tp,
 	struct list_head		*items,
@@ -399,17 +399,17 @@ xfs_rmap_update_create_intent(
 		list_sort(mp, items, xfs_rmap_update_diff_items);
 	list_for_each_entry(rmap, items, ri_list)
 		xfs_rmap_update_log_item(tp, ruip, rmap);
-	return ruip;
+	return &ruip->rui_item;
 }
 
 /* Get an RUD so we can process all the deferred rmap updates. */
 STATIC void *
 xfs_rmap_update_create_done(
 	struct xfs_trans		*tp,
-	void				*intent,
+	struct xfs_log_item		*intent,
 	unsigned int			count)
 {
-	return xfs_trans_get_rud(tp, intent);
+	return xfs_trans_get_rud(tp, RUI_ITEM(intent));
 }
 
 /* Process a deferred rmap update. */
@@ -451,9 +451,9 @@ xfs_rmap_update_finish_cleanup(
 /* Abort all pending RUIs. */
 STATIC void
 xfs_rmap_update_abort_intent(
-	void				*intent)
+	struct xfs_log_item	*intent)
 {
-	xfs_rui_release(intent);
+	xfs_rui_release(RUI_ITEM(intent));
 }
 
 /* Cancel a deferred rmap update. */
-- 
2.26.2

Re: [PATCH 07/11] xfs: turn dfp_intent into a xfs_log_item

[Brian Foster]

On Wed, Apr 29, 2020 at 05:05:07PM +0200, Christoph Hellwig wrote:
> All defer op instance place their own extension of the log item into
> the dfp_intent field.  Replace that with a xfs_log_item to improve type
> safety and make the code easier to follow.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/libxfs/xfs_defer.h  | 11 ++++++-----
>  fs/xfs/xfs_bmap_item.c     | 12 ++++++------
>  fs/xfs/xfs_extfree_item.c  | 12 ++++++------
>  fs/xfs/xfs_refcount_item.c | 12 ++++++------
>  fs/xfs/xfs_rmap_item.c     | 12 ++++++------
>  5 files changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index 660f5c3821d6b..7b6cc3808a91b 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -28,7 +28,7 @@ enum xfs_defer_ops_type {
>  struct xfs_defer_pending {
>  	struct list_head		dfp_list;	/* pending items */
>  	struct list_head		dfp_work;	/* work items */
> -	void				*dfp_intent;	/* log intent item */
> +	struct xfs_log_item		*dfp_intent;	/* log intent item */
>  	void				*dfp_done;	/* log done item */
>  	unsigned int			dfp_count;	/* # extent items */
>  	enum xfs_defer_ops_type		dfp_type;
> @@ -43,14 +43,15 @@ void xfs_defer_move(struct xfs_trans *dtp, struct xfs_trans *stp);
>  
>  /* Description of a deferred type. */
>  struct xfs_defer_op_type {
> -	void (*abort_intent)(void *);
> -	void *(*create_done)(struct xfs_trans *, void *, unsigned int);
> +	struct xfs_log_item *(*create_intent)(struct xfs_trans *tp,
> +			struct list_head *items, unsigned int count, bool sort);
> +	void (*abort_intent)(struct xfs_log_item *intent);
> +	void *(*create_done)(struct xfs_trans *tp, struct xfs_log_item *intent,
> +			unsigned int count);
>  	int (*finish_item)(struct xfs_trans *, struct list_head *, void *,
>  			void **);
>  	void (*finish_cleanup)(struct xfs_trans *, void *, int);
>  	void (*cancel_item)(struct list_head *);
> -	void *(*create_intent)(struct xfs_trans *tp, struct list_head *items,
> -			unsigned int count, bool sort);
>  	unsigned int		max_items;
>  };
>  
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index f9505c5873bd2..7b2153fca2d9e 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -330,7 +330,7 @@ xfs_bmap_update_log_item(
>  			bmap->bi_bmap.br_state);
>  }
>  
> -STATIC void *
> +static struct xfs_log_item *
>  xfs_bmap_update_create_intent(
>  	struct xfs_trans		*tp,
>  	struct list_head		*items,
> @@ -348,17 +348,17 @@ xfs_bmap_update_create_intent(
>  		list_sort(mp, items, xfs_bmap_update_diff_items);
>  	list_for_each_entry(bmap, items, bi_list)
>  		xfs_bmap_update_log_item(tp, buip, bmap);
> -	return buip;
> +	return &buip->bui_item;
>  }
>  
>  /* Get an BUD so we can process all the deferred rmap updates. */
>  STATIC void *
>  xfs_bmap_update_create_done(
>  	struct xfs_trans		*tp,
> -	void				*intent,
> +	struct xfs_log_item		*intent,
>  	unsigned int			count)
>  {
> -	return xfs_trans_get_bud(tp, intent);
> +	return xfs_trans_get_bud(tp, BUI_ITEM(intent));
>  }
>  
>  /* Process a deferred rmap update. */
> @@ -394,9 +394,9 @@ xfs_bmap_update_finish_item(
>  /* Abort all pending BUIs. */
>  STATIC void
>  xfs_bmap_update_abort_intent(
> -	void				*intent)
> +	struct xfs_log_item		*intent)
>  {
> -	xfs_bui_release(intent);
> +	xfs_bui_release(BUI_ITEM(intent));
>  }
>  
>  /* Cancel a deferred rmap update. */
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 3e10eba9d22bd..0453b6f2b1d69 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -437,7 +437,7 @@ xfs_extent_free_log_item(
>  	extp->ext_len = free->xefi_blockcount;
>  }
>  
> -STATIC void *
> +static struct xfs_log_item *
>  xfs_extent_free_create_intent(
>  	struct xfs_trans		*tp,
>  	struct list_head		*items,
> @@ -455,17 +455,17 @@ xfs_extent_free_create_intent(
>  		list_sort(mp, items, xfs_extent_free_diff_items);
>  	list_for_each_entry(free, items, xefi_list)
>  		xfs_extent_free_log_item(tp, efip, free);
> -	return efip;
> +	return &efip->efi_item;
>  }
>  
>  /* Get an EFD so we can process all the free extents. */
>  STATIC void *
>  xfs_extent_free_create_done(
>  	struct xfs_trans		*tp,
> -	void				*intent,
> +	struct xfs_log_item		*intent,
>  	unsigned int			count)
>  {
> -	return xfs_trans_get_efd(tp, intent, count);
> +	return xfs_trans_get_efd(tp, EFI_ITEM(intent), count);
>  }
>  
>  /* Process a free extent. */
> @@ -491,9 +491,9 @@ xfs_extent_free_finish_item(
>  /* Abort all pending EFIs. */
>  STATIC void
>  xfs_extent_free_abort_intent(
> -	void				*intent)
> +	struct xfs_log_item		*intent)
>  {
> -	xfs_efi_release(intent);
> +	xfs_efi_release(EFI_ITEM(intent));
>  }
>  
>  /* Cancel a free extent. */
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index efc32ec55afdf..e8d3278e066e3 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -329,7 +329,7 @@ xfs_refcount_update_log_item(
>  	xfs_trans_set_refcount_flags(ext, refc->ri_type);
>  }
>  
> -STATIC void *
> +static struct xfs_log_item *
>  xfs_refcount_update_create_intent(
>  	struct xfs_trans		*tp,
>  	struct list_head		*items,
> @@ -347,17 +347,17 @@ xfs_refcount_update_create_intent(
>  		list_sort(mp, items, xfs_refcount_update_diff_items);
>  	list_for_each_entry(refc, items, ri_list)
>  		xfs_refcount_update_log_item(tp, cuip, refc);
> -	return cuip;
> +	return &cuip->cui_item;
>  }
>  
>  /* Get an CUD so we can process all the deferred refcount updates. */
>  STATIC void *
>  xfs_refcount_update_create_done(
>  	struct xfs_trans		*tp,
> -	void				*intent,
> +	struct xfs_log_item		*intent,
>  	unsigned int			count)
>  {
> -	return xfs_trans_get_cud(tp, intent);
> +	return xfs_trans_get_cud(tp, CUI_ITEM(intent));
>  }
>  
>  /* Process a deferred refcount update. */
> @@ -407,9 +407,9 @@ xfs_refcount_update_finish_cleanup(
>  /* Abort all pending CUIs. */
>  STATIC void
>  xfs_refcount_update_abort_intent(
> -	void				*intent)
> +	struct xfs_log_item		*intent)
>  {
> -	xfs_cui_release(intent);
> +	xfs_cui_release(CUI_ITEM(intent));
>  }
>  
>  /* Cancel a deferred refcount update. */
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index 40567cf0c216e..a417e15fd0ce4 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -381,7 +381,7 @@ xfs_rmap_update_log_item(
>  			rmap->ri_bmap.br_state);
>  }
>  
> -STATIC void *
> +static struct xfs_log_item *
>  xfs_rmap_update_create_intent(
>  	struct xfs_trans		*tp,
>  	struct list_head		*items,
> @@ -399,17 +399,17 @@ xfs_rmap_update_create_intent(
>  		list_sort(mp, items, xfs_rmap_update_diff_items);
>  	list_for_each_entry(rmap, items, ri_list)
>  		xfs_rmap_update_log_item(tp, ruip, rmap);
> -	return ruip;
> +	return &ruip->rui_item;
>  }
>  
>  /* Get an RUD so we can process all the deferred rmap updates. */
>  STATIC void *
>  xfs_rmap_update_create_done(
>  	struct xfs_trans		*tp,
> -	void				*intent,
> +	struct xfs_log_item		*intent,
>  	unsigned int			count)
>  {
> -	return xfs_trans_get_rud(tp, intent);
> +	return xfs_trans_get_rud(tp, RUI_ITEM(intent));
>  }
>  
>  /* Process a deferred rmap update. */
> @@ -451,9 +451,9 @@ xfs_rmap_update_finish_cleanup(
>  /* Abort all pending RUIs. */
>  STATIC void
>  xfs_rmap_update_abort_intent(
> -	void				*intent)
> +	struct xfs_log_item	*intent)
>  {
> -	xfs_rui_release(intent);
> +	xfs_rui_release(RUI_ITEM(intent));
>  }
>  
>  /* Cancel a deferred rmap update. */
> -- 
> 2.26.2
> 

[PATCH 08/11] xfs: refactor xfs_defer_finish_noroll

[Christoph Hellwig]

Split out a helper that operates on a single xfs_defer_pending structure
to untangle the code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_defer.c | 128 ++++++++++++++++++--------------------
 1 file changed, 59 insertions(+), 69 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 5402a7bf31108..20950b56cdd07 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -351,6 +351,53 @@ xfs_defer_cancel_list(
 	}
 }
 
+/*
+ * Log an intent-done item for the first pending intent, and finish the work
+ * items.
+ */
+static int
+xfs_defer_finish_one(
+	struct xfs_trans		*tp,
+	struct xfs_defer_pending	*dfp)
+{
+	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
+	void				*state = NULL;
+	struct list_head		*li, *n;
+	int				error;
+
+	trace_xfs_defer_pending_finish(tp->t_mountp, dfp);
+
+	dfp->dfp_done = ops->create_done(tp, dfp->dfp_intent, dfp->dfp_count);
+	list_for_each_safe(li, n, &dfp->dfp_work) {
+		list_del(li);
+		dfp->dfp_count--;
+		error = ops->finish_item(tp, li, dfp->dfp_done, &state);
+		if (error == -EAGAIN) {
+			/*
+			 * Caller wants a fresh transaction; put the work item
+			 * back on the list and log a new log intent item to
+			 * replace the old one.  See "Requesting a Fresh
+			 * Transaction while Finishing Deferred Work" above.
+			 */
+			list_add(li, &dfp->dfp_work);
+			dfp->dfp_count++;
+			dfp->dfp_done = NULL;
+			xfs_defer_create_intent(tp, dfp, false);
+		}
+
+		if (error)
+			goto out;
+	}
+
+	/* Done with the dfp, free it. */
+	list_del(&dfp->dfp_list);
+	kmem_free(dfp);
+out:
+	if (ops->finish_cleanup)
+		ops->finish_cleanup(tp, state, error);
+	return error;
+}
+
 /*
  * Finish all the pending work.  This involves logging intent items for
  * any work items that wandered in since the last transaction roll (if
@@ -364,11 +411,7 @@ xfs_defer_finish_noroll(
 	struct xfs_trans		**tp)
 {
 	struct xfs_defer_pending	*dfp;
-	struct list_head		*li;
-	struct list_head		*n;
-	void				*state;
 	int				error = 0;
-	const struct xfs_defer_op_type	*ops;
 	LIST_HEAD(dop_pending);
 
 	ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
@@ -377,83 +420,30 @@ xfs_defer_finish_noroll(
 
 	/* Until we run out of pending work to finish... */
 	while (!list_empty(&dop_pending) || !list_empty(&(*tp)->t_dfops)) {
-		/* log intents and pull in intake items */
 		xfs_defer_create_intents(*tp);
 		list_splice_tail_init(&(*tp)->t_dfops, &dop_pending);
 
-		/*
-		 * Roll the transaction.
-		 */
 		error = xfs_defer_trans_roll(tp);
 		if (error)
-			goto out;
+			goto out_shutdown;
 
-		/* Log an intent-done item for the first pending item. */
 		dfp = list_first_entry(&dop_pending, struct xfs_defer_pending,
 				       dfp_list);
-		ops = defer_op_types[dfp->dfp_type];
-		trace_xfs_defer_pending_finish((*tp)->t_mountp, dfp);
-		dfp->dfp_done = ops->create_done(*tp, dfp->dfp_intent,
-				dfp->dfp_count);
-
-		/* Finish the work items. */
-		state = NULL;
-		list_for_each_safe(li, n, &dfp->dfp_work) {
-			list_del(li);
-			dfp->dfp_count--;
-			error = ops->finish_item(*tp, li, dfp->dfp_done,
-					&state);
-			if (error == -EAGAIN) {
-				/*
-				 * Caller wants a fresh transaction;
-				 * put the work item back on the list
-				 * and jump out.
-				 */
-				list_add(li, &dfp->dfp_work);
-				dfp->dfp_count++;
-				break;
-			} else if (error) {
-				/*
-				 * Clean up after ourselves and jump out.
-				 * xfs_defer_cancel will take care of freeing
-				 * all these lists and stuff.
-				 */
-				if (ops->finish_cleanup)
-					ops->finish_cleanup(*tp, state, error);
-				goto out;
-			}
-		}
-		if (error == -EAGAIN) {
-			/*
-			 * Caller wants a fresh transaction, so log a new log
-			 * intent item to replace the old one and roll the
-			 * transaction.  See "Requesting a Fresh Transaction
-			 * while Finishing Deferred Work" above.
-			 */
-			dfp->dfp_done = NULL;
-			xfs_defer_create_intent(*tp, dfp, false);
-		} else {
-			/* Done with the dfp, free it. */
-			list_del(&dfp->dfp_list);
-			kmem_free(dfp);
-		}
-
-		if (ops->finish_cleanup)
-			ops->finish_cleanup(*tp, state, error);
-	}
-
-out:
-	if (error) {
-		xfs_defer_trans_abort(*tp, &dop_pending);
-		xfs_force_shutdown((*tp)->t_mountp, SHUTDOWN_CORRUPT_INCORE);
-		trace_xfs_defer_finish_error(*tp, error);
-		xfs_defer_cancel_list((*tp)->t_mountp, &dop_pending);
-		xfs_defer_cancel(*tp);
-		return error;
+		error = xfs_defer_finish_one(*tp, dfp);
+		if (error && error != -EAGAIN)
+			goto out_shutdown;
 	}
 
 	trace_xfs_defer_finish_done(*tp, _RET_IP_);
 	return 0;
+
+out_shutdown:
+	xfs_defer_trans_abort(*tp, &dop_pending);
+	xfs_force_shutdown((*tp)->t_mountp, SHUTDOWN_CORRUPT_INCORE);
+	trace_xfs_defer_finish_error(*tp, error);
+	xfs_defer_cancel_list((*tp)->t_mountp, &dop_pending);
+	xfs_defer_cancel(*tp);
+	return error;
 }
 
 int
-- 
2.26.2

Re: [PATCH 08/11] xfs: refactor xfs_defer_finish_noroll

[Brian Foster]

On Wed, Apr 29, 2020 at 05:05:08PM +0200, Christoph Hellwig wrote:
> Split out a helper that operates on a single xfs_defer_pending structure
> to untangle the code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/libxfs/xfs_defer.c | 128 ++++++++++++++++++--------------------
>  1 file changed, 59 insertions(+), 69 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 5402a7bf31108..20950b56cdd07 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -351,6 +351,53 @@ xfs_defer_cancel_list(
>  	}
>  }
>  
> +/*
> + * Log an intent-done item for the first pending intent, and finish the work
> + * items.
> + */
> +static int
> +xfs_defer_finish_one(
> +	struct xfs_trans		*tp,
> +	struct xfs_defer_pending	*dfp)
> +{
> +	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
> +	void				*state = NULL;
> +	struct list_head		*li, *n;
> +	int				error;
> +
> +	trace_xfs_defer_pending_finish(tp->t_mountp, dfp);
> +
> +	dfp->dfp_done = ops->create_done(tp, dfp->dfp_intent, dfp->dfp_count);
> +	list_for_each_safe(li, n, &dfp->dfp_work) {
> +		list_del(li);
> +		dfp->dfp_count--;
> +		error = ops->finish_item(tp, li, dfp->dfp_done, &state);
> +		if (error == -EAGAIN) {
> +			/*
> +			 * Caller wants a fresh transaction; put the work item
> +			 * back on the list and log a new log intent item to
> +			 * replace the old one.  See "Requesting a Fresh
> +			 * Transaction while Finishing Deferred Work" above.
> +			 */
> +			list_add(li, &dfp->dfp_work);
> +			dfp->dfp_count++;
> +			dfp->dfp_done = NULL;
> +			xfs_defer_create_intent(tp, dfp, false);
> +		}
> +
> +		if (error)
> +			goto out;
> +	}
> +
> +	/* Done with the dfp, free it. */
> +	list_del(&dfp->dfp_list);
> +	kmem_free(dfp);
> +out:
> +	if (ops->finish_cleanup)
> +		ops->finish_cleanup(tp, state, error);
> +	return error;
> +}
> +
>  /*
>   * Finish all the pending work.  This involves logging intent items for
>   * any work items that wandered in since the last transaction roll (if
> @@ -364,11 +411,7 @@ xfs_defer_finish_noroll(
>  	struct xfs_trans		**tp)
>  {
>  	struct xfs_defer_pending	*dfp;
> -	struct list_head		*li;
> -	struct list_head		*n;
> -	void				*state;
>  	int				error = 0;
> -	const struct xfs_defer_op_type	*ops;
>  	LIST_HEAD(dop_pending);
>  
>  	ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
> @@ -377,83 +420,30 @@ xfs_defer_finish_noroll(
>  
>  	/* Until we run out of pending work to finish... */
>  	while (!list_empty(&dop_pending) || !list_empty(&(*tp)->t_dfops)) {
> -		/* log intents and pull in intake items */
>  		xfs_defer_create_intents(*tp);
>  		list_splice_tail_init(&(*tp)->t_dfops, &dop_pending);
>  
> -		/*
> -		 * Roll the transaction.
> -		 */
>  		error = xfs_defer_trans_roll(tp);
>  		if (error)
> -			goto out;
> +			goto out_shutdown;
>  
> -		/* Log an intent-done item for the first pending item. */
>  		dfp = list_first_entry(&dop_pending, struct xfs_defer_pending,
>  				       dfp_list);
> -		ops = defer_op_types[dfp->dfp_type];
> -		trace_xfs_defer_pending_finish((*tp)->t_mountp, dfp);
> -		dfp->dfp_done = ops->create_done(*tp, dfp->dfp_intent,
> -				dfp->dfp_count);
> -
> -		/* Finish the work items. */
> -		state = NULL;
> -		list_for_each_safe(li, n, &dfp->dfp_work) {
> -			list_del(li);
> -			dfp->dfp_count--;
> -			error = ops->finish_item(*tp, li, dfp->dfp_done,
> -					&state);
> -			if (error == -EAGAIN) {
> -				/*
> -				 * Caller wants a fresh transaction;
> -				 * put the work item back on the list
> -				 * and jump out.
> -				 */
> -				list_add(li, &dfp->dfp_work);
> -				dfp->dfp_count++;
> -				break;
> -			} else if (error) {
> -				/*
> -				 * Clean up after ourselves and jump out.
> -				 * xfs_defer_cancel will take care of freeing
> -				 * all these lists and stuff.
> -				 */
> -				if (ops->finish_cleanup)
> -					ops->finish_cleanup(*tp, state, error);
> -				goto out;
> -			}
> -		}
> -		if (error == -EAGAIN) {
> -			/*
> -			 * Caller wants a fresh transaction, so log a new log
> -			 * intent item to replace the old one and roll the
> -			 * transaction.  See "Requesting a Fresh Transaction
> -			 * while Finishing Deferred Work" above.
> -			 */
> -			dfp->dfp_done = NULL;
> -			xfs_defer_create_intent(*tp, dfp, false);
> -		} else {
> -			/* Done with the dfp, free it. */
> -			list_del(&dfp->dfp_list);
> -			kmem_free(dfp);
> -		}
> -
> -		if (ops->finish_cleanup)
> -			ops->finish_cleanup(*tp, state, error);
> -	}
> -
> -out:
> -	if (error) {
> -		xfs_defer_trans_abort(*tp, &dop_pending);
> -		xfs_force_shutdown((*tp)->t_mountp, SHUTDOWN_CORRUPT_INCORE);
> -		trace_xfs_defer_finish_error(*tp, error);
> -		xfs_defer_cancel_list((*tp)->t_mountp, &dop_pending);
> -		xfs_defer_cancel(*tp);
> -		return error;
> +		error = xfs_defer_finish_one(*tp, dfp);
> +		if (error && error != -EAGAIN)
> +			goto out_shutdown;
>  	}
>  
>  	trace_xfs_defer_finish_done(*tp, _RET_IP_);
>  	return 0;
> +
> +out_shutdown:
> +	xfs_defer_trans_abort(*tp, &dop_pending);
> +	xfs_force_shutdown((*tp)->t_mountp, SHUTDOWN_CORRUPT_INCORE);
> +	trace_xfs_defer_finish_error(*tp, error);
> +	xfs_defer_cancel_list((*tp)->t_mountp, &dop_pending);
> +	xfs_defer_cancel(*tp);
> +	return error;
>  }
>  
>  int
> -- 
> 2.26.2
> 

[PATCH 09/11] xfs: turn dfp_done into a xfs_log_item

[Christoph Hellwig]

All defer op instance place their own extension of the log item into
the dfp_done field.  Replace that with a xfs_log_item to improve type
safety and make the code easier to follow.

Also use the opportunity to improve the ->finish_item calling conventions
to place the done log item as the higher level structure before the
list_entry used for the individual items.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_defer.c  |  2 +-
 fs/xfs/libxfs/xfs_defer.h  | 10 +++++-----
 fs/xfs/xfs_bmap_item.c     |  8 ++++----
 fs/xfs/xfs_extfree_item.c  | 12 ++++++------
 fs/xfs/xfs_refcount_item.c |  8 ++++----
 fs/xfs/xfs_rmap_item.c     |  8 ++++----
 6 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 20950b56cdd07..5f37f42cda67b 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -371,7 +371,7 @@ xfs_defer_finish_one(
 	list_for_each_safe(li, n, &dfp->dfp_work) {
 		list_del(li);
 		dfp->dfp_count--;
-		error = ops->finish_item(tp, li, dfp->dfp_done, &state);
+		error = ops->finish_item(tp, dfp->dfp_done, li, &state);
 		if (error == -EAGAIN) {
 			/*
 			 * Caller wants a fresh transaction; put the work item
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index 7b6cc3808a91b..a86c890e63d20 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -29,7 +29,7 @@ struct xfs_defer_pending {
 	struct list_head		dfp_list;	/* pending items */
 	struct list_head		dfp_work;	/* work items */
 	struct xfs_log_item		*dfp_intent;	/* log intent item */
-	void				*dfp_done;	/* log done item */
+	struct xfs_log_item		*dfp_done;	/* log done item */
 	unsigned int			dfp_count;	/* # extent items */
 	enum xfs_defer_ops_type		dfp_type;
 };
@@ -46,10 +46,10 @@ struct xfs_defer_op_type {
 	struct xfs_log_item *(*create_intent)(struct xfs_trans *tp,
 			struct list_head *items, unsigned int count, bool sort);
 	void (*abort_intent)(struct xfs_log_item *intent);
-	void *(*create_done)(struct xfs_trans *tp, struct xfs_log_item *intent,
-			unsigned int count);
-	int (*finish_item)(struct xfs_trans *, struct list_head *, void *,
-			void **);
+	struct xfs_log_item *(*create_done)(struct xfs_trans *tp,
+			struct xfs_log_item *intent, unsigned int count);
+	int (*finish_item)(struct xfs_trans *tp, struct xfs_log_item *done,
+			struct list_head *item, void **state);
 	void (*finish_cleanup)(struct xfs_trans *, void *, int);
 	void (*cancel_item)(struct list_head *);
 	unsigned int		max_items;
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 7b2153fca2d9e..feadd44a67e4b 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -352,21 +352,21 @@ xfs_bmap_update_create_intent(
 }
 
 /* Get an BUD so we can process all the deferred rmap updates. */
-STATIC void *
+static struct xfs_log_item *
 xfs_bmap_update_create_done(
 	struct xfs_trans		*tp,
 	struct xfs_log_item		*intent,
 	unsigned int			count)
 {
-	return xfs_trans_get_bud(tp, BUI_ITEM(intent));
+	return &xfs_trans_get_bud(tp, BUI_ITEM(intent))->bud_item;
 }
 
 /* Process a deferred rmap update. */
 STATIC int
 xfs_bmap_update_finish_item(
 	struct xfs_trans		*tp,
+	struct xfs_log_item		*done,
 	struct list_head		*item,
-	void				*done_item,
 	void				**state)
 {
 	struct xfs_bmap_intent		*bmap;
@@ -375,7 +375,7 @@ xfs_bmap_update_finish_item(
 
 	bmap = container_of(item, struct xfs_bmap_intent, bi_list);
 	count = bmap->bi_bmap.br_blockcount;
-	error = xfs_trans_log_finish_bmap_update(tp, done_item,
+	error = xfs_trans_log_finish_bmap_update(tp, BUD_ITEM(done),
 			bmap->bi_type,
 			bmap->bi_owner, bmap->bi_whichfork,
 			bmap->bi_bmap.br_startoff,
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 0453b6f2b1d69..633628f70e128 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -459,28 +459,28 @@ xfs_extent_free_create_intent(
 }
 
 /* Get an EFD so we can process all the free extents. */
-STATIC void *
+static struct xfs_log_item *
 xfs_extent_free_create_done(
 	struct xfs_trans		*tp,
 	struct xfs_log_item		*intent,
 	unsigned int			count)
 {
-	return xfs_trans_get_efd(tp, EFI_ITEM(intent), count);
+	return &xfs_trans_get_efd(tp, EFI_ITEM(intent), count)->efd_item;
 }
 
 /* Process a free extent. */
 STATIC int
 xfs_extent_free_finish_item(
 	struct xfs_trans		*tp,
+	struct xfs_log_item		*done,
 	struct list_head		*item,
-	void				*done_item,
 	void				**state)
 {
 	struct xfs_extent_free_item	*free;
 	int				error;
 
 	free = container_of(item, struct xfs_extent_free_item, xefi_list);
-	error = xfs_trans_free_extent(tp, done_item,
+	error = xfs_trans_free_extent(tp, EFD_ITEM(done),
 			free->xefi_startblock,
 			free->xefi_blockcount,
 			&free->xefi_oinfo, free->xefi_skip_discard);
@@ -523,12 +523,12 @@ const struct xfs_defer_op_type xfs_extent_free_defer_type = {
 STATIC int
 xfs_agfl_free_finish_item(
 	struct xfs_trans		*tp,
+	struct xfs_log_item		*done,
 	struct list_head		*item,
-	void				*done_item,
 	void				**state)
 {
 	struct xfs_mount		*mp = tp->t_mountp;
-	struct xfs_efd_log_item		*efdp = done_item;
+	struct xfs_efd_log_item		*efdp = EFD_ITEM(done);
 	struct xfs_extent_free_item	*free;
 	struct xfs_extent		*extp;
 	struct xfs_buf			*agbp;
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index e8d3278e066e3..f1c2e559a7ae7 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -351,21 +351,21 @@ xfs_refcount_update_create_intent(
 }
 
 /* Get an CUD so we can process all the deferred refcount updates. */
-STATIC void *
+static struct xfs_log_item *
 xfs_refcount_update_create_done(
 	struct xfs_trans		*tp,
 	struct xfs_log_item		*intent,
 	unsigned int			count)
 {
-	return xfs_trans_get_cud(tp, CUI_ITEM(intent));
+	return &xfs_trans_get_cud(tp, CUI_ITEM(intent))->cud_item;
 }
 
 /* Process a deferred refcount update. */
 STATIC int
 xfs_refcount_update_finish_item(
 	struct xfs_trans		*tp,
+	struct xfs_log_item		*done,
 	struct list_head		*item,
-	void				*done_item,
 	void				**state)
 {
 	struct xfs_refcount_intent	*refc;
@@ -374,7 +374,7 @@ xfs_refcount_update_finish_item(
 	int				error;
 
 	refc = container_of(item, struct xfs_refcount_intent, ri_list);
-	error = xfs_trans_log_finish_refcount_update(tp, done_item,
+	error = xfs_trans_log_finish_refcount_update(tp, CUD_ITEM(done),
 			refc->ri_type,
 			refc->ri_startblock,
 			refc->ri_blockcount,
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index a417e15fd0ce4..f6a2a388e5ac9 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -403,28 +403,28 @@ xfs_rmap_update_create_intent(
 }
 
 /* Get an RUD so we can process all the deferred rmap updates. */
-STATIC void *
+static struct xfs_log_item *
 xfs_rmap_update_create_done(
 	struct xfs_trans		*tp,
 	struct xfs_log_item		*intent,
 	unsigned int			count)
 {
-	return xfs_trans_get_rud(tp, RUI_ITEM(intent));
+	return &xfs_trans_get_rud(tp, RUI_ITEM(intent))->rud_item;
 }
 
 /* Process a deferred rmap update. */
 STATIC int
 xfs_rmap_update_finish_item(
 	struct xfs_trans		*tp,
+	struct xfs_log_item		*done,
 	struct list_head		*item,
-	void				*done_item,
 	void				**state)
 {
 	struct xfs_rmap_intent		*rmap;
 	int				error;
 
 	rmap = container_of(item, struct xfs_rmap_intent, ri_list);
-	error = xfs_trans_log_finish_rmap_update(tp, done_item,
+	error = xfs_trans_log_finish_rmap_update(tp, RUD_ITEM(done),
 			rmap->ri_type,
 			rmap->ri_owner, rmap->ri_whichfork,
 			rmap->ri_bmap.br_startoff,
-- 
2.26.2

Re: [PATCH 09/11] xfs: turn dfp_done into a xfs_log_item

[Brian Foster]

On Wed, Apr 29, 2020 at 05:05:09PM +0200, Christoph Hellwig wrote:
> All defer op instance place their own extension of the log item into
> the dfp_done field.  Replace that with a xfs_log_item to improve type
> safety and make the code easier to follow.
> 
> Also use the opportunity to improve the ->finish_item calling conventions
> to place the done log item as the higher level structure before the
> list_entry used for the individual items.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/libxfs/xfs_defer.c  |  2 +-
>  fs/xfs/libxfs/xfs_defer.h  | 10 +++++-----
>  fs/xfs/xfs_bmap_item.c     |  8 ++++----
>  fs/xfs/xfs_extfree_item.c  | 12 ++++++------
>  fs/xfs/xfs_refcount_item.c |  8 ++++----
>  fs/xfs/xfs_rmap_item.c     |  8 ++++----
>  6 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 20950b56cdd07..5f37f42cda67b 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -371,7 +371,7 @@ xfs_defer_finish_one(
>  	list_for_each_safe(li, n, &dfp->dfp_work) {
>  		list_del(li);
>  		dfp->dfp_count--;
> -		error = ops->finish_item(tp, li, dfp->dfp_done, &state);
> +		error = ops->finish_item(tp, dfp->dfp_done, li, &state);
>  		if (error == -EAGAIN) {
>  			/*
>  			 * Caller wants a fresh transaction; put the work item
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index 7b6cc3808a91b..a86c890e63d20 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -29,7 +29,7 @@ struct xfs_defer_pending {
>  	struct list_head		dfp_list;	/* pending items */
>  	struct list_head		dfp_work;	/* work items */
>  	struct xfs_log_item		*dfp_intent;	/* log intent item */
> -	void				*dfp_done;	/* log done item */
> +	struct xfs_log_item		*dfp_done;	/* log done item */
>  	unsigned int			dfp_count;	/* # extent items */
>  	enum xfs_defer_ops_type		dfp_type;
>  };
> @@ -46,10 +46,10 @@ struct xfs_defer_op_type {
>  	struct xfs_log_item *(*create_intent)(struct xfs_trans *tp,
>  			struct list_head *items, unsigned int count, bool sort);
>  	void (*abort_intent)(struct xfs_log_item *intent);
> -	void *(*create_done)(struct xfs_trans *tp, struct xfs_log_item *intent,
> -			unsigned int count);
> -	int (*finish_item)(struct xfs_trans *, struct list_head *, void *,
> -			void **);
> +	struct xfs_log_item *(*create_done)(struct xfs_trans *tp,
> +			struct xfs_log_item *intent, unsigned int count);
> +	int (*finish_item)(struct xfs_trans *tp, struct xfs_log_item *done,
> +			struct list_head *item, void **state);
>  	void (*finish_cleanup)(struct xfs_trans *, void *, int);
>  	void (*cancel_item)(struct list_head *);
>  	unsigned int		max_items;
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 7b2153fca2d9e..feadd44a67e4b 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -352,21 +352,21 @@ xfs_bmap_update_create_intent(
>  }
>  
>  /* Get an BUD so we can process all the deferred rmap updates. */
> -STATIC void *
> +static struct xfs_log_item *
>  xfs_bmap_update_create_done(
>  	struct xfs_trans		*tp,
>  	struct xfs_log_item		*intent,
>  	unsigned int			count)
>  {
> -	return xfs_trans_get_bud(tp, BUI_ITEM(intent));
> +	return &xfs_trans_get_bud(tp, BUI_ITEM(intent))->bud_item;
>  }
>  
>  /* Process a deferred rmap update. */
>  STATIC int
>  xfs_bmap_update_finish_item(
>  	struct xfs_trans		*tp,
> +	struct xfs_log_item		*done,
>  	struct list_head		*item,
> -	void				*done_item,
>  	void				**state)
>  {
>  	struct xfs_bmap_intent		*bmap;
> @@ -375,7 +375,7 @@ xfs_bmap_update_finish_item(
>  
>  	bmap = container_of(item, struct xfs_bmap_intent, bi_list);
>  	count = bmap->bi_bmap.br_blockcount;
> -	error = xfs_trans_log_finish_bmap_update(tp, done_item,
> +	error = xfs_trans_log_finish_bmap_update(tp, BUD_ITEM(done),
>  			bmap->bi_type,
>  			bmap->bi_owner, bmap->bi_whichfork,
>  			bmap->bi_bmap.br_startoff,
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 0453b6f2b1d69..633628f70e128 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -459,28 +459,28 @@ xfs_extent_free_create_intent(
>  }
>  
>  /* Get an EFD so we can process all the free extents. */
> -STATIC void *
> +static struct xfs_log_item *
>  xfs_extent_free_create_done(
>  	struct xfs_trans		*tp,
>  	struct xfs_log_item		*intent,
>  	unsigned int			count)
>  {
> -	return xfs_trans_get_efd(tp, EFI_ITEM(intent), count);
> +	return &xfs_trans_get_efd(tp, EFI_ITEM(intent), count)->efd_item;
>  }
>  
>  /* Process a free extent. */
>  STATIC int
>  xfs_extent_free_finish_item(
>  	struct xfs_trans		*tp,
> +	struct xfs_log_item		*done,
>  	struct list_head		*item,
> -	void				*done_item,
>  	void				**state)
>  {
>  	struct xfs_extent_free_item	*free;
>  	int				error;
>  
>  	free = container_of(item, struct xfs_extent_free_item, xefi_list);
> -	error = xfs_trans_free_extent(tp, done_item,
> +	error = xfs_trans_free_extent(tp, EFD_ITEM(done),
>  			free->xefi_startblock,
>  			free->xefi_blockcount,
>  			&free->xefi_oinfo, free->xefi_skip_discard);
> @@ -523,12 +523,12 @@ const struct xfs_defer_op_type xfs_extent_free_defer_type = {
>  STATIC int
>  xfs_agfl_free_finish_item(
>  	struct xfs_trans		*tp,
> +	struct xfs_log_item		*done,
>  	struct list_head		*item,
> -	void				*done_item,
>  	void				**state)
>  {
>  	struct xfs_mount		*mp = tp->t_mountp;
> -	struct xfs_efd_log_item		*efdp = done_item;
> +	struct xfs_efd_log_item		*efdp = EFD_ITEM(done);
>  	struct xfs_extent_free_item	*free;
>  	struct xfs_extent		*extp;
>  	struct xfs_buf			*agbp;
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index e8d3278e066e3..f1c2e559a7ae7 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -351,21 +351,21 @@ xfs_refcount_update_create_intent(
>  }
>  
>  /* Get an CUD so we can process all the deferred refcount updates. */
> -STATIC void *
> +static struct xfs_log_item *
>  xfs_refcount_update_create_done(
>  	struct xfs_trans		*tp,
>  	struct xfs_log_item		*intent,
>  	unsigned int			count)
>  {
> -	return xfs_trans_get_cud(tp, CUI_ITEM(intent));
> +	return &xfs_trans_get_cud(tp, CUI_ITEM(intent))->cud_item;
>  }
>  
>  /* Process a deferred refcount update. */
>  STATIC int
>  xfs_refcount_update_finish_item(
>  	struct xfs_trans		*tp,
> +	struct xfs_log_item		*done,
>  	struct list_head		*item,
> -	void				*done_item,
>  	void				**state)
>  {
>  	struct xfs_refcount_intent	*refc;
> @@ -374,7 +374,7 @@ xfs_refcount_update_finish_item(
>  	int				error;
>  
>  	refc = container_of(item, struct xfs_refcount_intent, ri_list);
> -	error = xfs_trans_log_finish_refcount_update(tp, done_item,
> +	error = xfs_trans_log_finish_refcount_update(tp, CUD_ITEM(done),
>  			refc->ri_type,
>  			refc->ri_startblock,
>  			refc->ri_blockcount,
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index a417e15fd0ce4..f6a2a388e5ac9 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -403,28 +403,28 @@ xfs_rmap_update_create_intent(
>  }
>  
>  /* Get an RUD so we can process all the deferred rmap updates. */
> -STATIC void *
> +static struct xfs_log_item *
>  xfs_rmap_update_create_done(
>  	struct xfs_trans		*tp,
>  	struct xfs_log_item		*intent,
>  	unsigned int			count)
>  {
> -	return xfs_trans_get_rud(tp, RUI_ITEM(intent));
> +	return &xfs_trans_get_rud(tp, RUI_ITEM(intent))->rud_item;
>  }
>  
>  /* Process a deferred rmap update. */
>  STATIC int
>  xfs_rmap_update_finish_item(
>  	struct xfs_trans		*tp,
> +	struct xfs_log_item		*done,
>  	struct list_head		*item,
> -	void				*done_item,
>  	void				**state)
>  {
>  	struct xfs_rmap_intent		*rmap;
>  	int				error;
>  
>  	rmap = container_of(item, struct xfs_rmap_intent, ri_list);
> -	error = xfs_trans_log_finish_rmap_update(tp, done_item,
> +	error = xfs_trans_log_finish_rmap_update(tp, RUD_ITEM(done),
>  			rmap->ri_type,
>  			rmap->ri_owner, rmap->ri_whichfork,
>  			rmap->ri_bmap.br_startoff,
> -- 
> 2.26.2
> 

[PATCH 10/11] xfs: use a xfs_btree_cur for the ->finish_cleanup state

[Christoph Hellwig]

Given how XFS is all based around btrees it doesn't make much sense
to offer a totally generic state when we can just use the btree cursor.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_defer.c  |  2 +-
 fs/xfs/libxfs/xfs_defer.h  |  6 ++++--
 fs/xfs/xfs_bmap_item.c     |  2 +-
 fs/xfs/xfs_extfree_item.c  |  4 ++--
 fs/xfs/xfs_refcount_item.c | 24 +++++-------------------
 fs/xfs/xfs_rmap_item.c     | 27 ++++++---------------------
 6 files changed, 19 insertions(+), 46 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 5f37f42cda67b..1172fbf072d84 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -361,7 +361,7 @@ xfs_defer_finish_one(
 	struct xfs_defer_pending	*dfp)
 {
 	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
-	void				*state = NULL;
+	struct xfs_btree_cur		*state = NULL;
 	struct list_head		*li, *n;
 	int				error;
 
diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index a86c890e63d20..f2b65981bace4 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -6,6 +6,7 @@
 #ifndef __XFS_DEFER_H__
 #define	__XFS_DEFER_H__
 
+struct xfs_btree_cur;
 struct xfs_defer_op_type;
 
 /*
@@ -49,8 +50,9 @@ struct xfs_defer_op_type {
 	struct xfs_log_item *(*create_done)(struct xfs_trans *tp,
 			struct xfs_log_item *intent, unsigned int count);
 	int (*finish_item)(struct xfs_trans *tp, struct xfs_log_item *done,
-			struct list_head *item, void **state);
-	void (*finish_cleanup)(struct xfs_trans *, void *, int);
+			struct list_head *item, struct xfs_btree_cur **state);
+	void (*finish_cleanup)(struct xfs_trans *tp,
+			struct xfs_btree_cur *state, int error);
 	void (*cancel_item)(struct list_head *);
 	unsigned int		max_items;
 };
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index feadd44a67e4b..7768fb2b71357 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -367,7 +367,7 @@ xfs_bmap_update_finish_item(
 	struct xfs_trans		*tp,
 	struct xfs_log_item		*done,
 	struct list_head		*item,
-	void				**state)
+	struct xfs_btree_cur		**state)
 {
 	struct xfs_bmap_intent		*bmap;
 	xfs_filblks_t			count;
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 633628f70e128..c8cde4122a0fe 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -474,7 +474,7 @@ xfs_extent_free_finish_item(
 	struct xfs_trans		*tp,
 	struct xfs_log_item		*done,
 	struct list_head		*item,
-	void				**state)
+	struct xfs_btree_cur		**state)
 {
 	struct xfs_extent_free_item	*free;
 	int				error;
@@ -525,7 +525,7 @@ xfs_agfl_free_finish_item(
 	struct xfs_trans		*tp,
 	struct xfs_log_item		*done,
 	struct list_head		*item,
-	void				**state)
+	struct xfs_btree_cur		**state)
 {
 	struct xfs_mount		*mp = tp->t_mountp;
 	struct xfs_efd_log_item		*efdp = EFD_ITEM(done);
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index f1c2e559a7ae7..0316eab2fc351 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -366,7 +366,7 @@ xfs_refcount_update_finish_item(
 	struct xfs_trans		*tp,
 	struct xfs_log_item		*done,
 	struct list_head		*item,
-	void				**state)
+	struct xfs_btree_cur		**state)
 {
 	struct xfs_refcount_intent	*refc;
 	xfs_fsblock_t			new_fsb;
@@ -375,11 +375,9 @@ xfs_refcount_update_finish_item(
 
 	refc = container_of(item, struct xfs_refcount_intent, ri_list);
 	error = xfs_trans_log_finish_refcount_update(tp, CUD_ITEM(done),
-			refc->ri_type,
-			refc->ri_startblock,
-			refc->ri_blockcount,
-			&new_fsb, &new_aglen,
-			(struct xfs_btree_cur **)state);
+			refc->ri_type, refc->ri_startblock, refc->ri_blockcount,
+			&new_fsb, &new_aglen, state);
+
 	/* Did we run out of reservation?  Requeue what we didn't finish. */
 	if (!error && new_aglen > 0) {
 		ASSERT(refc->ri_type == XFS_REFCOUNT_INCREASE ||
@@ -392,18 +390,6 @@ xfs_refcount_update_finish_item(
 	return error;
 }
 
-/* Clean up after processing deferred refcounts. */
-STATIC void
-xfs_refcount_update_finish_cleanup(
-	struct xfs_trans	*tp,
-	void			*state,
-	int			error)
-{
-	struct xfs_btree_cur	*rcur = state;
-
-	xfs_refcount_finish_one_cleanup(tp, rcur, error);
-}
-
 /* Abort all pending CUIs. */
 STATIC void
 xfs_refcount_update_abort_intent(
@@ -429,7 +415,7 @@ const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
 	.abort_intent	= xfs_refcount_update_abort_intent,
 	.create_done	= xfs_refcount_update_create_done,
 	.finish_item	= xfs_refcount_update_finish_item,
-	.finish_cleanup = xfs_refcount_update_finish_cleanup,
+	.finish_cleanup = xfs_refcount_finish_one_cleanup,
 	.cancel_item	= xfs_refcount_update_cancel_item,
 };
 
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index f6a2a388e5ac9..e3bba2aec8682 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -418,36 +418,21 @@ xfs_rmap_update_finish_item(
 	struct xfs_trans		*tp,
 	struct xfs_log_item		*done,
 	struct list_head		*item,
-	void				**state)
+	struct xfs_btree_cur		**state)
 {
 	struct xfs_rmap_intent		*rmap;
 	int				error;
 
 	rmap = container_of(item, struct xfs_rmap_intent, ri_list);
 	error = xfs_trans_log_finish_rmap_update(tp, RUD_ITEM(done),
-			rmap->ri_type,
-			rmap->ri_owner, rmap->ri_whichfork,
-			rmap->ri_bmap.br_startoff,
-			rmap->ri_bmap.br_startblock,
-			rmap->ri_bmap.br_blockcount,
-			rmap->ri_bmap.br_state,
-			(struct xfs_btree_cur **)state);
+			rmap->ri_type, rmap->ri_owner, rmap->ri_whichfork,
+			rmap->ri_bmap.br_startoff, rmap->ri_bmap.br_startblock,
+			rmap->ri_bmap.br_blockcount, rmap->ri_bmap.br_state,
+			state);
 	kmem_free(rmap);
 	return error;
 }
 
-/* Clean up after processing deferred rmaps. */
-STATIC void
-xfs_rmap_update_finish_cleanup(
-	struct xfs_trans	*tp,
-	void			*state,
-	int			error)
-{
-	struct xfs_btree_cur	*rcur = state;
-
-	xfs_rmap_finish_one_cleanup(tp, rcur, error);
-}
-
 /* Abort all pending RUIs. */
 STATIC void
 xfs_rmap_update_abort_intent(
@@ -473,7 +458,7 @@ const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
 	.abort_intent	= xfs_rmap_update_abort_intent,
 	.create_done	= xfs_rmap_update_create_done,
 	.finish_item	= xfs_rmap_update_finish_item,
-	.finish_cleanup = xfs_rmap_update_finish_cleanup,
+	.finish_cleanup = xfs_rmap_finish_one_cleanup,
 	.cancel_item	= xfs_rmap_update_cancel_item,
 };
 
-- 
2.26.2

Re: [PATCH 10/11] xfs: use a xfs_btree_cur for the ->finish_cleanup state

[Brian Foster]

On Wed, Apr 29, 2020 at 05:05:10PM +0200, Christoph Hellwig wrote:
> Given how XFS is all based around btrees it doesn't make much sense
> to offer a totally generic state when we can just use the btree cursor.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/libxfs/xfs_defer.c  |  2 +-
>  fs/xfs/libxfs/xfs_defer.h  |  6 ++++--
>  fs/xfs/xfs_bmap_item.c     |  2 +-
>  fs/xfs/xfs_extfree_item.c  |  4 ++--
>  fs/xfs/xfs_refcount_item.c | 24 +++++-------------------
>  fs/xfs/xfs_rmap_item.c     | 27 ++++++---------------------
>  6 files changed, 19 insertions(+), 46 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 5f37f42cda67b..1172fbf072d84 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -361,7 +361,7 @@ xfs_defer_finish_one(
>  	struct xfs_defer_pending	*dfp)
>  {
>  	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
> -	void				*state = NULL;
> +	struct xfs_btree_cur		*state = NULL;
>  	struct list_head		*li, *n;
>  	int				error;
>  
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index a86c890e63d20..f2b65981bace4 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -6,6 +6,7 @@
>  #ifndef __XFS_DEFER_H__
>  #define	__XFS_DEFER_H__
>  
> +struct xfs_btree_cur;
>  struct xfs_defer_op_type;
>  
>  /*
> @@ -49,8 +50,9 @@ struct xfs_defer_op_type {
>  	struct xfs_log_item *(*create_done)(struct xfs_trans *tp,
>  			struct xfs_log_item *intent, unsigned int count);
>  	int (*finish_item)(struct xfs_trans *tp, struct xfs_log_item *done,
> -			struct list_head *item, void **state);
> -	void (*finish_cleanup)(struct xfs_trans *, void *, int);
> +			struct list_head *item, struct xfs_btree_cur **state);
> +	void (*finish_cleanup)(struct xfs_trans *tp,
> +			struct xfs_btree_cur *state, int error);
>  	void (*cancel_item)(struct list_head *);
>  	unsigned int		max_items;
>  };
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index feadd44a67e4b..7768fb2b71357 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -367,7 +367,7 @@ xfs_bmap_update_finish_item(
>  	struct xfs_trans		*tp,
>  	struct xfs_log_item		*done,
>  	struct list_head		*item,
> -	void				**state)
> +	struct xfs_btree_cur		**state)
>  {
>  	struct xfs_bmap_intent		*bmap;
>  	xfs_filblks_t			count;
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 633628f70e128..c8cde4122a0fe 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -474,7 +474,7 @@ xfs_extent_free_finish_item(
>  	struct xfs_trans		*tp,
>  	struct xfs_log_item		*done,
>  	struct list_head		*item,
> -	void				**state)
> +	struct xfs_btree_cur		**state)
>  {
>  	struct xfs_extent_free_item	*free;
>  	int				error;
> @@ -525,7 +525,7 @@ xfs_agfl_free_finish_item(
>  	struct xfs_trans		*tp,
>  	struct xfs_log_item		*done,
>  	struct list_head		*item,
> -	void				**state)
> +	struct xfs_btree_cur		**state)
>  {
>  	struct xfs_mount		*mp = tp->t_mountp;
>  	struct xfs_efd_log_item		*efdp = EFD_ITEM(done);
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index f1c2e559a7ae7..0316eab2fc351 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -366,7 +366,7 @@ xfs_refcount_update_finish_item(
>  	struct xfs_trans		*tp,
>  	struct xfs_log_item		*done,
>  	struct list_head		*item,
> -	void				**state)
> +	struct xfs_btree_cur		**state)
>  {
>  	struct xfs_refcount_intent	*refc;
>  	xfs_fsblock_t			new_fsb;
> @@ -375,11 +375,9 @@ xfs_refcount_update_finish_item(
>  
>  	refc = container_of(item, struct xfs_refcount_intent, ri_list);
>  	error = xfs_trans_log_finish_refcount_update(tp, CUD_ITEM(done),
> -			refc->ri_type,
> -			refc->ri_startblock,
> -			refc->ri_blockcount,
> -			&new_fsb, &new_aglen,
> -			(struct xfs_btree_cur **)state);
> +			refc->ri_type, refc->ri_startblock, refc->ri_blockcount,
> +			&new_fsb, &new_aglen, state);
> +
>  	/* Did we run out of reservation?  Requeue what we didn't finish. */
>  	if (!error && new_aglen > 0) {
>  		ASSERT(refc->ri_type == XFS_REFCOUNT_INCREASE ||
> @@ -392,18 +390,6 @@ xfs_refcount_update_finish_item(
>  	return error;
>  }
>  
> -/* Clean up after processing deferred refcounts. */
> -STATIC void
> -xfs_refcount_update_finish_cleanup(
> -	struct xfs_trans	*tp,
> -	void			*state,
> -	int			error)
> -{
> -	struct xfs_btree_cur	*rcur = state;
> -
> -	xfs_refcount_finish_one_cleanup(tp, rcur, error);
> -}
> -
>  /* Abort all pending CUIs. */
>  STATIC void
>  xfs_refcount_update_abort_intent(
> @@ -429,7 +415,7 @@ const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
>  	.abort_intent	= xfs_refcount_update_abort_intent,
>  	.create_done	= xfs_refcount_update_create_done,
>  	.finish_item	= xfs_refcount_update_finish_item,
> -	.finish_cleanup = xfs_refcount_update_finish_cleanup,
> +	.finish_cleanup = xfs_refcount_finish_one_cleanup,
>  	.cancel_item	= xfs_refcount_update_cancel_item,
>  };
>  
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index f6a2a388e5ac9..e3bba2aec8682 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -418,36 +418,21 @@ xfs_rmap_update_finish_item(
>  	struct xfs_trans		*tp,
>  	struct xfs_log_item		*done,
>  	struct list_head		*item,
> -	void				**state)
> +	struct xfs_btree_cur		**state)
>  {
>  	struct xfs_rmap_intent		*rmap;
>  	int				error;
>  
>  	rmap = container_of(item, struct xfs_rmap_intent, ri_list);
>  	error = xfs_trans_log_finish_rmap_update(tp, RUD_ITEM(done),
> -			rmap->ri_type,
> -			rmap->ri_owner, rmap->ri_whichfork,
> -			rmap->ri_bmap.br_startoff,
> -			rmap->ri_bmap.br_startblock,
> -			rmap->ri_bmap.br_blockcount,
> -			rmap->ri_bmap.br_state,
> -			(struct xfs_btree_cur **)state);
> +			rmap->ri_type, rmap->ri_owner, rmap->ri_whichfork,
> +			rmap->ri_bmap.br_startoff, rmap->ri_bmap.br_startblock,
> +			rmap->ri_bmap.br_blockcount, rmap->ri_bmap.br_state,
> +			state);
>  	kmem_free(rmap);
>  	return error;
>  }
>  
> -/* Clean up after processing deferred rmaps. */
> -STATIC void
> -xfs_rmap_update_finish_cleanup(
> -	struct xfs_trans	*tp,
> -	void			*state,
> -	int			error)
> -{
> -	struct xfs_btree_cur	*rcur = state;
> -
> -	xfs_rmap_finish_one_cleanup(tp, rcur, error);
> -}
> -
>  /* Abort all pending RUIs. */
>  STATIC void
>  xfs_rmap_update_abort_intent(
> @@ -473,7 +458,7 @@ const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
>  	.abort_intent	= xfs_rmap_update_abort_intent,
>  	.create_done	= xfs_rmap_update_create_done,
>  	.finish_item	= xfs_rmap_update_finish_item,
> -	.finish_cleanup = xfs_rmap_update_finish_cleanup,
> +	.finish_cleanup = xfs_rmap_finish_one_cleanup,
>  	.cancel_item	= xfs_rmap_update_cancel_item,
>  };
>  
> -- 
> 2.26.2
> 

[PATCH 11/11] xfs: spell out the parameter name for ->cancel_item

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_defer.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
index f2b65981bace4..3bf7c2c4d8514 100644
--- a/fs/xfs/libxfs/xfs_defer.h
+++ b/fs/xfs/libxfs/xfs_defer.h
@@ -53,7 +53,7 @@ struct xfs_defer_op_type {
 			struct list_head *item, struct xfs_btree_cur **state);
 	void (*finish_cleanup)(struct xfs_trans *tp,
 			struct xfs_btree_cur *state, int error);
-	void (*cancel_item)(struct list_head *);
+	void (*cancel_item)(struct list_head *item);
 	unsigned int		max_items;
 };
 
-- 
2.26.2

Re: [PATCH 11/11] xfs: spell out the parameter name for ->cancel_item

[Brian Foster]

On Wed, Apr 29, 2020 at 05:05:11PM +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/libxfs/xfs_defer.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index f2b65981bace4..3bf7c2c4d8514 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -53,7 +53,7 @@ struct xfs_defer_op_type {
>  			struct list_head *item, struct xfs_btree_cur **state);
>  	void (*finish_cleanup)(struct xfs_trans *tp,
>  			struct xfs_btree_cur *state, int error);
> -	void (*cancel_item)(struct list_head *);
> +	void (*cancel_item)(struct list_head *item);
>  	unsigned int		max_items;
>  };
>  
> -- 
> 2.26.2
> 

Re: deferred operations cleanup

[Darrick J. Wong]

On Wed, Apr 29, 2020 at 05:05:00PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series cleans up the deferred operations code by merging a few
> methods, and using more type safe types.

The series looks pretty straightforward to me, so for all of it:

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

--D