[PATCH 0/4] btrfs: Convert kmaps to core page calls

[PATCH 0/4] btrfs: Convert kmaps to core page calls

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

There are many places where kmap/<operation>/kunmap patterns occur.  We lift
these various patterns to core common functions and use them in the btrfs file
system.  At the same time we convert those core functions to use
kmap_local_page() which is more efficient in those calls.

I think this is best accepted through Andrew's tree as it has the mem*_page
functions in it.  But I'd like to get an ack from David or one of the other
btrfs maintainers before the btrfs patches go through.

There are a lot more kmap->kmap_local_page() conversions but kmap_local_page()
requires some care with the unmapping order and so I'm still reviewing those
changes because btrfs uses a lot of loops for it's kmaps.

Thanks,
Ira

Ira Weiny (4):
  mm/highmem: Lift memcpy_[to|from]_page to core
  fs/btrfs: Use memcpy_[to|from]_page()
  fs/btrfs: Use copy_highpage() instead of 2 kmaps()
  fs/btrfs: Convert to zero_user()

 fs/btrfs/compression.c  | 11 +++------
 fs/btrfs/extent_io.c    | 22 ++++-------------
 fs/btrfs/inode.c        | 33 ++++++++-----------------
 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 | 53 +++++++++++++++++++++++++++++++++++++++++
 lib/iov_iter.c          | 26 +++-----------------
 11 files changed, 86 insertions(+), 113 deletions(-)

-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 1/4] mm/highmem: Lift memcpy_[to|from]_page to core

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Working through a conversion to a call kmap_local_page() instead of
kmap() revealed many places where the pattern kmap/memcpy/kunmap
occurred.

Eric Biggers, Matthew Wilcox, Christoph Hellwig, Dan Williams, and Al
Viro all suggested putting this code into helper functions.  Al Viro
further pointed out that these functions already existed in the iov_iter
code.[1]

Various locations for the lifted functions were considered.

Headers like mm.h or string.h seem ok but don't really portray the
functionality well.  pagemap.h made some sense but is for page cache
functionality.[2]

Another alternative would be to create a new header for the promoted
memcpy functions, but it masks the fact that these are designed to copy
to/from pages using the kernel direct mappings and complicates matters
with a new header.

Placing these functions in 'highmem.h' is suboptimal especially with the
changes being proposed in the functionality of kmap.  From a caller
perspective including/using 'highmem.h' implies that the functions
defined in that header are only required when highmem is in use which is
increasingly not the case with modern processors.  However, highmem.h is
where all the current functions like this reside (zero_user(),
clear_highpage(), clear_user_highpage(), copy_user_highpage(), and
copy_highpage()).  So it makes the most sense even though it is
distasteful for some.[3]

Lift memcpy_to_page() and memcpy_from_page() to pagemap.h.

Remove memzero_page() in favor of zero_user() to zero a page.

Add a memcpy_page(), memmove_page, and memset_page() to cover more
kmap/mem*/kunmap. patterns.

Add BUG_ON bounds checks to ensure the newly lifted page memory
operations do not result in corrupted data in neighbor pages and to make
them consistent with zero_user().[4]

Finally use kmap_local_page() in all the new calls.

[1] https://lore.kernel.org/lkml/20201013200149.GI3576660@ZenIV.linux.org.uk/
    https://lore.kernel.org/lkml/20201013112544.GA5249@infradead.org/

[2] https://lore.kernel.org/lkml/20201208122316.GH7338@casper.infradead.org/

[3] https://lore.kernel.org/lkml/20201013200149.GI3576660@ZenIV.linux.org.uk/#t
    https://lore.kernel.org/lkml/20201208163814.GN1563847@iweiny-DESK2.sc.intel.com/

[4] https://lore.kernel.org/lkml/20201210053502.GS1563847@iweiny-DESK2.sc.intel.com/

Cc: Boris Pismenny <borisp@mellanox.com>
Cc: Or Gerlitz <gerlitz.or@gmail.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Suggested-by: Matthew Wilcox <willy@infradead.org>
Suggested-by: Christoph Hellwig <hch@infradead.org>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Suggested-by: Eric Biggers <ebiggers@kernel.org>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Chagnes for V4:
	Update commit message to say kmap_local_page() since
	kmap_thread() is no longer valid

Changes for V3:
	From Matthew Wilcox
		Move calls to highmem.h
		Add BUG_ON()

Changes for V2:
	From Thomas Gleixner
		Change kmap_atomic() to kmap_local_page() after basing
		on tip/core/mm
	From Joonas Lahtinen
		Reverse offset/val in memset_page()
---
 include/linux/highmem.h | 53 +++++++++++++++++++++++++++++++++++++++++
 lib/iov_iter.c          | 26 +++-----------------
 2 files changed, 56 insertions(+), 23 deletions(-)

diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index d2c70d3772a3..c642dd64ea3f 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -276,4 +276,57 @@ static inline void copy_highpage(struct page *to, struct page *from)
 
 #endif
 
+static inline void memcpy_page(struct page *dst_page, size_t dst_off,
+			       struct page *src_page, size_t src_off,
+			       size_t len)
+{
+	char *dst = kmap_local_page(dst_page);
+	char *src = kmap_local_page(src_page);
+
+	BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
+	memcpy(dst + dst_off, src + src_off, len);
+	kunmap_local(src);
+	kunmap_local(dst);
+}
+
+static inline void memmove_page(struct page *dst_page, size_t dst_off,
+			       struct page *src_page, size_t src_off,
+			       size_t len)
+{
+	char *dst = kmap_local_page(dst_page);
+	char *src = kmap_local_page(src_page);
+
+	BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
+	memmove(dst + dst_off, src + src_off, len);
+	kunmap_local(src);
+	kunmap_local(dst);
+}
+
+static inline void memcpy_from_page(char *to, struct page *page, size_t offset, size_t len)
+{
+	char *from = kmap_local_page(page);
+
+	BUG_ON(offset + len > PAGE_SIZE);
+	memcpy(to, from + offset, len);
+	kunmap_local(from);
+}
+
+static inline void memcpy_to_page(struct page *page, size_t offset, const char *from, size_t len)
+{
+	char *to = kmap_local_page(page);
+
+	BUG_ON(offset + len > PAGE_SIZE);
+	memcpy(to + offset, from, len);
+	kunmap_local(to);
+}
+
+static inline void memset_page(struct page *page, size_t offset, int val, size_t len)
+{
+	char *addr = kmap_local_page(page);
+
+	BUG_ON(offset + len > PAGE_SIZE);
+	memset(addr + offset, val, len);
+	kunmap_local(addr);
+}
+
 #endif /* _LINUX_HIGHMEM_H */
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index a21e6a5792c5..aa0d03b33a1e 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -5,6 +5,7 @@
 #include <linux/fault-inject-usercopy.h>
 #include <linux/uio.h>
 #include <linux/pagemap.h>
