xfs
[Top] [All Lists]

Re: [PATCH] xfs: avoid synchronous transaction in xfs_fs_write_inode

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: avoid synchronous transaction in xfs_fs_write_inode
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 9 Jun 2010 17:29:11 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100608195905.GA577@xxxxxxxxxxxxx>
References: <20100608195905.GA577@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Tue, Jun 08, 2010 at 03:59:07PM -0400, Christoph Hellwig wrote:
> 
> We already rely on the fact that the sync code will cause a synchronous
> log force later on (currently via xfs_fs_sync_fs -> xfs_quiesce_data ->
> xfs_sync_data), so no need to do this here.  This allows us to avoid
> a lot of synchronous log forces during sync, which pays of especially
> with delayed logging enabled.   Some compilebench numbers that show
> this:
> 
> xfs (delayed logging, 256k logbufs)
> ===================================
> 
> intial create           25.94 MB/s      25.75 MB/s      25.64 MB/s
> create                           8.54 MB/s       9.12 MB/s       9.15 MB/s
> patch                    2.47 MB/s       2.47 MB/s       3.17 MB/s
> compile                         29.65 MB/s      30.51 MB/s      27.33 MB/s
> clean                   90.92 MB/s      98.83 MB/s     128.87 MB/s
> read tree               11.90 MB/s      11.84 MB/s       8.56 MB/s
> read compiled           28.75 MB/s      29.96 MB/s      24.25 MB/s
> delete tree           8.39 seconds    8.12 seconds    8.46 seconds
> delete compiled               8.35 seconds    8.44 seconds    5.11 seconds
> stat tree             6.03 seconds    5.59 seconds    5.19 seconds
> stat compiled tree    9.00 seconds    9.52 seconds    8.49 seconds
> 
> 
> xfs + write_inode log_force removal
> ===================================
> intial create           25.87 MB/s      25.76 MB/s      25.87 MB/s
> create                          15.18 MB/s      14.80 MB/s      14.94 MB/s
> patch                    3.13 MB/s       3.14 MB/s       3.11 MB/s
> compile                         36.74 MB/s      37.17 MB/s      36.84 MB/s
> clean                  226.02 MB/s     222.58 MB/s     217.94 MB/s
> read tree               15.14 MB/s      15.02 MB/s      15.14 MB/s
> read compiled tree      29.30 MB/s      29.31 MB/s      29.32 MB/s
> delete tree           6.22 seconds    6.14 seconds    6.15 seconds
> delete compiled tree  5.75 seconds    5.92 seconds    5.81 seconds
> stat tree             4.60 seconds    4.51 seconds    4.56 seconds
> stat compiled tree    4.07 seconds    3.87 seconds    3.96 seconds
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

All great - I attempted this myself - but it breaks bulkstat. See
xfstest 183:

183 2s ... - output mismatch (see 183.out.bad)
--- 183.out     2010-04-28 15:00:22.000000000 +1000
+++ 183.out.bad 2010-06-09 17:10:23.000000000 +1000
@@ -1,4 +1,4 @@
 QA output created by 183
 Start original bulkstat_unlink_test with -r switch
 Runing extended checks.
-Iteration 0 ... (100 files)passed
+Iteration 0 ... (100 files)ERROR, count(2) != scount(1).
Ran: 183
Failures: 183
Failed 1 of 1 tests

Test 183 fails with this patch because it leaves the inode pinned in
the WB_SYNC_ALL case after calling xfs_log_inode(), and so the inode
can't be flushed to the backing buffer. Hence bulkstat may not see
changes to an inode after a sync becuase they weren't flushed during
the sync.

I think that we need to revisit bulkstat's use of the backing
buffers for performance reasons now we have a much more scalable
inode cache. Having to keep the backing buffers coherent is only
going to be more problematic in future as things get even more
asynchronous....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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