xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 5/5] xfs: use shared ilock mode for direct IO writes by default
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 27 Mar 2012 12:39:13 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120326211603.856796827@xxxxxxxxxxxxxxxxxxxxxx>
References: <20120326211421.518374058@xxxxxxxxxxxxxxxxxxxxxx> <20120326211603.856796827@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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

<Prev in Thread] Current Thread [Next in Thread>