+#include <linux/highmem.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/splice.h>
@@ -466,27 +467,6 @@ void iov_iter_init(struct iov_iter *i, unsigned int direction,
 }
 EXPORT_SYMBOL(iov_iter_init);
 
-static void memcpy_from_page(char *to, struct page *page, size_t offset, size_t len)
-{
-	char *from = kmap_atomic(page);
-	memcpy(to, from + offset, len);
-	kunmap_atomic(from);
-}
-
-static void memcpy_to_page(struct page *page, size_t offset, const char *from, size_t len)
-{
-	char *to = kmap_atomic(page);
-	memcpy(to + offset, from, len);
-	kunmap_atomic(to);
-}
-
-static void memzero_page(struct page *page, size_t offset, size_t len)
-{
-	char *addr = kmap_atomic(page);
-	memset(addr + offset, 0, len);
-	kunmap_atomic(addr);
-}
-
 static inline bool allocated(struct pipe_buffer *buf)
 {
 	return buf->ops == &default_pipe_buf_ops;
@@ -964,7 +944,7 @@ static size_t pipe_zero(size_t bytes, struct iov_iter *i)
 
 	do {
 		size_t chunk = min_t(size_t, n, PAGE_SIZE - off);
-		memzero_page(pipe->bufs[i_head & p_mask].page, off, chunk);
+		zero_user(pipe->bufs[i_head & p_mask].page, off, chunk);
 		i->head = i_head;
 		i->iov_offset = off + chunk;
 		n -= chunk;
@@ -981,7 +961,7 @@ size_t iov_iter_zero(size_t bytes, struct iov_iter *i)
 		return pipe_zero(bytes, i);
 	iterate_and_advance(i, bytes, v,
 		clear_user(v.iov_base, v.iov_len),
-		memzero_page(v.bv_page, v.bv_offset, v.bv_len),
+		zero_user(v.bv_page, v.bv_offset, v.bv_len),
 		memset(v.iov_base, 0, v.iov_len)
 	)
 
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH 1/4] mm/highmem: Lift memcpy_[to|from]_page to core

[Chaitanya Kulkarni]

On 2/5/21 18:35, ira.weiny@intel.com wrote:
> +static inline void memmove_page(struct page *dst_page, size_t dst_off,
> +			       struct page *src_page, size_t src_off,
> +			       size_t len)
> +{
> +	char *dst = kmap_local_page(dst_page);
> +	char *src = kmap_local_page(src_page);
> +
> +	BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> +	memmove(dst + dst_off, src + src_off, len);
> +	kunmap_local(src);
> +	kunmap_local(dst);
> +}
> +
> +static inline void memcpy_from_page(char *to, struct page *page, size_t offset, size_t len)
How about following ?
static inline void memcpy_from_page(char *to, struct page *page, size_t
offset,
                                    size_t len)  
> +{
> +	char *from = kmap_local_page(page);
> +
> +	BUG_ON(offset + len > PAGE_SIZE);
> +	memcpy(to, from + offset, len);
> +	kunmap_local(from);
> +}
> +
> +static inline void memcpy_to_page(struct page *page, size_t offset, const char *from, size_t len)
How about following ?
static inline void memcpy_to_page(struct page *page, size_t offset,
                                  const char *from, size_t len)
> +{
> +	char *to = kmap_local_page(page);
> +
> +	BUG_ON(offset + len > PAGE_SIZE);
> +	memcpy(to + offset, from, len);
> +	kunmap_local(to);
> +}
> +
> +static inline void memset_page(struct page *page, size_t offset, int val, size_t len)
How about following ?
static inline void memset_page(struct page *page, size_t offset, int val,
                               size_t len)  
> +{
> +	char *addr = kmap_local_page(page);
> +
> +	BUG_ON(offset + len > PAGE_SIZE);
> +	memset(addr + offset, val, len);
> +	kunmap_local(addr);
> +}
> +

Re: [PATCH 1/4] mm/highmem: Lift memcpy_[to|from]_page to core

[Ira Weiny]

On Sun, Feb 07, 2021 at 01:46:47AM +0000, Chaitanya Kulkarni wrote:
> On 2/5/21 18:35, ira.weiny@intel.com wrote:
> > +static inline void memmove_page(struct page *dst_page, size_t dst_off,
> > +			       struct page *src_page, size_t src_off,
> > +			       size_t len)
> > +{
> > +	char *dst = kmap_local_page(dst_page);
> > +	char *src = kmap_local_page(src_page);
> > +
> > +	BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> > +	memmove(dst + dst_off, src + src_off, len);
> > +	kunmap_local(src);
> > +	kunmap_local(dst);
> > +}
> > +
> > +static inline void memcpy_from_page(char *to, struct page *page, size_t offset, size_t len)
> How about following ?
> static inline void memcpy_from_page(char *to, struct page *page, size_t
> offset,
>                                     size_t len)  

It is an easy change and It is up to Andrew.  But I thought we were making the
line length limit longer now.

Ira

> > +{
> > +	char *from = kmap_local_page(page);
> > +
> > +	BUG_ON(offset + len > PAGE_SIZE);
> > +	memcpy(to, from + offset, len);
> > +	kunmap_local(from);
> > +}
> > +
> > +static inline void memcpy_to_page(struct page *page, size_t offset, const char *from, size_t len)
> How about following ?
> static inline void memcpy_to_page(struct page *page, size_t offset,
>                                   const char *from, size_t len)
> > +{
> > +	char *to = kmap_local_page(page);
> > +
> > +	BUG_ON(offset + len > PAGE_SIZE);
> > +	memcpy(to + offset, from, len);
> > +	kunmap_local(to);
> > +}
> > +
> > +static inline void memset_page(struct page *page, size_t offset, int val, size_t len)
> How about following ?
> static inline void memset_page(struct page *page, size_t offset, int val,
>                                size_t len)  
> > +{
> > +	char *addr = kmap_local_page(page);
> > +
> > +	BUG_ON(offset + len > PAGE_SIZE);
> > +	memset(addr + offset, val, len);
> > +	kunmap_local(addr);
> > +}
> > +

Re: [PATCH 1/4] mm/highmem: Lift memcpy_[to|from]_page to core

[Chaitanya Kulkarni]

On 2/7/21 19:13, Ira Weiny wrote:
>>> +static inline void memcpy_from_page(char *to, struct page *page, size_t offset, size_t len)
>> How about following ?
>> static inline void memcpy_from_page(char *to, struct page *page, size_t
>> offset,
>>                                     size_t len)  
> It is an easy change and It is up to Andrew.  But I thought we were making the
> line length limit longer now.
>
> Ira
>
True, not sure what is the right thing going forward especially when new
changes
are mixed with the old ones, I'll leave it to the maintainer to decide.

[PATCH 2/4] fs/btrfs: Use memcpy_[to|from]_page()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Development of this patch was aided by the coccinelle script:

// <smpl>
// SPDX-License-Identifier: GPL-2.0-only
// Find kmap/memcpy/kunmap pattern and replace with memcpy*page calls
//
// NOTE: Offsets and other expressions may be more complex than what the script
// will automatically generate.  Therefore a catchall rule is provided to find
// the pattern which then must be evaluated by hand.
//
// Confidence: Low
// Copyright: (C) 2021 Intel Corporation
// URL: http://coccinelle.lip6.fr/
// Comments:
// Options:

//
// simple memcpy version
//
@ memcpy_rule1 @
expression page, T, F, B, Off;
identifier ptr;
type VP;
@@

(
-VP ptr = kmap(page);
|
-ptr = kmap(page);
|
-VP ptr = kmap_atomic(page);
|
-ptr = kmap_atomic(page);
)
<+...
(
-memcpy(ptr + Off, F, B);
+memcpy_to_page(page, Off, F, B);
|
-memcpy(ptr, F, B);
+memcpy_to_page(page, 0, F, B);
|
-memcpy(T, ptr + Off, B);
+memcpy_from_page(T, page, Off, B);
|
-memcpy(T, ptr, B);
+memcpy_from_page(T, page, 0, B);
)
...+>
(
-kunmap(page);
|
-kunmap_atomic(ptr);
)

// Remove any pointers left unused
@
depends on memcpy_rule1
@
identifier memcpy_rule1.ptr;
type VP, VP1;
@@

-VP ptr;
	... when != ptr;
? VP1 ptr;

//
// Some callers kmap without a temp pointer
//
@ memcpy_rule2 @
expression page, T, Off, F, B;
@@

<+...
(
-memcpy(kmap(page) + Off, F, B);
+memcpy_to_page(page, Off, F, B);
|
-memcpy(kmap(page), F, B);
+memcpy_to_page(page, 0, F, B);
|
-memcpy(T, kmap(page) + Off, B);
+memcpy_from_page(T, page, Off, B);
|
-memcpy(T, kmap(page), B);
+memcpy_from_page(T, page, 0, B);
)
...+>
-kunmap(page);
// No need for the ptr variable removal

//
// Catch all
//
@ memcpy_rule3 @
expression page;
expression GenTo, GenFrom, GenSize;
identifier ptr;
type VP;
@@

(
-VP ptr = kmap(page);
|
-ptr = kmap(page);
|
-VP ptr = kmap_atomic(page);
|
-ptr = kmap_atomic(page);
)
<+...
(
//
// Some call sites have complex expressions within the memcpy
// match a catch all to be evaluated by hand.
//
-memcpy(GenTo, GenFrom, GenSize);
+memcpy_to_pageExtra(page, GenTo, GenFrom, GenSize);
+memcpy_from_pageExtra(GenTo, page, GenFrom, GenSize);
)
...+>
(
-kunmap(page);
|
-kunmap_atomic(ptr);
)

// Remove any pointers left unused
@
depends on memcpy_rule3
@
identifier memcpy_rule3.ptr;
type VP, VP1;
@@

-VP ptr;
	... when != ptr;
? VP1 ptr;

// <smpl>

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

squash: memcpy
---
 fs/btrfs/compression.c | 6 ++----
 fs/btrfs/lzo.c         | 4 ++--
 fs/btrfs/reflink.c     | 6 +-----
 fs/btrfs/send.c        | 7 ++-----
 fs/btrfs/zlib.c        | 5 ++---
 fs/btrfs/zstd.c        | 6 ++----
 6 files changed, 11 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 5ae3fa0386b7..047b632b4139 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -1231,7 +1231,6 @@ int btrfs_decompress_buf2page(const char *buf, unsigned long buf_start,
 	unsigned long prev_start_byte;
 	unsigned long working_bytes = total_out - buf_start;
 	unsigned long bytes;
-	char *kaddr;
 	struct bio_vec bvec = bio_iter_iovec(bio, bio->bi_iter);
 
 	/*
@@ -1262,9 +1261,8 @@ int btrfs_decompress_buf2page(const char *buf, unsigned long buf_start,
 				PAGE_SIZE - (buf_offset % PAGE_SIZE));
 		bytes = min(bytes, working_bytes);
 
-		kaddr = kmap_atomic(bvec.bv_page);
-		memcpy(kaddr + bvec.bv_offset, buf + buf_offset, bytes);
-		kunmap_atomic(kaddr);
+		memcpy_to_page(bvec.bv_page, bvec.bv_offset, buf + buf_offset,
+			       bytes);
 		flush_dcache_page(bvec.bv_page);
 
 		buf_offset += bytes;
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index aa9cd11f4b78..9084a950dc09 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -467,7 +467,7 @@ int lzo_decompress(struct list_head *ws, unsigned char *data_in,
 	destlen = min_t(unsigned long, destlen, PAGE_SIZE);
 	bytes = min_t(unsigned long, destlen, out_len - start_byte);
 
-	kaddr = kmap_atomic(dest_page);
+	kaddr = kmap_local_page(dest_page);
 	memcpy(kaddr, workspace->buf + start_byte, bytes);
 
 	/*
@@ -477,7 +477,7 @@ int lzo_decompress(struct list_head *ws, unsigned char *data_in,
 	 */
 	if (bytes < destlen)
 		memset(kaddr+bytes, 0, destlen-bytes);
-	kunmap_atomic(kaddr);
+	kunmap_local(kaddr);
 out:
 	return ret;
 }
diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
index b03e7891394e..74c62e49c0c9 100644
--- a/fs/btrfs/reflink.c
+++ b/fs/btrfs/reflink.c
@@ -103,12 +103,8 @@ static int copy_inline_to_page(struct btrfs_inode *inode,
 	set_bit(BTRFS_INODE_NO_DELALLOC_FLUSH, &inode->runtime_flags);
 
 	if (comp_type == BTRFS_COMPRESS_NONE) {
-		char *map;
-
-		map = kmap(page);
-		memcpy(map, data_start, datal);
+		memcpy_to_page(page, 0, data_start, datal);
 		flush_dcache_page(page);
-		kunmap(page);
 	} else {
 		ret = btrfs_decompress(comp_type, data_start, page, 0,
 				       inline_size, datal);
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 78a35374d492..83982b3b7057 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -4948,7 +4948,6 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct inode *inode;
 	struct page *page;
-	char *addr;
 	pgoff_t index = offset >> PAGE_SHIFT;
 	pgoff_t last_index;
 	unsigned pg_offset = offset_in_page(offset);
@@ -5001,10 +5000,8 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
 			}
 		}
 
-		addr = kmap(page);
-		memcpy(sctx->send_buf + sctx->send_size, addr + pg_offset,
-		       cur_len);
-		kunmap(page);
+		memcpy_from_page(sctx->send_buf + sctx->send_size, page,
+				 pg_offset, cur_len);
 		unlock_page(page);
 		put_page(page);
 		index++;
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index 05615a1099db..d524acf7b3e5 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -432,9 +432,8 @@ int zlib_decompress(struct list_head *ws, unsigned char *data_in,
 			    PAGE_SIZE - (buf_offset % PAGE_SIZE));
 		bytes = min(bytes, bytes_left);
 
-		kaddr = kmap_atomic(dest_page);
-		memcpy(kaddr + pg_offset, workspace->buf + buf_offset, bytes);
-		kunmap_atomic(kaddr);
+		memcpy_to_page(dest_page, pg_offset,
+			       workspace->buf + buf_offset, bytes);
 
 		pg_offset += bytes;
 		bytes_left -= bytes;
diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
index 9a4871636c6c..8e9626d63976 100644
--- a/fs/btrfs/zstd.c
+++ b/fs/btrfs/zstd.c
@@ -688,10 +688,8 @@ int zstd_decompress(struct list_head *ws, unsigned char *data_in,
 		bytes = min_t(unsigned long, destlen - pg_offset,
 				workspace->out_buf.size - buf_offset);
 
-		kaddr = kmap_atomic(dest_page);
-		memcpy(kaddr + pg_offset, workspace->out_buf.dst + buf_offset,
-				bytes);
-		kunmap_atomic(kaddr);
+		memcpy_to_page(dest_page, pg_offset,
+			       workspace->out_buf.dst + buf_offset, bytes);
 
 		pg_offset += bytes;
 	}
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH 2/4] fs/btrfs: Use memcpy_[to|from]_page()

