From: Christoph Hellwig <hch@lst.de> Date: 2021-06-15 13:26:08
Hi all,
this series switches the core block layer code and all users of the
existing bvec kmap helpers to use kmap_local_page. Drivers that
currently use open coded kmap_atomic calls will converted in a follow
on series.
To do so a new kunmap variant is added that calls
flush_kernel_dcache_page. I'm not entirely sure where to call
flush_dcache_page vs flush_kernel_dcache_page, so I've tried to follow
the documentation here, but additional feedback would be welcome.
Changes since v1:
- add more/better comments
- add a new kunmap_local_dirty helper to feal with
flush(_kernel)_dcache_page
Diffstat:
arch/mips/include/asm/mach-rc32434/rb.h | 2 -
block/bio-integrity.c | 14 +++-----
block/bio.c | 37 ++++++----------------
block/blk-map.c | 2 -
block/bounce.c | 39 +++++-------------------
block/t10-pi.c | 16 +++------
drivers/block/ps3disk.c | 19 +----------
drivers/block/rbd.c | 15 +--------
drivers/md/dm-writecache.c | 5 +--
include/linux/bio.h | 42 -------------------------
include/linux/bvec.h | 52 ++++++++++++++++++++++++++++++--
include/linux/highmem-internal.h | 7 ++++
include/linux/highmem.h | 10 ++++--
13 files changed, 102 insertions(+), 158 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-15 13:26:11
Add a helper that calls flush_kernel_dcache_page before unmapping the
local mapping. flush_kernel_dcache_page is required for all pages
potentially mapped into userspace that were written to using kmap*,
so having a helper that does the right thing can be very convenient.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
include/linux/highmem-internal.h | 7 +++++++
include/linux/highmem.h | 4 ++++
2 files changed, 11 insertions(+)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-15 13:26:30
memcpy_to_page can write to potentially mapped page cache pages, so
use kunmap_local_dirty to make sure flush_kernel_dcache_pages is
called.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
include/linux/highmem.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-15 13:27:18
There is no need to include genhd.h from a random arch header, and not
doing so prevents the possibility for nasty include loops.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <redacted>
Reviewed-by: Ira Weiny <redacted>
---
arch/mips/include/asm/mach-rc32434/rb.h | 2 --
1 file changed, 2 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-15 13:27:38
Fix the include guards to match the file naming.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Chaitanya Kulkarni <redacted>
Reviewed-by: Ira Weiny <redacted>
---
include/linux/bvec.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-15 13:27:58
Add a helper to call kmap_local_page on a bvec. There is no need for
an unmap helper given that kunmap_local accept any address in the mapped
page.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <redacted>
Reviewed-by: Ira Weiny <redacted>
---
include/linux/bvec.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-15 13:29:12
Use memzero_bvec to zero each segment in the bio instead of manually
mapping and zeroing the data.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <redacted>
Reviewed-by: Ira Weiny <redacted>
---
block/bio.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-15 13:30:49
There is no need to disable interrupts in bio_copy_block, and the local
only mappings helps to avoid any sort of problems with stray writes
into the bio data.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ira Weiny <redacted>
---
drivers/md/dm-writecache.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-15 13:31:03
Use the bvec helpers instead of open coding the copy.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/block/ps3disk.c | 19 +++----------------
1 file changed, 3 insertions(+), 16 deletions(-)
@@ -5,7 +5,6 @@#ifndef __LINUX_BIO_H#define __LINUX_BIO_H-#include<linux/highmem.h>#include<linux/mempool.h>#include<linux/ioprio.h>/* struct bio, bio_vec and BIO_* flags are defined in blk_types.h */
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-15 13:31:43
Use the proper helpers instead of open coding the copy.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/bio.c | 28 ++++++++--------------------
1 file changed, 8 insertions(+), 20 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-15 13:32:13
Use memcpy_from_bvec instead of open coding the logic.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/blk-map.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-15 13:32:45
Rewrite the actual bounce buffering loop in __blk_queue_bounce to that
the memcpy_to_bvec helper can be used to perform the data copies.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/bounce.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-15 13:33:16
Using local kmaps slightly reduces the chances to stray writes, and
the bvec interface cleans up the code a little bit.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/t10-pi.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-15 13:34:05
Using local kmaps slightly reduces the chances to stray writes, and
the bvec interface cleans up the code a little bit.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/bio-integrity.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
Hi Christoph,
On 6/15/21 6:24 AM, Christoph Hellwig wrote:
Use the bvec helpers instead of open coding the copy.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/block/ps3disk.c | 19 +++----------------
1 file changed, 3 insertions(+), 16 deletions(-)
I tested your patch set applied to v5.13-rc6 on PS3 and it seemed to be
working OK.
I did some rsync's, some dd's, some fsck's, etc. If you have anything
you could suggest that you think would exercise your changes I could
try that also.
Tested-by: Geoff Levand <geoff@infradead.org>
From: Bart Van Assche <bvanassche@acm.org> Date: 2021-06-16 16:52:35
On 6/15/21 6:24 AM, Christoph Hellwig wrote:
quoted hunk
+/**+ * bvec_kmap_local - map a bvec into the kernel virtual address space+ * @bvec: bvec to map+ *+ * Must be called on single-page bvecs only. Call kunmap_local on the returned+ * address to unmap.+ */+static inline void *bvec_kmap_local(struct bio_vec *bvec)+{+ return kmap_local_page(bvec->bv_page) + bvec->bv_offset;+}
Hi Christoph,
Would it be appropriate to add WARN_ON_ONCE(bvec->bv_offset >=
PAGE_SIZE) in this function?
Thanks,
Bart.
From: Ira Weiny <hidden> Date: 2021-06-18 03:02:02
On Tue, Jun 15, 2021 at 03:24:39PM +0200, Christoph Hellwig wrote:
quoted hunk
Add a helper that calls flush_kernel_dcache_page before unmapping the
local mapping. flush_kernel_dcache_page is required for all pages
potentially mapped into userspace that were written to using kmap*,
so having a helper that does the right thing can be very convenient.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
include/linux/highmem-internal.h | 7 +++++++
include/linux/highmem.h | 4 ++++
2 files changed, 11 insertions(+)
@@ -224,4 +224,11 @@ do { \__kunmap_local(__addr);\}while(0)+#define kunmap_local_dirty(__page, __addr) \
I think having to store the page and addr to return to kunmap_local_dirty() is
going to be a pain in some code paths. Not a show stopper but see below...
+do { \
+ if (!PageSlab(__page)) \
Was there some clarification why the page can't be a Slab page? Or is this
just an optimization?
+ flush_kernel_dcache_page(__page); \
Is this required on 32bit systems? Why is kunmap_flush_on_unmap() not
sufficient on 64bit systems? The normal kunmap_local() path does that.
I'm sorry but I did not see a conclusion to my query on V1. Herbert implied the
he just copied from the crypto code.[1] I'm concerned that this _dirty() call
is just going to confuse the users of kmap even more. So why can't we get to
the bottom of why flush_kernel_dcache_page() needs so much logic around it
before complicating the general kernel users.
I would like to see it go away if possible.
Ira
[1] https://lore.kernel.org/lkml/20210615050258.GA5208@gondor.apana.org.au/
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2021-06-18 03:38:34
On Thu, Jun 17, 2021 at 08:01:57PM -0700, Ira Weiny wrote:
quoted
+ flush_kernel_dcache_page(__page); \
Is this required on 32bit systems? Why is kunmap_flush_on_unmap() not
sufficient on 64bit systems? The normal kunmap_local() path does that.
I'm sorry but I did not see a conclusion to my query on V1. Herbert implied the
he just copied from the crypto code.[1] I'm concerned that this _dirty() call
is just going to confuse the users of kmap even more. So why can't we get to
the bottom of why flush_kernel_dcache_page() needs so much logic around it
before complicating the general kernel users.
I would like to see it go away if possible.
From: Ira Weiny <hidden> Date: 2021-06-18 18:13:03
On Fri, Jun 18, 2021 at 11:37:28AM +0800, Herbert Xu wrote:
On Thu, Jun 17, 2021 at 08:01:57PM -0700, Ira Weiny wrote:
quoted
quoted
+ flush_kernel_dcache_page(__page); \
Is this required on 32bit systems? Why is kunmap_flush_on_unmap() not
sufficient on 64bit systems? The normal kunmap_local() path does that.
I'm sorry but I did not see a conclusion to my query on V1. Herbert implied the
he just copied from the crypto code.[1] I'm concerned that this _dirty() call
is just going to confuse the users of kmap even more. So why can't we get to
the bottom of why flush_kernel_dcache_page() needs so much logic around it
before complicating the general kernel users.
I would like to see it go away if possible.
Interesting! Thanks!
Digging around a bit more I found:
https://lore.kernel.org/patchwork/patch/439637/
Auditing all the flush_dcache_page() arch code reveals that the mapping field
is either unused, or is checked for NULL. Furthermore, all the implementations
call page_mapping_file() which further limits the page to not be a swap page.
All flush_kernel_dcache_page() implementations appears to operate the same way
in all arch's which define that call.
So I'm confident now that additional !PageSlab(__page) checks are not needed
and this patch is unnecessary. Christoph, can we leave this out of the kmap
API and just fold the flush_kernel_dcache_page() calls back into the bvec code?
Unfortunately, I'm not convinced this can be handled completely by
kunmap_local() nor the mem*_page() calls because there is a difference between
flush_dcache_page() and flush_kernel_dcache_page() in most archs... [parisc
being an exception which falls back to flush_kernel_dcache_page()]...
It seems like the generic unmap path _should_ be able to determine which call
to make based on the page but I'd have to look at that more.
Ira
Nice find. So we can at least get rid of the PageSlab call from
the Crypto API.
---8<---
As it is now legal to call flush_dcache_page on slab pages we
no longer need to do the check in the Crypto API.
Reported-by: Ira Weiny <redacted>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>