added true async in zfs filesystem by adding a thin async zpl proxy - #18902
added true async in zfs filesystem by adding a thin async zpl proxy#18902tiehexue wants to merge 3 commits into
Conversation
Not exactly. You are measured "randread IOPS" in 128K size, too big for random... OLTP workloads usually operates 4-16K sizes. With 128K you are measured more bandwidth than IOPS. Here is single PCIe 5.0 NVMe SSD (CM7-V 3.2TB), formatted 4096 bytes for data + 64 bytes for metadata, and encrypted by dm-crypt with LUKS2/AEAD with --integrity-inline option. [root@qdevice ~]# fio --name=test --rw=randread --bs=8k --filename=/dev/mapper/crypt_nvme1 --direct=1 --numjobs=16 --iodepth=1 --exitall --group_reporting --ioengine=psync --runtime=60 --time_based Run status group 0 (all jobs): |
|
Reusing zfs_read_impl() eliminates most of the "forgot one obscure read semantic" bugs. Awesome. In general I like this a lot more, but there's still a lot to do.
The taskq proxy also puts another concurrency scheduler above ZIO. The unbounded queue/global taskq issues are correctness and isolation problems. the extra taskq handoff and worker concurrency cap are the parts I am guessing affect the benchmarks. |
This is another attempt for async which just push zpl_iter_read in a threadpool, then return directly. New tests added, which shows scaling for iodepth. Signed-off-by: tiehexue <tiehexue@hotmail.com>
In async path, pin page before submission, later a IO may fallback to sync path in async thread, however, the context is not for sync. So make sync path use pinned page directly. Signed-off-by: tiehexue <tiehexue@hotmail.com>
Set to 512M as default which should be enough to do benchmark. Signed-off-by: tiehexue <tiehexue@hotmail.com>
aa77bc8 to
7d47dd7
Compare
|
@mjc Thanks for your quick review, I did a force push for further review, keep only one-commit so far. Reusing zfs_read_impl() eliminates most of the "forgot one obscure read semantic" bugs. Awesome. In general I like this a lot more, but there's still a lot to do. zpl_async_read_queue() calls zfs_setup_direct() before dispatch. that ends up pinning the whole userspace DIO buffer. after, it enqueues the op onto a global taskq, that taskq has a fixed number (default 4) of workers, and that queue is allowed to grow unbounded. so that goes from active workers × request size = pinned memory to queued requests × request size = pinned memory. this must be fixed. bounding the taskq after the pages are pinned isn’t enough; admission needs to happen before the pin. the taskq is global and head-of-line blocking. should probably be per-pool. zpl_async_read_task calls zpl_file_accessed even when _impl failed. the module param looks writable (0644) but it is only called by zpl_async_read_init, so runtime changes don't seem to do anything. the benchmark above uses zfs_async_read_task_depth=32, but the code defaults to 4. so we’re not seeing performance for the proposed default. I would benchmark the default too, and probably sweep task depth against actual hardware rather than picking it from the VM result. zfs_async_dio_enabled should default off pre-running zfs_setup_direct means zfs_read_impl skips it whenever UIO_DIRECT is on, so in those cases the setup_direct stuff gets frozen at queue time. this doesn't follow the "same code path for shared things" idea we were discussing. probably prudent to audit everything in zfs_setup_direct and see if all of it is fine with an arbitrarily long queue. The taskq proxy also puts another concurrency scheduler above ZIO. zfs_async_read_task_depth now caps how many requests can actually enter the normal read path at once. That may explain some of the gap you’re seeing: with depth=64 and task depth=32, half the requests can be waiting for a worker rather than being visible to ZIO, while increasing task depth adds scheduler/context-switch overhead. I’d profile this before concluding sync threads and async are equivalent. the extra taskq handoff and worker concurrency cap are the parts I am guessing affect the benchmarks. |
|
@AntonHPE Thanks for your following up. I had created packages include almalinux10 at https://github.com/tiehexue/zfs/releases/tag/w2.5.0 . Let me know if the package can be downloaded and tested in your real hardware. There are three new module parameters:
Below is the latest result in my local vm (8-core, ZTS test pool, raidz on 3 file-backed loop devices). As you pointed out, set record size to 4, the numbers looks good. You may test with more configuration on numjobs, iodepth, recordsize for both sync and async. Just ask AI for a script to automate it :-) 10:17:26.48 NOTE: O_DIRECT randread IOPS (fio libaio, direct=1, bs=4K):
10:17:26.48 NOTE: numjobs iodepth sync async
10:17:26.48 NOTE: 1 1 3997 2590
10:17:26.48 NOTE: 1 8 3720 10381
10:17:26.48 NOTE: 1 32 4036 37832
10:17:26.48 NOTE: 1 64 4902 45770
10:17:26.48 NOTE: 32 1 63417 52118
10:17:26.48 NOTE: 32 8 60809 63827
10:17:26.48 NOTE: 32 32 62200 54644
10:17:26.48 NOTE: 32 64 60072 63324 |
7d47dd7 to
1482f70
Compare
I downloaded the package, but I will be able to run tests on Wednesday and beyond. I'm currently on [root@memverge3 repo]# cat /etc/*release What type of installation should I select, DKMS (epel-testing/10.2/x86_64/) or KMOD (epel-testing/10.2/kmod/x86_64/) ? |
|
so the taskq is no longer global, great - but I meant pool as in SPA/storage resource domain, not one taskq per zfsvfs - zfsvfs is per-dataset I think? zfs_async_read_max_inflight makes sense as an admission gate. it’s per-request, though, and requests can have radically different sizes, so the actual pinned-memory bound is roughly 256 * max accepted request size. probably worth sanity-checking that default. the pin-only split seems not right: submission calls zfs_dio_pin_pages(&aio->uio, UIO_READ); which pins but also clears UIO_DIRECT intentionally -> the worker calls read_impl -> calls zfs_setup_direct which re-runs the eligibility, and can reuse the pre-pinned pages if DIO is still eligible. However, if DIO is not eligible by the time execution is ready, bc cached data appeared, mmap state changed or something similar, then zfs_setup_direct might leave UIO_DIRECT cleared, and then normal read path can continue from a taskq thread using the saved userspace iterator. I think it should decide or fallback before handing execution to a context that cannot safely perform ordinary userspace copies - but really I'm out of my depth there. it could instead find a way to guarantee that completion accesses the pinned pages rather than user VA. I have no idea which approach is correct or if there's an obvious one I'm missing. sync it would still be good to default-off the tunable and have the benchmark turn it on. |
Is it almalinux 10, you test before? You can download, just before you test it. I may update the package for coming changes. |
7b7b599 to
a28c11f
Compare
|
@mjc Thanks for review again. The per-pool, atime, defualt off should be fixed. The zfs_async_read_max_inflight I set to default 1024, even larger than 256, it is for benchmark, so 32 numjobs and 32 iodepth does not fallback to sync. Let me make another commit once we have a better value, or, make both zfs_async_read_max_inflight and zfsvfs->z_async_dio_inflight as data length. For the pin page issue, call zfs_setup_direct before submission, there still a chance that fallback to sync in the thread. That is why I just do the pin only with a new function. "guarantee that completion accesses the pinned pages rather than user VA", I thought this too. Yes, we already make room for the read. If fallback to sync, despite other potential error, the pin is a waste. Let me go on this way, and if good, make another commit. |
b6b46ad to
d21b344
Compare
|
@mjc two new commit for zfs_async_read_max_inflight and pin page issue. Now zfs_async_read_max_inflight is bytes total and checked per dataset. This could be "prompted" per pool, or a property of pool or dataset. For pinned page, now if dio is declined, the pinned page is still used, so no user VA touched in async context. |
d21b344 to
d6918a6
Compare
|
@AntonHPE checkout latest build before you test at https://github.com/tiehexue/zfs/releases . Now, zfs_async_read_max_inflight is default to 512M, as total inflight request size. An IO is fallback to sync path if inflight is bigger than zfs_async_read_max_inflight. This valus should be large enough for benchmark. |
|
This is my first comment here, so briefly: I worked through the crash-class bugs The pinned-page approach came out of @mjc's 08-08 review and the implementation Rig: Linux 6.8.0-137 guest, The pinned-page copy fixes the non-page-aligned EOF tailI had not raised this one before, and it is already fixed by Ten reps per condition per row, async alternated with sync:
The The eligibility decision is still taken on freed memoryThe
The middle row is the control: the identical iovec, evaluated in the submitting if (!zfs_uio_page_aligned(uio) ||
!zfs_uio_aligned(uio, PAGE_SIZE)) {
/*
* Misaligned requests can be executed through the ARC as
* uncached I/O. But if O_DIRECT was set by user and we
* were set to be strict, then it is a failure.
*/
if ((*ioflagp & O_DIRECT) && zfs_dio_strict)
error = SET_ERROR(EINVAL);
goto out;
}
struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;
...
ret = aio_setup_rw(ITER_DEST, iocb, &iovec, vectored, compat, &iter);
if (ret < 0)
return ret;
ret = rw_verify_area(READ, file, &req->ki_pos, iov_iter_count(&iter));
if (!ret)
aio_rw_done(req, call_read_iter(file, req, &iter));
kfree(iovec);The single-segment shape is the control for that: The comment in * The iov_iter passed by libaio lives on the io_submit() stack; copy
* it by value and point the uio at our copy. The backing iovec array
* is kept valid by the aio/io_uring request until ki_complete().For libaio the second sentence does not hold, which is what the numbers are Two consequences, both separate from the corruption that is now fixed. With To check that this really is the cause rather than something correlated with it, if (uio->uio_dio.pages == NULL &&
(!zfs_uio_page_aligned(uio) ||
!zfs_uio_aligned(uio, PAGE_SIZE))) {That one change takes all three conditions to clean, 25 trials each:
13 of 25 and 14 of 25 both go to 0 of 25. On the two async rows the Direct I/O Nothing else changes. I am offering this as evidence about the cause rather than as a patch, though the The teardown drain deadlocks, and the broken precondition is mineThe
On the async read path both halves of that are now false. The middle stanza is the submitting process, parked in I do not think reordering the two calls is enough on its own, which is why I am One thing worth flagging about how reachable this is. The atime update only runs Why the direct and async groups pass on this buildI ran both groups against the stock head on the same rig: all 22 tests pass, 28 The reason looks to be request shape. The fio read invocations in Two cheap additions would close it, if you think it is worth covering. A vectored The teardown cycle is not covered either. Both groups do export the pool, Two smaller thingsYou already said the in-flight accounting is per dataset. Two follow-ons. The The pinned-page copy is not limited to the async path. It triggers on OverlapMy #18844 adds a flag on the file handle after a benign Direct I/O verify failure |
|
@mkhllr thanks! I am not surprised you are here :-) Give me some time to consolidate your comments. Would you like to provide a patch, especially test cases which bring bugs? |
Motivation and Context
Long story short, this PR is another way to add true async to zfs. There are a lot discussion in #18684 , refer to it for more background.
Description
Now we have three PR for true async IO in zfs, #18684 is the deepest one which relies on zio_nowait, #10377 is in the middle which try to build async dmu, now this PR is on the top. It just adds a new zpl_iter_read_async to replace zpl_iter_read in zpl_file_operations as .read_iter.
Two module parameter added: zfs_async_dio_enabled and zfs_async_read_task_depth. The first one is a switch to enable or disable async, once enabled, the second one defined how many threads will be created as async context. The first one could be merge to the second one if necessary.
If it is a async IO from libaio/io_uring and also direct IO, async path is active, the request is submit to the taskq, and all code path are same with sync one, except ki_complete must be called. So from the data integrity view, it is same as sync path.
Most of code changes comes from calling zfs_setup_direct which pins user user pages before submit to taskq. Other code are self-explained.
Honestly, I do not like this way. It is like a fake. However, the other two ways have much more problems. The deepest one have to make zpl, vfs, dmu code into two parts, ones before zio_nowait(root zio), the other called in callbacks, which not easy to ensure data integrity, even for read. The middle one is not possible too, dmu is already complex enough. So this PR is the only way I think possible.
And again, as I learned from the community, using fio as an example, numjobs=16 iodepth=1 in sync VS. numjobs=1 iodepth=16 in async, the IOPS are comparable. And in the newly added test, I run a numjobs*iodepth matrix, got following nubmers (A 8-core ubuntu26 vm with zfs_async_read_task_depth 32):
This result said to me "why do you want async, just add more threads in application"?
Anyway, I put this PR here, because I believed "true async" should be valuable, and later I may try async write in another commit or PR.
And one more thing, this PR does bring "overhead" for async infrastructure, but the IOPS improvement nearly same as #18684 which just using zio_nowait. I also tried a unbounded lockless per-worker stack in https://github.com/tiehexue/zfs/tree/async_zpl_lockless_stack , I did not see distinct performance up which I hope to see. All test are run in vm with loop devices.
How Has This Been Tested?
Test locally and in personal fork CI
Types of Changes
Checklist
Signed-off-by.