[David Sterba]

On Fri, Feb 05, 2021 at 03:23:02PM -0800, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>

There should be a short explanation what the patch does, eg.
"use helper for open coded kmap_atomic/memcpy/kunmap_atomic",
although I see there are conversions kmap_atomic -> kmap_local not in
the coccinelle script, probably done manually. This should be mentioned
too.

Also please use "btrfs:" followed by lowercase in the subject.

[PATCH 3/4] fs/btrfs: Use copy_highpage() instead of 2 kmaps()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Development of this patch was aided by the following coccinelle script:

// <smpl>
// SPDX-License-Identifier: GPL-2.0-only
// Find kmap/copypage/kunmap pattern and replace with copy_highpage calls
//
// NOTE: The expressions in the copy page version of this kmap pattern are
// overly complex and so these all need individual attention.
//
// Confidence: Low
// Copyright: (C) 2021 Intel Corporation
// URL: http://coccinelle.lip6.fr/
// Comments:
// Options:

//
// Then a copy_page where we have 2 pages involved.
//
@ copy_page_rule @
expression page, page2, To, From, Size;
identifier ptr, ptr2;
type VP, VP2;
@@

/* kmap */
(
-VP ptr = kmap(page);
...
-VP2 ptr2 = kmap(page2);
|
-VP ptr = kmap_atomic(page);
...
-VP2 ptr2 = kmap_atomic(page2);
|
-ptr = kmap(page);
...
-ptr2 = kmap(page2);
|
-ptr = kmap_atomic(page);
...
-ptr2 = kmap_atomic(page2);
)

