On 10/25/23 12:13, Mikulas Patocka wrote: > So, I forward this to memory management maintainers. Hi, > What do you think? - We have a problem that if dm-crypt allocates pages > with order > 3 (PAGE_ALLOC_COSTLY_ORDER), the system occasionally freezes > waiting in writeback. Hmm, so I've checked the backtraces provided and none seems to show the actual allocating task doing the crypt_alloc_buffer(), do we know what it's doing? Is it blocked or perhaps spinning in some infinite loop? > dm-crypt allocates the pages with GFP_NOWAIT | __GFP_HIGHMEM | > __GFP_NOMEMALLOC | __GFP_NORETRY | __GFP_NOWARN | __GFP_COMP, so it > shouldn't put any pressure on the system. If the allocations fails, it > falls back to smaller order allocation or to mempool as a last resort. Right. I noticed it may also fallback to __GFP_DIRECT_RECLAIM but then it's only order-0. > When the freeze happens, there is "349264kB" free memory - so the system > is not short on memory. Yeah, it may still be fragmented, although in [1] the sysqr show memory report suggests it's not, pages do exist up to MAX_ORDER. Weird. [1] https://lore.kernel.org/all/ZTiJ3CO8w0jauOzW@mail-itl/ > Should we restrict the dm-crypt allocation size to > PAGE_ALLOC_COSTLY_ORDER? Or is it a bug somewhere in memory management > system that needs to be fixes there? Haven't found any. However I'd like to point out some things I noticed in crypt_alloc_buffer(), although they are probably not related. > static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size) > { > struct crypt_config *cc = io->cc; > struct bio *clone; > unsigned int nr_iovecs = (size + PAGE_SIZE - 1) >> PAGE_SHIFT; > gfp_t gfp_mask = GFP_NOWAIT | __GFP_HIGHMEM; > unsigned int remaining_size; > unsigned int order = MAX_ORDER - 1; > > retry: > if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM)) > mutex_lock(&cc->bio_alloc_lock); What if we end up in "goto retry" more than once? I don't see a matching unlock. Yeah, very unlikely to happen that order-0 in page allocator which includes __GFP_DIRECT_RECLAIM would fail, but not impossible, and also I see crypt_page_alloc() for the mempool can fail for another reason, due to a counter being too high. Looks dangerous. > > clone = bio_alloc_bioset(cc->dev->bdev, nr_iovecs, io->base_bio->bi_opf, > GFP_NOIO, &cc->bs); > clone->bi_private = io; > clone->bi_end_io = crypt_endio; > > remaining_size = size; > > while (remaining_size) { > struct page *pages; > unsigned size_to_add; > unsigned remaining_order = __fls((remaining_size + PAGE_SIZE - 1) >> PAGE_SHIFT); Tip: you can use get_order(remaining_size) here. > order = min(order, remaining_order); > > while (order > 0) { Is this intentionally > 0 and not >= 0? We could still succeed avoiding mempool with order-0... > pages = alloc_pages(gfp_mask > | __GFP_NOMEMALLOC | __GFP_NORETRY | __GFP_NOWARN | __GFP_COMP, > order); > if (likely(pages != NULL)) > goto have_pages; > order--; > } > > pages = mempool_alloc(&cc->page_pool, gfp_mask); > if (!pages) { > crypt_free_buffer_pages(cc, clone); > bio_put(clone); > gfp_mask |= __GFP_DIRECT_RECLAIM; > order = 0; > goto retry; > } > > have_pages: > size_to_add = min((unsigned)PAGE_SIZE << order, remaining_size); > __bio_add_page(clone, pages, size_to_add, 0); > remaining_size -= size_to_add; > } > > /* Allocate space for integrity tags */ > if (dm_crypt_integrity_io_alloc(io, clone)) { > crypt_free_buffer_pages(cc, clone); > bio_put(clone); > clone = NULL; > } > > if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM)) > mutex_unlock(&cc->bio_alloc_lock); > > return clone; > }
