On Mon, Mar 26, 2012 at 05:14:26PM -0400, Christoph Hellwig wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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@xxxxxx>
>
> ---
> 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@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
