26.10.2019 20:37, Nir Soffer wrote: > On Fri, Oct 25, 2019 at 1:11 PM Max Reitz <mre...@redhat.com> wrote: >> >> Hi, >> >> It seems to me that there is a bug in Linux’s XFS kernel driver, as >> I’ve explained here: >> >> https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg01429.html >> >> In combination with our commit c8bb23cbdbe32f, this may lead to guest >> data corruption when using qcow2 images on XFS with aio=native. >> >> We can’t wait until the XFS kernel driver is fixed, we should work >> around the problem ourselves. >> >> This is an RFC for two reasons: >> (1) I don’t know whether this is the right way to address the issue, >> (2) Ideally, we should detect whether the XFS kernel driver is fixed and >> if so stop applying the workaround. >> I don’t know how we would go about this, so this series doesn’t do >> it. (Hence it’s an RFC.) >> (3) Perhaps it’s a bit of a layering violation to let the file-posix >> driver access and modify a BdrvTrackedRequest object. >> >> As for how we can address the issue, I see three ways: >> (1) The one presented in this series: On XFS with aio=native, we extend >> tracked requests for post-EOF fallocate() calls (i.e., write-zero >> operations) to reach until infinity (INT64_MAX in practice), mark >> them serializing and wait for other conflicting requests. >> >> Advantages: >> + Limits the impact to very specific cases >> (And that means it wouldn’t hurt too much to keep this workaround >> even when the XFS driver has been fixed) >> + Works around the bug where it happens, namely in file-posix >> >> Disadvantages: >> - A bit complex >> - A bit of a layering violation (should file-posix have access to >> tracked requests?) >> >> (2) Always skip qcow2’s handle_alloc_space() on XFS. The XFS bug only >> becomes visible due to that function: I don’t think qcow2 writes >> zeroes in any other I/O path, and raw images are fixed in size so >> post-EOF writes won’t happen. >> >> Advantages: >> + Maybe simpler, depending on how difficult it is to handle the >> layering violation >> + Also fixes the performance problem of handle_alloc_space() being >> slow on ppc64+XFS. >> >> Disadvantages: >> - Huge layering violation because qcow2 would need to know whether >> the image is stored on XFS or not. >> - We’d definitely want to skip this workaround when the XFS driver >> has been fixed, so we need some method to find out whether it has >> >> (3) Drop handle_alloc_space(), i.e. revert c8bb23cbdbe32f. >> To my knowledge I’m the only one who has provided any benchmarks for >> this commit, and even then I was a bit skeptical because it performs >> well in some cases and bad in others. I concluded that it’s >> probably worth it because the “some cases” are more likely to occur. >> >> Now we have this problem of corruption here (granted due to a bug in >> the XFS driver), and another report of massively degraded >> performance on ppc64 >> (https://bugzilla.redhat.com/show_bug.cgi?id=1745823 – sorry, a >> private BZ; I hate that :-/ The report is about 40 % worse >> performance for an in-guest fio write benchmark.) >> >> So I have to ask the question about what the justification for >> keeping c8bb23cbdbe32f is. How much does performance increase with >> it actually? (On non-(ppc64+XFS) machines, obviously) >> >> Advantages: >> + Trivial >> + No layering violations >> + We wouldn’t need to keep track of whether the kernel bug has been >> fixed or not >> + Fixes the ppc64+XFS performance problem >> >> Disadvantages: >> - Reverts cluster allocation performance to pre-c8bb23cbdbe32f >> levels, whatever that means > > Correctness is more important than performance, so this is my > preference as a user. >
Hmm, still, incorrect is XFS, not Qemu. This bug may be triggered by another software, or may be another scenario in Qemu (not sure). > >> So this is the main reason this is an RFC: What should we do? Is (1) >> really the best choice? >> >> >> In any case, I’ve ran the test case I showed in >> https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg01282.html >> more than ten times with this series applied and the installation >> succeeded every time. (Without this series, it fails like every other >> time.) >> >> >> Max Reitz (3): >> block: Make wait/mark serialising requests public >> block/file-posix: Detect XFS with CONFIG_FALLOCATE >> block/file-posix: Let post-EOF fallocate serialize >> >> include/block/block_int.h | 3 +++ >> block/file-posix.c | 46 +++++++++++++++++++++++++++++++++++++-- >> block/io.c | 24 ++++++++++---------- >> 3 files changed, 59 insertions(+), 14 deletions(-) >> >> -- >> 2.21.0 >> >> -- Best regards, Vladimir