| Message ID | cover.1614090658.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] Kmap conversions for 5.12 | expand |
On Tue, Feb 23, 2021 at 7:03 AM David Sterba <dsterba@suse.com> wrote: > Ira Weiny (8): > iov_iter: Remove memzero_page() in favor of zero_user() Ugh. I absolutely _detest_ this patch. "zero_user()" is a completely horrendous function, and not at all the same as memzero_page(). Just look at it. Yes, it's mis-used in a lot of places that really always wanted "memzero_page()", but this conversion is going exactly the wrong way around. Existing users of that zero_user() should have been converted to memzero_page(), rather than doing it this way. The "user" naming should have given it away. It's a very very magical interface for user-mapped pages that have additional odd issues (ie look at the dcache flushing etc). I'll think some more about this pull request, but honestly, this one broken is pretty much enough for me to say "No way in hell", because it shows a complete disregard for sanity. The last commit in the series: > btrfs: convert to zero_user() is also very mixed up about whether it actually wants the dcache flushing or not (and thus zero_user() or memzero_page()). Honestly, I suspect all the dcache flushing is totally pointless, because any architecture with virtual caches that does kmap needs to flush at kunmap anyway, afaik. Some of it is probably just voodoo programming and copying a pattern. But in any case, zero_user() is not the same thing as memzero_page(), and even if they *were* the same thing, zero_user() is objectively the horribly much worse name. Linus
On Tue, Feb 23, 2021 at 09:13:42AM -0800, Linus Torvalds wrote: > On Tue, Feb 23, 2021 at 7:03 AM David Sterba <dsterba@suse.com> wrote: > > Ira Weiny (8): > > iov_iter: Remove memzero_page() in favor of zero_user() > > Ugh. I absolutely _detest_ this patch. Sorry. > > "zero_user()" is a completely horrendous function, and not at all the > same as memzero_page(). > > Just look at it. > > Yes, it's mis-used in a lot of places that really always wanted > "memzero_page()", but this conversion is going exactly the wrong way > around. Originally I lifted memzero_page()[1] but was pointed to zero_user_segments() which lead me astray to use zero_user(). I should have thought about it more rather than blindly changing to zero_user(). > > Existing users of that zero_user() should have been converted to > memzero_page(), rather than doing it this way. I can do that. No Problem. > > The "user" naming should have given it away. It's a very very magical > interface for user-mapped pages that have additional odd issues (ie > look at the dcache flushing etc). Agreed. > > I'll think some more about this pull request, but honestly, this one > broken is pretty much enough for me to say "No way in hell", because > it shows a complete disregard for sanity. Can we just drop the zero_user() patches? Christoph and others would like to see memcpy_[to|from]_page() lifted to the core for other work which is pending. Would you agree to those? > > The last commit in the series: > > > btrfs: convert to zero_user() > > is also very mixed up about whether it actually wants the dcache > flushing or not (and thus zero_user() or memzero_page()). Drop this patch too? > > Honestly, I suspect all the dcache flushing is totally pointless, > because any architecture with virtual caches that does kmap needs to > flush at kunmap anyway, afaik. Some of it is probably just voodoo > programming and copying a pattern. > > But in any case, zero_user() is not the same thing as memzero_page(), > and even if they *were* the same thing, zero_user() is objectively the > horribly much worse name. Sorry. I will change it. Ira > > Linus [1] https://lore.kernel.org/lkml/20201124141941.GB4327@casper.infradead.org/#t
On Tue, Feb 23, 2021 at 11:25:06AM -0800, Ira Weiny wrote: > On Tue, Feb 23, 2021 at 09:13:42AM -0800, Linus Torvalds wrote: > > On Tue, Feb 23, 2021 at 7:03 AM David Sterba <dsterba@suse.com> wrote: [...] > Sorry. I will change it. Let me know how you want to proceed with the patchset/pull request. I can play the messenger again but now it seems a round of review is needed and with some testing it'll be possible in some -rc. At that point you may take the patches via the mm tree, unless Linus is ok with a late pull.
On Wed, Feb 24, 2021 at 01:30:49PM +0100, David Sterba wrote: > On Tue, Feb 23, 2021 at 11:25:06AM -0800, Ira Weiny wrote: > > On Tue, Feb 23, 2021 at 09:13:42AM -0800, Linus Torvalds wrote: > > > On Tue, Feb 23, 2021 at 7:03 AM David Sterba <dsterba@suse.com> wrote: > [...] > > > Sorry. I will change it. > > Let me know how you want to proceed with the patchset/pull request. To be clear I'd like to just drop the 2 patches which use zero_user() for this merge window. I've already submitted some additional btrfs changes for 5.13[1]. I can rework these zero_user() patches and submit them through Andrew for 5.13 as separate set. That is what I meant by 'I will change it'. > I > can play the messenger again but now it seems a round of review is > needed and with some testing it'll be possible in some -rc. At that > point you may take the patches via the mm tree, unless Linus is ok with > a late pull. I'm ok with delaying the memzero_page() change to 5.13. There are a lot of kmap changes to come. But I'm trying to do them as smaller series just for this reason. I don't want valid changes to be denied due to my messing up just a few patches... :-( Hopefully you and Linus can forgive me on this one. Is ok to just drop them and merge the rest of this series in 5.12? Ira [1] https://lore.kernel.org/lkml/20210217024826.3466046-1-ira.weiny@intel.com/
On Wed, Feb 24, 2021 at 9:59 AM Ira Weiny <ira.weiny@intel.com> wrote: > > To be clear I'd like to just drop the 2 patches which use zero_user() for this > merge window. > > Is ok to just drop them and merge the rest of this series in 5.12? Ack, that sounds like the best way forward. Linus
On Wed, Feb 24, 2021 at 09:59:12AM -0800, Ira Weiny wrote: > On Wed, Feb 24, 2021 at 01:30:49PM +0100, David Sterba wrote: > > On Tue, Feb 23, 2021 at 11:25:06AM -0800, Ira Weiny wrote: > > > On Tue, Feb 23, 2021 at 09:13:42AM -0800, Linus Torvalds wrote: > > > > On Tue, Feb 23, 2021 at 7:03 AM David Sterba <dsterba@suse.com> wrote: > > [...] > > > > > Sorry. I will change it. > > > > Let me know how you want to proceed with the patchset/pull request. > > To be clear I'd like to just drop the 2 patches which use zero_user() for this > merge window. > > I've already submitted some additional btrfs changes for 5.13[1]. I can rework > these zero_user() patches and submit them through Andrew for 5.13 as separate > set. That is what I meant by 'I will change it'. > > > I > > can play the messenger again but now it seems a round of review is > > needed and with some testing it'll be possible in some -rc. At that > > point you may take the patches via the mm tree, unless Linus is ok with > > a late pull. > > I'm ok with delaying the memzero_page() change to 5.13. There are a lot of > kmap changes to come. But I'm trying to do them as smaller series just for > this reason. I don't want valid changes to be denied due to my messing up just > a few patches... :-( Hopefully you and Linus can forgive me on this one. > > Is ok to just drop them and merge the rest of this series in 5.12? Ok, no problem. Please let me know exactly which patches to drop, I'll respin the branch. Thanks.
On Thu, Feb 25, 2021 at 02:12:52PM +0100, David Sterba wrote: > On Wed, Feb 24, 2021 at 09:59:12AM -0800, Ira Weiny wrote: > > On Wed, Feb 24, 2021 at 01:30:49PM +0100, David Sterba wrote: > > > On Tue, Feb 23, 2021 at 11:25:06AM -0800, Ira Weiny wrote: > > > > On Tue, Feb 23, 2021 at 09:13:42AM -0800, Linus Torvalds wrote: > > > > > On Tue, Feb 23, 2021 at 7:03 AM David Sterba <dsterba@suse.com> wrote: > > > [...] > > > > > > > Sorry. I will change it. > > > > > > Let me know how you want to proceed with the patchset/pull request. > > > > To be clear I'd like to just drop the 2 patches which use zero_user() for this > > merge window. > > > > I've already submitted some additional btrfs changes for 5.13[1]. I can rework > > these zero_user() patches and submit them through Andrew for 5.13 as separate > > set. That is what I meant by 'I will change it'. > > > > > I > > > can play the messenger again but now it seems a round of review is > > > needed and with some testing it'll be possible in some -rc. At that > > > point you may take the patches via the mm tree, unless Linus is ok with > > > a late pull. > > > > I'm ok with delaying the memzero_page() change to 5.13. There are a lot of > > kmap changes to come. But I'm trying to do them as smaller series just for > > this reason. I don't want valid changes to be denied due to my messing up just > > a few patches... :-( Hopefully you and Linus can forgive me on this one. > > > > Is ok to just drop them and merge the rest of this series in 5.12? > > Ok, no problem. Please let me know exactly which patches to drop, I'll > respin the branch. Thanks. Drop These 2: [PATCH V2 5/8] iov_iter: Remove memzero_page() in favor of zero_user() https://lore.kernel.org/lkml/20210210062221.3023586-6-ira.weiny@intel.com/ [PATCH V2 8/8] btrfs: convert to zero_user() https://lore.kernel.org/lkml/20210210062221.3023586-9-ira.weiny@intel.com/ Keep: [PATCH V2 1/8] mm/highmem: Lift memcpy_[to|from]_page to core [PATCH V2 2/8] mm/highmem: Convert memcpy_[to|from]_page() to kmap_local_page() [PATCH V2 3/8] mm/highmem: Introduce memcpy_page(), memmove_page(), and memset_page() [PATCH V2 4/8] mm/highmem: Add VM_BUG_ON() to mem*_page() calls ... [PATCH V2 6/8] btrfs: use memcpy_[to|from]_page() and kmap_local_page() [PATCH V2 7/8] btrfs: use copy_highpage() instead of 2 kmaps() ... I would resend but I'd rather keep the exact commits you had in your testing rather than potentially messing up the rebase this late. Thank you and sorry for the extra work, Ira
On Thu, Feb 25, 2021 at 08:32:34AM -0800, Ira Weiny wrote: > On Thu, Feb 25, 2021 at 02:12:52PM +0100, David Sterba wrote: > > On Wed, Feb 24, 2021 at 09:59:12AM -0800, Ira Weiny wrote: > > > On Wed, Feb 24, 2021 at 01:30:49PM +0100, David Sterba wrote: > > > > On Tue, Feb 23, 2021 at 11:25:06AM -0800, Ira Weiny wrote: > > > > > On Tue, Feb 23, 2021 at 09:13:42AM -0800, Linus Torvalds wrote: > > > > > > On Tue, Feb 23, 2021 at 7:03 AM David Sterba <dsterba@suse.com> wrote: > > > > [...] > > > > > > > > > Sorry. I will change it. > > > > > > > > Let me know how you want to proceed with the patchset/pull request. > > > > > > To be clear I'd like to just drop the 2 patches which use zero_user() for this > > > merge window. > > > > > > I've already submitted some additional btrfs changes for 5.13[1]. I can rework > > > these zero_user() patches and submit them through Andrew for 5.13 as separate > > > set. That is what I meant by 'I will change it'. > > > > > > > I > > > > can play the messenger again but now it seems a round of review is > > > > needed and with some testing it'll be possible in some -rc. At that > > > > point you may take the patches via the mm tree, unless Linus is ok with > > > > a late pull. > > > > > > I'm ok with delaying the memzero_page() change to 5.13. There are a lot of > > > kmap changes to come. But I'm trying to do them as smaller series just for > > > this reason. I don't want valid changes to be denied due to my messing up just > > > a few patches... :-( Hopefully you and Linus can forgive me on this one. > > > > > > Is ok to just drop them and merge the rest of this series in 5.12? > > > > Ok, no problem. Please let me know exactly which patches to drop, I'll > > respin the branch. Thanks. > > Drop These 2: > > [PATCH V2 5/8] iov_iter: Remove memzero_page() in favor of zero_user() > https://lore.kernel.org/lkml/20210210062221.3023586-6-ira.weiny@intel.com/ > > [PATCH V2 8/8] btrfs: convert to zero_user() > https://lore.kernel.org/lkml/20210210062221.3023586-9-ira.weiny@intel.com/ > > > Keep: > > [PATCH V2 1/8] mm/highmem: Lift memcpy_[to|from]_page to core > [PATCH V2 2/8] mm/highmem: Convert memcpy_[to|from]_page() to kmap_local_page() > [PATCH V2 3/8] mm/highmem: Introduce memcpy_page(), memmove_page(), and memset_page() > [PATCH V2 4/8] mm/highmem: Add VM_BUG_ON() to mem*_page() calls > ... > [PATCH V2 6/8] btrfs: use memcpy_[to|from]_page() and kmap_local_page() > [PATCH V2 7/8] btrfs: use copy_highpage() instead of 2 kmaps() > ... > > I would resend but I'd rather keep the exact commits you had in your testing > rather than potentially messing up the rebase this late. Got it, thanks. It's easier for me to delete the patches once I have them in the branch, that's been updated and now pushed to kernel org again (https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git/log/?h=kmap-conversion-for-5.12) I'll add it to testing branches and let it test over the weekend, sending the pull request next week.
On Fri, Feb 26, 2021 at 03:23:40PM +0100, David Sterba wrote: > On Thu, Feb 25, 2021 at 08:32:34AM -0800, Ira Weiny wrote: > > On Thu, Feb 25, 2021 at 02:12:52PM +0100, David Sterba wrote: > > > On Wed, Feb 24, 2021 at 09:59:12AM -0800, Ira Weiny wrote: > > > > On Wed, Feb 24, 2021 at 01:30:49PM +0100, David Sterba wrote: > > > > > On Tue, Feb 23, 2021 at 11:25:06AM -0800, Ira Weiny wrote: > > > > > > On Tue, Feb 23, 2021 at 09:13:42AM -0800, Linus Torvalds wrote: > > > > > > > On Tue, Feb 23, 2021 at 7:03 AM David Sterba <dsterba@suse.com> wrote: > > > > > [...] > > > > > > > > > > > Sorry. I will change it. > > > > > > > > > > Let me know how you want to proceed with the patchset/pull request. > > > > > > > > To be clear I'd like to just drop the 2 patches which use zero_user() for this > > > > merge window. > > > > > > > > I've already submitted some additional btrfs changes for 5.13[1]. I can rework > > > > these zero_user() patches and submit them through Andrew for 5.13 as separate > > > > set. That is what I meant by 'I will change it'. > > > > > > > > > I > > > > > can play the messenger again but now it seems a round of review is > > > > > needed and with some testing it'll be possible in some -rc. At that > > > > > point you may take the patches via the mm tree, unless Linus is ok with > > > > > a late pull. > > > > > > > > I'm ok with delaying the memzero_page() change to 5.13. There are a lot of > > > > kmap changes to come. But I'm trying to do them as smaller series just for > > > > this reason. I don't want valid changes to be denied due to my messing up just > > > > a few patches... :-( Hopefully you and Linus can forgive me on this one. > > > > > > > > Is ok to just drop them and merge the rest of this series in 5.12? > > > > > > Ok, no problem. Please let me know exactly which patches to drop, I'll > > > respin the branch. Thanks. > > > > Drop These 2: > > > > [PATCH V2 5/8] iov_iter: Remove memzero_page() in favor of zero_user() > > https://lore.kernel.org/lkml/20210210062221.3023586-6-ira.weiny@intel.com/ > > > > [PATCH V2 8/8] btrfs: convert to zero_user() > > https://lore.kernel.org/lkml/20210210062221.3023586-9-ira.weiny@intel.com/ > > > > > > Keep: > > > > [PATCH V2 1/8] mm/highmem: Lift memcpy_[to|from]_page to core > > [PATCH V2 2/8] mm/highmem: Convert memcpy_[to|from]_page() to kmap_local_page() > > [PATCH V2 3/8] mm/highmem: Introduce memcpy_page(), memmove_page(), and memset_page() > > [PATCH V2 4/8] mm/highmem: Add VM_BUG_ON() to mem*_page() calls > > ... > > [PATCH V2 6/8] btrfs: use memcpy_[to|from]_page() and kmap_local_page() > > [PATCH V2 7/8] btrfs: use copy_highpage() instead of 2 kmaps() > > ... > > > > I would resend but I'd rather keep the exact commits you had in your testing > > rather than potentially messing up the rebase this late. > > Got it, thanks. It's easier for me to delete the patches once I have > them in the branch, that's been updated and now pushed to kernel org > again (https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git/log/?h=kmap-conversion-for-5.12) Looks good. thank you. > > I'll add it to testing branches and let it test over the weekend, > sending the pull request next week. > Sounds like a good plan. Once again thank you for dealing with this. Sorry for the mix up, Ira
From: David Sterba <dsterba@suse.cz> Hi, this pull request contains changes regarding kmap API use and eg. conversion from kmap_atomic to kmap_local_page. The API belongs to memory management but to save cross-tree dependency headaches we've agreed to take it through the btrfs tree because there are some trivial conversions possible, while the rest will need some time and getting the easy cases out of the way would be convenient. The final patchset arrived shortly before merge window, which is not perfect, but given that it's straightforward I don't think it's too risky. I've added it to my for-next branch and it's been in linux-next for more than a week. Meanwhile I've been testing it among my regular branches with additional MM related debugging options. The changes can be grouped: - function exports, new helpers - new VM_BUG_ON for additional verification; it's been discussed if it should be VM_BUG_ON or BUG_ON, the former was chosen due to performance reasons - code replaced by relevant helpers Please pull, thanks. ---------------------------------------------------------------- The following changes since commit 92bf22614b21a2706f4993b278017e437f7785b3: Linux 5.11-rc7 (2021-02-07 13:57:38 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git kmap-conversion-for-5.12-tag for you to fetch changes up to bbc24c42f2c0ea037db3c7f319c860fd790aeb28: btrfs: convert to zero_user() (2021-02-11 20:18:25 +0100) ---------------------------------------------------------------- Ira Weiny (8): mm/highmem: Lift memcpy_[to|from]_page to core mm/highmem: Convert memcpy_[to|from]_page() to kmap_local_page() mm/highmem: Introduce memcpy_page(), memmove_page(), and memset_page() mm/highmem: Add VM_BUG_ON() to mem*_page() calls iov_iter: Remove memzero_page() in favor of zero_user() btrfs: use memcpy_[to|from]_page() and kmap_local_page() btrfs: use copy_highpage() instead of 2 kmaps() btrfs: convert to zero_user() fs/btrfs/compression.c | 11 +++------- fs/btrfs/extent_io.c | 22 ++++--------------- fs/btrfs/inode.c | 32 ++++++++-------------------- fs/btrfs/lzo.c | 4 ++-- fs/btrfs/raid56.c | 10 +-------- fs/btrfs/reflink.c | 12 ++--------- fs/btrfs/send.c | 7 ++----- fs/btrfs/zlib.c | 10 +++------ fs/btrfs/zstd.c | 11 +++------- include/linux/highmem.h | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ lib/iov_iter.c | 26 +++-------------------- 11 files changed, 88 insertions(+), 113 deletions(-)