[RFC PATCH 0/8] use core page calls instead of kmaps

This RFC is based on the discussion going on the linux-fsdevel [1].

I've tested this on the brd and null_blk. The fio verify job seems to
run without any error on the top of the original series applied [1].

Any feedback is welcome to move this forward.

-ck

[1] https://lore.kernel.org/linux-fsdevel/20210205232304.1670522-1-ira.weiny@intel.com/T/#m53145c155fa3631595594877da96a3a75b71e909

Chaitanya Kulkarni (8):
  brd: use memcpy_from_page() in copy_from_brd()
  brd: use memcpy_from_page() in copy_from_brd()
  null_blk: use memcpy_page() in copy_to_nullb()
  null_blk: use memcpy_page() in copy_from_nullb()
  ext4: use memcpy_from_page() in pagecache_read()
  ext4: use memcpy_to_page() in pagecache_write()
  f2fs: use memcpy_from_page() in pagecache_read()
  f2fs: use memcpy_to_page() in pagecache_write()

 drivers/block/brd.c           | 17 ++++++-----------
 drivers/block/null_blk/main.c | 23 ++++++-----------------
 fs/ext4/verity.c              | 10 ++--------
 fs/f2fs/verity.c              | 10 ++--------
 4 files changed, 16 insertions(+), 44 deletions(-)

-- 
2.22.1

Signed-off-by: Chaitanya Kulkarni <redacted>
---
 drivers/block/null_blk/main.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index d6c821d48090..c9b6db82b07c 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c

@@ -1010,7 +1010,6 @@ static int copy_to_nullb(struct nullb *nullb, struct page *source,
 	size_t temp, count = 0;
 	unsigned int offset;
 	struct nullb_page *t_page;
-	void *dst, *src;
 
 	while (count < n) {
 		temp = min_t(size_t, nullb->dev->blocksize, n - count);

@@ -1024,11 +1023,7 @@ static int copy_to_nullb(struct nullb *nullb, struct page *source,
 		if (!t_page)
 			return -ENOSPC;
 
-		src = kmap_atomic(source);
-		dst = kmap_atomic(t_page->page);
-		memcpy(dst + offset, src + off + count, temp);
-		kunmap_atomic(dst);
-		kunmap_atomic(src);
+		memcpy_page(t_page->page, offset, source, off + count, temp);
 
 		__set_bit(sector & SECTOR_MASK, t_page->bitmap);
 

-- 
2.22.1

Signed-off-by: Chaitanya Kulkarni <redacted>
---
 drivers/block/brd.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index cb5c872ac9b2..d41b7d489e9f 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c

@@ -226,11 +226,9 @@ static void copy_from_brd(void *dst, struct brd_device *brd,
 
 	copy = min_t(size_t, n, PAGE_SIZE - offset);
 	page = brd_lookup_page(brd, sector);
-	if (page) {
-		src = kmap_atomic(page);
-		memcpy(dst, src + offset, copy);
-		kunmap_atomic(src);
-	} else
+	if (page)
+		memcpy_from_page(dst, page, offset, copy);
+	else
 		memset(dst, 0, copy);
 
 	if (copy < n) {

-- 
2.22.1

Signed-off-by: Chaitanya Kulkarni <redacted>
---
 fs/ext4/verity.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
index 5b7ba8f71153..c8e07f8a792d 100644
--- a/fs/ext4/verity.c
+++ b/fs/ext4/verity.c

@@ -45,16 +45,13 @@ static int pagecache_read(struct inode *inode, void *buf, size_t count,
 		size_t n = min_t(size_t, count,
 				 PAGE_SIZE - offset_in_page(pos));
 		struct page *page;
-		void *addr;
 
 		page = read_mapping_page(inode->i_mapping, pos >> PAGE_SHIFT,
 					 NULL);
 		if (IS_ERR(page))
 			return PTR_ERR(page);
 
-		addr = kmap_atomic(page);
-		memcpy(buf, addr + offset_in_page(pos), n);
-		kunmap_atomic(addr);
+		memcpy_from_page(buf, page, offset_in_page(pos), n);
 
 		put_page(page);
 

-- 
2.22.1

Signed-off-by: Chaitanya Kulkarni <redacted>
---
 drivers/block/brd.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index d41b7d489e9f..c1f6d768a1b3 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c

@@ -220,7 +220,6 @@ static void copy_from_brd(void *dst, struct brd_device *brd,
 			sector_t sector, size_t n)
 {
 	struct page *page;
-	void *src;
 	unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
 	size_t copy;
 

@@ -236,11 +235,9 @@ static void copy_from_brd(void *dst, struct brd_device *brd,
 		sector += copy >> SECTOR_SHIFT;
 		copy = n - copy;
 		page = brd_lookup_page(brd, sector);
-		if (page) {
-			src = kmap_atomic(page);
-			memcpy(dst, src, copy);
-			kunmap_atomic(src);
-		} else
+		if (page)
+			memcpy_from_page(dst, page, offset, copy);
+		else
 			memset(dst, 0, copy);
 	}
 }

-- 
2.22.1

Signed-off-by: Chaitanya Kulkarni <redacted>
---
 drivers/block/null_blk/main.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index c9b6db82b07c..1c0e1a295e90 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c

@@ -1042,7 +1042,6 @@ static int copy_from_nullb(struct nullb *nullb, struct page *dest,
 	size_t temp, count = 0;
 	unsigned int offset;
 	struct nullb_page *t_page;
-	void *dst, *src;
 
 	while (count < n) {
 		temp = min_t(size_t, nullb->dev->blocksize, n - count);

@@ -1051,16 +1050,11 @@ static int copy_from_nullb(struct nullb *nullb, struct page *dest,
 		t_page = null_lookup_page(nullb, sector, false,
 			!null_cache_active(nullb));
 
-		dst = kmap_atomic(dest);
-		if (!t_page) {
-			memset(dst + off + count, 0, temp);
-			goto next;
-		}
-		src = kmap_atomic(t_page->page);
-		memcpy(dst + off + count, src + offset, temp);
-		kunmap_atomic(src);
-next:
-		kunmap_atomic(dst);
+		if (t_page)
+			memcpy_page(dest, off + count, t_page->page, offset,
+				    temp);
+		else
+			zero_user(dest, off + count, temp);
 
 		count += temp;
 		sector += temp >> SECTOR_SHIFT;

-- 
2.22.1

Signed-off-by: Chaitanya Kulkarni <redacted>
---
 fs/ext4/verity.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
index c8e07f8a792d..cc4b046490b0 100644
--- a/fs/ext4/verity.c
+++ b/fs/ext4/verity.c

@@ -77,7 +77,6 @@ static int pagecache_write(struct inode *inode, const void *buf, size_t count,
 				 PAGE_SIZE - offset_in_page(pos));
 		struct page *page;
 		void *fsdata;
-		void *addr;
 		int res;
 
 		res = pagecache_write_begin(NULL, inode->i_mapping, pos, n, 0,

@@ -85,9 +84,7 @@ static int pagecache_write(struct inode *inode, const void *buf, size_t count,
 		if (res)
 			return res;
 
-		addr = kmap_atomic(page);
-		memcpy(addr + offset_in_page(pos), buf, n);
-		kunmap_atomic(addr);
+		memcpy_to_page(page, offset_in_page(pos), buf, n);
 
 		res = pagecache_write_end(NULL, inode->i_mapping, pos, n, n,
 					  page, fsdata);

-- 
2.22.1

Signed-off-by: Chaitanya Kulkarni <redacted>
---
 fs/f2fs/verity.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/f2fs/verity.c b/fs/f2fs/verity.c
index 44e057bdc416..ca019685a944 100644
--- a/fs/f2fs/verity.c
+++ b/fs/f2fs/verity.c

@@ -79,7 +79,6 @@ static int pagecache_write(struct inode *inode, const void *buf, size_t count,
 				 PAGE_SIZE - offset_in_page(pos));
 		struct page *page;
 		void *fsdata;
-		void *addr;
 		int res;
 
 		res = pagecache_write_begin(NULL, inode->i_mapping, pos, n, 0,

@@ -87,9 +86,7 @@ static int pagecache_write(struct inode *inode, const void *buf, size_t count,
 		if (res)
 			return res;
 
-		addr = kmap_atomic(page);
-		memcpy(addr + offset_in_page(pos), buf, n);
-		kunmap_atomic(addr);
+		memcpy_to_page(page, offset_in_page(pos) buf, n);
 
 		res = pagecache_write_end(NULL, inode->i_mapping, pos, n, n,
 					  page, fsdata);

-- 
2.22.1

Signed-off-by: Chaitanya Kulkarni <redacted>
---
 fs/f2fs/verity.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/f2fs/verity.c b/fs/f2fs/verity.c
index 054ec852b5ea..44e057bdc416 100644
--- a/fs/f2fs/verity.c
+++ b/fs/f2fs/verity.c

@@ -47,16 +47,13 @@ static int pagecache_read(struct inode *inode, void *buf, size_t count,
 		size_t n = min_t(size_t, count,
 				 PAGE_SIZE - offset_in_page(pos));
 		struct page *page;
-		void *addr;
 
 		page = read_mapping_page(inode->i_mapping, pos >> PAGE_SHIFT,
 					 NULL);
 		if (IS_ERR(page))
 			return PTR_ERR(page);
 
-		addr = kmap_atomic(page);
-		memcpy(buf, addr + offset_in_page(pos), n);
-		kunmap_atomic(addr);
+		memcpy_from_page(buf, page, offset_in_page(pos), n);
 
 		put_page(page);
 

-- 
2.22.1

On 2/7/21 11:04, Chaitanya Kulkarni wrote:

Chaitanya Kulkarni (8):
  brd: use memcpy_from_page() in copy_from_brd()
  brd: use memcpy_from_page() in copy_from_brd()

I'm aware that couple of places in brd code we can use memcpy_to_page()
and get rid the local variable, once I get some feedback I'll add those
to the V1.

On Sun, Feb 07, 2021 at 11:04:19AM -0800, Chaitanya Kulkarni wrote:

Why 'offset'?

Ira

+		else
 			memset(dst, 0, copy);
 	}
 }
-- 
2.22.1

On Sun, Feb 07, 2021 at 07:10:41PM +0000, Chaitanya Kulkarni wrote:

On 2/7/21 11:04, Chaitanya Kulkarni wrote:

quoted

Chaitanya Kulkarni (8):
  brd: use memcpy_from_page() in copy_from_brd()
  brd: use memcpy_from_page() in copy_from_brd()

I'm aware that couple of places in brd code we can use memcpy_to_page()
and get rid the local variable, once I get some feedback I'll add those
to the V1.

Except for the one comment I had this series look's good to me.

Reviewed-by: Ira Weiny <redacted>

Thanks!
Ira

On 2/7/21 20:42, Ira Weiny wrote:

On Sun, Feb 07, 2021 at 07:10:41PM +0000, Chaitanya Kulkarni wrote:

quoted

On 2/7/21 11:04, Chaitanya Kulkarni wrote:

quoted

Chaitanya Kulkarni (8):
  brd: use memcpy_from_page() in copy_from_brd()
  brd: use memcpy_from_page() in copy_from_brd()

I'm aware that couple of places in brd code we can use memcpy_to_page()
and get rid the local variable, once I get some feedback I'll add those
to the V1.

Except for the one comment I had this series look's good to me.

Reviewed-by: Ira Weiny <redacted>

Thanks!
Ira

Thanks Ira, I'll add your fix in the V1 and send with your review-by tag,
when this code is upstream.

Thanks a lot for doing this we can get rid of go in null_blk which makes
code smooth.

quoted

@@ -236,11 +235,9 @@ static void copy_from_brd(void *dst, struct brd_device *brd,
 		sector += copy >> SECTOR_SHIFT;
 		copy = n - copy;
 		page = brd_lookup_page(brd, sector);
-		if (page) {
-			src = kmap_atomic(page);
-			memcpy(dst, src, copy);
-			kunmap_atomic(src);
-		} else
+		if (page)
+			memcpy_from_page(dst, page, offset, copy);

Why 'offset'?

Will fix it in the V1.

Ira

quoted

+		else
 			memset(dst, 0, copy);
 	}
 }
-- 
2.22.1

On Sun, Feb 07, 2021 at 11:04:25AM -0800, Chaitanya Kulkarni wrote:

Signed-off-by: Chaitanya Kulkarni <redacted>

No explanation in commit message?  There isn't much explanation needed for this,
but there should be at least one sentence.

Likewise for the other patches.

This is missing a comma between 'offset_in_page(pos)' and 'buf'.

Otherwise the patches to fs/{ext4,f2fs}/verity.c look fine; thanks for doing
this!

- Eric

On 2/10/21 10:01, Eric Biggers wrote:

On Sun, Feb 07, 2021 at 11:04:25AM -0800, Chaitanya Kulkarni wrote:

quoted

Signed-off-by: Chaitanya Kulkarni <redacted>

No explanation in commit message?  There isn't much explanation needed for this,
but there should be at least one sentence.

Likewise for the other patches.

Thanks Eric for your comment.

Sent out minimal RFC, I'll add detail explanation also having original
series
in the tree helps.

On Sun, Feb 07, 2021 at 11:04:23AM -0800, Chaitanya Kulkarni wrote:

Signed-off-by: Chaitanya Kulkarni <redacted>
---
 fs/ext4/verity.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Hi, were you expecting to have file system maintainers take these
patches into their own trees?  The ext4 patches look good, and unless
you have any objections, I can take them through the ext4 tree.

Thanks,

    	     		       	    - Ted

On Thu, Mar 25, 2021 at 10:22:54AM -0400, Theodore Y. Ts'o wrote:

On Sun, Feb 07, 2021 at 11:04:23AM -0800, Chaitanya Kulkarni wrote:

quoted

Signed-off-by: Chaitanya Kulkarni <redacted>
---
 fs/ext4/verity.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Hi, were you expecting to have file system maintainers take these
patches into their own trees?  The ext4 patches look good, and unless
you have any objections, I can take them through the ext4 tree.

I think going through the ext4 tree would be fine.

However, there was a fix needed in patch 2/8.  Chaitanya was a V1 sent?

Ira

On Thu, Mar 25, 2021 at 10:22:54AM -0400, Theodore Y. Ts'o wrote:

On Sun, Feb 07, 2021 at 11:04:23AM -0800, Chaitanya Kulkarni wrote:

quoted

Signed-off-by: Chaitanya Kulkarni <redacted>
---
 fs/ext4/verity.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Hi, were you expecting to have file system maintainers take these
patches into their own trees?  The ext4 patches look good, and unless
you have any objections, I can take them through the ext4 tree.

I should have sent the lore link to the fix:

https://lore.kernel.org/linux-f2fs-devel/BYAPR04MB496564B786E293FDA21D06E6868F9@BYAPR04MB4965.namprd04.prod.outlook.com/

Sorry,
Ira