// 1 or more copy versions of the entire page
<+...
(
-copy_page(To, From);
+copy_highpage(To, From);
|
-memmove(To, From, Size);
+memmoveExtra(To, From, Size);
)
...+>

/* kunmap */
(
-kunmap(page2);
...
-kunmap(page);
|
-kunmap(page);
...
-kunmap(page2);
|
-kmap_atomic(ptr2);
...
-kmap_atomic(ptr);
)

// Remove any pointers left unused
@
depends on copy_page_rule
@
identifier copy_page_rule.ptr;
identifier copy_page_rule.ptr2;
type VP, VP1;
type VP2, VP21;
@@

-VP ptr;
	... when != ptr;
? VP1 ptr;
-VP2 ptr2;
	... when != ptr2;
? VP21 ptr2;

// </smpl>

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 fs/btrfs/raid56.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index dbf52f1a379d..7c3f6dc918c1 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -250,8 +250,6 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
 static void cache_rbio_pages(struct btrfs_raid_bio *rbio)
 {
 	int i;
-	char *s;
-	char *d;
 	int ret;
 
 	ret = alloc_rbio_pages(rbio);
@@ -262,13 +260,7 @@ static void cache_rbio_pages(struct btrfs_raid_bio *rbio)
 		if (!rbio->bio_pages[i])
 			continue;
 
-		s = kmap(rbio->bio_pages[i]);
-		d = kmap(rbio->stripe_pages[i]);
-
-		copy_page(d, s);
-
-		kunmap(rbio->bio_pages[i]);
-		kunmap(rbio->stripe_pages[i]);
+		copy_highpage(rbio->stripe_pages[i], rbio->bio_pages[i]);
 		SetPageUptodate(rbio->stripe_pages[i]);
 	}
 	set_bit(RBIO_CACHE_READY_BIT, &rbio->flags);
-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 4/4] fs/btrfs: Convert to zero_user()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

The development of this patch was aided by the following coccinelle
script:

// <smpl>
// SPDX-License-Identifier: GPL-2.0-only
// Find kmap/memset/kunmap pattern and replace with memset*page calls
//
// NOTE: Offsets and other expressions may be more complex than what the script
// will automatically generate.  Therefore a catchall rule is provided to find
// the pattern which then must be evaluated by hand.
//
// Confidence: Low
// Copyright: (C) 2021 Intel Corporation
// URL: http://coccinelle.lip6.fr/
// Comments:
// Options:

//
// Then the memset pattern
//
@ memset_rule1 @
expression page, V, L, Off;
identifier ptr;
type VP;
@@

(
-VP ptr = kmap(page);
|
-ptr = kmap(page);
|
-VP ptr = kmap_atomic(page);
|
-ptr = kmap_atomic(page);
)
<+...
(
-memset(ptr, 0, L);
+zero_user(page, 0, L);
|
-memset(ptr + Off, 0, L);
+zero_user(page, Off, L);
|
-memset(ptr, V, L);
+memset_page(page, V, 0, L);
|
-memset(ptr + Off, V, L);
+memset_page(page, V, Off, L);
)
...+>
(
-kunmap(page);
|
-kunmap_atomic(ptr);
)

// Remove any pointers left unused
@
depends on memset_rule1
@
identifier memset_rule1.ptr;
type VP, VP1;
@@

-VP ptr;
	... when != ptr;
? VP1 ptr;

//
// Catch all
//
@ memset_rule2 @
expression page;
identifier ptr;
expression GenTo, GenSize, GenValue;
type VP;
@@

(
-VP ptr = kmap(page);
|
-ptr = kmap(page);
|
-VP ptr = kmap_atomic(page);
|
-ptr = kmap_atomic(page);
)
<+...
(
//
// Some call sites have complex expressions within the memset/memcpy
// The follow are catch alls which need to be evaluated by hand.
//
-memset(GenTo, 0, GenSize);
+zero_userExtra(page, GenTo, GenSize);
|
-memset(GenTo, GenValue, GenSize);
+memset_pageExtra(page, GenValue, GenTo, GenSize);
)
...+>
(
-kunmap(page);
|
-kunmap_atomic(ptr);
)

// Remove any pointers left unused
@
depends on memset_rule2
@
identifier memset_rule2.ptr;
type VP, VP1;
@@

-VP ptr;
	... when != ptr;
? VP1 ptr;

// </smpl>

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 fs/btrfs/compression.c |  5 +----
 fs/btrfs/extent_io.c   | 22 ++++------------------
 fs/btrfs/inode.c       | 33 ++++++++++-----------------------
 fs/btrfs/reflink.c     |  6 +-----
 fs/btrfs/zlib.c        |  5 +----
 fs/btrfs/zstd.c        |  5 +----
 6 files changed, 18 insertions(+), 58 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 047b632b4139..a219dcdb749e 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -567,16 +567,13 @@ static noinline int add_ra_bio_pages(struct inode *inode,
 		free_extent_map(em);
 
 		if (page->index == end_index) {
-			char *userpage;
 			size_t zero_offset = offset_in_page(isize);
 
 			if (zero_offset) {
 				int zeros;
 				zeros = PAGE_SIZE - zero_offset;
-				userpage = kmap_atomic(page);
-				memset(userpage + zero_offset, 0, zeros);
+				zero_user(page, zero_offset, zeros);
 				flush_dcache_page(page);
-				kunmap_atomic(userpage);
 			}
 		}
 
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index c9cee458e001..bdc9389bff59 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3229,15 +3229,12 @@ int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
 	}
 
 	if (page->index == last_byte >> PAGE_SHIFT) {
-		char *userpage;
 		size_t zero_offset = offset_in_page(last_byte);
 
 		if (zero_offset) {
 			iosize = PAGE_SIZE - zero_offset;
-			userpage = kmap_atomic(page);
-			memset(userpage + zero_offset, 0, iosize);
+			zero_user(page, zero_offset, iosize);
 			flush_dcache_page(page);
-			kunmap_atomic(userpage);
 		}
 	}
 	while (cur <= end) {
@@ -3245,14 +3242,11 @@ int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
 		u64 offset;
 
 		if (cur >= last_byte) {
-			char *userpage;
 			struct extent_state *cached = NULL;
 
 			iosize = PAGE_SIZE - pg_offset;
-			userpage = kmap_atomic(page);
-			memset(userpage + pg_offset, 0, iosize);
+			zero_user(page, pg_offset, iosize);
 			flush_dcache_page(page);
-			kunmap_atomic(userpage);
 			set_extent_uptodate(tree, cur, cur + iosize - 1,
 					    &cached, GFP_NOFS);
 			unlock_extent_cached(tree, cur,
@@ -3334,13 +3328,10 @@ int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
 
 		/* we've found a hole, just zero and go on */
 		if (block_start == EXTENT_MAP_HOLE) {
-			char *userpage;
 			struct extent_state *cached = NULL;
 
-			userpage = kmap_atomic(page);
-			memset(userpage + pg_offset, 0, iosize);
+			zero_user(page, pg_offset, iosize);
 			flush_dcache_page(page);
-			kunmap_atomic(userpage);
 
 			set_extent_uptodate(tree, cur, cur + iosize - 1,
 					    &cached, GFP_NOFS);
@@ -3654,12 +3645,7 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc,
 	}
 
 	if (page->index == end_index) {
-		char *userpage;
-
-		userpage = kmap_atomic(page);
-		memset(userpage + pg_offset, 0,
-		       PAGE_SIZE - pg_offset);
-		kunmap_atomic(userpage);
+		zero_user(page, pg_offset, PAGE_SIZE - pg_offset);
 		flush_dcache_page(page);
 	}
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a8e0a6b038d3..641f21a11722 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -640,17 +640,12 @@ static noinline int compress_file_range(struct async_chunk *async_chunk)
 		if (!ret) {
 			unsigned long offset = offset_in_page(total_compressed);
 			struct page *page = pages[nr_pages - 1];
-			char *kaddr;
 
 			/* zero the tail end of the last page, we might be
 			 * sending it down to disk
 			 */
-			if (offset) {
-				kaddr = kmap_atomic(page);
-				memset(kaddr + offset, 0,
-				       PAGE_SIZE - offset);
-				kunmap_atomic(kaddr);
-			}
+			if (offset)
+				zero_user(page, offset, PAGE_SIZE - offset);
 			will_compress = 1;
 		}
 	}
@@ -4675,7 +4670,6 @@ int btrfs_truncate_block(struct btrfs_inode *inode, loff_t from, loff_t len,
 	struct btrfs_ordered_extent *ordered;
 	struct extent_state *cached_state = NULL;
 	struct extent_changeset *data_reserved = NULL;
-	char *kaddr;
 	bool only_release_metadata = false;
 	u32 blocksize = fs_info->sectorsize;
 	pgoff_t index = from >> PAGE_SHIFT;
@@ -4765,15 +4759,13 @@ int btrfs_truncate_block(struct btrfs_inode *inode, loff_t from, loff_t len,
 	if (offset != blocksize) {
 		if (!len)
 			len = blocksize - offset;
-		kaddr = kmap(page);
 		if (front)
-			memset(kaddr + (block_start - page_offset(page)),
-				0, offset);
+			zero_user(page, (block_start - page_offset(page)),
+				  offset);
 		else
-			memset(kaddr + (block_start - page_offset(page)) +  offset,
-				0, len);
+			zero_user(page, (block_start - page_offset(page)) + offset,
+				  len);
 		flush_dcache_page(page);
-		kunmap(page);
 	}
 	ClearPageChecked(page);
 	set_page_dirty(page);
@@ -6660,11 +6652,9 @@ static noinline int uncompress_inline(struct btrfs_path *path,
 	 * cover that region here.
 	 */
 
-	if (max_size + pg_offset < PAGE_SIZE) {
-		char *map = kmap(page);
-		memset(map + pg_offset + max_size, 0, PAGE_SIZE - max_size - pg_offset);
-		kunmap(page);
-	}
+	if (max_size + pg_offset < PAGE_SIZE)
+		zero_user(page,  pg_offset + max_size,
+			  PAGE_SIZE - max_size - pg_offset);
 	kfree(tmp);
 	return ret;
 }
@@ -8303,7 +8293,6 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 	struct btrfs_ordered_extent *ordered;
 	struct extent_state *cached_state = NULL;
 	struct extent_changeset *data_reserved = NULL;
-	char *kaddr;
 	unsigned long zero_start;
 	loff_t size;
 	vm_fault_t ret;
@@ -8410,10 +8399,8 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 		zero_start = PAGE_SIZE;
 
 	if (zero_start != PAGE_SIZE) {
-		kaddr = kmap(page);
-		memset(kaddr + zero_start, 0, PAGE_SIZE - zero_start);
+		zero_user(page, zero_start, PAGE_SIZE - zero_start);
 		flush_dcache_page(page);
-		kunmap(page);
 	}
 	ClearPageChecked(page);
 	set_page_dirty(page);
diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
index 74c62e49c0c9..c052c4878670 100644
--- a/fs/btrfs/reflink.c
+++ b/fs/btrfs/reflink.c
@@ -126,12 +126,8 @@ static int copy_inline_to_page(struct btrfs_inode *inode,
 	 * So what's in the range [500, 4095] corresponds to zeroes.
 	 */
 	if (datal < block_size) {
-		char *map;
-
-		map = kmap(page);
-		memset(map + datal, 0, block_size - datal);
+		zero_user(page, datal, block_size - datal);
 		flush_dcache_page(page);
-		kunmap(page);
 	}
 
 	SetPageUptodate(page);
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index d524acf7b3e5..fd612a509463 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -375,7 +375,6 @@ int zlib_decompress(struct list_head *ws, unsigned char *data_in,
 	unsigned long bytes_left;
 	unsigned long total_out = 0;
 	unsigned long pg_offset = 0;
-	char *kaddr;
 
 	destlen = min_t(unsigned long, destlen, PAGE_SIZE);
 	bytes_left = destlen;
@@ -455,9 +454,7 @@ int zlib_decompress(struct list_head *ws, unsigned char *data_in,
 	 * end of the inline extent (destlen) to the end of the page
 	 */
 	if (pg_offset < destlen) {
-		kaddr = kmap_atomic(dest_page);
-		memset(kaddr + pg_offset, 0, destlen - pg_offset);
-		kunmap_atomic(kaddr);
+		zero_user(dest_page, pg_offset, destlen - pg_offset);
 	}
 	return ret;
 }
diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
index 8e9626d63976..b6f687b8a3da 100644
--- a/fs/btrfs/zstd.c
+++ b/fs/btrfs/zstd.c
@@ -631,7 +631,6 @@ int zstd_decompress(struct list_head *ws, unsigned char *data_in,
 	size_t ret2;
 	unsigned long total_out = 0;
 	unsigned long pg_offset = 0;
-	char *kaddr;
 
 	stream = ZSTD_initDStream(
 			ZSTD_BTRFS_MAX_INPUT, workspace->mem, workspace->size);
@@ -696,9 +695,7 @@ int zstd_decompress(struct list_head *ws, unsigned char *data_in,
 	ret = 0;
 finish:
 	if (pg_offset < destlen) {
-		kaddr = kmap_atomic(dest_page);
-		memset(kaddr + pg_offset, 0, destlen - pg_offset);
-		kunmap_atomic(kaddr);
+		zero_user(dest_page, pg_offset, destlen - pg_offset);
 	}
 	return ret;
 }
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH 0/4] btrfs: Convert kmaps to core page calls

[David Sterba]

On Fri, Feb 05, 2021 at 03:23:00PM -0800, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> There are many places where kmap/<operation>/kunmap patterns occur.  We lift
> these various patterns to core common functions and use them in the btrfs file
> system.  At the same time we convert those core functions to use
> kmap_local_page() which is more efficient in those calls.
> 
> I think this is best accepted through Andrew's tree as it has the mem*_page
> functions in it.  But I'd like to get an ack from David or one of the other
> btrfs maintainers before the btrfs patches go through.

I'd rather take the non-mm patches through my tree so it gets tested
the same way as other btrfs changes, straightforward cleanups or not.

This brings the question how to do that as the first patch should go
through the MM tree. One option is to posptpone the actual cleanups
after the 1st patch is merged but this could take a long delay.

I'd suggest to take the 1st patch within MM tree in the upcoming merge
window and then I can prepare a separate pull with just the cleanups.
Removing an inter-tree patch dependency was a sufficient reason for
Linus in the past for such pull requests.

> There are a lot more kmap->kmap_local_page() conversions but kmap_local_page()
> requires some care with the unmapping order and so I'm still reviewing those
> changes because btrfs uses a lot of loops for it's kmaps.

It sounds to me that converting the kmaps will take some time anyway so
exporting the helpers first and then converting the subsystems might be
a good option. In case you'd like to get rid of the simple cases in
btrfs code now we can do the 2 pull requests.

Re: [PATCH 0/4] btrfs: Convert kmaps to core page calls

[Ira Weiny]

On Tue, Feb 09, 2021 at 04:11:23PM +0100, David Sterba wrote:
> On Fri, Feb 05, 2021 at 03:23:00PM -0800, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > There are many places where kmap/<operation>/kunmap patterns occur.  We lift
> > these various patterns to core common functions and use them in the btrfs file
> > system.  At the same time we convert those core functions to use
> > kmap_local_page() which is more efficient in those calls.
> > 
> > I think this is best accepted through Andrew's tree as it has the mem*_page
> > functions in it.  But I'd like to get an ack from David or one of the other
> > btrfs maintainers before the btrfs patches go through.
> 
> I'd rather take the non-mm patches through my tree so it gets tested
> the same way as other btrfs changes, straightforward cleanups or not.

True.

> 
> This brings the question how to do that as the first patch should go
> through the MM tree. One option is to posptpone the actual cleanups
> after the 1st patch is merged but this could take a long delay.
> 
> I'd suggest to take the 1st patch within MM tree in the upcoming merge
> window and then I can prepare a separate pull with just the cleanups.
> Removing an inter-tree patch dependency was a sufficient reason for
> Linus in the past for such pull requests.

There are others how want this base patch too.[1]  So I like this option.

>
> > There are a lot more kmap->kmap_local_page() conversions but kmap_local_page()
> > requires some care with the unmapping order and so I'm still reviewing those
> > changes because btrfs uses a lot of loops for it's kmaps.
> 
> It sounds to me that converting the kmaps will take some time anyway so
> exporting the helpers first and then converting the subsystems might be
> a good option. In case you'd like to get rid of the simple cases in
> btrfs code now we can do the 2 pull requests.

I would really like to get the simple case out of the way because the next
series has more difficult changes and the simple cases always cause me trouble
when grepping/coccinelle'ing for things.

So I would like a follow on pull request if possible.  But I'm willing to do
what works best for you.

For now I will spin a new version with the changes you've requested ASAP.

Ira

[1] https://lore.kernel.org/linux-f2fs-devel/20210207190425.38107-1-chaitanya.kulkarni@wdc.com/

Re: [PATCH 0/4] btrfs: Convert kmaps to core page calls

[Andrew Morton]

On Tue, 9 Feb 2021 16:11:23 +0100 David Sterba <dsterba@suse.cz> wrote:

> On Fri, Feb 05, 2021 at 03:23:00PM -0800, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > There are many places where kmap/<operation>/kunmap patterns occur.  We lift
> > these various patterns to core common functions and use them in the btrfs file
> > system.  At the same time we convert those core functions to use
> > kmap_local_page() which is more efficient in those calls.
> > 
> > I think this is best accepted through Andrew's tree as it has the mem*_page
> > functions in it.  But I'd like to get an ack from David or one of the other
> > btrfs maintainers before the btrfs patches go through.
> 
> I'd rather take the non-mm patches through my tree so it gets tested
> the same way as other btrfs changes, straightforward cleanups or not.
> 
> This brings the question how to do that as the first patch should go
> through the MM tree. One option is to posptpone the actual cleanups
> after the 1st patch is merged but this could take a long delay.
> 
> I'd suggest to take the 1st patch within MM tree in the upcoming merge
> window and then I can prepare a separate pull with just the cleanups.
> Removing an inter-tree patch dependency was a sufficient reason for
> Linus in the past for such pull requests.

It would be best to merge [1/4] via the btrfs tree.  Please add my

Acked-by: Andrew Morton <akpm@linux-foundation.org>


Although I think it would be better if [1/4] merely did the code
movement.  Adding those BUG_ON()s is a semantic/functional change and
really shouldn't be bound up with the other things this patch series
does.  This logically separate change raises questions such as

- What is the impact on overall code size?  Not huge, presumably, but
  every little bit hurts.

- Additional runtime costs of those extra comparisons?

- These impacts could be lessened by using VM_BUG_ON() rather than
  BUG_ON() - should we do this?

- Linus reeeeeeeally doesn't like new BUG_ON()s.  Maybe you can sneak
  it past him ;)

See what I mean?  I do think it would be best to take those assertions
out of the patch and to propose them separately, at a later time.

Re: [PATCH 0/4] btrfs: Convert kmaps to core page calls

[Ira Weiny]

On Tue, Feb 09, 2021 at 11:09:31AM -0800, Andrew Morton wrote:
> On Tue, 9 Feb 2021 16:11:23 +0100 David Sterba <dsterba@suse.cz> wrote:
> 
> > On Fri, Feb 05, 2021 at 03:23:00PM -0800, ira.weiny@intel.com wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > There are many places where kmap/<operation>/kunmap patterns occur.  We lift
> > > these various patterns to core common functions and use them in the btrfs file
> > > system.  At the same time we convert those core functions to use
> > > kmap_local_page() which is more efficient in those calls.
> > > 
> > > I think this is best accepted through Andrew's tree as it has the mem*_page
> > > functions in it.  But I'd like to get an ack from David or one of the other
> > > btrfs maintainers before the btrfs patches go through.
> > 
> > I'd rather take the non-mm patches through my tree so it gets tested
> > the same way as other btrfs changes, straightforward cleanups or not.
> > 
> > This brings the question how to do that as the first patch should go
> > through the MM tree. One option is to posptpone the actual cleanups
> > after the 1st patch is merged but this could take a long delay.
> > 
> > I'd suggest to take the 1st patch within MM tree in the upcoming merge
> > window and then I can prepare a separate pull with just the cleanups.
> > Removing an inter-tree patch dependency was a sufficient reason for
> > Linus in the past for such pull requests.
> 
> It would be best to merge [1/4] via the btrfs tree.  Please add my
> 
> Acked-by: Andrew Morton <akpm@linux-foundation.org>
> 
> 
> Although I think it would be better if [1/4] merely did the code
> movement.  Adding those BUG_ON()s is a semantic/functional change and
> really shouldn't be bound up with the other things this patch series
> does.

I proposed this too and was told 'no'...

<quote>
If we put in into a separate patch, someone will suggest backing out the
patch which tells us that there's a problem.
</quote>
	-- https://lore.kernel.org/lkml/20201209201415.GT7338@casper.infradead.org/

> This logically separate change raises questions such as
> 
> - What is the impact on overall code size?  Not huge, presumably, but
>   every little bit hurts.
> 
> - Additional runtime costs of those extra comparisons?
> 
> - These impacts could be lessened by using VM_BUG_ON() rather than
>   BUG_ON() - should we do this?

<sigh>  I lost that argument last time around.

<quote>
BUG() is our only option here.  Both limiting how much we copy or
copying the requested amount result in data corruption or leaking
information to a process that isn't supposed to see it.
</quote>

https://lore.kernel.org/lkml/20201209040312.GN7338@casper.infradead.org/

CC'ing Matthew because I _really_ don't want to argue this any longer.

> 
> - Linus reeeeeeeally doesn't like new BUG_ON()s.  Maybe you can sneak
>   it past him ;)

I'm worried too...  :-(

> 
> See what I mean?

Yes I do however ...  see above...  :-/

Ira

> I do think it would be best to take those assertions
> out of the patch and to propose them separately, at a later time.
> 

Re: [PATCH 0/4] btrfs: Convert kmaps to core page calls

[Andrew Morton]

On Tue, 9 Feb 2021 12:52:49 -0800 Ira Weiny <ira.weiny@intel.com> wrote:

> On Tue, Feb 09, 2021 at 11:09:31AM -0800, Andrew Morton wrote:
> > On Tue, 9 Feb 2021 16:11:23 +0100 David Sterba <dsterba@suse.cz> wrote:
> > 
> > > On Fri, Feb 05, 2021 at 03:23:00PM -0800, ira.weiny@intel.com wrote:
> > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > 
> > > > There are many places where kmap/<operation>/kunmap patterns occur.  We lift
> > > > these various patterns to core common functions and use them in the btrfs file
> > > > system.  At the same time we convert those core functions to use
> > > > kmap_local_page() which is more efficient in those calls.
> > > > 
> > > > I think this is best accepted through Andrew's tree as it has the mem*_page
> > > > functions in it.  But I'd like to get an ack from David or one of the other
> > > > btrfs maintainers before the btrfs patches go through.
> > > 
> > > I'd rather take the non-mm patches through my tree so it gets tested
> > > the same way as other btrfs changes, straightforward cleanups or not.
> > > 
> > > This brings the question how to do that as the first patch should go
> > > through the MM tree. One option is to posptpone the actual cleanups
> > > after the 1st patch is merged but this could take a long delay.
> > > 
> > > I'd suggest to take the 1st patch within MM tree in the upcoming merge
> > > window and then I can prepare a separate pull with just the cleanups.
> > > Removing an inter-tree patch dependency was a sufficient reason for
> > > Linus in the past for such pull requests.
> > 
> > It would be best to merge [1/4] via the btrfs tree.  Please add my
> > 
> > Acked-by: Andrew Morton <akpm@linux-foundation.org>
> > 
> > 
> > Although I think it would be better if [1/4] merely did the code
> > movement.  Adding those BUG_ON()s is a semantic/functional change and
> > really shouldn't be bound up with the other things this patch series
> > does.
> 
> I proposed this too and was told 'no'...
> 
> <quote>
> If we put in into a separate patch, someone will suggest backing out the
> patch which tells us that there's a problem.
> </quote>
> 	-- https://lore.kernel.org/lkml/20201209201415.GT7338@casper.infradead.org/

Yeah, no, please let's not do this.  Bundling an offtopic change into
[1/4] then making three more patches dependent on the ontopic parts of
[1/4] is just rude.

I think the case for adding the BUG_ONs can be clearly made.  And that
case should at least have been clearly made in the [1/4] changelog!

(Although I expect VM_BUG_ON() would be better - will give us sufficient
coverage without the overall impact.)

Let's please queue this up separately.

Re: [PATCH 0/4] btrfs: Convert kmaps to core page calls

[Ira Weiny]

On Tue, Feb 09, 2021 at 01:11:03PM -0800, Andrew Morton wrote:
> > > 
> > > It would be best to merge [1/4] via the btrfs tree.  Please add my
> > > 
> > > Acked-by: Andrew Morton <akpm@linux-foundation.org>
> > > 
> > > 
> > > Although I think it would be better if [1/4] merely did the code
> > > movement.  Adding those BUG_ON()s is a semantic/functional change and
> > > really shouldn't be bound up with the other things this patch series
> > > does.
> > 
> > I proposed this too and was told 'no'...
> > 
> > <quote>
> > If we put in into a separate patch, someone will suggest backing out the
> > patch which tells us that there's a problem.
> > </quote>
> > 	-- https://lore.kernel.org/lkml/20201209201415.GT7338@casper.infradead.org/
> 
> Yeah, no, please let's not do this.  Bundling an offtopic change into
> [1/4] then making three more patches dependent on the ontopic parts of
> [1/4] is just rude.
> 
> I think the case for adding the BUG_ONs can be clearly made.  And that
> case should at least have been clearly made in the [1/4] changelog!
> 
> (Although I expect VM_BUG_ON() would be better - will give us sufficient
> coverage without the overall impact.)

I'm ok with VM_BUG_ON()

> 
> Let's please queue this up separately.

Ok can I retain your Ack on the move part of the patch?  Note that it does
change kmap_atomic() to kmap_local_page() currently.

Would you prefer a separate change for that as well?

Ira

PS really CC'ing Matthew now...

Re: [PATCH 0/4] btrfs: Convert kmaps to core page calls

[Andrew Morton]

On Tue, 9 Feb 2021 13:52:29 -0800 Ira Weiny <ira.weiny@intel.com> wrote:

> > 
> > Let's please queue this up separately.
> 
> Ok can I retain your Ack on the move part of the patch?

I missed that.

>  Note that it does change kmap_atomic() to kmap_local_page() currently.
> 
> Would you prefer a separate change for that as well?

Really that should be separated out as well, coming after the move, to
make it more easily reverted.  With a standalone changlog for this.

All a bit of a pain, but it's best in the long run.

Re: [PATCH 0/4] btrfs: Convert kmaps to core page calls

[Ira Weiny]

On Tue, Feb 09, 2021 at 01:58:37PM -0800, Andrew Morton wrote:
> On Tue, 9 Feb 2021 13:52:29 -0800 Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > > 
> > > Let's please queue this up separately.
> > 
> > Ok can I retain your Ack on the move part of the patch?
> 
> I missed that.
> 
> >  Note that it does change kmap_atomic() to kmap_local_page() currently.
> > 
> > Would you prefer a separate change for that as well?
> 
> Really that should be separated out as well, coming after the move, to
> make it more easily reverted.  With a standalone changlog for this.
> 
> All a bit of a pain, but it's best in the long run.

Consider it done.

Ira

BTW does anyone know the reason this thread is not making it to lore?  I don't
see any of the emails between Andrew and me?

	https://lore.kernel.org/lkml/20210205232304.1670522-1-ira.weiny@intel.com/

Re: [PATCH 0/4] btrfs: Convert kmaps to core page calls

[Andrew Morton]

On Tue, 9 Feb 2021 14:03:29 -0800 Ira Weiny <ira.weiny@intel.com> wrote:

> On Tue, Feb 09, 2021 at 01:58:37PM -0800, Andrew Morton wrote:
> > On Tue, 9 Feb 2021 13:52:29 -0800 Ira Weiny <ira.weiny@intel.com> wrote:
> > 
> > > > 
> > > > Let's please queue this up separately.
> > > 
> > > Ok can I retain your Ack on the move part of the patch?
> > 
> > I missed that.
> > 
> > >  Note that it does change kmap_atomic() to kmap_local_page() currently.
> > > 
> > > Would you prefer a separate change for that as well?
> > 
> > Really that should be separated out as well, coming after the move, to
> > make it more easily reverted.  With a standalone changlog for this.
> > 
> > All a bit of a pain, but it's best in the long run.
> 
> Consider it done.
> 
> Ira
> 
> BTW does anyone know the reason this thread is not making it to lore?  I don't
> see any of the emails between Andrew and me?
> 
> 	https://lore.kernel.org/lkml/20210205232304.1670522-1-ira.weiny@intel.com/


https://lkml.kernel.org/r/20210209220329.GF2975576@iweiny-DESK2.sc.intel.com
works OK.  It found the message in the linux-fsdevel archive.