qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC 0/3] block/file-posix: Work around XFS bug


From: Nir Soffer
Subject: Re: [RFC 0/3] block/file-posix: Work around XFS bug
Date: Sat, 26 Oct 2019 20:37:19 +0300

On Fri, Oct 25, 2019 at 1:11 PM Max Reitz <address@hidden> 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.

Nir

> 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
>
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]