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/[email protected]/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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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:
> Signed-off-by: Chaitanya Kulkarni <[email protected]>
> ---
> 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
>
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 <[email protected]>
Thanks!
Ira
>> @@ -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
>>
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 <[email protected]>
>
> 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.
On Sun, Feb 07, 2021 at 11:04:25AM -0800, Chaitanya Kulkarni wrote:
> Signed-off-by: Chaitanya Kulkarni <[email protected]>
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
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 <[email protected]>
> 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 <[email protected]>
> ---
> 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:
> > Signed-off-by: Chaitanya Kulkarni <[email protected]>
> > ---
> > 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:
> > Signed-off-by: Chaitanya Kulkarni <[email protected]>
> > ---
> > 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