[PATCH v2 0/3] fs/ecryptfs: Replace kmap{,_atomic}() with kmap_local_page()

[PATCH v2 0/3] fs/ecryptfs: Replace kmap{,_atomic}() with kmap_local_page()

[Fabio M. De Francesco]

kmap() and kmap_atomic() have been deprecated in favor of
kmap_local_page().

Therefore, replace kmap() and kmap_atomic() with kmap_local_page().

Tested in a QEMU/KVM x86_32 VM, 6GB RAM, booting a kernel with
HIGHMEM64GB enabled.

v1->v2: Patches 1/3 and 2/3 were not grouped in a series. Patch 3/3 is
new. Therefore, currently one series groups all conversions needed if
fs/ecryptfs. The subject of 2/3 changed to differentiate between it and
3/3. Furthermore, the commit messages of 1/3 and 2/3 have been largely
reworked.

Fabio M. De Francesco (3):
  fs/ecryptfs: Replace kmap() with kmap_local_page()
  fs/ecryptfs: Use kmap_local_page() in ecryptfs_write()
  fs/ecryptfs: Use kmap_local_page() in copy_up_encrypted_with_header()

 fs/ecryptfs/crypto.c     |  8 ++++----
 fs/ecryptfs/mmap.c       |  4 ++--
 fs/ecryptfs/read_write.c | 12 ++++++------
 3 files changed, 12 insertions(+), 12 deletions(-)

-- 
2.40.0

[PATCH v2 1/3] fs/ecryptfs: Replace kmap() with kmap_local_page()

[Fabio M. De Francesco]

kmap() has been deprecated in favor of kmap_local_page().

Therefore, replace kmap() with kmap_local_page() in fs/ecryptfs.

There are two main problems with kmap(): (1) It comes with an overhead as
the mapping space is restricted and protected by a global lock for
synchronization and (2) it also requires global TLB invalidation when the
kmap’s pool wraps and it might block when the mapping space is fully
utilized until a slot becomes available.

With kmap_local_page() the mappings are per thread, CPU local, can take
page faults, and can be called from any context (including interrupts).
It is faster than kmap() in kernels with HIGHMEM enabled. The tasks can
be preempted and, when they are scheduled to run again, the kernel
virtual addresses are restored and still valid.

Obviously, thread locality implies that the kernel virtual addresses
returned by kmap_local_page() are only valid in the context of the
callers (i.e., they cannot be handed to other threads).

The use of kmap_local_page() in fs/ecryptfs does not break the
above-mentioned assumption, so it is allowed and preferred.

Tested in a QEMU/KVM x86_32 VM, 6GB RAM, booting a kernel with
HIGHMEM64GB enabled.

Suggested-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 fs/ecryptfs/crypto.c     | 8 ++++----
 fs/ecryptfs/read_write.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index c16f0d660cb7..03bd55069d86 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -441,10 +441,10 @@ int ecryptfs_encrypt_page(struct page *page)
 	}
 
 	lower_offset = lower_offset_for_page(crypt_stat, page);
-	enc_extent_virt = kmap(enc_extent_page);
+	enc_extent_virt = kmap_local_page(enc_extent_page);
 	rc = ecryptfs_write_lower(ecryptfs_inode, enc_extent_virt, lower_offset,
 				  PAGE_SIZE);
