[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC 0/3] block/file-posix: Work around XFS bug
From: |
Kevin Wolf |
Subject: |
Re: [RFC 0/3] block/file-posix: Work around XFS bug |
Date: |
Fri, 25 Oct 2019 16:35:18 +0200 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
Am 25.10.2019 um 16:19 hat Max Reitz geschrieben:
> On 25.10.19 15:56, Vladimir Sementsov-Ogievskiy wrote:
> > 25.10.2019 16:40, Vladimir Sementsov-Ogievskiy wrote:
> >> 25.10.2019 12:58, Max Reitz 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
> >>>
> >>> 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.)
> >>>
> >>>
> >>
> >> Hi!
> >>
> >> First, great thanks for your investigation!
> >>
> >> We need c8bb23cbdbe3 patch, because we use 1M clusters, and zeroing 1M is
> >> significant
> >> in time.
> >>
> >> I've tested a bit:
> >>
> >> test:
> >> for img in /ssd/test.img /test.img; do for cl in 64K 1M; do for step in 4K
> >> 64K 1M; do ./qemu-img create -f qcow2 -o cluster_size=$cl $img 15G >
> >> /dev/null; printf '%-15s%-7s%-10s : ' $img cl=$cl step=$step; ./qemu-img
> >> bench -c $((15 * 1024)) -n -s 4K -S $step -t none -w $img | tail -1 | awk
> >> '{print $4}'; done; done; done
> >>
> >> on master:
> >>
> >> /ssd/test.img cl=64K step=4K : 0.291
> >> /ssd/test.img cl=64K step=64K : 0.813
> >> /ssd/test.img cl=64K step=1M : 2.799
> >> /ssd/test.img cl=1M step=4K : 0.217
> >> /ssd/test.img cl=1M step=64K : 0.332
> >> /ssd/test.img cl=1M step=1M : 0.685
> >> /test.img cl=64K step=4K : 1.751
> >> /test.img cl=64K step=64K : 14.811
> >> /test.img cl=64K step=1M : 18.321
> >> /test.img cl=1M step=4K : 0.759
> >> /test.img cl=1M step=64K : 13.574
> >> /test.img cl=1M step=1M : 28.970
> >>
> >> rerun on master:
> >>
> >> /ssd/test.img cl=64K step=4K : 0.295
> >> /ssd/test.img cl=64K step=64K : 0.803
> >> /ssd/test.img cl=64K step=1M : 2.921
> >> /ssd/test.img cl=1M step=4K : 0.233
> >> /ssd/test.img cl=1M step=64K : 0.321
> >> /ssd/test.img cl=1M step=1M : 0.762
> >> /test.img cl=64K step=4K : 1.873
> >> /test.img cl=64K step=64K : 15.621
> >> /test.img cl=64K step=1M : 18.428
> >> /test.img cl=1M step=4K : 0.883
> >> /test.img cl=1M step=64K : 13.484
> >> /test.img cl=1M step=1M : 26.244
> >>
> >>
> >> on master + revert c8bb23cbdbe32f5c326
> >>
> >> /ssd/test.img cl=64K step=4K : 0.395
> >> /ssd/test.img cl=64K step=64K : 4.231
> >> /ssd/test.img cl=64K step=1M : 5.598
> >> /ssd/test.img cl=1M step=4K : 0.352
> >> /ssd/test.img cl=1M step=64K : 2.519
> >> /ssd/test.img cl=1M step=1M : 38.919
> >> /test.img cl=64K step=4K : 1.758
> >> /test.img cl=64K step=64K : 9.838
> >> /test.img cl=64K step=1M : 13.384
> >> /test.img cl=1M step=4K : 1.849
> >> /test.img cl=1M step=64K : 19.405
> >> /test.img cl=1M step=1M : 157.090
> >>
> >> rerun:
> >>
> >> /ssd/test.img cl=64K step=4K : 0.407
> >> /ssd/test.img cl=64K step=64K : 3.325
> >> /ssd/test.img cl=64K step=1M : 5.641
> >> /ssd/test.img cl=1M step=4K : 0.346
> >> /ssd/test.img cl=1M step=64K : 2.583
> >> /ssd/test.img cl=1M step=1M : 39.692
> >> /test.img cl=64K step=4K : 1.727
> >> /test.img cl=64K step=64K : 10.058
> >> /test.img cl=64K step=1M : 13.441
> >> /test.img cl=1M step=4K : 1.926
> >> /test.img cl=1M step=64K : 19.738
> >> /test.img cl=1M step=1M : 158.268
> >>
> >>
> >> So, it's obvious that c8bb23cbdbe32f5c326 is significant for 1M
> >> cluster-size, even on rotational
> >> disk, which means that previous assumption about calling
> >> handle_alloc_space() only for ssd is
> >> wrong, we need smarter heuristics..
> >>
> >> So, I'd prefer (1) or (2).
>
> OK. I wonder whether that problem would go away with Berto’s subcluster
> series, though.
>
> > About degradation in some cases: I think the problem is that one (a bit
> > larger)
> > write may be faster than fast-write-zeroes + small write, as the latter
> > means
> > additional write to metadata. And it's expected for small clusters in
> > conjunction with rotational disk. But the actual limit is dependent on
> > specific
> > disk. So, I think possible solution is just sometimes try work with
> > handle_alloc_space and sometimes without, remember time and length of
> > request
> > and make dynamic limit...
>
> Maybe make a decision based both on the ratio of data size to COW area
> length (only invoke handle_alloc_space() under a certain threshold), and
> the absolute COW area length (always invoke it above a certain
> threshold, unless the ratio doesn’t allow it)?
I'm not sure that I would like this level of complexity in this code
path...
Kevin
signature.asc
Description: PGP signature
- Re: [RFC 2/3] block/file-posix: Detect XFS with CONFIG_FALLOCATE, (continued)
Re: [RFC 2/3] block/file-posix: Detect XFS with CONFIG_FALLOCATE, Nir Soffer, 2019/10/26
[RFC 3/3] block/file-posix: Let post-EOF fallocate serialize, Max Reitz, 2019/10/25
Re: [RFC 0/3] block/file-posix: Work around XFS bug, Vladimir Sementsov-Ogievskiy, 2019/10/25
Re: [RFC 0/3] block/file-posix: Work around XFS bug, Peter Maydell, 2019/10/25
Re: [RFC 0/3] block/file-posix: Work around XFS bug, no-reply, 2019/10/25
Re: [RFC 0/3] block/file-posix: Work around XFS bug, Nir Soffer, 2019/10/26