On 01.11.19 14:30, Max Reitz wrote: [...]
> So unless there are realistic guest benchmarks for ext4 that say we > should keep the patch, I’m going to queue the revert for now (“now” = > 4.1.1 and 4.2.0). I found one case where the performance is significantly improved by c8bb23cbdbe: In the cases so far I had XFS in the guest, now I used ext4, and with aio=native (on the ext4 host with 2 MB clusters), the performance goes from 63.9 - 65.0 MB/s to 75.7 - 76.4 MB/s (so +18%). The difference is smaller for 64 kB clusters, but still there at +13%. That’s probably the more important fact, because these are the default settings, and this is probably about what would happen with 2 MB clusters + subclusters. (Patch 4 in this series doesn’t decrease the performance.) This is a tough decision for me because from some people tell me “Let’s just revert it, there are problems with it and we don’t quite know what good it does in practice”, and others say “We have (not really practical) benchmarks that show it does something good for our specific case”. And all that while those two groups never seem to talk to each other directly. So I suppose I’m going to have to make a decision. I now know a case where c8bb23cbdbe is actually beneficial. I myself have never seen c8bb23cbdbe decrease performance, but I know Laurent has seen a drastic performance degradation, and he’s used it to bisect the XFS problem to that commit, so it’s really real. But I haven’t seen it, and as far as I know it really only happens on ppc64. In light of this, I would prefer to revert c8bb23cbdbe for 4.1.1, and keep it for 4.2.0. But I don’t know whether we can do that, all I know is that I’m not going to find out in time for 4.1.1. If we keep c8bb23cbdbe in any way, we need patches 2 through 4, that much is clear. I believe we can think about the performance problem after 4.2.0. I would love to benchmark c8bb23cbdbe on a fixed kernel, but there just isn’t time for that anymore. I’m not a fan of keeping c8bb23cbdbe behind a configure switch. If it’s beneficial, it should be there for everyone. OK. Some may see this as a wrong decision, but someone needs to make one now, so here goes: ext4 is the default Linux FS for most distributions. As far as I can tell from my own benchmarks, c8bb23cbdbe brings a significant performance improvement for qcow2 images with the default configuration on this default filesystem with aio=native and doesn’t change much in any other case. What happens on ppc64 is a problem, but that’s a RHEL problem because it’s specific to XFS (and also ppc64). It also won’t be a regression on 4.2 compared to 4.1. Dave’s argument was good that fallocate() and AIO cannot mix (at least on XFS), but I couldn’t see any impact of that in my benchmarks (maybe my benchmarks were just wrong). So I think for upstream it’ll be best if I send a v2 which doesn’t touch handle_alloc_space(), and instead just consists of patches 2 through 4. (And CC it all to stable.) I think we still need to keep track of the XFS/ppc64 issue and do more benchmarks especially with the fixed XFS driver. tl;dr: The main arguments for reverting c8bb23cbdbe were (AFAIU): - a general uneasy feeling about it - theoretical arguments that it must be bad on XFS - actual problems on ppc64/XFS - “what good does it do in practice?” - that subclusters would make it obsolete anyway What I could see is: - no impact on XFS in practice - significant practical benefit on ext4 - subclusters probably wouldn’t make it obsolete, because I can still see a +13% improvement for 64 kB clusters (2 MB clusters + subclusters gives you 64 kB subclusters) In addition, it needs to be considered that ext4 is the default FS for most Linux distributions. As such, I personally am not convinced of reverting this patch. Let’s keep it, have patches 2 through 4 for 4.1.1 and 4.2.0, and think about what to do for ppc64/XFS later. Max
signature.asc
Description: OpenPGP digital signature
