Re: [PATCH 4/8] xfs: don't log the inode in xfs_fs_map_blocks if it wasn't modified

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Oct 25, 2019 at 05:03:32PM +0200, Christoph Hellwig wrote:
> Even if we are asked for a write layout there is no point in logging
> the inode unless we actually modified it in some way.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_pnfs.c | 43 +++++++++++++++++++------------------------
>  1 file changed, 19 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> index 9c96493be9e0..fa90c6334c7c 100644
> --- a/fs/xfs/xfs_pnfs.c
> +++ b/fs/xfs/xfs_pnfs.c
> @@ -147,32 +147,27 @@ xfs_fs_map_blocks(
>  	if (error)
>  		goto out_unlock;
>  
> -	if (write) {
> -		enum xfs_prealloc_flags	flags = 0;
> -
> +	if (write &&
> +	    (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) {
>  		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);

The change in code flow makes this assert rather useless, I think, since
we only end up in this branch if we have a write and a hole.  If the
condition that it checks is important (and it seems to be?) then it
ought to be hoisted up a level and turned into:

ASSERT(!write || !nimaps || imap.br_startblock != DELAYSTARTBLOCK);

Right?

Otherwise looks ok.

--D

>  
> -		if (!nimaps || imap.br_startblock == HOLESTARTBLOCK) {
> -			/*
> -			 * xfs_iomap_write_direct() expects to take ownership of
> -			 * the shared ilock.
> -			 */
> -			xfs_ilock(ip, XFS_ILOCK_SHARED);
> -			error = xfs_iomap_write_direct(ip, offset, length,
> -						       &imap, nimaps);
> -			if (error)
> -				goto out_unlock;
> -
> -			/*
> -			 * Ensure the next transaction is committed
> -			 * synchronously so that the blocks allocated and
> -			 * handed out to the client are guaranteed to be
> -			 * present even after a server crash.
> -			 */
> -			flags |= XFS_PREALLOC_SET | XFS_PREALLOC_SYNC;
> -		}
> -
> -		error = xfs_update_prealloc_flags(ip, flags);
> +		/*
> +		 * xfs_iomap_write_direct() expects to take ownership of the
> +		 * shared ilock.
> +		 */
> +		xfs_ilock(ip, XFS_ILOCK_SHARED);
> +		error = xfs_iomap_write_direct(ip, offset, length, &imap,
> +					       nimaps);
> +		if (error)
> +			goto out_unlock;
> +
> +		/*
> +		 * Ensure the next transaction is committed synchronously so
> +		 * that the blocks allocated and handed out to the client are
> +		 * guaranteed to be present even after a server crash.
> +		 */
> +		error = xfs_update_prealloc_flags(ip,
> +				XFS_PREALLOC_SET | XFS_PREALLOC_SYNC);
>  		if (error)
>  			goto out_unlock;
>  	}
> -- 
> 2.20.1
>