On Thu, 16 Feb 2023, Matthew Wilcox wrote:
> > - len = (remaining_size > PAGE_SIZE) ? PAGE_SIZE : remaining_size;
> > -
> > - bio_add_page(clone, page, len, 0);
> > +have_pages:
> > + page->compound_order = order;
>
> No. You'll corrupt the next page if page is order-0, which it is if it
> came from the mempool. Also we've deleted page->compound_order in -next
> so you can't make this mistake. Using __GFP_COMP will set this field
> for you, so you can just drop this line.
OK
> > - remaining_size -= len;
> > + for (o = 0; o < 1U << order; o++) {
> > + unsigned len = min((unsigned)PAGE_SIZE, remaining_size);
> > + bio_add_page(clone, page, len, 0);
> > + remaining_size -= len;
> > + page++;
>
> You can add multiple pages at once, whether they're compound or not. So
> replace this entire loop with:
>
> bio_add_page(clone, page, remaining_size, 0);
This should be min((unsigned)PAGE_SIZE << order, remaining_size), because
we may allocate less than remaining_size.
> > @@ -1711,10 +1732,23 @@ static void crypt_free_buffer_pages(stru
> > {
> > struct bio_vec *bv;
> > struct bvec_iter_all iter_all;
> > + unsigned skip_entries = 0;
> >
> > bio_for_each_segment_all(bv, clone, iter_all) {
> > - BUG_ON(!bv->bv_page);
> > - mempool_free(bv->bv_page, &cc->page_pool);
> > + unsigned order;
> > + struct page *page = bv->bv_page;
> > + BUG_ON(!page);
> > + if (skip_entries) {
> > + skip_entries--;
> > + continue;
> > + }
> > + order = page->compound_order;
> > + if (order) {
> > + __free_pages(page, order);
> > + skip_entries = (1U << order) - 1;
> > + } else {
> > + mempool_free(page, &cc->page_pool);
> > + }
>
> You can simplify this by using the folio code.
>
> struct folio_iter fi;
>
> bio_for_each_folio_all(fi, bio) {
> if (folio_test_large(folio))
> folio_put(folio);
> else
> mempool_free(&folio->page, &cc->page_pool);
> }
OK. I'm sending version 2 of the patch.
> (further work would actually convert this driver to use folios instead
> of pages)
Mikulas
