xfs: pass a commit_mode to xfs_trans_commit

Message ID	20200409073650.1590904-1-hch@lst.de (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=CSRb=5Z=vger.kernel.org=linux-xfs-owner@kernel.org> From: Christoph Hellwig <hch@lst.de> To: linux-xfs@vger.kernel.org Subject: [PATCH] xfs: pass a commit_mode to xfs_trans_commit Date: Thu, 9 Apr 2020 09:36:50 +0200 Message-Id: <20200409073650.1590904-1-hch@lst.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-xfs-owner@vger.kernel.org Precedence: bulk
Series	xfs: pass a commit_mode to xfs_trans_commit \| expand xfs: pass a commit_mode to xfs_trans_commit

Christoph Hellwig April 9, 2020, 7:36 a.m. UTC

Instead of marking the transaction sync using a flag, often based on
copy and pasted mount options checks, pass an enum of sync modes
to xfs_trans_commit that clearly documents the intent.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_attr.c     | 15 ++--------
 fs/xfs/libxfs/xfs_bmap.c     |  4 +--
 fs/xfs/libxfs/xfs_refcount.c |  2 +-
 fs/xfs/libxfs/xfs_sb.c       |  7 ++---
 fs/xfs/libxfs/xfs_shared.h   |  1 -
 fs/xfs/scrub/scrub.c         |  2 +-
 fs/xfs/xfs_aops.c            |  2 +-
 fs/xfs/xfs_attr_inactive.c   |  2 +-
 fs/xfs/xfs_bmap_item.c       |  2 +-
 fs/xfs/xfs_bmap_util.c       | 19 ++++--------
 fs/xfs/xfs_dquot.c           |  2 +-
 fs/xfs/xfs_extfree_item.c    |  2 +-
 fs/xfs/xfs_file.c            |  4 +--
 fs/xfs/xfs_fsops.c           |  6 ++--
 fs/xfs/xfs_inode.c           | 58 +++++-------------------------------
 fs/xfs/xfs_ioctl.c           |  9 ++----
 fs/xfs/xfs_iomap.c           |  4 +--
 fs/xfs/xfs_iops.c            | 11 ++-----
 fs/xfs/xfs_log_recover.c     |  4 +--
 fs/xfs/xfs_pnfs.c            |  3 +-
 fs/xfs/xfs_qm.c              |  2 +-
 fs/xfs/xfs_qm_syscalls.c     | 10 +++----
 fs/xfs/xfs_refcount_item.c   |  2 +-
 fs/xfs/xfs_reflink.c         | 14 ++++-----
 fs/xfs/xfs_rmap_item.c       |  2 +-
 fs/xfs/xfs_rtalloc.c         |  6 ++--
 fs/xfs/xfs_super.c           |  2 +-
 fs/xfs/xfs_symlink.c         | 13 ++------
 fs/xfs/xfs_trans.c           | 32 +++++++++++++++-----
 fs/xfs/xfs_trans.h           | 15 +++++-----
 30 files changed, 94 insertions(+), 163 deletions(-)

Christoph Hellwig May 1, 2020, 8:07 a.m. UTC | #1

Any comments?

On Thu, Apr 09, 2020 at 09:36:50AM +0200, Christoph Hellwig wrote:
> Instead of marking the transaction sync using a flag, often based on
> copy and pasted mount options checks, pass an enum of sync modes
> to xfs_trans_commit that clearly documents the intent.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_attr.c     | 15 ++--------
>  fs/xfs/libxfs/xfs_bmap.c     |  4 +--
>  fs/xfs/libxfs/xfs_refcount.c |  2 +-
>  fs/xfs/libxfs/xfs_sb.c       |  7 ++---
>  fs/xfs/libxfs/xfs_shared.h   |  1 -
>  fs/xfs/scrub/scrub.c         |  2 +-
>  fs/xfs/xfs_aops.c            |  2 +-
>  fs/xfs/xfs_attr_inactive.c   |  2 +-
>  fs/xfs/xfs_bmap_item.c       |  2 +-
>  fs/xfs/xfs_bmap_util.c       | 19 ++++--------
>  fs/xfs/xfs_dquot.c           |  2 +-
>  fs/xfs/xfs_extfree_item.c    |  2 +-
>  fs/xfs/xfs_file.c            |  4 +--
>  fs/xfs/xfs_fsops.c           |  6 ++--
>  fs/xfs/xfs_inode.c           | 58 +++++-------------------------------
>  fs/xfs/xfs_ioctl.c           |  9 ++----
>  fs/xfs/xfs_iomap.c           |  4 +--
>  fs/xfs/xfs_iops.c            | 11 ++-----
>  fs/xfs/xfs_log_recover.c     |  4 +--
>  fs/xfs/xfs_pnfs.c            |  3 +-
>  fs/xfs/xfs_qm.c              |  2 +-
>  fs/xfs/xfs_qm_syscalls.c     | 10 +++----
>  fs/xfs/xfs_refcount_item.c   |  2 +-
>  fs/xfs/xfs_reflink.c         | 14 ++++-----
>  fs/xfs/xfs_rmap_item.c       |  2 +-
>  fs/xfs/xfs_rtalloc.c         |  6 ++--
>  fs/xfs/xfs_super.c           |  2 +-
>  fs/xfs/xfs_symlink.c         | 13 ++------
>  fs/xfs/xfs_trans.c           | 32 +++++++++++++++-----
>  fs/xfs/xfs_trans.h           | 15 +++++-----
>  30 files changed, 94 insertions(+), 163 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index e4fe3dca9883..a6a94bbea8a7 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -175,7 +175,6 @@ xfs_attr_try_sf_addname(
>  	struct xfs_da_args	*args)
>  {
>  
> -	struct xfs_mount	*mp = dp->i_mount;
>  	int			error, error2;
>  
>  	error = xfs_attr_shortform_addname(args);
> @@ -189,10 +188,7 @@ xfs_attr_try_sf_addname(
>  	if (!error && !(args->op_flags & XFS_DA_OP_NOTIME))
>  		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
>  
> -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> -		xfs_trans_set_sync(args->trans);
> -
> -	error2 = xfs_trans_commit(args->trans);
> +	error2 = xfs_trans_commit(args->trans, XFS_TRANS_COMMIT_WSYNC);
>  	args->trans = NULL;
>  	return error ? error : error2;
>  }
> @@ -382,13 +378,6 @@ xfs_attr_set(
>  			goto out_trans_cancel;
>  	}
>  
> -	/*
> -	 * If this is a synchronous mount, make sure that the
> -	 * transaction goes to disk before returning to the user.
> -	 */
> -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> -		xfs_trans_set_sync(args->trans);
> -
>  	if (!(args->op_flags & XFS_DA_OP_NOTIME))
>  		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
>  
> @@ -396,7 +385,7 @@ xfs_attr_set(
>  	 * Commit the last in the sequence of transactions.
>  	 */
>  	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE);
> -	error = xfs_trans_commit(args->trans);
> +	error = xfs_trans_commit(args->trans, XFS_TRANS_COMMIT_WSYNC);
>  out_unlock:
>  	xfs_iunlock(dp, XFS_ILOCK_EXCL);
>  	return error;
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index fda13cd7add0..aa16990e9b5f 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1148,7 +1148,7 @@ xfs_bmap_add_attrfork(
>  			xfs_log_sb(tp);
>  	}
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  	return error;
>  
> @@ -4644,7 +4644,7 @@ xfs_bmapi_convert_delalloc(
>  		goto out_finish;
>  
>  	xfs_bmapi_finish(&bma, whichfork, 0);
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  	return error;
>  
> diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> index 2076627243b0..095ab1202643 100644
> --- a/fs/xfs/libxfs/xfs_refcount.c
> +++ b/fs/xfs/libxfs/xfs_refcount.c
> @@ -1749,7 +1749,7 @@ xfs_refcount_recover_cow_leftovers(
>  		/* Free the block. */
>  		xfs_bmap_add_free(tp, fsb, rr->rr_rrec.rc_blockcount, NULL);
>  
> -		error = xfs_trans_commit(tp);
> +		error = xfs_trans_commit(tp, 0);
>  		if (error)
>  			goto out_free;
>  
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index c526c5e5ab76..c30ba1a6e819 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -990,9 +990,7 @@ xfs_sync_sb(
>  		return error;
>  
>  	xfs_log_sb(tp);
> -	if (wait)
> -		xfs_trans_set_sync(tp);
> -	return xfs_trans_commit(tp);
> +	return xfs_trans_commit(tp, wait ? XFS_TRANS_COMMIT_SYNC : 0);
>  }
>  
>  /*
> @@ -1087,8 +1085,7 @@ xfs_sync_sb_buf(
>  	bp = xfs_trans_getsb(tp, mp);
>  	xfs_log_sb(tp);
>  	xfs_trans_bhold(tp, bp);
> -	xfs_trans_set_sync(tp);
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_SYNC);
>  	if (error)
>  		goto out;
>  	/*
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index c45acbd3add9..586d5c0c912b 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -61,7 +61,6 @@ void	xfs_log_get_max_trans_res(struct xfs_mount *mp,
>  #define	XFS_TRANS_DIRTY		0x01	/* something needs to be logged */
>  #define	XFS_TRANS_SB_DIRTY	0x02	/* superblock is modified */
>  #define	XFS_TRANS_PERM_LOG_RES	0x04	/* xact took a permanent log res */
> -#define	XFS_TRANS_SYNC		0x08	/* make commit synchronous */
>  #define XFS_TRANS_DQ_DIRTY	0x10	/* at least one dquot in trx dirty */
>  #define XFS_TRANS_RESERVE	0x20    /* OK to use reserved data blocks */
>  #define XFS_TRANS_NO_WRITECOUNT 0x40	/* do not elevate SB writecount */
> diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> index 8ebf35b115ce..67bb71c98003 100644
> --- a/fs/xfs/scrub/scrub.c
> +++ b/fs/xfs/scrub/scrub.c
> @@ -155,7 +155,7 @@ xchk_teardown(
>  	xchk_ag_free(sc, &sc->sa);
>  	if (sc->tp) {
>  		if (error == 0 && (sc->sm->sm_flags & XFS_SCRUB_IFLAG_REPAIR))
> -			error = xfs_trans_commit(sc->tp);
> +			error = xfs_trans_commit(sc->tp, 0);
>  		else
>  			xfs_trans_cancel(sc->tp);
>  		sc->tp = NULL;
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 9d9cebf18726..c0504d5144ee 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -92,7 +92,7 @@ __xfs_setfilesize(
>  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  
> -	return xfs_trans_commit(tp);
> +	return xfs_trans_commit(tp, 0);
>  }
>  
>  int
> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> index c42f90e16b4f..c07389d07724 100644
> --- a/fs/xfs/xfs_attr_inactive.c
> +++ b/fs/xfs/xfs_attr_inactive.c
> @@ -380,7 +380,7 @@ xfs_attr_inactive(
>  	/* Reset the attribute fork - this also destroys the in-core fork */
>  	xfs_attr_fork_remove(dp, trans);
>  
> -	error = xfs_trans_commit(trans);
> +	error = xfs_trans_commit(trans, 0);
>  	xfs_iunlock(dp, lock_mode);
>  	return error;
>  
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index ee6f4229cebc..6d07581f8fc0 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -548,7 +548,7 @@ xfs_bui_recover(
>  
>  	set_bit(XFS_BUI_RECOVERED, &buip->bui_flags);
>  	xfs_defer_move(parent_tp, tp);
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  	xfs_irele(ip);
>  
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 4f800f7fe888..f1b24fa01432 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -702,7 +702,7 @@ xfs_free_eofblocks(
>  			 */
>  			xfs_trans_cancel(tp);
>  		} else {
> -			error = xfs_trans_commit(tp);
> +			error = xfs_trans_commit(tp, 0);
>  			if (!error)
>  				xfs_inode_clear_eofblocks_tag(ip);
>  		}
> @@ -833,7 +833,7 @@ xfs_alloc_file_space(
>  		/*
>  		 * Complete the transaction
>  		 */
> -		error = xfs_trans_commit(tp);
> +		error = xfs_trans_commit(tp, 0);
>  		xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  		if (error)
>  			break;
> @@ -890,7 +890,7 @@ xfs_unmap_extent(
>  	if (error)
>  		goto out_trans_cancel;
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  out_unlock:
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  	return error;
> @@ -1098,7 +1098,7 @@ xfs_collapse_file_space(
>  			goto out_trans_cancel;
>  	}
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  	return error;
>  
> @@ -1175,7 +1175,7 @@ xfs_insert_file_space(
>  			goto out_trans_cancel;
>  	} while (!done);
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  	return error;
>  
> @@ -1753,14 +1753,7 @@ xfs_swap_extents(
>  			goto out_trans_cancel;
>  	}
>  
> -	/*
> -	 * If this is a synchronous mount, make sure that the
> -	 * transaction goes to disk before returning to the user.
> -	 */
> -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> -		xfs_trans_set_sync(tp);
> -
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_WSYNC);
>  
>  	trace_xfs_swap_extent_after(ip, 0);
>  	trace_xfs_swap_extent_after(tip, 1);
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index af2c8e5ceea0..fec166e2dd11 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -530,7 +530,7 @@ xfs_qm_dqread_alloc(
>  	if (error)
>  		goto err_cancel;
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	if (error) {
>  		/*
>  		 * Buffer was held to the transaction, so we have to unlock it
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 6ea847f6e298..7572cddca074 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -645,7 +645,7 @@ xfs_efi_recover(
>  	}
>  
>  	set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	return error;
>  
>  abort_error:
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 4b8bdecc3863..45343e6d1643 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -61,9 +61,7 @@ xfs_update_prealloc_flags(
>  		ip->i_d.di_flags &= ~XFS_DIFLAG_PREALLOC;
>  
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> -	if (flags & XFS_PREALLOC_SYNC)
> -		xfs_trans_set_sync(tp);
> -	return xfs_trans_commit(tp);
> +	return xfs_trans_commit(tp, XFS_TRANS_COMMIT_SYNC);
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 3e61d0cc23f8..a45ab9397901 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -128,8 +128,7 @@ xfs_growfs_data_private(
>  				 nb - mp->m_sb.sb_dblocks);
>  	if (id.nfree)
>  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
> -	xfs_trans_set_sync(tp);
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_SYNC);
>  	if (error)
>  		return error;
>  
> @@ -209,8 +208,7 @@ xfs_growfs_imaxpct(
>  
>  	dpct = imaxpct - mp->m_sb.sb_imax_pct;
>  	xfs_trans_mod_sb(tp, XFS_TRANS_SB_IMAXPCT, dpct);
> -	xfs_trans_set_sync(tp);
> -	return xfs_trans_commit(tp);
> +	return xfs_trans_commit(tp, XFS_TRANS_COMMIT_SYNC);
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index d1772786af29..e04587a23bfa 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1216,14 +1216,6 @@ xfs_create(
>  		xfs_bumplink(tp, dp);
>  	}
>  
> -	/*
> -	 * If this is a synchronous mount, make sure that the
> -	 * create transaction goes to disk before returning to
> -	 * the user.
> -	 */
> -	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
> -		xfs_trans_set_sync(tp);
> -
>  	/*
>  	 * Attach the dquot(s) to the inodes and modify them incore.
>  	 * These ids of the inode couldn't have changed since the new
> @@ -1231,7 +1223,7 @@ xfs_create(
>  	 */
>  	xfs_qm_vop_create_dqattach(tp, ip, udqp, gdqp, pdqp);
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_DIRSYNC);
>  	if (error)
>  		goto out_release_inode;
>  
> @@ -1311,9 +1303,6 @@ xfs_create_tmpfile(
>  	if (error)
>  		goto out_trans_cancel;
>  
> -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> -		xfs_trans_set_sync(tp);
> -
>  	/*
>  	 * Attach the dquot(s) to the inodes and modify them incore.
>  	 * These ids of the inode couldn't have changed since the new
> @@ -1325,7 +1314,7 @@ xfs_create_tmpfile(
>  	if (error)
>  		goto out_trans_cancel;
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_WSYNC);
>  	if (error)
>  		goto out_release_inode;
>  
> @@ -1430,16 +1419,7 @@ xfs_link(
>  	xfs_trans_log_inode(tp, tdp, XFS_ILOG_CORE);
>  
>  	xfs_bumplink(tp, sip);
> -
> -	/*
> -	 * If this is a synchronous mount, make sure that the
> -	 * link transaction goes to disk before returning to
> -	 * the user.
> -	 */
> -	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
> -		xfs_trans_set_sync(tp);
> -
> -	return xfs_trans_commit(tp);
> +	return xfs_trans_commit(tp, XFS_TRANS_COMMIT_DIRSYNC);
>  
>   error_return:
>  	xfs_trans_cancel(tp);
> @@ -1688,7 +1668,7 @@ xfs_inactive_truncate(
>  
>  	ASSERT(ip->i_d.di_nextents == 0);
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	if (error)
>  		goto error_unlock;
>  
> @@ -1773,7 +1753,7 @@ xfs_inactive_ifree(
>  	 * Just ignore errors at this point.  There is nothing we can do except
>  	 * to try to keep going. Make sure it's not a silent error.
>  	 */
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	if (error)
>  		xfs_notice(mp, "%s: xfs_trans_commit returned error %d",
>  			__func__, error);
> @@ -2957,15 +2937,7 @@ xfs_remove(
>  		goto out_trans_cancel;
>  	}
>  
> -	/*
> -	 * If this is a synchronous mount, make sure that the
> -	 * remove transaction goes to disk before returning to
> -	 * the user.
> -	 */
> -	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
> -		xfs_trans_set_sync(tp);
> -
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_DIRSYNC);
>  	if (error)
>  		goto std_return;
>  
> @@ -3031,20 +3003,6 @@ xfs_sort_for_rename(
>  	}
>  }
>  
> -static int
> -xfs_finish_rename(
> -	struct xfs_trans	*tp)
> -{
> -	/*
> -	 * If this is a synchronous mount, make sure that the rename transaction
> -	 * goes to disk before returning to the user.
> -	 */
> -	if (tp->t_mountp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
> -		xfs_trans_set_sync(tp);
> -
> -	return xfs_trans_commit(tp);
> -}
> -
>  /*
>   * xfs_cross_rename()
>   *
> @@ -3147,7 +3105,7 @@ xfs_cross_rename(
>  	}
>  	xfs_trans_ichgtime(tp, dp1, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
>  	xfs_trans_log_inode(tp, dp1, XFS_ILOG_CORE);
> -	return xfs_finish_rename(tp);
> +	return xfs_trans_commit(tp, XFS_TRANS_COMMIT_DIRSYNC);
>  
>  out_trans_abort:
>  	xfs_trans_cancel(tp);
> @@ -3471,7 +3429,7 @@ xfs_rename(
>  	if (new_parent)
>  		xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE);
>  
> -	error = xfs_finish_rename(tp);
> +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_DIRSYNC);
>  	if (wip)
>  		xfs_irele(wip);
>  	return error;
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index cdfb3cd9a25b..c2e3d8181dd8 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1381,10 +1381,6 @@ xfs_ioctl_setattr_get_trans(
>  		error = -EPERM;
>  		goto out_cancel;
>  	}
> -
> -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> -		xfs_trans_set_sync(tp);
> -
>  	return tp;
>  
>  out_cancel:
> @@ -1620,7 +1616,8 @@ xfs_ioctl_setattr(
>  	else
>  		ip->i_d.di_cowextsize = 0;
>  
> -	code = xfs_trans_commit(tp);
> +
> +	code = xfs_trans_commit(tp, XFS_TRANS_COMMIT_WSYNC);
>  
>  	/*
>  	 * Release any dquot(s) the inode had kept before chown.
> @@ -1729,7 +1726,7 @@ xfs_ioc_setxflags(
>  		goto out_drop_write;
>  	}
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_WSYNC);
>  out_drop_write:
>  	mnt_drop_write_file(filp);
>  	return error;
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index bb590a267a7f..52c0fabe9117 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -265,7 +265,7 @@ xfs_iomap_write_direct(
>  	/*
>  	 * Complete the transaction
>  	 */
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	if (error)
>  		goto out_unlock;
>  
> @@ -593,7 +593,7 @@ xfs_iomap_write_unwritten(
>  			xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  		}
>  
> -		error = xfs_trans_commit(tp);
> +		error = xfs_trans_commit(tp, 0);
>  		xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  		if (error)
>  			return error;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index f7a99b3bbcf7..e95edd550ca8 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -789,9 +789,7 @@ xfs_setattr_nonsize(
>  
>  	XFS_STATS_INC(mp, xs_ig_attrchg);
>  
> -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> -		xfs_trans_set_sync(tp);
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_WSYNC);
>  
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  
> @@ -1028,10 +1026,7 @@ xfs_setattr_size(
>  
>  	XFS_STATS_INC(mp, xs_ig_attrchg);
>  
> -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> -		xfs_trans_set_sync(tp);
> -
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_WSYNC);
>  out_unlock:
>  	if (lock_flags)
>  		xfs_iunlock(ip, lock_flags);
> @@ -1125,7 +1120,7 @@ xfs_vn_update_time(
>  
>  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>  	xfs_trans_log_inode(tp, ip, log_flags);
> -	return xfs_trans_commit(tp);
> +	return xfs_trans_commit(tp, 0);
>  }
>  
>  STATIC int
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 11c3502b07b1..0e7c10911345 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -4768,7 +4768,7 @@ xlog_finish_defer_ops(
>  	/* transfer all collected dfops to this transaction */
>  	xfs_defer_move(tp, parent_tp);
>  
> -	return xfs_trans_commit(tp);
> +	return xfs_trans_commit(tp, 0);
>  }
>  
>  /*
> @@ -4954,7 +4954,7 @@ xlog_recover_clear_agi_bucket(
>  	xfs_trans_log_buf(tp, agibp, offset,
>  			  (offset + sizeof(xfs_agino_t) - 1));
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	if (error)
>  		goto out_error;
>  	return;
> diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> index bb3008d390aa..c24a760f5194 100644
> --- a/fs/xfs/xfs_pnfs.c
> +++ b/fs/xfs/xfs_pnfs.c
> @@ -290,8 +290,7 @@ xfs_fs_commit_blocks(
>  		ip->i_d.di_size = iattr->ia_size;
>  	}
>  
> -	xfs_trans_set_sync(tp);
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_SYNC);
>  
>  out_drop_iolock:
>  	xfs_iunlock(ip, XFS_IOLOCK_EXCL);
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index c225691fad15..6eb0d44429de 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -818,7 +818,7 @@ xfs_qm_qino_alloc(
>  	spin_unlock(&mp->m_sb_lock);
>  	xfs_log_sb(tp);
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	if (error) {
>  		ASSERT(XFS_FORCED_SHUTDOWN(mp));
>  		xfs_alert(mp, "%s failed (error %d)!", __func__, error);
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index 5d5ac65aa1cc..982083b1c142 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -47,8 +47,7 @@ xfs_qm_log_quotaoff(
>  	 * return and actually stop quota accounting. So, make it synchronous.
>  	 * We don't care about quotoff's performance.
>  	 */
> -	xfs_trans_set_sync(tp);
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_SYNC);
>  	if (error)
>  		goto out;
>  
> @@ -81,8 +80,7 @@ xfs_qm_log_quotaoff_end(
>  	 * return and actually stop quota accounting. So, make it synchronous.
>  	 * We don't care about quotoff's performance.
>  	 */
> -	xfs_trans_set_sync(tp);
> -	return xfs_trans_commit(tp);
> +	return xfs_trans_commit(tp, XFS_TRANS_COMMIT_SYNC);
>  }
>  
>  /*
> @@ -305,7 +303,7 @@ xfs_qm_scall_trunc_qfile(
>  	ASSERT(ip->i_d.di_nextents == 0);
>  
>  	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  
>  out_unlock:
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
> @@ -593,7 +591,7 @@ xfs_qm_scall_setqlim(
>  	dqp->dq_flags |= XFS_DQ_DIRTY;
>  	xfs_trans_log_dquot(tp, dqp);
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  
>  out_rele:
>  	xfs_qm_dqrele(dqp);
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index 8eeed73928cd..9786c1e22ca4 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -581,7 +581,7 @@ xfs_cui_recover(
>  	xfs_refcount_finish_one_cleanup(tp, rcur, error);
>  	set_bit(XFS_CUI_RECOVERED, &cuip->cui_flags);
>  	xfs_defer_move(parent_tp, tp);
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	return error;
>  
>  abort_error:
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index b0ce04ffd3cd..01e6691a1f61 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -414,7 +414,7 @@ xfs_reflink_allocate_cow(
>  		goto out_unreserve;
>  
>  	xfs_inode_set_cowblocks_tag(ip);
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	if (error)
>  		return error;
>  
> @@ -570,7 +570,7 @@ xfs_reflink_cancel_cow_range(
>  	if (error)
>  		goto out_cancel;
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  	return error;
> @@ -683,7 +683,7 @@ xfs_reflink_end_cow_extent(
>  	/* Remove the mapping from the CoW fork. */
>  	xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  	if (error)
>  		return error;
> @@ -901,7 +901,7 @@ xfs_reflink_set_inode_flag(
>  		xfs_iunlock(dest, XFS_ILOCK_EXCL);
>  
>  commit_flags:
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	if (error)
>  		goto out_error;
>  	return error;
> @@ -948,7 +948,7 @@ xfs_reflink_update_dest(
>  
>  	xfs_trans_log_inode(tp, dest, XFS_ILOG_CORE);
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	if (error)
>  		goto out_error;
>  	return error;
> @@ -1088,7 +1088,7 @@ xfs_reflink_remap_extent(
>  			goto out_cancel;
>  	}
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  	if (error)
>  		goto out;
> @@ -1493,7 +1493,7 @@ xfs_reflink_try_clear_inode_flag(
>  	if (error)
>  		goto cancel;
>  
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	if (error)
>  		goto out;
>  
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index 4911b68f95dd..29931d4fa0e7 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -598,7 +598,7 @@ xfs_rui_recover(
>  
>  	xfs_rmap_finish_one_cleanup(tp, rcur, error);
>  	set_bit(XFS_RUI_RECOVERED, &ruip->rui_flags);
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	return error;
>  
>  abort_error:
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 6209e7b6b895..6039fca2b34f 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -800,7 +800,7 @@ xfs_growfs_rt_alloc(
>  		/*
>  		 * Free any blocks freed up in the transaction, then commit.
>  		 */
> -		error = xfs_trans_commit(tp);
> +		error = xfs_trans_commit(tp, 0);
>  		if (error)
>  			return error;
>  		/*
> @@ -835,7 +835,7 @@ xfs_growfs_rt_alloc(
>  			/*
>  			 * Commit the transaction.
>  			 */
> -			error = xfs_trans_commit(tp);
> +			error = xfs_trans_commit(tp, 0);
>  			if (error)
>  				return error;
>  		}
> @@ -1072,7 +1072,7 @@ xfs_growfs_rt(
>  		mp->m_rsumlevels = nrsumlevels;
>  		mp->m_rsumsize = nrsumsize;
>  
> -		error = xfs_trans_commit(tp);
> +		error = xfs_trans_commit(tp, 0);
>  		if (error)
>  			break;
>  	}
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index abf06bf9c3f3..e652ca8a1ef7 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -633,7 +633,7 @@ xfs_fs_dirty_inode(
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_TIMESTAMP);
> -	xfs_trans_commit(tp);
> +	xfs_trans_commit(tp, 0);
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 13fb4b919648..24e228c0f477 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -312,16 +312,7 @@ xfs_symlink(
>  	xfs_trans_ichgtime(tp, dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
>  	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
>  
> -	/*
> -	 * If this is a synchronous mount, make sure that the
> -	 * symlink transaction goes to disk before returning to
> -	 * the user.
> -	 */
> -	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC)) {
> -		xfs_trans_set_sync(tp);
> -	}
> -
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_DIRSYNC);
>  	if (error)
>  		goto out_release_inode;
>  
> @@ -439,7 +430,7 @@ xfs_inactive_symlink_rmt(
>  	 * rolls and commits the transaction that frees the extents.
>  	 */
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> -	error = xfs_trans_commit(tp);
> +	error = xfs_trans_commit(tp, 0);
>  	if (error) {
>  		ASSERT(XFS_FORCED_SHUTDOWN(mp));
>  		goto error_unlock;
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 28b983ff8b11..8aad8b3572ed 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -918,6 +918,23 @@ xfs_trans_committed_bulk(
>  	spin_unlock(&ailp->ail_lock);
>  }
>  
> +static inline bool
> +xfs_trans_is_sync(
> +	struct xfs_mount	*mp,
> +	enum xfs_commit_mode	mode)
> +{
> +	switch (mode) {
> +	case XFS_TRANS_COMMIT_SYNC:
> +		return true;
> +	case XFS_TRANS_COMMIT_DIRSYNC:
> +		return mp->m_flags & (XFS_MOUNT_WSYNC | XFS_MOUNT_DIRSYNC);
> +	case XFS_TRANS_COMMIT_WSYNC:
> +		return mp->m_flags & XFS_MOUNT_WSYNC;
> +	default:
> +		return false;
> +	}
> +}
> +
>  /*
>   * Commit the given transaction to the log.
>   *
> @@ -933,12 +950,12 @@ xfs_trans_committed_bulk(
>  static int
>  __xfs_trans_commit(
>  	struct xfs_trans	*tp,
> +	enum xfs_commit_mode	mode,
>  	bool			regrant)
>  {
>  	struct xfs_mount	*mp = tp->t_mountp;
>  	xfs_lsn_t		commit_lsn = -1;
>  	int			error = 0;
> -	int			sync = tp->t_flags & XFS_TRANS_SYNC;
>  
>  	trace_xfs_trans_commit(tp, _RET_IP_);
>  
> @@ -984,10 +1001,10 @@ __xfs_trans_commit(
>  	xfs_trans_free(tp);
>  
>  	/*
> -	 * If the transaction needs to be synchronous, then force the
> -	 * log out now and wait for it.
> +	 * If the transaction needs to be synchronous, then force the log out
> +	 * now and wait for it.
>  	 */
> -	if (sync) {
> +	if (xfs_trans_is_sync(mp, mode)) {
>  		error = xfs_log_force_lsn(mp, commit_lsn, XFS_LOG_SYNC, NULL);
>  		XFS_STATS_INC(mp, xs_trans_sync);
>  	} else {
> @@ -1022,9 +1039,10 @@ __xfs_trans_commit(
>  
>  int
>  xfs_trans_commit(
> -	struct xfs_trans	*tp)
> +	struct xfs_trans	*tp,
> +	enum xfs_commit_mode	mode)
>  {
> -	return __xfs_trans_commit(tp, false);
> +	return __xfs_trans_commit(tp, mode, false);
>  }
>  
>  /*
> @@ -1111,7 +1129,7 @@ xfs_trans_roll(
>  	 * is in progress. The caller takes the responsibility to cancel
>  	 * the duplicate transaction that gets returned.
>  	 */
> -	error = __xfs_trans_commit(trans, true);
> +	error = __xfs_trans_commit(trans, 0, true);
>  	if (error)
>  		return error;
>  
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 752c7fef9de7..499b5da72df0 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -143,12 +143,6 @@ typedef struct xfs_trans {
>  	unsigned long		t_pflags;	/* saved process flags state */
>  } xfs_trans_t;
>  
> -/*
> - * XFS transaction mechanism exported interfaces that are
> - * actually macros.
> - */
> -#define	xfs_trans_set_sync(tp)		((tp)->t_flags |= XFS_TRANS_SYNC)
> -
>  #if defined(DEBUG) || defined(XFS_WARN)
>  #define	xfs_trans_agblocks_delta(tp, d)	((tp)->t_ag_freeblks_delta += (int64_t)d)
>  #define	xfs_trans_agflist_delta(tp, d)	((tp)->t_ag_flist_delta += (int64_t)d)
> @@ -230,7 +224,14 @@ void		xfs_trans_dirty_buf(struct xfs_trans *, struct xfs_buf *);
>  bool		xfs_trans_buf_is_dirty(struct xfs_buf *bp);
>  void		xfs_trans_log_inode(xfs_trans_t *, struct xfs_inode *, uint);
>  
> -int		xfs_trans_commit(struct xfs_trans *);
> +enum xfs_commit_mode {
> +	/* 0 means no sync required */
> +	XFS_TRANS_COMMIT_SYNC = 1,	/* always commit synchronously */
> +	XFS_TRANS_COMMIT_WSYNC,		/* ... only for wsync mounts */
> +	XFS_TRANS_COMMIT_DIRSYNC,	/* ... for dirsync and wsync mounts */
> +};
> +
> +int		xfs_trans_commit(struct xfs_trans *, enum xfs_commit_mode mode);
>  int		xfs_trans_roll(struct xfs_trans **);
>  int		xfs_trans_roll_inode(struct xfs_trans **, struct xfs_inode *);
>  void		xfs_trans_cancel(xfs_trans_t *);
> -- 
> 2.25.1
> 
---end quoted text---

Brian Foster May 1, 2020, 10:24 a.m. UTC | #2

On Fri, May 01, 2020 at 01:07:03AM -0700, Christoph Hellwig wrote:
> Any comments?
> 
> On Thu, Apr 09, 2020 at 09:36:50AM +0200, Christoph Hellwig wrote:
> > Instead of marking the transaction sync using a flag, often based on
> > copy and pasted mount options checks, pass an enum of sync modes
> > to xfs_trans_commit that clearly documents the intent.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---

I recall looking at this when it was first posted and my first reaction
was that I didn't really like the interface. I decided to think about it
to see if it grew on me and then just lost track (sorry). It's not so
much passing a flag to commit as opposed to the flags not directly
controlling behavior (i.e., one flag means sync if <something> is true,
another flag means sync if <something else> is true, etc.) tends to
confuse me. I don't feel terribly strongly about it if others prefer
this pattern, but I still find the existing code more readable.

I vaguely recall thinking it might be nice if we could dump this into
transaction state to avoid the aforementioned logic warts, but IIRC that
might not have been possible for all users of this functionality..

Brian

> >  fs/xfs/libxfs/xfs_attr.c     | 15 ++--------
> >  fs/xfs/libxfs/xfs_bmap.c     |  4 +--
> >  fs/xfs/libxfs/xfs_refcount.c |  2 +-
> >  fs/xfs/libxfs/xfs_sb.c       |  7 ++---
> >  fs/xfs/libxfs/xfs_shared.h   |  1 -
> >  fs/xfs/scrub/scrub.c         |  2 +-
> >  fs/xfs/xfs_aops.c            |  2 +-
> >  fs/xfs/xfs_attr_inactive.c   |  2 +-
> >  fs/xfs/xfs_bmap_item.c       |  2 +-
> >  fs/xfs/xfs_bmap_util.c       | 19 ++++--------
> >  fs/xfs/xfs_dquot.c           |  2 +-
> >  fs/xfs/xfs_extfree_item.c    |  2 +-
> >  fs/xfs/xfs_file.c            |  4 +--
> >  fs/xfs/xfs_fsops.c           |  6 ++--
> >  fs/xfs/xfs_inode.c           | 58 +++++-------------------------------
> >  fs/xfs/xfs_ioctl.c           |  9 ++----
> >  fs/xfs/xfs_iomap.c           |  4 +--
> >  fs/xfs/xfs_iops.c            | 11 ++-----
> >  fs/xfs/xfs_log_recover.c     |  4 +--
> >  fs/xfs/xfs_pnfs.c            |  3 +-
> >  fs/xfs/xfs_qm.c              |  2 +-
> >  fs/xfs/xfs_qm_syscalls.c     | 10 +++----
> >  fs/xfs/xfs_refcount_item.c   |  2 +-
> >  fs/xfs/xfs_reflink.c         | 14 ++++-----
> >  fs/xfs/xfs_rmap_item.c       |  2 +-
> >  fs/xfs/xfs_rtalloc.c         |  6 ++--
> >  fs/xfs/xfs_super.c           |  2 +-
> >  fs/xfs/xfs_symlink.c         | 13 ++------
> >  fs/xfs/xfs_trans.c           | 32 +++++++++++++++-----
> >  fs/xfs/xfs_trans.h           | 15 +++++-----
> >  30 files changed, 94 insertions(+), 163 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index e4fe3dca9883..a6a94bbea8a7 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -175,7 +175,6 @@ xfs_attr_try_sf_addname(
> >  	struct xfs_da_args	*args)
> >  {
> >  
> > -	struct xfs_mount	*mp = dp->i_mount;
> >  	int			error, error2;
> >  
> >  	error = xfs_attr_shortform_addname(args);
> > @@ -189,10 +188,7 @@ xfs_attr_try_sf_addname(
> >  	if (!error && !(args->op_flags & XFS_DA_OP_NOTIME))
> >  		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
> >  
> > -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> > -		xfs_trans_set_sync(args->trans);
> > -
> > -	error2 = xfs_trans_commit(args->trans);
> > +	error2 = xfs_trans_commit(args->trans, XFS_TRANS_COMMIT_WSYNC);
> >  	args->trans = NULL;
> >  	return error ? error : error2;
> >  }
> > @@ -382,13 +378,6 @@ xfs_attr_set(
> >  			goto out_trans_cancel;
> >  	}
> >  
> > -	/*
> > -	 * If this is a synchronous mount, make sure that the
> > -	 * transaction goes to disk before returning to the user.
> > -	 */
> > -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> > -		xfs_trans_set_sync(args->trans);
> > -
> >  	if (!(args->op_flags & XFS_DA_OP_NOTIME))
> >  		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
> >  
> > @@ -396,7 +385,7 @@ xfs_attr_set(
> >  	 * Commit the last in the sequence of transactions.
> >  	 */
> >  	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE);
> > -	error = xfs_trans_commit(args->trans);
> > +	error = xfs_trans_commit(args->trans, XFS_TRANS_COMMIT_WSYNC);
> >  out_unlock:
> >  	xfs_iunlock(dp, XFS_ILOCK_EXCL);
> >  	return error;
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index fda13cd7add0..aa16990e9b5f 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -1148,7 +1148,7 @@ xfs_bmap_add_attrfork(
> >  			xfs_log_sb(tp);
> >  	}
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >  	return error;
> >  
> > @@ -4644,7 +4644,7 @@ xfs_bmapi_convert_delalloc(
> >  		goto out_finish;
> >  
> >  	xfs_bmapi_finish(&bma, whichfork, 0);
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >  	return error;
> >  
> > diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> > index 2076627243b0..095ab1202643 100644
> > --- a/fs/xfs/libxfs/xfs_refcount.c
> > +++ b/fs/xfs/libxfs/xfs_refcount.c
> > @@ -1749,7 +1749,7 @@ xfs_refcount_recover_cow_leftovers(
> >  		/* Free the block. */
> >  		xfs_bmap_add_free(tp, fsb, rr->rr_rrec.rc_blockcount, NULL);
> >  
> > -		error = xfs_trans_commit(tp);
> > +		error = xfs_trans_commit(tp, 0);
> >  		if (error)
> >  			goto out_free;
> >  
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index c526c5e5ab76..c30ba1a6e819 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -990,9 +990,7 @@ xfs_sync_sb(
> >  		return error;
> >  
> >  	xfs_log_sb(tp);
> > -	if (wait)
> > -		xfs_trans_set_sync(tp);
> > -	return xfs_trans_commit(tp);
> > +	return xfs_trans_commit(tp, wait ? XFS_TRANS_COMMIT_SYNC : 0);
> >  }
> >  
> >  /*
> > @@ -1087,8 +1085,7 @@ xfs_sync_sb_buf(
> >  	bp = xfs_trans_getsb(tp, mp);
> >  	xfs_log_sb(tp);
> >  	xfs_trans_bhold(tp, bp);
> > -	xfs_trans_set_sync(tp);
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_SYNC);
> >  	if (error)
> >  		goto out;
> >  	/*
> > diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> > index c45acbd3add9..586d5c0c912b 100644
> > --- a/fs/xfs/libxfs/xfs_shared.h
> > +++ b/fs/xfs/libxfs/xfs_shared.h
> > @@ -61,7 +61,6 @@ void	xfs_log_get_max_trans_res(struct xfs_mount *mp,
> >  #define	XFS_TRANS_DIRTY		0x01	/* something needs to be logged */
> >  #define	XFS_TRANS_SB_DIRTY	0x02	/* superblock is modified */
> >  #define	XFS_TRANS_PERM_LOG_RES	0x04	/* xact took a permanent log res */
> > -#define	XFS_TRANS_SYNC		0x08	/* make commit synchronous */
> >  #define XFS_TRANS_DQ_DIRTY	0x10	/* at least one dquot in trx dirty */
> >  #define XFS_TRANS_RESERVE	0x20    /* OK to use reserved data blocks */
> >  #define XFS_TRANS_NO_WRITECOUNT 0x40	/* do not elevate SB writecount */
> > diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> > index 8ebf35b115ce..67bb71c98003 100644
> > --- a/fs/xfs/scrub/scrub.c
> > +++ b/fs/xfs/scrub/scrub.c
> > @@ -155,7 +155,7 @@ xchk_teardown(
> >  	xchk_ag_free(sc, &sc->sa);
> >  	if (sc->tp) {
> >  		if (error == 0 && (sc->sm->sm_flags & XFS_SCRUB_IFLAG_REPAIR))
> > -			error = xfs_trans_commit(sc->tp);
> > +			error = xfs_trans_commit(sc->tp, 0);
> >  		else
> >  			xfs_trans_cancel(sc->tp);
> >  		sc->tp = NULL;
> > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> > index 9d9cebf18726..c0504d5144ee 100644
> > --- a/fs/xfs/xfs_aops.c
> > +++ b/fs/xfs/xfs_aops.c
> > @@ -92,7 +92,7 @@ __xfs_setfilesize(
> >  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> >  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> >  
> > -	return xfs_trans_commit(tp);
> > +	return xfs_trans_commit(tp, 0);
> >  }
> >  
> >  int
> > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> > index c42f90e16b4f..c07389d07724 100644
> > --- a/fs/xfs/xfs_attr_inactive.c
> > +++ b/fs/xfs/xfs_attr_inactive.c
> > @@ -380,7 +380,7 @@ xfs_attr_inactive(
> >  	/* Reset the attribute fork - this also destroys the in-core fork */
> >  	xfs_attr_fork_remove(dp, trans);
> >  
> > -	error = xfs_trans_commit(trans);
> > +	error = xfs_trans_commit(trans, 0);
> >  	xfs_iunlock(dp, lock_mode);
> >  	return error;
> >  
> > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> > index ee6f4229cebc..6d07581f8fc0 100644
> > --- a/fs/xfs/xfs_bmap_item.c
> > +++ b/fs/xfs/xfs_bmap_item.c
> > @@ -548,7 +548,7 @@ xfs_bui_recover(
> >  
> >  	set_bit(XFS_BUI_RECOVERED, &buip->bui_flags);
> >  	xfs_defer_move(parent_tp, tp);
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >  	xfs_irele(ip);
> >  
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index 4f800f7fe888..f1b24fa01432 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -702,7 +702,7 @@ xfs_free_eofblocks(
> >  			 */
> >  			xfs_trans_cancel(tp);
> >  		} else {
> > -			error = xfs_trans_commit(tp);
> > +			error = xfs_trans_commit(tp, 0);
> >  			if (!error)
> >  				xfs_inode_clear_eofblocks_tag(ip);
> >  		}
> > @@ -833,7 +833,7 @@ xfs_alloc_file_space(
> >  		/*
> >  		 * Complete the transaction
> >  		 */
> > -		error = xfs_trans_commit(tp);
> > +		error = xfs_trans_commit(tp, 0);
> >  		xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >  		if (error)
> >  			break;
> > @@ -890,7 +890,7 @@ xfs_unmap_extent(
> >  	if (error)
> >  		goto out_trans_cancel;
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  out_unlock:
> >  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >  	return error;
> > @@ -1098,7 +1098,7 @@ xfs_collapse_file_space(
> >  			goto out_trans_cancel;
> >  	}
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >  	return error;
> >  
> > @@ -1175,7 +1175,7 @@ xfs_insert_file_space(
> >  			goto out_trans_cancel;
> >  	} while (!done);
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >  	return error;
> >  
> > @@ -1753,14 +1753,7 @@ xfs_swap_extents(
> >  			goto out_trans_cancel;
> >  	}
> >  
> > -	/*
> > -	 * If this is a synchronous mount, make sure that the
> > -	 * transaction goes to disk before returning to the user.
> > -	 */
> > -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> > -		xfs_trans_set_sync(tp);
> > -
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_WSYNC);
> >  
> >  	trace_xfs_swap_extent_after(ip, 0);
> >  	trace_xfs_swap_extent_after(tip, 1);
> > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > index af2c8e5ceea0..fec166e2dd11 100644
> > --- a/fs/xfs/xfs_dquot.c
> > +++ b/fs/xfs/xfs_dquot.c
> > @@ -530,7 +530,7 @@ xfs_qm_dqread_alloc(
> >  	if (error)
> >  		goto err_cancel;
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	if (error) {
> >  		/*
> >  		 * Buffer was held to the transaction, so we have to unlock it
> > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> > index 6ea847f6e298..7572cddca074 100644
> > --- a/fs/xfs/xfs_extfree_item.c
> > +++ b/fs/xfs/xfs_extfree_item.c
> > @@ -645,7 +645,7 @@ xfs_efi_recover(
> >  	}
> >  
> >  	set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	return error;
> >  
> >  abort_error:
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index 4b8bdecc3863..45343e6d1643 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -61,9 +61,7 @@ xfs_update_prealloc_flags(
> >  		ip->i_d.di_flags &= ~XFS_DIFLAG_PREALLOC;
> >  
> >  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > -	if (flags & XFS_PREALLOC_SYNC)
> > -		xfs_trans_set_sync(tp);
> > -	return xfs_trans_commit(tp);
> > +	return xfs_trans_commit(tp, XFS_TRANS_COMMIT_SYNC);
> >  }
> >  
> >  /*
> > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > index 3e61d0cc23f8..a45ab9397901 100644
> > --- a/fs/xfs/xfs_fsops.c
> > +++ b/fs/xfs/xfs_fsops.c
> > @@ -128,8 +128,7 @@ xfs_growfs_data_private(
> >  				 nb - mp->m_sb.sb_dblocks);
> >  	if (id.nfree)
> >  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
> > -	xfs_trans_set_sync(tp);
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_SYNC);
> >  	if (error)
> >  		return error;
> >  
> > @@ -209,8 +208,7 @@ xfs_growfs_imaxpct(
> >  
> >  	dpct = imaxpct - mp->m_sb.sb_imax_pct;
> >  	xfs_trans_mod_sb(tp, XFS_TRANS_SB_IMAXPCT, dpct);
> > -	xfs_trans_set_sync(tp);
> > -	return xfs_trans_commit(tp);
> > +	return xfs_trans_commit(tp, XFS_TRANS_COMMIT_SYNC);
> >  }
> >  
> >  /*
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index d1772786af29..e04587a23bfa 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -1216,14 +1216,6 @@ xfs_create(
> >  		xfs_bumplink(tp, dp);
> >  	}
> >  
> > -	/*
> > -	 * If this is a synchronous mount, make sure that the
> > -	 * create transaction goes to disk before returning to
> > -	 * the user.
> > -	 */
> > -	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
> > -		xfs_trans_set_sync(tp);
> > -
> >  	/*
> >  	 * Attach the dquot(s) to the inodes and modify them incore.
> >  	 * These ids of the inode couldn't have changed since the new
> > @@ -1231,7 +1223,7 @@ xfs_create(
> >  	 */
> >  	xfs_qm_vop_create_dqattach(tp, ip, udqp, gdqp, pdqp);
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_DIRSYNC);
> >  	if (error)
> >  		goto out_release_inode;
> >  
> > @@ -1311,9 +1303,6 @@ xfs_create_tmpfile(
> >  	if (error)
> >  		goto out_trans_cancel;
> >  
> > -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> > -		xfs_trans_set_sync(tp);
> > -
> >  	/*
> >  	 * Attach the dquot(s) to the inodes and modify them incore.
> >  	 * These ids of the inode couldn't have changed since the new
> > @@ -1325,7 +1314,7 @@ xfs_create_tmpfile(
> >  	if (error)
> >  		goto out_trans_cancel;
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_WSYNC);
> >  	if (error)
> >  		goto out_release_inode;
> >  
> > @@ -1430,16 +1419,7 @@ xfs_link(
> >  	xfs_trans_log_inode(tp, tdp, XFS_ILOG_CORE);
> >  
> >  	xfs_bumplink(tp, sip);
> > -
> > -	/*
> > -	 * If this is a synchronous mount, make sure that the
> > -	 * link transaction goes to disk before returning to
> > -	 * the user.
> > -	 */
> > -	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
> > -		xfs_trans_set_sync(tp);
> > -
> > -	return xfs_trans_commit(tp);
> > +	return xfs_trans_commit(tp, XFS_TRANS_COMMIT_DIRSYNC);
> >  
> >   error_return:
> >  	xfs_trans_cancel(tp);
> > @@ -1688,7 +1668,7 @@ xfs_inactive_truncate(
> >  
> >  	ASSERT(ip->i_d.di_nextents == 0);
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	if (error)
> >  		goto error_unlock;
> >  
> > @@ -1773,7 +1753,7 @@ xfs_inactive_ifree(
> >  	 * Just ignore errors at this point.  There is nothing we can do except
> >  	 * to try to keep going. Make sure it's not a silent error.
> >  	 */
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	if (error)
> >  		xfs_notice(mp, "%s: xfs_trans_commit returned error %d",
> >  			__func__, error);
> > @@ -2957,15 +2937,7 @@ xfs_remove(
> >  		goto out_trans_cancel;
> >  	}
> >  
> > -	/*
> > -	 * If this is a synchronous mount, make sure that the
> > -	 * remove transaction goes to disk before returning to
> > -	 * the user.
> > -	 */
> > -	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
> > -		xfs_trans_set_sync(tp);
> > -
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_DIRSYNC);
> >  	if (error)
> >  		goto std_return;
> >  
> > @@ -3031,20 +3003,6 @@ xfs_sort_for_rename(
> >  	}
> >  }
> >  
> > -static int
> > -xfs_finish_rename(
> > -	struct xfs_trans	*tp)
> > -{
> > -	/*
> > -	 * If this is a synchronous mount, make sure that the rename transaction
> > -	 * goes to disk before returning to the user.
> > -	 */
> > -	if (tp->t_mountp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
> > -		xfs_trans_set_sync(tp);
> > -
> > -	return xfs_trans_commit(tp);
> > -}
> > -
> >  /*
> >   * xfs_cross_rename()
> >   *
> > @@ -3147,7 +3105,7 @@ xfs_cross_rename(
> >  	}
> >  	xfs_trans_ichgtime(tp, dp1, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> >  	xfs_trans_log_inode(tp, dp1, XFS_ILOG_CORE);
> > -	return xfs_finish_rename(tp);
> > +	return xfs_trans_commit(tp, XFS_TRANS_COMMIT_DIRSYNC);
> >  
> >  out_trans_abort:
> >  	xfs_trans_cancel(tp);
> > @@ -3471,7 +3429,7 @@ xfs_rename(
> >  	if (new_parent)
> >  		xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE);
> >  
> > -	error = xfs_finish_rename(tp);
> > +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_DIRSYNC);
> >  	if (wip)
> >  		xfs_irele(wip);
> >  	return error;
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index cdfb3cd9a25b..c2e3d8181dd8 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1381,10 +1381,6 @@ xfs_ioctl_setattr_get_trans(
> >  		error = -EPERM;
> >  		goto out_cancel;
> >  	}
> > -
> > -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> > -		xfs_trans_set_sync(tp);
> > -
> >  	return tp;
> >  
> >  out_cancel:
> > @@ -1620,7 +1616,8 @@ xfs_ioctl_setattr(
> >  	else
> >  		ip->i_d.di_cowextsize = 0;
> >  
> > -	code = xfs_trans_commit(tp);
> > +
> > +	code = xfs_trans_commit(tp, XFS_TRANS_COMMIT_WSYNC);
> >  
> >  	/*
> >  	 * Release any dquot(s) the inode had kept before chown.
> > @@ -1729,7 +1726,7 @@ xfs_ioc_setxflags(
> >  		goto out_drop_write;
> >  	}
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_WSYNC);
> >  out_drop_write:
> >  	mnt_drop_write_file(filp);
> >  	return error;
> > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> > index bb590a267a7f..52c0fabe9117 100644
> > --- a/fs/xfs/xfs_iomap.c
> > +++ b/fs/xfs/xfs_iomap.c
> > @@ -265,7 +265,7 @@ xfs_iomap_write_direct(
> >  	/*
> >  	 * Complete the transaction
> >  	 */
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	if (error)
> >  		goto out_unlock;
> >  
> > @@ -593,7 +593,7 @@ xfs_iomap_write_unwritten(
> >  			xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> >  		}
> >  
> > -		error = xfs_trans_commit(tp);
> > +		error = xfs_trans_commit(tp, 0);
> >  		xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >  		if (error)
> >  			return error;
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index f7a99b3bbcf7..e95edd550ca8 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -789,9 +789,7 @@ xfs_setattr_nonsize(
> >  
> >  	XFS_STATS_INC(mp, xs_ig_attrchg);
> >  
> > -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> > -		xfs_trans_set_sync(tp);
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_WSYNC);
> >  
> >  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >  
> > @@ -1028,10 +1026,7 @@ xfs_setattr_size(
> >  
> >  	XFS_STATS_INC(mp, xs_ig_attrchg);
> >  
> > -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> > -		xfs_trans_set_sync(tp);
> > -
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_WSYNC);
> >  out_unlock:
> >  	if (lock_flags)
> >  		xfs_iunlock(ip, lock_flags);
> > @@ -1125,7 +1120,7 @@ xfs_vn_update_time(
> >  
> >  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> >  	xfs_trans_log_inode(tp, ip, log_flags);
> > -	return xfs_trans_commit(tp);
> > +	return xfs_trans_commit(tp, 0);
> >  }
> >  
> >  STATIC int
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 11c3502b07b1..0e7c10911345 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -4768,7 +4768,7 @@ xlog_finish_defer_ops(
> >  	/* transfer all collected dfops to this transaction */
> >  	xfs_defer_move(tp, parent_tp);
> >  
> > -	return xfs_trans_commit(tp);
> > +	return xfs_trans_commit(tp, 0);
> >  }
> >  
> >  /*
> > @@ -4954,7 +4954,7 @@ xlog_recover_clear_agi_bucket(
> >  	xfs_trans_log_buf(tp, agibp, offset,
> >  			  (offset + sizeof(xfs_agino_t) - 1));
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	if (error)
> >  		goto out_error;
> >  	return;
> > diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> > index bb3008d390aa..c24a760f5194 100644
> > --- a/fs/xfs/xfs_pnfs.c
> > +++ b/fs/xfs/xfs_pnfs.c
> > @@ -290,8 +290,7 @@ xfs_fs_commit_blocks(
> >  		ip->i_d.di_size = iattr->ia_size;
> >  	}
> >  
> > -	xfs_trans_set_sync(tp);
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_SYNC);
> >  
> >  out_drop_iolock:
> >  	xfs_iunlock(ip, XFS_IOLOCK_EXCL);
> > diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> > index c225691fad15..6eb0d44429de 100644
> > --- a/fs/xfs/xfs_qm.c
> > +++ b/fs/xfs/xfs_qm.c
> > @@ -818,7 +818,7 @@ xfs_qm_qino_alloc(
> >  	spin_unlock(&mp->m_sb_lock);
> >  	xfs_log_sb(tp);
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	if (error) {
> >  		ASSERT(XFS_FORCED_SHUTDOWN(mp));
> >  		xfs_alert(mp, "%s failed (error %d)!", __func__, error);
> > diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> > index 5d5ac65aa1cc..982083b1c142 100644
> > --- a/fs/xfs/xfs_qm_syscalls.c
> > +++ b/fs/xfs/xfs_qm_syscalls.c
> > @@ -47,8 +47,7 @@ xfs_qm_log_quotaoff(
> >  	 * return and actually stop quota accounting. So, make it synchronous.
> >  	 * We don't care about quotoff's performance.
> >  	 */
> > -	xfs_trans_set_sync(tp);
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_SYNC);
> >  	if (error)
> >  		goto out;
> >  
> > @@ -81,8 +80,7 @@ xfs_qm_log_quotaoff_end(
> >  	 * return and actually stop quota accounting. So, make it synchronous.
> >  	 * We don't care about quotoff's performance.
> >  	 */
> > -	xfs_trans_set_sync(tp);
> > -	return xfs_trans_commit(tp);
> > +	return xfs_trans_commit(tp, XFS_TRANS_COMMIT_SYNC);
> >  }
> >  
> >  /*
> > @@ -305,7 +303,7 @@ xfs_qm_scall_trunc_qfile(
> >  	ASSERT(ip->i_d.di_nextents == 0);
> >  
> >  	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  
> >  out_unlock:
> >  	xfs_iunlock(ip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
> > @@ -593,7 +591,7 @@ xfs_qm_scall_setqlim(
> >  	dqp->dq_flags |= XFS_DQ_DIRTY;
> >  	xfs_trans_log_dquot(tp, dqp);
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  
> >  out_rele:
> >  	xfs_qm_dqrele(dqp);
> > diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> > index 8eeed73928cd..9786c1e22ca4 100644
> > --- a/fs/xfs/xfs_refcount_item.c
> > +++ b/fs/xfs/xfs_refcount_item.c
> > @@ -581,7 +581,7 @@ xfs_cui_recover(
> >  	xfs_refcount_finish_one_cleanup(tp, rcur, error);
> >  	set_bit(XFS_CUI_RECOVERED, &cuip->cui_flags);
> >  	xfs_defer_move(parent_tp, tp);
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	return error;
> >  
> >  abort_error:
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index b0ce04ffd3cd..01e6691a1f61 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -414,7 +414,7 @@ xfs_reflink_allocate_cow(
> >  		goto out_unreserve;
> >  
> >  	xfs_inode_set_cowblocks_tag(ip);
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	if (error)
> >  		return error;
> >  
> > @@ -570,7 +570,7 @@ xfs_reflink_cancel_cow_range(
> >  	if (error)
> >  		goto out_cancel;
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  
> >  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >  	return error;
> > @@ -683,7 +683,7 @@ xfs_reflink_end_cow_extent(
> >  	/* Remove the mapping from the CoW fork. */
> >  	xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >  	if (error)
> >  		return error;
> > @@ -901,7 +901,7 @@ xfs_reflink_set_inode_flag(
> >  		xfs_iunlock(dest, XFS_ILOCK_EXCL);
> >  
> >  commit_flags:
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	if (error)
> >  		goto out_error;
> >  	return error;
> > @@ -948,7 +948,7 @@ xfs_reflink_update_dest(
> >  
> >  	xfs_trans_log_inode(tp, dest, XFS_ILOG_CORE);
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	if (error)
> >  		goto out_error;
> >  	return error;
> > @@ -1088,7 +1088,7 @@ xfs_reflink_remap_extent(
> >  			goto out_cancel;
> >  	}
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >  	if (error)
> >  		goto out;
> > @@ -1493,7 +1493,7 @@ xfs_reflink_try_clear_inode_flag(
> >  	if (error)
> >  		goto cancel;
> >  
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	if (error)
> >  		goto out;
> >  
> > diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> > index 4911b68f95dd..29931d4fa0e7 100644
> > --- a/fs/xfs/xfs_rmap_item.c
> > +++ b/fs/xfs/xfs_rmap_item.c
> > @@ -598,7 +598,7 @@ xfs_rui_recover(
> >  
> >  	xfs_rmap_finish_one_cleanup(tp, rcur, error);
> >  	set_bit(XFS_RUI_RECOVERED, &ruip->rui_flags);
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	return error;
> >  
> >  abort_error:
> > diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> > index 6209e7b6b895..6039fca2b34f 100644
> > --- a/fs/xfs/xfs_rtalloc.c
> > +++ b/fs/xfs/xfs_rtalloc.c
> > @@ -800,7 +800,7 @@ xfs_growfs_rt_alloc(
> >  		/*
> >  		 * Free any blocks freed up in the transaction, then commit.
> >  		 */
> > -		error = xfs_trans_commit(tp);
> > +		error = xfs_trans_commit(tp, 0);
> >  		if (error)
> >  			return error;
> >  		/*
> > @@ -835,7 +835,7 @@ xfs_growfs_rt_alloc(
> >  			/*
> >  			 * Commit the transaction.
> >  			 */
> > -			error = xfs_trans_commit(tp);
> > +			error = xfs_trans_commit(tp, 0);
> >  			if (error)
> >  				return error;
> >  		}
> > @@ -1072,7 +1072,7 @@ xfs_growfs_rt(
> >  		mp->m_rsumlevels = nrsumlevels;
> >  		mp->m_rsumsize = nrsumsize;
> >  
> > -		error = xfs_trans_commit(tp);
> > +		error = xfs_trans_commit(tp, 0);
> >  		if (error)
> >  			break;
> >  	}
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index abf06bf9c3f3..e652ca8a1ef7 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -633,7 +633,7 @@ xfs_fs_dirty_inode(
> >  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> >  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> >  	xfs_trans_log_inode(tp, ip, XFS_ILOG_TIMESTAMP);
> > -	xfs_trans_commit(tp);
> > +	xfs_trans_commit(tp, 0);
> >  }
> >  
> >  /*
> > diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> > index 13fb4b919648..24e228c0f477 100644
> > --- a/fs/xfs/xfs_symlink.c
> > +++ b/fs/xfs/xfs_symlink.c
> > @@ -312,16 +312,7 @@ xfs_symlink(
> >  	xfs_trans_ichgtime(tp, dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> >  	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
> >  
> > -	/*
> > -	 * If this is a synchronous mount, make sure that the
> > -	 * symlink transaction goes to disk before returning to
> > -	 * the user.
> > -	 */
> > -	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC)) {
> > -		xfs_trans_set_sync(tp);
> > -	}
> > -
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, XFS_TRANS_COMMIT_DIRSYNC);
> >  	if (error)
> >  		goto out_release_inode;
> >  
> > @@ -439,7 +430,7 @@ xfs_inactive_symlink_rmt(
> >  	 * rolls and commits the transaction that frees the extents.
> >  	 */
> >  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > -	error = xfs_trans_commit(tp);
> > +	error = xfs_trans_commit(tp, 0);
> >  	if (error) {
> >  		ASSERT(XFS_FORCED_SHUTDOWN(mp));
> >  		goto error_unlock;
> > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> > index 28b983ff8b11..8aad8b3572ed 100644
> > --- a/fs/xfs/xfs_trans.c
> > +++ b/fs/xfs/xfs_trans.c
> > @@ -918,6 +918,23 @@ xfs_trans_committed_bulk(
> >  	spin_unlock(&ailp->ail_lock);
> >  }
> >  
> > +static inline bool
> > +xfs_trans_is_sync(
> > +	struct xfs_mount	*mp,
> > +	enum xfs_commit_mode	mode)
> > +{
> > +	switch (mode) {
> > +	case XFS_TRANS_COMMIT_SYNC:
> > +		return true;
> > +	case XFS_TRANS_COMMIT_DIRSYNC:
> > +		return mp->m_flags & (XFS_MOUNT_WSYNC | XFS_MOUNT_DIRSYNC);
> > +	case XFS_TRANS_COMMIT_WSYNC:
> > +		return mp->m_flags & XFS_MOUNT_WSYNC;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> >  /*
> >   * Commit the given transaction to the log.
> >   *
> > @@ -933,12 +950,12 @@ xfs_trans_committed_bulk(
> >  static int
> >  __xfs_trans_commit(
> >  	struct xfs_trans	*tp,
> > +	enum xfs_commit_mode	mode,
> >  	bool			regrant)
> >  {
> >  	struct xfs_mount	*mp = tp->t_mountp;
> >  	xfs_lsn_t		commit_lsn = -1;
> >  	int			error = 0;
> > -	int			sync = tp->t_flags & XFS_TRANS_SYNC;
> >  
> >  	trace_xfs_trans_commit(tp, _RET_IP_);
> >  
> > @@ -984,10 +1001,10 @@ __xfs_trans_commit(
> >  	xfs_trans_free(tp);
> >  
> >  	/*
> > -	 * If the transaction needs to be synchronous, then force the
> > -	 * log out now and wait for it.
> > +	 * If the transaction needs to be synchronous, then force the log out
> > +	 * now and wait for it.
> >  	 */
> > -	if (sync) {
> > +	if (xfs_trans_is_sync(mp, mode)) {
> >  		error = xfs_log_force_lsn(mp, commit_lsn, XFS_LOG_SYNC, NULL);
> >  		XFS_STATS_INC(mp, xs_trans_sync);
> >  	} else {
> > @@ -1022,9 +1039,10 @@ __xfs_trans_commit(
> >  
> >  int
> >  xfs_trans_commit(
> > -	struct xfs_trans	*tp)
> > +	struct xfs_trans	*tp,
> > +	enum xfs_commit_mode	mode)
> >  {
> > -	return __xfs_trans_commit(tp, false);
> > +	return __xfs_trans_commit(tp, mode, false);
> >  }
> >  
> >  /*
> > @@ -1111,7 +1129,7 @@ xfs_trans_roll(
> >  	 * is in progress. The caller takes the responsibility to cancel
> >  	 * the duplicate transaction that gets returned.
> >  	 */
> > -	error = __xfs_trans_commit(trans, true);
> > +	error = __xfs_trans_commit(trans, 0, true);
> >  	if (error)
> >  		return error;
> >  
> > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> > index 752c7fef9de7..499b5da72df0 100644
> > --- a/fs/xfs/xfs_trans.h
> > +++ b/fs/xfs/xfs_trans.h
> > @@ -143,12 +143,6 @@ typedef struct xfs_trans {
> >  	unsigned long		t_pflags;	/* saved process flags state */
> >  } xfs_trans_t;
> >  
> > -/*
> > - * XFS transaction mechanism exported interfaces that are
> > - * actually macros.
> > - */
> > -#define	xfs_trans_set_sync(tp)		((tp)->t_flags |= XFS_TRANS_SYNC)
> > -
> >  #if defined(DEBUG) || defined(XFS_WARN)
> >  #define	xfs_trans_agblocks_delta(tp, d)	((tp)->t_ag_freeblks_delta += (int64_t)d)
> >  #define	xfs_trans_agflist_delta(tp, d)	((tp)->t_ag_flist_delta += (int64_t)d)
> > @@ -230,7 +224,14 @@ void		xfs_trans_dirty_buf(struct xfs_trans *, struct xfs_buf *);
> >  bool		xfs_trans_buf_is_dirty(struct xfs_buf *bp);
> >  void		xfs_trans_log_inode(xfs_trans_t *, struct xfs_inode *, uint);
> >  
> > -int		xfs_trans_commit(struct xfs_trans *);
> > +enum xfs_commit_mode {
> > +	/* 0 means no sync required */
> > +	XFS_TRANS_COMMIT_SYNC = 1,	/* always commit synchronously */
> > +	XFS_TRANS_COMMIT_WSYNC,		/* ... only for wsync mounts */
> > +	XFS_TRANS_COMMIT_DIRSYNC,	/* ... for dirsync and wsync mounts */
> > +};
> > +
> > +int		xfs_trans_commit(struct xfs_trans *, enum xfs_commit_mode mode);
> >  int		xfs_trans_roll(struct xfs_trans **);
> >  int		xfs_trans_roll_inode(struct xfs_trans **, struct xfs_inode *);
> >  void		xfs_trans_cancel(xfs_trans_t *);
> > -- 
> > 2.25.1
> > 
> ---end quoted text---
>

Christoph Hellwig May 1, 2020, 10:42 a.m. UTC | #3

On Fri, May 01, 2020 at 06:24:03AM -0400, Brian Foster wrote:
> I recall looking at this when it was first posted and my first reaction
> was that I didn't really like the interface. I decided to think about it
> to see if it grew on me and then just lost track (sorry). It's not so
> much passing a flag to commit as opposed to the flags not directly
> controlling behavior (i.e., one flag means sync if <something> is true,
> another flag means sync if <something else> is true, etc.) tends to
> confuse me. I don't feel terribly strongly about it if others prefer
> this pattern, but I still find the existing code more readable.
> 
> I vaguely recall thinking it might be nice if we could dump this into
> transaction state to avoid the aforementioned logic warts, but IIRC that
> might not have been possible for all users of this functionality..

Moving the flag out of the transaction structure was the main motivation
for this series - the fact that we need different arguments to
xfs_trans_commit is just a fallout from that.  The rationale is that
I found it highly confusing to figure out how and where we set the sync
flag vs having it obvious in the one place where we commit the
transaction.

Brian Foster May 1, 2020, 11:51 a.m. UTC | #4

On Fri, May 01, 2020 at 12:42:45PM +0200, Christoph Hellwig wrote:
> On Fri, May 01, 2020 at 06:24:03AM -0400, Brian Foster wrote:
> > I recall looking at this when it was first posted and my first reaction
> > was that I didn't really like the interface. I decided to think about it
> > to see if it grew on me and then just lost track (sorry). It's not so
> > much passing a flag to commit as opposed to the flags not directly
> > controlling behavior (i.e., one flag means sync if <something> is true,
> > another flag means sync if <something else> is true, etc.) tends to
> > confuse me. I don't feel terribly strongly about it if others prefer
> > this pattern, but I still find the existing code more readable.
> > 
> > I vaguely recall thinking it might be nice if we could dump this into
> > transaction state to avoid the aforementioned logic warts, but IIRC that
> > might not have been possible for all users of this functionality..
> 
> Moving the flag out of the transaction structure was the main motivation
> for this series - the fact that we need different arguments to
> xfs_trans_commit is just a fallout from that.  The rationale is that
> I found it highly confusing to figure out how and where we set the sync
> flag vs having it obvious in the one place where we commit the
> transaction.
> 

Sorry, I was referring to moving your new [W|DIR]SYNC variants to
somewhere like xfs_trans_res->tr_logflags in the comment above, not the
existing XFS_TRANS_SYNC flag (which I would keep). Regardless, I didn't
think that would work across the board from looking at it before.
Perhaps it would work in some cases..

I agree that the current approach is confusing in that it's not always
clear when to set the sync flag. I disagree that this patch makes it
obvious and in one place because when I see this:

	error = xfs_trans_commit(args->trans, XFS_TRANS_COMMIT_WSYNC);

... it makes me think the flag has an immediate effect (like COMMIT_SYNC
does) and subsequently raises the same questions around the existing
code of when or when not to use which flag in the context of the
individual transaction. *shrug* Just my .02.

Brian

Darrick J. Wong May 1, 2020, 3:53 p.m. UTC | #5

On Fri, May 01, 2020 at 07:51:32AM -0400, Brian Foster wrote:
> On Fri, May 01, 2020 at 12:42:45PM +0200, Christoph Hellwig wrote:
> > On Fri, May 01, 2020 at 06:24:03AM -0400, Brian Foster wrote:
> > > I recall looking at this when it was first posted and my first reaction
> > > was that I didn't really like the interface. I decided to think about it
> > > to see if it grew on me and then just lost track (sorry). It's not so
> > > much passing a flag to commit as opposed to the flags not directly
> > > controlling behavior (i.e., one flag means sync if <something> is true,
> > > another flag means sync if <something else> is true, etc.) tends to
> > > confuse me. I don't feel terribly strongly about it if others prefer
> > > this pattern, but I still find the existing code more readable.
> > > 
> > > I vaguely recall thinking it might be nice if we could dump this into
> > > transaction state to avoid the aforementioned logic warts, but IIRC that
> > > might not have been possible for all users of this functionality..
> > 
> > Moving the flag out of the transaction structure was the main motivation
> > for this series - the fact that we need different arguments to
> > xfs_trans_commit is just a fallout from that.  The rationale is that
> > I found it highly confusing to figure out how and where we set the sync
> > flag vs having it obvious in the one place where we commit the
> > transaction.
> > 
> 
> Sorry, I was referring to moving your new [W|DIR]SYNC variants to
> somewhere like xfs_trans_res->tr_logflags in the comment above, not the
> existing XFS_TRANS_SYNC flag (which I would keep). Regardless, I didn't
> think that would work across the board from looking at it before.
> Perhaps it would work in some cases..
> 
> I agree that the current approach is confusing in that it's not always
> clear when to set the sync flag. I disagree that this patch makes it
> obvious and in one place because when I see this:
> 
> 	error = xfs_trans_commit(args->trans, XFS_TRANS_COMMIT_WSYNC);
> 
> ... it makes me think the flag has an immediate effect (like COMMIT_SYNC
> does) and subsequently raises the same questions around the existing
> code of when or when not to use which flag in the context of the
> individual transaction. *shrug* Just my .02.

Similarly, this fell off my radar screen once the three of us started
lobbing log refactoring patchsets at each other. :)

AFAICT the goal of the WSYNC/DIRSYNC logic is basically to annotate the
transaction to say "sysadmin wants all (write|namespace) operations to
commit synchronously", so at first I was a little surprised that this
added a flags argument to xfs_trans_commit instead of adding a couple of
flags to capture the "I'm a write op" or  "I'm a namespace op" to
xfs_trans_alloc.

/Then/ I realized that xfs_trans_alloc lets you set t_flags directly
with almost no validation and died a little inside.  Then it occurred
to me that maybe this is deliberately done just prior to the final
commit so that the intermediate transaction rolls are not committed
synchronously, just one big log force at the end.

/And then/ I noticed that in the places where we set SYNC just prior to
xfs_trans_commit, each deferred op that gets run via xfs_trans_commit's
call to xfs_defer_finish_noroll blocks waiting for xfs_log_force_lsn,
when we could probably get away with only forcing the log at the very
end of the transaction chain.

So, uh, my question is, would it make more sense to (a) create a
separate trans_alloc flags namespace so that (b) we can add the
wsync/dirsync annotations from the outset, while keeping in mind that
(c) we could possibly let the intermediate transaction rolls commit in
the usual asynchronous fashion so long as the last one forces the log?

--D

> Brian
>

xfs: pass a commit_mode to xfs_trans_commit

Commit Message

Comments

Patch