Re: [PATCH 5/5] xfs: use shared ilock mode for direct IO writes by default

From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 5/5] xfs: use shared ilock mode for direct IO writes by default
Date: Tue, 27 Mar 2012 12:39:13 +1100	[thread overview]
Message-ID: <20120327013913.GU5091@dastard> (raw)
In-Reply-To: <20120326211603.856796827@bombadil.infradead.org>

On Mon, Mar 26, 2012 at 05:14:26PM -0400, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> For the direct IO write path, we only really need the ilock to be taken in
> exclusive mode during IO submission if we need to do extent allocation
> instaled of all the time.
> 
> Change the block mapping code to take the ilock in shared mode for the
> initial block mapping, and only retake it exclusively when we actually
> have to perform extent allocations.  We were already dropping the ilock
> for the transaction allocation, so this doesn't introduce new race windows.
> 
> Based on an earlier patch from Dave Chinner.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> ---
>  fs/xfs/xfs_aops.c  |   21 +++++++++++++++++----
>  fs/xfs/xfs_iomap.c |   45 +++++++++++++++++++--------------------------
>  2 files changed, 36 insertions(+), 30 deletions(-)
> 
> Index: xfs/fs/xfs/xfs_aops.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_aops.c	2012-03-26 21:06:52.184213212 +0200
> +++ xfs/fs/xfs/xfs_aops.c	2012-03-26 21:16:40.744358040 +0200
> @@ -1146,7 +1146,14 @@ __xfs_get_blocks(
>  	if (!create && direct && offset >= i_size_read(inode))
>  		return 0;
>  
> -	if (create) {
> +	/*
> +	 * Direct I/O is usually done on preallocated files, so try getting
> +	 * a block mapping without an exclusive lock first.  For buffer
                                                                 buffered

.....
> @@ -1169,22 +1176,28 @@ __xfs_get_blocks(
>  	     (imap.br_startblock == HOLESTARTBLOCK ||
>  	      imap.br_startblock == DELAYSTARTBLOCK))) {
>  		if (direct) {
> +			xfs_iunlock(ip, lockmode);
> +
>  			error = xfs_iomap_write_direct(ip, offset, size,
>  						       &imap, nimaps);
> +			if (error)
> +				return -error;
>  		} else {
>  			error = xfs_iomap_write_delay(ip, offset, size, &imap);
> +			if (error)
> +				goto out_unlock;
> +
> +			xfs_iunlock(ip, lockmode);

I'm not sure that I like the implicit different lock semantics here.
One function drops the lock first, the other requires the lock to be
held. Perhaps a comment is in order, such that direct IO requires
transactions for allocation here and hence will drop the lock to do
so and this avoids a needless lock round-trip?

Actually, when would we *ever* take the direct IO path when we have
imap.br_startblock == DELAYSTARTBLOCK? Buffered write regions are
supposed to have been flushed before the DIO write gets here.
Perhaps there should be an ASSERT(imap.br_startblock ==
HOLESTARTBLOCK) in the direct IO path.

Otherwise it looks OK.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs