improve batching done in zil_commit() #6337
Conversation
|
It would be great if you could update the commit message comment to reference the SPL PR. With that hint the buildbot will be able to build and test this PR. [edit] Oh you did! Nevermind! |
|
yup, I added that directive. I'm not quite sure how to interpret the test failures yet (although I haven't spent much time trying); is there any documentation to help me out with that? |
|
The console and ztest logs are my favorite place to start if the test suite doesn't run to completion. We're currently not collecting cores but stack trace is often more than enough. We've add a little bit of documention describing how to run the same tests locally but it'd be nice to add more. |
|
perfect, thanks! |
|
Any clues about these build failures: ? I don't see these when I build locally, and I didn't modify the |
module/zfs/zio.c
Outdated
| ASSERT(zio->io_orig_size == zio->io_size); | ||
| ASSERT(size <= zio->io_size); | ||
| ASSERT3P(zio->io_executor, ==, NULL); | ||
| ASSERT3P(zio->io_orig_size, ==, zio->io_size); |
There was a problem hiding this comment.
This should be ASSERT3U. The build failure you're seeing is coming from 32-bit system where the pointer size is 32-bit and not 64-bit. The ASSERT3P macro is forcing a cast of a uint64_t to a uintptr_t which is only 32-bit on failing architectures. It looks like ASSERT3P is used incorrectly in a zio_wait() too and maybe elsewhere.
There was a problem hiding this comment.
thanks! I should have caught that myself, sorry for the trouble.
This is a first attempt at porting the OpenZFS pull request here: - openzfs/openzfs#421 This hasn't seen much testing apart from verifying it compiles in an Ubuntu VM, and it doesn't panic when using a very light sync write workload using fio. I'll have to rely on the automated testing and volunteers to verify the correctness and performance; if needed, I can try to allocate some time to do this myself, but I doubt I'll have the time to do that in the near future. One big difference between this version, and the version posted to OpenZFS, is how the lifetime of an `itx_t` structure differs between the codebases. In this Linux port, the `itx_t` structure maintains an `itx_callback` field, and this function needs to be called after the itx has been committed to disk. Thus, the `itx_t` must wait to be destroyed until it's been committed to disk. This is in contrast to how the `itx_t` is immediately destroyed once it's committed to an lwb in the OpenZFS codebase, but prior to the lwb being committed to disk. To adapt the new algorithm to this `itx_callback` requirement, an `lwb_itxs` list was added to the `lwb_t` structure. Each time an itx is committed to an lwb, it is added to the lwb's `lwb_itxs` list. This list is used to keep track of the itxs such that their callback can be called after the itx is safe on disk, and only then will the itx be destroyed. Additionally, this attempts to address the same deficiencies that Richard Yao is addressing in the following pull requests: - #6133 - openzfs/openzfs#404 I'd be great to perform some head-to-head comparisons of this with Richard's work, and see which parts of each design work best. Signed-off-by: Prakash Surya <prakash.surya@delphix.com>
|
@behlendorf this is causing a hang when running the do you have any clues as to what might be going on? I'm not familiar with the mmap code, is there linux specific changes that I may have broken in this patch? I'll have to do some reading to better understand how FWIW, I can run the |
I'm not sure of the detail but I've got a good guess where to start looking. The stack you posted is what I'd expect to happen if for some reason the I haven't had a chance to carefully go over the patch but is it possible there's a case which was missed where the callback doesn't run? For background there's a nice description in commit 119a394 regarding why this was needed for Linux. |
|
Thanks, Brian. That's basically what I was thinking too, so it's good to get a second opinion. I'll revisit the code and see if botched something. OpenZFS doesn't have the "itx callback", so I wouldn't be surprised if I got that subtly wrong when pulling this code over to linux. |
This is a first attempt at porting the OpenZFS pull request here:
This hasn't seen much testing apart from verifying it compiles in an
Ubuntu VM, and it doesn't panic when using a very light sync write
workload using fio. I'll have to rely on the automated testing and
volunteers to verify the correctness and performance; if needed, I can
try to allocate some time to do this myself, but I doubt I'll have the
time to do that in the near future.
One big difference between this version, and the version posted to
OpenZFS, is how the lifetime of an
itx_tstructure differs between thecodebases. In this Linux port, the
itx_tstructure maintains anitx_callbackfield, and this function needs to be called after the itxhas been committed to disk. Thus, the
itx_tmust wait to be destroyeduntil it's been committed to disk. This is in contrast to how the
itx_tis immediately destroyed once it's committed to an lwb in theOpenZFS codebase, but prior to the lwb being committed to disk.
To adapt the new algorithm to this
itx_callbackrequirement, anlwb_itxslist was added to thelwb_tstructure. Each time an itx iscommitted to an lwb, it is added to the lwb's
lwb_itxslist. This listis used to keep track of the itxs such that their callback can be called
after the itx is safe on disk, and only then will the itx be destroyed.
Additionally, this attempts to address the same deficiencies that
Richard Yao is addressing in the following pull requests:
I'd be great to perform some head-to-head comparisons of this with
Richard's work, and see which parts of each design work best.
Signed-off-by: Prakash Surya prakash.surya@delphix.com
Types of changes
Checklist:
Signed-off-by.