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

[Dave Chinner <david@fromorbit.com>]

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@lst.de>

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@fromorbit.com

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