On Tue, 15 Jul 2025, Dongsheng Yang wrote: > Hi Mikulas, > This is V4 for dm-pcache, please help to review. Hi The mempool operations seem ok. Here are some comments: * backing_req_cache and backing_bvec_cache should be static * get_n_vecs: what if "data" is not page-aligned? is that possible? * cache_dev_get_empty_segment_id: change set_bit to __set_bit because it doesn't have to be atomic - you already hold the lock * you usually hold seg_map_lock around modifications of seg_map bitmap, but not in kset_replay and cache_replay - that seems like a bug. When you hold the spinlock always, you can change set_bit/clear_bit to __set_bit and __clear_bit * cache.c: cache->seg_map = bitmap_zalloc(cache_dev->seg_num, GFP_KERNEL); * cache_dev.c: cache_dev->seg_bitmap = bitmap_zalloc(cache_dev->seg_num, GFP_KERNEL); how big seg_num can be? allocating more than 32768 bytes with kmalloc is unreliable (kvmalloc should be used in this case). * cache->ksets = kcalloc(cache->n_ksets, PCACHE_KSET_SIZE, GFP_KERNEL); * kset_onmedia = kzalloc(PCACHE_KSET_ONMEDIA_SIZE_MAX, GFP_KERNEL); - could these allocations be larger than 32768 bytes? * There's cpu_to_le32 when you set sb->seg_num: sb->seg_num = cpu_to_le32(nr_segs), but not le32_to_cpu when you read it: ret = cache_dev_init(cache_dev, sb.seg_num); - it seems that this will not work on big-endian machines. Just a suggestion for performance improvement: when you hold a spinlock a you need to allocate some memory, you must drop the spinlock, allocate it with GFP_NOIO, reacquire the spinlock and recheck the state. You can improve this logic by allocating with mempool_alloc(GFP_NOWAIT) while you hold the spinlock (GFP_NOWAIT will not sleep, so it's allowed), and fallback to dropping the spinlock only if the GFP_NOWAIT allocation fails. Mikulas
