On 06/14/2016 07:06 PM, Dave Chinner wrote:
On Tue, Jun 14, 2016 at 02:12:39PM -0400, Waiman Long wrote:This patch enables reader optimistic spinning for inodes that are under a DAX-based mount point. On a 4-socket Haswell machine running on a 4.7-rc1 tip-based kernel, the fio test with multithreaded randrw and randwrite tests on the same file on a XFS partition on top of a NVDIMM with DAX were run, the aggregated bandwidths before and after the patch were as follows: Test BW before patch BW after patch % change ---- --------------- -------------- -------- randrw 1352 MB/s 2164 MB/s +60% randwrite 1710 MB/s 2550 MB/s +49% Signed-off-by: Waiman Long<Waiman.Long@xxxxxxx> --- fs/xfs/xfs_icache.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-) diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c index 99ee6ee..09f284f 100644 --- a/fs/xfs/xfs_icache.c +++ b/fs/xfs/xfs_icache.c @@ -71,6 +71,15 @@ xfs_inode_alloc( mrlock_init(&ip->i_iolock, MRLOCK_BARRIER, "xfsio", ip->i_ino); + /* + * Enable reader spinning for DAX nount point + */ + if (mp->m_flags& XFS_MOUNT_DAX) { + rwsem_set_rspin_threshold(&ip->i_iolock.mr_lock); + rwsem_set_rspin_threshold(&ip->i_mmaplock.mr_lock); + rwsem_set_rspin_threshold(&ip->i_lock.mr_lock); + }That's wrong. DAX is a per-inode flag, not a mount wide flag. This needs to be done once the inode has been fully initialised and IS_DAX(inode) can be run. Also, the benchmark doesn't show that all these locks are being tested by this benchmark. e.g. the i_mmaplock isn't involved in the benchmark's IO paths at all. It's only taken in page faults and truncate paths.... I'd also like to see how much of the gain comes from the iolock vs the ilock, as the ilock is nested inside the iolock and so contention is much rarer....
This patch has now been superseded by a second one where changes to the xfs code is no longer needed. The new patch will enable reader spinning for all rwsem and dynamically disable it depending on past history.
As it is, I'm *extremely* paranoid when it comes to changes to core locking like this. Performance is secondary to correctness, and we need much more than just a few benchmarks to verify there aren't locking bugs being introduced....
The core rwsem locking logic hasn't been changed. There are some minor changes, however, on what RWSEM_WAITING_BIAS value to use that requires more eyeballs to make sure that it hasn't introduced any new bug.
Cheers, Longman -- To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
