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

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

[Chaitanya Kulkarni]

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

[RFC PATCH 1/8] brd: use memcpy_from_page() in copy_from_brd()

[Chaitanya Kulkarni]

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 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

[RFC PATCH 2/8] brd: use memcpy_from_page() in copy_from_brd()

[Chaitanya Kulkarni]

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 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

[RFC PATCH 3/8] null_blk: use memcpy_page() in copy_to_nullb()

[Chaitanya Kulkarni]

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 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

[RFC PATCH 4/8] null_blk: use memcpy_page() in copy_from_nullb()

[Chaitanya Kulkarni]

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 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

[RFC PATCH 5/8] ext4: use memcpy_from_page() in pagecache_read()

[Chaitanya Kulkarni]

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 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

[RFC PATCH 6/8] ext4: use memcpy_to_page() in pagecache_write()

[Chaitanya Kulkarni]

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 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

[RFC PATCH 7/8] f2fs: use memcpy_from_page() in pagecache_read()

[Chaitanya Kulkarni]

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 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

[RFC PATCH 8/8] f2fs: use memcpy_to_page() in pagecache_write()

[Chaitanya Kulkarni]

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 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

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

[Chaitanya Kulkarni]

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.

Re: [RFC PATCH 2/8] brd: use memcpy_from_page() in copy_from_brd()

[Ira Weiny]

On Sun, Feb 07, 2021 at 11:04:19AM -0800, Chaitanya Kulkarni wrote:
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  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);

Why 'offset'?

Ira

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

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

[Ira Weiny]

On Sun, Feb 07, 2021 at 07:10:41PM +0000, Chaitanya Kulkarni wrote:
> 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.

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

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

Thanks!
Ira

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

[Chaitanya Kulkarni]

On 2/7/21 20:42, Ira Weiny wrote:
> On Sun, Feb 07, 2021 at 07:10:41PM +0000, Chaitanya Kulkarni wrote:
>> 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.
> Except for the one comment I had this series look's good to me.
>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>
> 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.

Re: [RFC PATCH 2/8] brd: use memcpy_from_page() in copy_from_brd()

[Chaitanya Kulkarni]

>> @@ -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
>
>> +		else
>>  			memset(dst, 0, copy);
>>  	}
>>  }
>> -- 
>> 2.22.1
>>

Re: [RFC PATCH 8/8] f2fs: use memcpy_to_page() in pagecache_write()

[Eric Biggers]

On Sun, Feb 07, 2021 at 11:04:25AM -0800, Chaitanya Kulkarni wrote:
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

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.

>  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);

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

Re: [RFC PATCH 8/8] f2fs: use memcpy_to_page() in pagecache_write()

[Chaitanya Kulkarni]

On 2/10/21 10:01, Eric Biggers wrote:
> On Sun, Feb 07, 2021 at 11:04:25AM -0800, Chaitanya Kulkarni wrote:
>> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> 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.

Re: [RFC PATCH 6/8] ext4: use memcpy_to_page() in pagecache_write()

[Theodore Ts'o]

On Sun, Feb 07, 2021 at 11:04:23AM -0800, Chaitanya Kulkarni wrote:
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  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

Re: [RFC PATCH 6/8] ext4: use memcpy_to_page() in pagecache_write()

[Ira Weiny]

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:
> > Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> > ---
> >  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

Re: [RFC PATCH 6/8] ext4: use memcpy_to_page() in pagecache_write()

[Ira Weiny]

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:
> > Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> > ---
> >  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