-	kunmap(enc_extent_page);
+	kunmap_local(enc_extent_virt);
 	if (rc < 0) {
 		ecryptfs_printk(KERN_ERR,
 			"Error attempting to write lower page; rc = [%d]\n",
@@ -490,10 +490,10 @@ int ecryptfs_decrypt_page(struct page *page)
 	BUG_ON(!(crypt_stat->flags & ECRYPTFS_ENCRYPTED));
 
 	lower_offset = lower_offset_for_page(crypt_stat, page);
-	page_virt = kmap(page);
+	page_virt = kmap_local_page(page);
 	rc = ecryptfs_read_lower(page_virt, lower_offset, PAGE_SIZE,
 				 ecryptfs_inode);
-	kunmap(page);
+	kunmap_local(page_virt);
 	if (rc < 0) {
 		ecryptfs_printk(KERN_ERR,
 			"Error attempting to read lower page; rc = [%d]\n",
diff --git a/fs/ecryptfs/read_write.c b/fs/ecryptfs/read_write.c
index 60bdcaddcbe5..5edf027c8359 100644
--- a/fs/ecryptfs/read_write.c
+++ b/fs/ecryptfs/read_write.c
@@ -64,11 +64,11 @@ int ecryptfs_write_lower_page_segment(struct inode *ecryptfs_inode,
 
 	offset = ((((loff_t)page_for_lower->index) << PAGE_SHIFT)
 		  + offset_in_page);
-	virt = kmap(page_for_lower);
+	virt = kmap_local_page(page_for_lower);
 	rc = ecryptfs_write_lower(ecryptfs_inode, virt, offset, size);
 	if (rc > 0)
 		rc = 0;
-	kunmap(page_for_lower);
+	kunmap_local(virt);
 	return rc;
 }
 
@@ -253,11 +253,11 @@ int ecryptfs_read_lower_page_segment(struct page *page_for_ecryptfs,
 	int rc;
 
 	offset = ((((loff_t)page_index) << PAGE_SHIFT) + offset_in_page);
-	virt = kmap(page_for_ecryptfs);
+	virt = kmap_local_page(page_for_ecryptfs);
 	rc = ecryptfs_read_lower(virt, offset, size, ecryptfs_inode);
 	if (rc > 0)
 		rc = 0;
-	kunmap(page_for_ecryptfs);
+	kunmap_local(virt);
 	flush_dcache_page(page_for_ecryptfs);
 	return rc;
 }
-- 
2.40.0

[PATCH v2 2/3] fs/ecryptfs: Use kmap_local_page() in ecryptfs_write()

[Fabio M. De Francesco]

kmap_atomic() is deprecated in favor of kmap_local_page().

Therefore, replace kmap_atomic() with kmap_local_page() in
ecryptfs_write().

kmap_atomic() is implemented like kmap_local_page() which also disables
page-faults and preemption (the latter only for !PREEMPT_RT kernels).

The code within the mapping/un-mapping in ecryptfs_write() does not
depend on the above-mentioned side effects so that a mere replacement of
the old API with the new one is all that is required (i.e., there is no
need to explicitly call pagefault_disable() and/or preempt_disable()).

Tested in a QEMU/KVM x86_32 VM, 6GB RAM, booting a kernel with
HIGHMEM64GB enabled.

Suggested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 fs/ecryptfs/read_write.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ecryptfs/read_write.c b/fs/ecryptfs/read_write.c
index 5edf027c8359..3458f153a588 100644
--- a/fs/ecryptfs/read_write.c
+++ b/fs/ecryptfs/read_write.c
@@ -140,7 +140,7 @@ int ecryptfs_write(struct inode *ecryptfs_inode, char *data, loff_t offset,
 			       ecryptfs_page_idx, rc);
 			goto out;
 		}
-		ecryptfs_page_virt = kmap_atomic(ecryptfs_page);
+		ecryptfs_page_virt = kmap_local_page(ecryptfs_page);
 
 		/*
 		 * pos: where we're now writing, offset: where the request was
@@ -163,7 +163,7 @@ int ecryptfs_write(struct inode *ecryptfs_inode, char *data, loff_t offset,
 			       (data + data_offset), num_bytes);
 			data_offset += num_bytes;
 		}
-		kunmap_atomic(ecryptfs_page_virt);
+		kunmap_local(ecryptfs_page_virt);
 		flush_dcache_page(ecryptfs_page);
 		SetPageUptodate(ecryptfs_page);
 		unlock_page(ecryptfs_page);
-- 
2.40.0

[PATCH v2 3/3] fs/ecryptfs: Use kmap_local_page() in copy_up_encrypted_with_header()

[Fabio M. De Francesco]

kmap_atomic() has been deprecated in favor of kmap_local_page().

Therefore, replace kmap_atomic() with kmap_local_page() in
ecryptfs_copy_up_encrypted_with_header().

kmap_atomic() is implemented like a kmap_local_page() which also
disables page-faults and preemption (the latter only in !PREEMPT_RT
kernels). The kernel virtual addresses returned by these two API are
only valid in the context of the callers (i.e., they cannot be handed to
other threads).

With kmap_local_page() the mappings are per thread and CPU local like
in kmap_atomic(); however, they can handle page-faults and can be called
from any context (including interrupts). The tasks that call
kmap_local_page() can be preempted and, when they are scheduled to run
again, the kernel virtual addresses are restored and are still valid.

In ecryptfs_copy_up_encrypted_with_header(), the block of code between
the mapping and un-mapping does not depend on the above-mentioned side
effects of kmap_aatomic(), so that the mere replacements of the old API
with the new one is all that is required (i.e., there is no need to
explicitly call pagefault_disable() and/or preempt_disable()).

Tested in a QEMU/KVM x86_32 VM, 6GB RAM, booting a kernel with
HIGHMEM64GB enabled.

Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 fs/ecryptfs/mmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
index 373c3e5747e6..cb1e998ce54d 100644
--- a/fs/ecryptfs/mmap.c
+++ b/fs/ecryptfs/mmap.c
@@ -125,7 +125,7 @@ ecryptfs_copy_up_encrypted_with_header(struct page *page,
 			/* This is a header extent */
 			char *page_virt;
 
-			page_virt = kmap_atomic(page);
+			page_virt = kmap_local_page(page);
 			memset(page_virt, 0, PAGE_SIZE);
 			/* TODO: Support more than one header extent */
 			if (view_extent_num == 0) {
@@ -138,7 +138,7 @@ ecryptfs_copy_up_encrypted_with_header(struct page *page,
 							       crypt_stat,
 							       &written);
 			}
-			kunmap_atomic(page_virt);
+			kunmap_local(page_virt);
 			flush_dcache_page(page);
 			if (rc) {
 				printk(KERN_ERR "%s: Error reading xattr "
-- 
2.40.0

Re: [PATCH v2 0/3] fs/ecryptfs: Replace kmap{,_atomic}() with kmap_local_page()

[Fabio M. De Francesco]

On mercoledì 26 aprile 2023 19:22:20 CEST Fabio M. De Francesco wrote:
> kmap() and kmap_atomic() have been deprecated in favor of
> kmap_local_page().
> 
> Therefore, replace kmap() and kmap_atomic() with kmap_local_page().

After two months from submission, I haven't received any comments on this 
short series yet, except for a "Reviewed by" tag from Ira on patch 1/3 only.

I would appreciate any comments/reviews/acks and would especially like to know 
if anything is preventing these patches from being applied.

Thank you all in advance,

Fabio

> Tested in a QEMU/KVM x86_32 VM, 6GB RAM, booting a kernel with
> HIGHMEM64GB enabled.
> 
> v1->v2: Patches 1/3 and 2/3 were not grouped in a series. Patch 3/3 is
> new. Therefore, currently one series groups all conversions needed if
> fs/ecryptfs. The subject of 2/3 changed to differentiate between it and
> 3/3. Furthermore, the commit messages of 1/3 and 2/3 have been largely
> reworked.
> 
> Fabio M. De Francesco (3):
>   fs/ecryptfs: Replace kmap() with kmap_local_page()
>   fs/ecryptfs: Use kmap_local_page() in ecryptfs_write()
>   fs/ecryptfs: Use kmap_local_page() in copy_up_encrypted_with_header()
> 
>  fs/ecryptfs/crypto.c     |  8 ++++----
>  fs/ecryptfs/mmap.c       |  4 ++--
>  fs/ecryptfs/read_write.c | 12 ++++++------
>  3 files changed, 12 insertions(+), 12 deletions(-)
> 
> --
> 2.40.0

Re: [PATCH v2 0/3] fs/ecryptfs: Replace kmap{,_atomic}() with kmap_local_page()

[Eric Biggers]

On Wed, Jun 28, 2023 at 03:50:30PM +0200, Fabio M. De Francesco wrote:
> On mercoledì 26 aprile 2023 19:22:20 CEST Fabio M. De Francesco wrote:
> > kmap() and kmap_atomic() have been deprecated in favor of
> > kmap_local_page().
> > 
> > Therefore, replace kmap() and kmap_atomic() with kmap_local_page().
> 
> After two months from submission, I haven't received any comments on this 
> short series yet, except for a "Reviewed by" tag from Ira on patch 1/3 only.
> 
> I would appreciate any comments/reviews/acks and would especially like to know 
> if anything is preventing these patches from being applied.
> 

eCryptfs is in "Odd Fixes" status.  See the thread
https://lore.kernel.org/ecryptfs/ZB4nYykRg6UwZ0cj@sequoia/T/#u

I would suggest that if Tyler is not responding, that Christian or Al take these
patches through the VFS tree instead.

FWIW, I took a quick look at these three patches, and all look correct.  I'm not
sure I want to give a formal R-b, as I don't want people to start bothering me
about eCryptfs stuff because they saw my name on it :-)

- Eric

Re: [PATCH v2 0/3] fs/ecryptfs: Replace kmap{,_atomic}() with kmap_local_page()

[Christian Brauner]

On Thu, Jun 29, 2023 at 07:31:07PM -0700, Eric Biggers wrote:
> On Wed, Jun 28, 2023 at 03:50:30PM +0200, Fabio M. De Francesco wrote:
> > On mercoledì 26 aprile 2023 19:22:20 CEST Fabio M. De Francesco wrote:
> > > kmap() and kmap_atomic() have been deprecated in favor of
> > > kmap_local_page().
> > > 
> > > Therefore, replace kmap() and kmap_atomic() with kmap_local_page().
> > 
> > After two months from submission, I haven't received any comments on this 
> > short series yet, except for a "Reviewed by" tag from Ira on patch 1/3 only.
> > 
> > I would appreciate any comments/reviews/acks and would especially like to know 
> > if anything is preventing these patches from being applied.
> > 
> 
> eCryptfs is in "Odd Fixes" status.  See the thread
> https://lore.kernel.org/ecryptfs/ZB4nYykRg6UwZ0cj@sequoia/T/#u
> 
> I would suggest that if Tyler is not responding, that Christian or Al take these
> patches through the VFS tree instead.
> 
> FWIW, I took a quick look at these three patches, and all look correct.  I'm not
> sure I want to give a formal R-b, as I don't want people to start bothering me
> about eCryptfs stuff because they saw my name on it :-)

Ah well, you saw right through that. :)
That usually means you've been doing kernel development for way too long...

Re: [PATCH v2 0/3] fs/ecryptfs: Replace kmap{,_atomic}() with kmap_local_page()

[Christian Brauner]

On Wed, 26 Apr 2023 19:22:20 +0200, Fabio M. De Francesco wrote:
> kmap() and kmap_atomic() have been deprecated in favor of
> kmap_local_page().
> 
> Therefore, replace kmap() and kmap_atomic() with kmap_local_page().
> 
> Tested in a QEMU/KVM x86_32 VM, 6GB RAM, booting a kernel with
> HIGHMEM64GB enabled.
> 
> [...]

Picking this up. Please tell me if this should be routed somewhere else.
vfs.misc will be rebased once v6.5-rc1 is released.

---

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/3] fs/ecryptfs: Replace kmap() with kmap_local_page()
      https://git.kernel.org/vfs/vfs/c/7a367455b6a5
[2/3] fs/ecryptfs: Use kmap_local_page() in ecryptfs_write()
      https://git.kernel.org/vfs/vfs/c/55f13011af9d
[3/3] fs/ecryptfs: Use kmap_local_page() in copy_up_encrypted_with_header()
      https://git.kernel.org/vfs/vfs/c/de9f5a15080f

Re: [PATCH v2 0/3] fs/ecryptfs: Replace kmap{,_atomic}() with kmap_local_page()

[Fabio M. De Francesco]

On venerdì 30 giugno 2023 10:45:17 CEST Christian Brauner wrote:
> On Wed, 26 Apr 2023 19:22:20 +0200, Fabio M. De Francesco wrote:
> > kmap() and kmap_atomic() have been deprecated in favor of
> > kmap_local_page().
> > 
> > Therefore, replace kmap() and kmap_atomic() with kmap_local_page().
> > 
> > Tested in a QEMU/KVM x86_32 VM, 6GB RAM, booting a kernel with
> > HIGHMEM64GB enabled.
> > 
> > [...]
> 
> Picking this up.

Well, you anticipated me before I could ask you to take these patches through 
the VFS tree, as suggested by Eric.

> Please tell me if this should be routed somewhere else.
> vfs.misc will be rebased once v6.5-rc1 is released.

Actually, I really don't care which route they take, what really matters to me 
is that they get upstream one way or another :-)

