Messages in this thread | | | Date | Tue, 14 Jun 2022 07:13:49 -0700 | From | Ira Weiny <> | Subject | Re: [RFC PATCH] btrfs: Replace kmap() with kmap_local_page() in zlib.c |
| |
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 >
| |