| Message ID | 20220614104718.9193-1-fmdefrancesco@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] btrfs: Replace kmap() with kmap_local_page() in zlib.c | expand |
On Tue, Jun 14, 2022 at 12:47:18PM +0200, Fabio M. De Francesco wrote: [snip] > @@ -148,26 +148,26 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping, > int i; > > for (i = 0; i < in_buf_pages; i++) { > - if (in_page) { > - kunmap(in_page); > + if (data_in) { > + kunmap_local(data_in); > put_page(in_page); > } > in_page = find_get_page(mapping, > start >> PAGE_SHIFT); > - data_in = kmap(in_page); > + data_in = kmap_local_page(in_page); > memcpy(workspace->buf + i * PAGE_SIZE, > data_in, PAGE_SIZE); > start += PAGE_SIZE; This is moving start forward... > } > workspace->strm.next_in = workspace->buf; > } else { > - if (in_page) { > - kunmap(in_page); > + if (data_in) { > + kunmap_local(data_in); > put_page(in_page); > } > in_page = find_get_page(mapping, > start >> PAGE_SHIFT); > - data_in = kmap(in_page); > + data_in = kmap_local_page(in_page); > start += PAGE_SIZE; > workspace->strm.next_in = data_in; > } > @@ -196,9 +196,14 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping, > * the stream end if required > */ > if (workspace->strm.avail_out == 0) { > - kunmap(out_page); > + kunmap_local(data_in); > + data_in = NULL; > + put_page(in_page); I don't think you need to put/get the page to do the remapping. > + > + kunmap_local(cpage_out); > if (nr_pages == nr_dest_pages) { > - out_page = NULL; > + cpage_out = NULL; > + put_page(out_page); > ret = -E2BIG; > goto out; > } > @@ -207,7 +212,14 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping, > ret = -ENOMEM; > goto out; > } > - cpage_out = kmap(out_page); > + cpage_out = kmap_local_page(out_page); > + > + in_page = find_get_page(mapping, start >> PAGE_SHIFT); I think start is the wrong value here. It got incremented above. > + data_in = kmap_local_page(in_page); > + workspace->strm.next_in = data_in; > + workspace->strm.avail_in = min(bytes_left, > + (unsigned long)workspace->buf_size); I'm not sure about changing avail_in here either because nothing has happened with the in buffer whilst doing this re-mapping AFAICS. [snip] I think the issue with the tests is contained to zlib_compress_pages(). But an idea to verify that is to split this patch between the changes in zlib_compress_pages() [above] and zlib_decompress_bio() [below] because below looks very straight forward. Ira > @@ -287,7 +311,7 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb) > unsigned long buf_start; > struct page **pages_in = cb->compressed_pages; > > - data_in = kmap(pages_in[page_in_index]); > + data_in = kmap_local_page(pages_in[page_in_index]); > workspace->strm.next_in = data_in; > workspace->strm.avail_in = min_t(size_t, srclen, PAGE_SIZE); > workspace->strm.total_in = 0; > @@ -309,7 +333,7 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb) > > if (Z_OK != zlib_inflateInit2(&workspace->strm, wbits)) { > pr_warn("BTRFS: inflateInit failed\n"); > - kunmap(pages_in[page_in_index]); > + kunmap_local(data_in); > return -EIO; > } > while (workspace->strm.total_in < srclen) { > @@ -336,13 +360,14 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb) > > if (workspace->strm.avail_in == 0) { > unsigned long tmp; > - kunmap(pages_in[page_in_index]); > + > + kunmap_local(data_in); > page_in_index++; > if (page_in_index >= total_pages_in) { > data_in = NULL; > break; > } > - data_in = kmap(pages_in[page_in_index]); > + data_in = kmap_local_page(pages_in[page_in_index]); > workspace->strm.next_in = data_in; > tmp = srclen - workspace->strm.total_in; > workspace->strm.avail_in = min(tmp, PAGE_SIZE); > @@ -355,7 +380,7 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb) > done: > zlib_inflateEnd(&workspace->strm); > if (data_in) > - kunmap(pages_in[page_in_index]); > + kunmap_local(data_in); > if (!ret) > zero_fill_bio(cb->orig_bio); > return ret; > -- > 2.36.1 >
On martedì 14 giugno 2022 12:47:18 CEST 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 zlib.c because in > this file the mappings are per thread and are not visible in other > contexts; meanwhile refactor zlib_compress_pages() to comply with nested > local mapping / unmapping ordering rules. > > Tested with xfstests on QEMU + KVM 32 bits VM with 4GB of RAM and > HIGHMEM64G enabled. > > This is an RFC PATCH because it passes all tests of the "compress" > group, with the only two exceptions of tests/btrfs/138 (it freezes the > VM) and tests/btrfs/251 (it runs forever but doesn't freeze the VM). > > Can anyone please take a look and tell me what I'm still overlooking > after days of code inspections? > > Suggested-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > fs/btrfs/zlib.c | 75 ++++++++++++++++++++++++++++++++----------------- > 1 file changed, 50 insertions(+), 25 deletions(-) > Please don't respond to this RFC and drop it. I've just split it into three new versions and send a series of RFC PATCH(es) which seems to work properly. They can pass all tests of the group "compress", but I decided to implement some questionable solutions. Therefore, I'd like to hear about them: https://lore.kernel.org/lkml/20220617120538.18091-1-fmdefrancesco@gmail.com/T/#t Thanks, Fabio
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c index 767a0c6c9694..18f111bb2deb 100644 --- a/fs/btrfs/zlib.c +++ b/fs/btrfs/zlib.c @@ -97,8 +97,8 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping, { struct workspace *workspace = list_entry(ws, struct workspace, list); int ret; - char *data_in; - char *cpage_out; + char *data_in = NULL; + char *cpage_out = NULL; int nr_pages = 0; struct page *in_page = NULL; struct page *out_page = NULL; @@ -126,7 +126,7 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping, ret = -ENOMEM; goto out; } - cpage_out = kmap(out_page); + cpage_out = kmap_local_page(out_page); pages[0] = out_page; nr_pages = 1; @@ -148,26 +148,26 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping, int i; for (i = 0; i < in_buf_pages; i++) { - if (in_page) { - kunmap(in_page); + if (data_in) { + kunmap_local(data_in); put_page(in_page); } in_page = find_get_page(mapping, start >> PAGE_SHIFT); - data_in = kmap(in_page); + data_in = kmap_local_page(in_page); memcpy(workspace->buf + i * PAGE_SIZE, data_in, PAGE_SIZE); start += PAGE_SIZE; } workspace->strm.next_in = workspace->buf; } else { - if (in_page) { - kunmap(in_page); + if (data_in) { + kunmap_local(data_in); put_page(in_page); } in_page = find_get_page(mapping, start >> PAGE_SHIFT); - data_in = kmap(in_page); + data_in = kmap_local_page(in_page); start += PAGE_SIZE; workspace->strm.next_in = data_in; } @@ -196,9 +196,14 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping, * the stream end if required */ if (workspace->strm.avail_out == 0) { - kunmap(out_page); + kunmap_local(data_in); + data_in = NULL; + put_page(in_page); + + kunmap_local(cpage_out); if (nr_pages == nr_dest_pages) { - out_page = NULL; + cpage_out = NULL; + put_page(out_page); ret = -E2BIG; goto out; } @@ -207,7 +212,14 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping, ret = -ENOMEM; goto out; } - cpage_out = kmap(out_page); + cpage_out = kmap_local_page(out_page); + + in_page = find_get_page(mapping, start >> PAGE_SHIFT); + data_in = kmap_local_page(in_page); + workspace->strm.next_in = data_in; + workspace->strm.avail_in = min(bytes_left, + (unsigned long)workspace->buf_size); + pages[nr_pages] = out_page; nr_pages++; workspace->strm.avail_out = PAGE_SIZE; @@ -233,10 +245,15 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping, ret = -EIO; goto out; } else if (workspace->strm.avail_out == 0) { + kunmap_local(data_in); + data_in = NULL; + put_page(in_page); + /* get another page for the stream end */ - kunmap(out_page); + kunmap_local(cpage_out); if (nr_pages == nr_dest_pages) { - out_page = NULL; + cpage_out = NULL; + put_page(out_page); ret = -E2BIG; goto out; } @@ -245,7 +262,14 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping, ret = -ENOMEM; goto out; } - cpage_out = kmap(out_page); + cpage_out = kmap_local_page(out_page); + + in_page = find_get_page(mapping, start >> PAGE_SHIFT); + data_in = kmap_local_page(in_page); + workspace->strm.next_in = data_in; + workspace->strm.avail_in = min(bytes_left, + (unsigned long)workspace->buf_size); + pages[nr_pages] = out_page; nr_pages++; workspace->strm.avail_out = PAGE_SIZE; @@ -264,13 +288,13 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping, *total_in = workspace->strm.total_in; out: *out_pages = nr_pages; - if (out_page) - kunmap(out_page); - - if (in_page) { - kunmap(in_page); + if (data_in) { + kunmap_local(data_in); put_page(in_page); } + if (cpage_out) + kunmap_local(cpage_out); + return ret; } @@ -287,7 +311,7 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb) unsigned long buf_start; struct page **pages_in = cb->compressed_pages; - data_in = kmap(pages_in[page_in_index]); + data_in = kmap_local_page(pages_in[page_in_index]); workspace->strm.next_in = data_in; workspace->strm.avail_in = min_t(size_t, srclen, PAGE_SIZE); workspace->strm.total_in = 0; @@ -309,7 +333,7 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb) if (Z_OK != zlib_inflateInit2(&workspace->strm, wbits)) { pr_warn("BTRFS: inflateInit failed\n"); - kunmap(pages_in[page_in_index]); + kunmap_local(data_in); return -EIO; } while (workspace->strm.total_in < srclen) { @@ -336,13 +360,14 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb) if (workspace->strm.avail_in == 0) { unsigned long tmp; - kunmap(pages_in[page_in_index]); + + kunmap_local(data_in); page_in_index++; if (page_in_index >= total_pages_in) { data_in = NULL; break; } - data_in = kmap(pages_in[page_in_index]); + data_in = kmap_local_page(pages_in[page_in_index]); workspace->strm.next_in = data_in; tmp = srclen - workspace->strm.total_in; workspace->strm.avail_in = min(tmp, PAGE_SIZE); @@ -355,7 +380,7 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb) done: zlib_inflateEnd(&workspace->strm); if (data_in) - kunmap(pages_in[page_in_index]); + kunmap_local(data_in); if (!ret) zero_fill_bio(cb->orig_bio); 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 zlib.c because in this file the mappings are per thread and are not visible in other contexts; meanwhile refactor zlib_compress_pages() to comply with nested local mapping / unmapping ordering rules. Tested with xfstests on QEMU + KVM 32 bits VM with 4GB of RAM and HIGHMEM64G enabled. This is an RFC PATCH because it passes all tests of the "compress" group, with the only two exceptions of tests/btrfs/138 (it freezes the VM) and tests/btrfs/251 (it runs forever but doesn't freeze the VM). Can anyone please take a look and tell me what I'm still overlooking after days of code inspections? Suggested-by: Ira Weiny <ira.weiny@intel.com> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> --- fs/btrfs/zlib.c | 75 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 50 insertions(+), 25 deletions(-)