Thank you very much,

Fabio

> ---
> 
> Applied to the vfs.misc branch of the vfs/vfs.git tree.
> Patches in the vfs.misc branch should appear in linux-next soon.
> 
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
> 
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
> 
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs.misc
> 
> [1/3] fs/ecryptfs: Replace kmap() with kmap_local_page()
>       https://git.kernel.org/vfs/vfs/c/7a367455b6a5
> [2/3] fs/ecryptfs: Use kmap_local_page() in ecryptfs_write()
>       https://git.kernel.org/vfs/vfs/c/55f13011af9d
> [3/3] fs/ecryptfs: Use kmap_local_page() in copy_up_encrypted_with_header()
>       https://git.kernel.org/vfs/vfs/c/de9f5a15080f

Re: [PATCH v2 0/3] fs/ecryptfs: Replace kmap{,_atomic}() with kmap_local_page()

[Fabio M. De Francesco]

On venerdì 30 giugno 2023 10:45:17 CEST Christian Brauner wrote:
> On Wed, 26 Apr 2023 19:22:20 +0200, Fabio M. De Francesco wrote:
> > kmap() and kmap_atomic() have been deprecated in favor of
> > kmap_local_page().
> > 
> > Therefore, replace kmap() and kmap_atomic() with kmap_local_page().
> > 
> > Tested in a QEMU/KVM x86_32 VM, 6GB RAM, booting a kernel with
> > HIGHMEM64GB enabled.
> > 
> > [...]
> 
> Picking this up. Please tell me if this should be routed somewhere else.
> vfs.misc will be rebased once v6.5-rc1 is released.

