| Message ID | 20220704152322.20955-3-fmdefrancesco@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: Replace kmap() with kmap_local_page() in zstd.c | expand |
On Mon, Jul 04, 2022 at 05:23:22PM +0200, Fabio M. De Francesco wrote: > The use of kmap() is being deprecated in favor of kmap_local_page(). With > kmap_local_page(), the mapping is per thread, CPU local and not globally > visible. > > Therefore, use kmap_local_page() / kunmap_local() in zstd.c because in this > file the mappings are per thread and are not visible in other contexts. In > the meanwhile use plain page_address() on pages allocated with the GFP_NOFS > flag instead of calling kmap*() on them (since they are always allocated > from ZONE_NORMAL). > > Tested with xfstests on QEMU + KVM 32 bits VM with 4GB of RAM and > booting a kernel with HIGHMEM64G enabled. > > Cc: Filipe Manana <fdmanana@kernel.org> > Suggested-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > fs/btrfs/zstd.c | 34 ++++++++++++++-------------------- > 1 file changed, 14 insertions(+), 20 deletions(-) > > diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c > index 0fe31a6f6e68..78e0272e770e 100644 > --- a/fs/btrfs/zstd.c > +++ b/fs/btrfs/zstd.c > @@ -403,7 +403,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, > > /* map in the first page of input data */ > in_page = find_get_page(mapping, start >> PAGE_SHIFT); > - workspace->in_buf.src = kmap(in_page); > + workspace->in_buf.src = kmap_local_page(in_page); > workspace->in_buf.pos = 0; > workspace->in_buf.size = min_t(size_t, len, PAGE_SIZE); > > @@ -415,7 +415,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, > goto out; > } > pages[nr_pages++] = out_page; > - workspace->out_buf.dst = kmap(out_page); > + workspace->out_buf.dst = page_address(out_page); > workspace->out_buf.pos = 0; > workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE); > > @@ -450,9 +450,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, > if (workspace->out_buf.pos == workspace->out_buf.size) { > tot_out += PAGE_SIZE; > max_out -= PAGE_SIZE; > - kunmap(out_page); > if (nr_pages == nr_dest_pages) { > - out_page = NULL; > ret = -E2BIG; > goto out; > } > @@ -462,7 +460,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, > goto out; > } > pages[nr_pages++] = out_page; > - workspace->out_buf.dst = kmap(out_page); > + workspace->out_buf.dst = page_address(out_page); > workspace->out_buf.pos = 0; > workspace->out_buf.size = min_t(size_t, max_out, > PAGE_SIZE); > @@ -477,15 +475,15 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, > /* Check if we need more input */ > if (workspace->in_buf.pos == workspace->in_buf.size) { > tot_in += PAGE_SIZE; > - kunmap(in_page); > + kunmap_local(workspace->in_buf.src); > put_page(in_page); > - > start += PAGE_SIZE; > len -= PAGE_SIZE; > in_page = find_get_page(mapping, start >> PAGE_SHIFT); > - workspace->in_buf.src = kmap(in_page); > + workspace->in_buf.src = kmap_local_page(in_page); > workspace->in_buf.pos = 0; > workspace->in_buf.size = min_t(size_t, len, PAGE_SIZE); > + workspace->out_buf.dst = page_address(out_page); Why is this needed? The rest looks good, Ira [snip]
On martedì 5 luglio 2022 21:04:04 CEST Ira Weiny wrote: > On Mon, Jul 04, 2022 at 05:23:22PM +0200, Fabio M. De Francesco wrote: > > The use of kmap() is being deprecated in favor of kmap_local_page(). With > > kmap_local_page(), the mapping is per thread, CPU local and not globally > > visible. > > > > Therefore, use kmap_local_page() / kunmap_local() in zstd.c because in this > > file the mappings are per thread and are not visible in other contexts. In > > the meanwhile use plain page_address() on pages allocated with the GFP_NOFS > > flag instead of calling kmap*() on them (since they are always allocated > > from ZONE_NORMAL). > > > > Tested with xfstests on QEMU + KVM 32 bits VM with 4GB of RAM and > > booting a kernel with HIGHMEM64G enabled. > > > > Cc: Filipe Manana <fdmanana@kernel.org> > > Suggested-by: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > > --- > > fs/btrfs/zstd.c | 34 ++++++++++++++-------------------- > > 1 file changed, 14 insertions(+), 20 deletions(-) > > > > diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c > > index 0fe31a6f6e68..78e0272e770e 100644 > > --- a/fs/btrfs/zstd.c > > +++ b/fs/btrfs/zstd.c > > @@ -403,7 +403,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, > > > > /* map in the first page of input data */ > > in_page = find_get_page(mapping, start >> PAGE_SHIFT); > > - workspace->in_buf.src = kmap(in_page); > > + workspace->in_buf.src = kmap_local_page(in_page); > > workspace->in_buf.pos = 0; > > workspace->in_buf.size = min_t(size_t, len, PAGE_SIZE); > > > > @@ -415,7 +415,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, > > goto out; > > } > > pages[nr_pages++] = out_page; > > - workspace->out_buf.dst = kmap(out_page); > > + workspace->out_buf.dst = page_address(out_page); > > workspace->out_buf.pos = 0; > > workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE); > > > > @@ -450,9 +450,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, > > if (workspace->out_buf.pos == workspace->out_buf.size) { > > tot_out += PAGE_SIZE; > > max_out -= PAGE_SIZE; > > - kunmap(out_page); > > if (nr_pages == nr_dest_pages) { > > - out_page = NULL; > > ret = -E2BIG; > > goto out; > > } > > @@ -462,7 +460,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, > > goto out; > > } > > pages[nr_pages++] = out_page; > > - workspace->out_buf.dst = kmap(out_page); > > + workspace->out_buf.dst = page_address(out_page); > > workspace->out_buf.pos = 0; > > workspace->out_buf.size = min_t(size_t, max_out, > > PAGE_SIZE); > > @@ -477,15 +475,15 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, > > /* Check if we need more input */ > > if (workspace->in_buf.pos == workspace->in_buf.size) { > > tot_in += PAGE_SIZE; > > - kunmap(in_page); > > + kunmap_local(workspace->in_buf.src); > > put_page(in_page); > > - > > start += PAGE_SIZE; > > len -= PAGE_SIZE; > > in_page = find_get_page(mapping, start >> PAGE_SHIFT); > > - workspace->in_buf.src = kmap(in_page); > > + workspace->in_buf.src = kmap_local_page(in_page); > > workspace->in_buf.pos = 0; > > workspace->in_buf.size = min_t(size_t, len, PAGE_SIZE); > > + workspace->out_buf.dst = page_address(out_page); > > Why is this needed? Sorry. This initialization is not needed at all. Probably made a mistake with copy-pasting snippets of code. I'm going to send ASAP the fifth version of this series. > The rest looks good, Thanks, Fabio > Ira > > [snip] >
On mercoledì 6 luglio 2022 12:38:29 CEST Fabio M. De Francesco wrote: > On martedì 5 luglio 2022 21:04:04 CEST Ira Weiny wrote: > > On Mon, Jul 04, 2022 at 05:23:22PM +0200, Fabio M. De Francesco wrote: > > > The use of kmap() is being deprecated in favor of kmap_local_page(). > With > > > kmap_local_page(), the mapping is per thread, CPU local and not > globally > > > visible. > > > > > > Therefore, use kmap_local_page() / kunmap_local() in zstd.c because in > this > > > file the mappings are per thread and are not visible in other contexts. > In > > > the meanwhile use plain page_address() on pages allocated with the > GFP_NOFS > > > flag instead of calling kmap*() on them (since they are always > allocated > > > from ZONE_NORMAL). > > > > > > Tested with xfstests on QEMU + KVM 32 bits VM with 4GB of RAM and > > > booting a kernel with HIGHMEM64G enabled. > > > > > > Cc: Filipe Manana <fdmanana@kernel.org> > > > Suggested-by: Ira Weiny <ira.weiny@intel.com> > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > > > --- > > > fs/btrfs/zstd.c | 34 ++++++++++++++-------------------- > > > 1 file changed, 14 insertions(+), 20 deletions(-) > > > > > > diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c > > > index 0fe31a6f6e68..78e0272e770e 100644 > > > --- a/fs/btrfs/zstd.c > > > +++ b/fs/btrfs/zstd.c > > > @@ -403,7 +403,7 @@ int zstd_compress_pages(struct list_head *ws, > struct address_space *mapping, > > > > > > /* map in the first page of input data */ > > > in_page = find_get_page(mapping, start >> PAGE_SHIFT); > > > - workspace->in_buf.src = kmap(in_page); > > > + workspace->in_buf.src = kmap_local_page(in_page); > > > workspace->in_buf.pos = 0; > > > workspace->in_buf.size = min_t(size_t, len, PAGE_SIZE); > > > > > > @@ -415,7 +415,7 @@ int zstd_compress_pages(struct list_head *ws, > struct address_space *mapping, > > > goto out; > > > } > > > pages[nr_pages++] = out_page; > > > - workspace->out_buf.dst = kmap(out_page); > > > + workspace->out_buf.dst = page_address(out_page); > > > workspace->out_buf.pos = 0; > > > workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE); > > > > > > @@ -450,9 +450,7 @@ int zstd_compress_pages(struct list_head *ws, > struct address_space *mapping, > > > if (workspace->out_buf.pos == workspace->out_buf.size) > { > > > tot_out += PAGE_SIZE; > > > max_out -= PAGE_SIZE; > > > - kunmap(out_page); > > > if (nr_pages == nr_dest_pages) { > > > - out_page = NULL; > > > ret = -E2BIG; > > > goto out; > > > } > > > @@ -462,7 +460,7 @@ int zstd_compress_pages(struct list_head *ws, > struct address_space *mapping, > > > goto out; > > > } > > > pages[nr_pages++] = out_page; > > > - workspace->out_buf.dst = kmap(out_page); > > > + workspace->out_buf.dst = > page_address(out_page); > > > workspace->out_buf.pos = 0; > > > workspace->out_buf.size = min_t(size_t, > max_out, > > > > PAGE_SIZE); > > > @@ -477,15 +475,15 @@ int zstd_compress_pages(struct list_head *ws, > struct address_space *mapping, > > > /* Check if we need more input */ > > > if (workspace->in_buf.pos == workspace->in_buf.size) { > > > tot_in += PAGE_SIZE; > > > - kunmap(in_page); > > > + kunmap_local(workspace->in_buf.src); > > > put_page(in_page); > > > - > > > start += PAGE_SIZE; > > > len -= PAGE_SIZE; > > > in_page = find_get_page(mapping, start >> > PAGE_SHIFT); > > > - workspace->in_buf.src = kmap(in_page); > > > + workspace->in_buf.src = > kmap_local_page(in_page); > > > workspace->in_buf.pos = 0; > > > workspace->in_buf.size = min_t(size_t, len, > PAGE_SIZE); > > > + workspace->out_buf.dst = > page_address(out_page); > > > > Why is this needed? > > Sorry. This initialization is not needed at all. > Probably made a mistake with copy-pasting snippets of code. > > I'm going to send ASAP the fifth version of this series. "fifth" -> "sixth". > > > The rest looks good, > > Thanks, > > Fabio > > > Ira > > > > [snip] > > > > > >
diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c index 0fe31a6f6e68..78e0272e770e 100644 --- a/fs/btrfs/zstd.c +++ b/fs/btrfs/zstd.c @@ -403,7 +403,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, /* map in the first page of input data */ in_page = find_get_page(mapping, start >> PAGE_SHIFT); - workspace->in_buf.src = kmap(in_page); + workspace->in_buf.src = kmap_local_page(in_page); workspace->in_buf.pos = 0; workspace->in_buf.size = min_t(size_t, len, PAGE_SIZE); @@ -415,7 +415,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, goto out; } pages[nr_pages++] = out_page; - workspace->out_buf.dst = kmap(out_page); + workspace->out_buf.dst = page_address(out_page); workspace->out_buf.pos = 0; workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE); @@ -450,9 +450,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, if (workspace->out_buf.pos == workspace->out_buf.size) { tot_out += PAGE_SIZE; max_out -= PAGE_SIZE; - kunmap(out_page); if (nr_pages == nr_dest_pages) { - out_page = NULL; ret = -E2BIG; goto out; } @@ -462,7 +460,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, goto out; } pages[nr_pages++] = out_page; - workspace->out_buf.dst = kmap(out_page); + workspace->out_buf.dst = page_address(out_page); workspace->out_buf.pos = 0; workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE); @@ -477,15 +475,15 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, /* Check if we need more input */ if (workspace->in_buf.pos == workspace->in_buf.size) { tot_in += PAGE_SIZE; - kunmap(in_page); + kunmap_local(workspace->in_buf.src); put_page(in_page); - start += PAGE_SIZE; len -= PAGE_SIZE; in_page = find_get_page(mapping, start >> PAGE_SHIFT); - workspace->in_buf.src = kmap(in_page); + workspace->in_buf.src = kmap_local_page(in_page); workspace->in_buf.pos = 0; workspace->in_buf.size = min_t(size_t, len, PAGE_SIZE); + workspace->out_buf.dst = page_address(out_page); } } while (1) { @@ -510,9 +508,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, tot_out += PAGE_SIZE; max_out -= PAGE_SIZE; - kunmap(out_page); if (nr_pages == nr_dest_pages) { - out_page = NULL; ret = -E2BIG; goto out; } @@ -522,7 +518,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, goto out; } pages[nr_pages++] = out_page; - workspace->out_buf.dst = kmap(out_page); + workspace->out_buf.dst = page_address(out_page); workspace->out_buf.pos = 0; workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE); } @@ -537,13 +533,10 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, *total_out = tot_out; out: *out_pages = nr_pages; - /* Cleanup */ - if (in_page) { - kunmap(in_page); + if (workspace->in_buf.src) { + kunmap_local(workspace->in_buf.src); put_page(in_page); } - if (out_page) - kunmap(out_page); return ret; } @@ -567,7 +560,7 @@ int zstd_decompress_bio(struct list_head *ws, struct compressed_bio *cb) goto done; } - workspace->in_buf.src = kmap(pages_in[page_in_index]); + workspace->in_buf.src = kmap_local_page(pages_in[page_in_index]); workspace->in_buf.pos = 0; workspace->in_buf.size = min_t(size_t, srclen, PAGE_SIZE); @@ -603,14 +596,15 @@ int zstd_decompress_bio(struct list_head *ws, struct compressed_bio *cb) break; if (workspace->in_buf.pos == workspace->in_buf.size) { - kunmap(pages_in[page_in_index++]); + kunmap_local(workspace->in_buf.src); + page_in_index++; if (page_in_index >= total_pages_in) { workspace->in_buf.src = NULL; ret = -EIO; goto done; } srclen -= PAGE_SIZE; - workspace->in_buf.src = kmap(pages_in[page_in_index]); + workspace->in_buf.src = kmap_local_page(pages_in[page_in_index]); workspace->in_buf.pos = 0; workspace->in_buf.size = min_t(size_t, srclen, PAGE_SIZE); } @@ -619,7 +613,7 @@ int zstd_decompress_bio(struct list_head *ws, struct compressed_bio *cb) zero_fill_bio(cb->orig_bio); done: if (workspace->in_buf.src) - kunmap(pages_in[page_in_index]); + kunmap_local(workspace->in_buf.src); return ret; }
The use of kmap() is being deprecated in favor of kmap_local_page(). With kmap_local_page(), the mapping is per thread, CPU local and not globally visible. Therefore, use kmap_local_page() / kunmap_local() in zstd.c because in this file the mappings are per thread and are not visible in other contexts. In the meanwhile use plain page_address() on pages allocated with the GFP_NOFS flag instead of calling kmap*() on them (since they are always allocated from ZONE_NORMAL). Tested with xfstests on QEMU + KVM 32 bits VM with 4GB of RAM and booting a kernel with HIGHMEM64G enabled. Cc: Filipe Manana <fdmanana@kernel.org> Suggested-by: Ira Weiny <ira.weiny@intel.com> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> --- fs/btrfs/zstd.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-)