| Message ID | 20220616210037.7060-3-fmdefrancesco@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Replace kmap() with kmap_local_page() in zstd.c | expand |
On Thu, Jun 16, 2022 at 11:00:36PM +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; meanwhile refactor zstd_compress_pages() to comply with the > ordering rules about nested local mapping / unmapping. > > Tested with xfstests on QEMU + KVM 32 bits VM with 4GB of RAM and > HIGHMEM64G enabled. These changes passed all the tests of the group > "compress". > > Cc: Filipe Manana <fdmanana@kernel.org> > Suggested-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > > v3->v4: Cc Maintainers and lists that had been overlooked when v3 was > sent (mostly regarding patch 1/2). > > v2->v3: Remove unnecessary casts to arguments of kunmap_local() now that > this API can take pointers to const void. > > v1->v2: No changes. > > Thanks to Ira Weiny for his invaluable help and persevering support. > Thanks also to Filipe Manana for identifying a fundamental detail I had > overlooked in RFC: > https://lore.kernel.org/lkml/20220611093411.GA3779054@falcondesktop/ > > fs/btrfs/zstd.c | 42 +++++++++++++++++++++++------------------- > 1 file changed, 23 insertions(+), 19 deletions(-) > > diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c > index 0fe31a6f6e68..5d2ab0bac9d2 100644 > --- a/fs/btrfs/zstd.c > +++ b/fs/btrfs/zstd.c > @@ -391,6 +391,8 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, > *out_pages = 0; > *total_out = 0; > *total_in = 0; > + workspace->in_buf.src = NULL; > + workspace->out_buf.dst = NULL; I don't think either of these are needed as they are both set straight away below. > > /* Initialize the stream */ > stream = zstd_init_cstream(¶ms, len, workspace->mem, > @@ -403,7 +405,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 +417,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 = kmap_local_page(out_page); Given the conversation in the other thread for zlib_compress_pages(); I think we should also just use page_address() here. That simplifies the algorithm immensely. I know there was a lot of thought put into how to make this conversion when this was posted but that is now the cleaner solution. Ira > workspace->out_buf.pos = 0; > workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE); > > @@ -450,9 +452,9 @@ 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); > + kunmap_local(workspace->out_buf.dst); > if (nr_pages == nr_dest_pages) { > - out_page = NULL; > + workspace->out_buf.dst = NULL; > ret = -E2BIG; > goto out; > } > @@ -462,7 +464,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 = kmap_local_page(out_page); > workspace->out_buf.pos = 0; > workspace->out_buf.size = min_t(size_t, max_out, > PAGE_SIZE); > @@ -477,15 +479,16 @@ 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->out_buf.dst); > + 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 = kmap_local_page(out_page); > } > } > while (1) { > @@ -510,9 +513,9 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, > > tot_out += PAGE_SIZE; > max_out -= PAGE_SIZE; > - kunmap(out_page); > + kunmap_local(workspace->out_buf.dst); > if (nr_pages == nr_dest_pages) { > - out_page = NULL; > + workspace->out_buf.dst = NULL; > ret = -E2BIG; > goto out; > } > @@ -522,7 +525,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 = kmap_local_page(out_page); > workspace->out_buf.pos = 0; > workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE); > } > @@ -538,12 +541,12 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, > out: > *out_pages = nr_pages; > /* Cleanup */ > - if (in_page) { > - kunmap(in_page); > + if (workspace->out_buf.dst) > + kunmap_local(workspace->out_buf.dst); > + 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 +570,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 +606,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 +623,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; > } > > -- > 2.36.1 >
diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c index 0fe31a6f6e68..5d2ab0bac9d2 100644 --- a/fs/btrfs/zstd.c +++ b/fs/btrfs/zstd.c @@ -391,6 +391,8 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, *out_pages = 0; *total_out = 0; *total_in = 0; + workspace->in_buf.src = NULL; + workspace->out_buf.dst = NULL; /* Initialize the stream */ stream = zstd_init_cstream(¶ms, len, workspace->mem, @@ -403,7 +405,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 +417,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 = kmap_local_page(out_page); workspace->out_buf.pos = 0; workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE); @@ -450,9 +452,9 @@ 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); + kunmap_local(workspace->out_buf.dst); if (nr_pages == nr_dest_pages) { - out_page = NULL; + workspace->out_buf.dst = NULL; ret = -E2BIG; goto out; } @@ -462,7 +464,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 = kmap_local_page(out_page); workspace->out_buf.pos = 0; workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE); @@ -477,15 +479,16 @@ 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->out_buf.dst); + 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 = kmap_local_page(out_page); } } while (1) { @@ -510,9 +513,9 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, tot_out += PAGE_SIZE; max_out -= PAGE_SIZE; - kunmap(out_page); + kunmap_local(workspace->out_buf.dst); if (nr_pages == nr_dest_pages) { - out_page = NULL; + workspace->out_buf.dst = NULL; ret = -E2BIG; goto out; } @@ -522,7 +525,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 = kmap_local_page(out_page); workspace->out_buf.pos = 0; workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE); } @@ -538,12 +541,12 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping, out: *out_pages = nr_pages; /* Cleanup */ - if (in_page) { - kunmap(in_page); + if (workspace->out_buf.dst) + kunmap_local(workspace->out_buf.dst); + 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 +570,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 +606,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 +623,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; meanwhile refactor zstd_compress_pages() to comply with the ordering rules about nested local mapping / unmapping. Tested with xfstests on QEMU + KVM 32 bits VM with 4GB of RAM and HIGHMEM64G enabled. These changes passed all the tests of the group "compress". Cc: Filipe Manana <fdmanana@kernel.org> Suggested-by: Ira Weiny <ira.weiny@intel.com> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> --- v3->v4: Cc Maintainers and lists that had been overlooked when v3 was sent (mostly regarding patch 1/2). v2->v3: Remove unnecessary casts to arguments of kunmap_local() now that this API can take pointers to const void. v1->v2: No changes. Thanks to Ira Weiny for his invaluable help and persevering support. Thanks also to Filipe Manana for identifying a fundamental detail I had overlooked in RFC: https://lore.kernel.org/lkml/20220611093411.GA3779054@falcondesktop/ fs/btrfs/zstd.c | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-)