Christian,

v6.5-rc1 has been released since a while, but I can't yet see this series. Are 
there problems with these patches that stop their merge?

Looking forward for your reply.

Thanks,

Fabio

> 
> ---
> 
> Applied to the vfs.misc branch of the vfs/vfs.git tree.
> Patches in the vfs.misc branch should appear in linux-next soon.
> 
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
> 
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
> 
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs.misc
> 
> [1/3] fs/ecryptfs: Replace kmap() with kmap_local_page()
>       https://git.kernel.org/vfs/vfs/c/7a367455b6a5
> [2/3] fs/ecryptfs: Use kmap_local_page() in ecryptfs_write()
>       https://git.kernel.org/vfs/vfs/c/55f13011af9d
> [3/3] fs/ecryptfs: Use kmap_local_page() in copy_up_encrypted_with_header()
>       https://git.kernel.org/vfs/vfs/c/de9f5a15080f

Re: [PATCH v2 0/3] fs/ecryptfs: Replace kmap{,_atomic}() with kmap_local_page()

[Christian Brauner]

On Thu, Aug 17, 2023 at 07:13:56PM +0200, Fabio M. De Francesco wrote:
> On venerdì 30 giugno 2023 10:45:17 CEST Christian Brauner wrote:
> > On Wed, 26 Apr 2023 19:22:20 +0200, Fabio M. De Francesco wrote:
> > > kmap() and kmap_atomic() have been deprecated in favor of
> > > kmap_local_page().
> > > 
> > > Therefore, replace kmap() and kmap_atomic() with kmap_local_page().
> > > 
> > > Tested in a QEMU/KVM x86_32 VM, 6GB RAM, booting a kernel with
> > > HIGHMEM64GB enabled.
> > > 
> > > [...]
> > 
> > Picking this up. Please tell me if this should be routed somewhere else.
> > vfs.misc will be rebased once v6.5-rc1 is released.
> 
> Christian,
> 
> v6.5-rc1 has been released since a while, but I can't yet see this series. Are 
> there problems with these patches that stop their merge?

Nothing stops them. I just planned to send all of this for v6.6. as I
didn't see a need to sent it earlier.

This message made neomutt crash like crazy btw. So I had to get create
to be able to reply to so hopefully that message gets through...