Improve dirty page writeout performance #2250
Conversation
|
Been using this in conjunction with #2129 and here's how they stack up against current HEAD: With both patches: Benchmarks are pretty pointless in real world terms though. One thing to note, and i've no idea where the blame lies here, is that i did experience a kernel panic with these patches applied. Ended up hurting the ext2 /boot partition. I've since added openzfs/spl#328 to the mix and so far no issues in 24 hours. Just completed a pretty robust kernel build of 3.14.2-pf, AUFS (and a bunch of other patches plus just about the whole device tree) in 10m flat... will move to that for testing now that i'm building dkms debs from the github sources instead of per-kernel modules. |
|
3.14-pf is a no go - immediate crashes on head of SPL and patched version, kind of difficult to export the stack traces since they happen during boot, or hard lock the host almost immediately on login to lightdm (its a laptop to boot). Additionally either this patch or #2129, or both, are causing kernel panics still, though not as quickly as the 3.14 bit. No stack traces to show for it so far. |
|
@sempervictus This pull request contains two patches. One is the performance patch and another is a fix for an ancient bug that predated OpenSolaris. I wrote the latter after it was found that the former had a tendency to trigger an assertion in AVL trees. Are you using both patches? Also, some early versions of the latter patch were buggy and consequently, different versions have been pushed. The buggy versions had been pushed for Brian's regression test systems because they were the only ones able to reproduce problems with the former patch. Additionally, the patches in #2129 are causing issues in regression tests. I do not advise using them in production. As for UKSM, you are the first that I know to do anything with it and ZFS, so you are in uncharted territory with it. |
|
Thanks for the clarification. |
|
Let me know if it works without 2129. On Apr 29, 2014, at 1:42 PM, RageLtMan notifications@github.com wrote:
|
|
@sempervictus please also post some test results if time allows I'm also using the sha256 checksums and use ZFS heavily on a desktop and am interested in the latency results by this (and/or the slab patchset, which I'm also using) alone thanks ! |
|
4 Days up, no kernel messages, no crashes, very usable. One thing to note is that i had to kill all of my auto-snapshots and create a new recursive snapshot from the pool on down. When i tried sending the mess to auto-snaps i kept getting errors: |
|
@sempervictus thanks for the feedback, we're getting closer to being able to merge this and this are looking good. However a few remaining issues have been discovered. For example, I'm able to consistently cause the following crash with CentOS6.5 (sorry about the abbreviated stack). |
|
what precedes this behavior and has it been observed on 6.4? |
|
I think I learned what I need to know to fix it yesterday. Due to my schedule, I will not be able to try it for another few hours. On May 2, 2014, at 6:00 PM, RageLtMan notifications@github.com wrote:
|
|
@ryao Here's a full backtrace and some additional debugging from gdb. This was from xfstests 074, time permitting I'll dig in to this too. |
|
@behlendorf does this only occur on older kernels or also on newer ones ? any risk of data loss ? if not and only a potential hardlock is in store - I'd be willing to give it a try with 3.14.3 @ryao any news on the fix or did life call back ? =) |
|
I've only been able to reproduce it with 2.6.32 kernels under a fairly abusive test case. There shouldn't be any risk of data loss here, just potentially a lockup. Any additional testing you can offer would be welcome! I'll try and look at this too time permitting. |
|
|
maybe it's too early but I seem to observe following errors: status: One or more devices has experienced an unrecoverable error. An after using the patched ZFS & ZFS-KMOD parts with this patchset there are in total 2 drives (mirror mode) the master drive /home and the mirror, the checksum errors always occur on the mirror (2-5 checksum errors, so the number is not fixed) RAM is fine, harddrive should be fine, too (no smartctl, or badblocks errors, no strange noises) so far I've pinpointed it down to following: when shutting down system via shutdown -h now, the errors occur during import when logging out of X, and "zpool export" the pool/drives before shutting down or rebooting the system, during the next import the errors don't occur following extra settings, besides copies=. , are used zfs set atime=off zfs set aclinherit=discard zfs set compression=lz4 zfs set sync=standard zfs set logbias=latency |
|
okay, so it's not related to this patchset but the fact that the mirrored pool which is my /home "partition" doesn't seem to get exported, unmounted anymore during shutdown - weird :/ |
|
FYI: still using this patch(set) and no related issues so far thanks =) |
|
Thanks for the update. |
|
Linux MythMaster 3.13.0-29-generic #53-Ubuntu SMP Wed Jun 4 21:00:20 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Just tried running with 2250, 2129 and spl_kmem_slab at 16384. Got a fairly unresponsive system after a page fail with light load in about 1 hour. ZFS has one iSCSI ZFS block device and a standard ZFS filesystem served by SAMBA. All have lz4 compression on. Was able to shutdown safely. log message listed below along with system configuration and hardware. ZPool configuration: Hardware: |
This refactors ->writepages and ->writepage to improve performance. ->writepages has been refactored to call zfs_range_lock() once per call. Each zfs_range_lock() does its own memory allocation and various tree operations, so making it occur only once reduces the computational complexity of writepages, by at least a factor of log(N) where N is the number of pages. This avoids significant virtual memory contention that is prevalent in Linux's vmalloc(). In addition, we now pass WB_SYNC_NONE to write_cache_pages. This moves responsibility for ensuring synchronous semantics entirely to zil_commit(), where it should have been in the first place. It also eliminates an opportunity for concurrent threads to block each other on writeback. ->writepage has been refactored to permit the changes to ->writepages. Situations where it is told to write out an invalidated page is now properly handled by ignoring them. This avoids a theoretical NULL pointer dereference. These changes substantially improve 32-thread 4K randomwrite IO on a RAM disk in filebench's randomwrite benchmark. This has been tested with the following commands on a boot that passed ramdisk_size=8388604 to the kernel commandline: modprobe loop losetup loop0 /dev/ram0 zpool create -o ashift=12 -O recordsize=4K test /dev/loop0 zpool export test losetup -D zpool import -d /dev test filebench << END load randomwrite set \$dir=/test set \$nthreads=32 set \$iosize=4096 run 60 END zpool destroy test Previous tests on Gentoo Linux using a kernel compiled from the CentOS 2.6.32-431.11.2.el6.x86_64 kernel with the default .config showed single threaded performance at roughly 10k to 11k IOPS with 32-thread performance at 5k-6k in this benchmark. Tests with this patch show a performance improvement to 15k to 17k in the 32-thread case. These tests were conducted on a single processor Xeon E5-2620 with 64GB of RAM. Signed-off-by: Richard Yao <ryao@gentoo.org>
|
@ryao I've spent all day looking at this trying to find a clean way to refactor the code along the lines of what your suggesting in this patch. Ideally we would be able to take the writer range lock over the call to write_cache_pages() but in the WB_SYNC_ALL case that may deadlock. To avoid the deadlock we must call write_cache_pages() with WB_SYNC_NONE but that means we must implement our own data-integrity logic. That may sound simple enough on the surface but getting it right isn't as simple as calling zil_commit(). That won't handle the case where a page in writeback was re-dirtied and that new page needs to be written. Calling zil_commit() will only ensure that the previous version which was in writeback was written. This issue likely won't cause any test failures but it's just functionally wrong. I took a crack at implementing this data-integrity logic in #2413 by tagging all the dirty pages for WB_SYNC_ALL callers. However, the logic is a little overly broad and it could result in is writing out dirty pages multiple times. That said, I believe it's functionality correct. I've also rolled some other minor cleanup in to the #2413 patch. Next I ran some benchmarks using the stock 0.6.3 code, your patched version, and my patched version. I used fio to generate a workload of smalll 4k IOs from multiple threads and ran it against a ramdisk. To make thing more realistic I also mixed in calls to msync() every 32 writes. Here's the test script. My results don't show any significant performance win for the new code. For any version of this patch to get merged we're going to need to show the following.
|
|
@behlendorf If I understand correctly, you are concerned about the following scenario:
If we drop the rangelock before zil_commit(), then writepages can skip over the redirtied page by running before the callback from the DMU. If we drop the rangelock after zil_commit(), then writepages will not skip over the redirtied page. We can avoid the problem of having redirted pages to write-out when in writeback by holding the range lock in ->writepage{,s}() during the critical section in which pages are in writeback. The SUSv2 only requires us to ensure that everything at the time of the syscall entry is written out, which holding the rangelock until zil_commit() returns will do: http://pubs.opengroup.org/onlinepubs/7908799/xsh/fsync.html I will try to get some numbers for our revised patches later this weekend (where mine will be changed to drop the rangelock after zil_commit()). Doing precisely 1 rangelock per ->writepages will make our dirty page writeback more like that of Illumos, which does only 1 rangelock per VOP_PUTPAGE call. I suspect that making us consistent with that will yield an improvement solely from the reduction in the asymtotic complexity of ->writepages() from O(N*log(N)) to O(N), where taking a rangelock is O(log(N)). That being said, I suspect that our testing methodologies are different. I used filebench to test this on a ramdisk (using the Linux brd driver). How are you testing this? |
|
@behlendorf I probably should elaborate that the key idea in this patch is to change locking such that each ->writepages() call does precisely 1 rangelock. My main mistake here was in locking too little by dropping the lock before zil_commit(). That is wrong and it is fortunate that you caught it. A nice thing about doing precisely 1 rangelock per ->writepage{s,} call (provided that it is dropped after zil_commit() returns) is that it allows us to eliminate a possible pingpong effect between a thread doing zil_commit() and a thread blocked in WB_SYNC_ALL. In that situation, thread B wakes thread A upon each clear of a writeback bit, thread A takes the CPU and then blocks until B wakes it up at the next page. |
|
@ryao Yes, that's exactly the situation I'm worried about.
Unfortunately, we can't. Extending the range lock as you suggest will introduce a deadlock. The zill_commit() function calls the zfs_get_data() function we may need to acquire the range lock. If it happens to need to take a read lock on the range we're holding a write lock for the system will deadlock. We could perhaps extend the range lock interfaces to handle this case by indicating we already have the lock. But that's going to take us farther away from the upstream code so I'd like to avoid that if possible. The cleanest solution I think is to preserve what was done in the previous code. We call write_cache_pages() a second time with the correct sync mode. I'm going to try this our today and refresh the patch.
I agree with this entirely. Pulling the range lock up a level so there's just one for the entire call writepages() call should be a good thing. For certain workloads I expect it will significantly decrease the amount of contention. The thing is before we merge a change like this we need to quantify how much it helps. So what I'd like to see is at least one specific test case where it's shown to improve performance, or decrease cpu utilization, or improve some other metric. I've been using a mmap() based fio workload described in the commit message against a ramdisk based zpool in a VM. So far my testing was inconclusive so I'll need to refine my process a bit. I'm all for testing other workloads, I noticed you used the filebench randwrite test for your testing. It looked at the briefly but it wasn't clear to me that it used mmap for it's IO so I opted for something else. |
|
I am withdrawing this pull request because benchmark results that I posted in #2413 show that 0.6.3 has the performance characteristics that my past benchmarks had attributed to this patch and a change such as this is ineffective. |
Just to clarify, this means that 0.6.3 performs as well as the previous code did with this patch? |
|
0.6.3 performs as well as the previous code did with whatever happened to be in my local tree. Some patches that I wrote alongside this to fix bugs were merged, which might explain the discrepancy. |
This refactors ->writepages and ->writepage to improve performance.
->writepages has been refactored to call zfs_range_lock() once per call.
Each zfs_range_lock() does its own memory allocation and various tree
operations, so making it occur only once reduces the computational
complexity of writepages, by at least a factor of log(N) where N is the
number of pages. This avoids significant virtual memory contention that
is prevalent in Linux's vmalloc(). In addition, we now pass WB_SYNC_NONE
to write_cache_pages. This moves responsibility for ensuring synchronous
semantics entirely to zil_commit(), where it should have been in the
first place. It also eliminates an opportunity for concurrent threads to
block each other on writeback.
->writepage has been refactored to permit the changes to ->writepages.
Situations where it is told to write out an invalidated page is now
properly handled by ignoring them. This avoids a theoretical NULL
pointer dereference.
These changes substantially improve 32-thread 4K randomwrite IO on a RAM
disk in filebench's randomwrite benchmark. This has been tested with the
following commands on a boot that passed ramdisk_size=8388604 to the
kernel commandline:
modprobe loop
losetup loop0 /dev/ram0
zpool create -o ashift=12 -O recordsize=4K test /dev/loop0
zpool export test
losetup -D
zpool import -d /dev test
filebench << END
load randomwrite
set $dir=/test
set $nthreads=32
set $iosize=4096
run 60
END
zpool destroy test
Previous tests on Gentoo Linux using a kernel compiled from the CentOS
2.6.32-431.11.2.el6.x86_64 kernel with the default .config showed single
threaded performance at roughly 10k to 11k IOPS with 32-thread
performance at 5k-6k in this benchmark. Tests with this patch show a
performance improvement to 15k to 17k in the 32-thread case. These tests
were conducted on a single processor Xeon E5-2620 with 64GB of RAM.
Signed-off-by: Richard Yao ryao@gentoo.org