Message ID | 20230329204652.52785-1-kch@nvidia.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 3/29/23 2:46 PM, Chaitanya Kulkarni wrote: > Hi, > > From :include/linux/highmem.h: > "kmap_atomic - Atomically map a page for temporary usage - Deprecated!" > > Use memcpy_from_page() since does the same job of mapping, copying, and > unmaping except it uses non deprecated kmap_local_page() and > kunmap_local(). Following are the differences between kmal_local_page() > and kmap_atomic() :- Looks fine to me, but I'd fold patches 1-3 rather than split them up.
On 3/29/23 15:48, Jens Axboe wrote: > On 3/29/23 2:46 PM, Chaitanya Kulkarni wrote: >> Hi, >> >> From :include/linux/highmem.h: >> "kmap_atomic - Atomically map a page for temporary usage - Deprecated!" >> >> Use memcpy_from_page() since does the same job of mapping, copying, and >> unmaping except it uses non deprecated kmap_local_page() and >> kunmap_local(). Following are the differences between kmal_local_page() >> and kmap_atomic() :- > Looks fine to me, but I'd fold patches 1-3 rather than split them up. > Sent V2 with above comment, first three patches are from different code path and they are doing unrelated changes:- 1. WRITE :- copy_to_nullb() only use memcpy_page(). 2. READ :- copy_from_nullb() only use memcapy_page() and zero_user(). 3. I guess zoned read beyond write pointer null_fill_pattern() memset_page(). if anything goes wrong in any of 3 code paths we will have to entire change which we shouldn't, that's why kept it separate, I'm fine with whatever you decide ... -ck
On 3/30/23 12:56?PM, Chaitanya Kulkarni wrote: > On 3/29/23 15:48, Jens Axboe wrote: >> On 3/29/23 2:46?PM, Chaitanya Kulkarni wrote: >>> Hi, >>> >>> From :include/linux/highmem.h: >>> "kmap_atomic - Atomically map a page for temporary usage - Deprecated!" >>> >>> Use memcpy_from_page() since does the same job of mapping, copying, and >>> unmaping except it uses non deprecated kmap_local_page() and >>> kunmap_local(). Following are the differences between kmal_local_page() >>> and kmap_atomic() :- >> Looks fine to me, but I'd fold patches 1-3 rather than split them up. >> > > Sent V2 with above comment, first three patches are from > different code path and they are doing unrelated changes:- > > 1. WRITE :- copy_to_nullb() only use memcpy_page(). > 2. READ :- copy_from_nullb() only use memcapy_page() and zero_user(). > 3. I guess zoned read beyond write pointer null_fill_pattern() > memset_page(). > > if anything goes wrong in any of 3 code paths we will have to > entire change which we shouldn't, that's why kept it separate, > I'm fine with whatever you decide ... It doesn't matter... It's not like this is a hugely complicated thing. It's just a straight forward conversion. They all just switch to using a helper, which is the right thing to do as it reduces the stuff we have to do in there. Only reason why I excluded patch 4 is that it is a bit different than the others. But it really could just be one patch.
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index bc2c58724df3..46b3381cd42b 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -1030,6 +1030,7 @@ static int null_flush_cache_page(struct nullb *nullb, struct nullb_page *c_page) if (!t_page) return -ENOMEM; + pr_info("%s %d kmap_local_page()\n", __func__, __LINE__); src = kmap_local_page(c_page->page); dst = kmap_local_page(t_page->page); @@ -1125,6 +1126,7 @@ static int copy_to_nullb(struct nullb *nullb, struct page *source, if (!t_page) return -ENOSPC; + pr_info("%s %d memcpy_page()\n", __func__, __LINE__); memcpy_page(t_page->page, offset, source, off + count, temp); __set_bit(sector & SECTOR_MASK, t_page->bitmap); @@ -1152,11 +1154,14 @@ static int copy_from_nullb(struct nullb *nullb, struct page *dest, t_page = null_lookup_page(nullb, sector, false, !null_cache_active(nullb)); - if (t_page) + if (t_page) { memcpy_page(dest, off + count, t_page->page, offset, temp); - else + pr_info("%s %d memcpy_page()\n", __func__, __LINE__); + } else { zero_user(dest, off + count, temp); + pr_info("%s %d zero_user()\n", __func__, __LINE__); + } count += temp; sector += temp >> SECTOR_SHIFT; @@ -1167,6 +1172,7 @@ static int copy_from_nullb(struct nullb *nullb, struct page *dest, static void nullb_fill_pattern(struct nullb *nullb, struct page *page, unsigned int len, unsigned int off) { + pr_info("%s %d memset_page()\n", __func__, __LINE__); memset_page(page, off, 0xff, len); }