28.10.2019 14:04, Kevin Wolf wrote: > Am 27.10.2019 um 13:35 hat Stefan Hajnoczi geschrieben: >> On Fri, Oct 25, 2019 at 11:58:46AM +0200, Max Reitz wrote: >>> 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?) >> >> Your patch series is reasonable. I don't think it's too bad. >> >> The main question is how to detect the XFS fix once it ships. XFS >> already has a ton of ioctls, so maybe they don't mind adding a >> feature/quirk bit map ioctl for publishing information about bug fixes >> to userspace. I didn't see another obvious way of doing it, maybe a >> mount option that the kernel automatically sets and that gets reported >> to userspace? > > I think the CC list is too short for this question. We should involve > the XFS people here. > >> If we imagine that XFS will not provide a mechanism to detect the >> presence of the fix, then could we ask QEMU package maintainers to >> ./configure --disable-xfs-fallocate-beyond-eof-workaround at some point >> in the future when their distro has been shipping a fixed kernel for a >> while? It's ugly because it doesn't work if the user installs an older >> custom-built kernel on the host. But at least it will cover 98% of >> users... >> >>> (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 >> >> My favorite because it is clean and simple, but Vladimir has a valid >> use-case for requiring this performance optimization so reverting isn't >> an option. > > Vladimir also said that qcow2 subclusters would probably also solve his > problem, so maybe reverting and applying the subcluster patches instead > is a possible solution, too?
I'm not sure about ssd case, it may need write-zero optimization anyway. > > We already have some cases where the existing handle_alloc_space() > causes performance to actually become worse, and serialising requests as > a workaround isn't going to make performance any better. So even on > these grounds, keeping commit c8bb23cbdbe32f is questionable. > Can keeping handle_alloc_space under some config option be an option? -- Best regards, Vladimir
