Hi On Fri, 25 Jul 2025, Dongsheng Yang wrote:
> > * get_n_vecs: what if "data" is not page-aligned? is that possible? > Yes, it's possible, so I am using DIV_ROUND_UP() to calculate the n_vecs. DIV_ROUND_UP is not sufficient - suppose that "data" is 0xf00 and "len" is 0x200. DIV_ROUND_UP(len, PAGE_SIZE) returns "1", while you need two bio vector entries in fact. I'd change DIV_ROUND_UP(len, PAGE_SIZE) to DIV_ROUND_UP(offset_in_page(data) + len, PAGE_SIZE) > > * 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 > Okey > > > > * 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 > > actually, that's intented, because there is no other accessor in replaying > stage. So I dont hold the lock here, that would be faster. OK, so you don't have to hold the lock, you can just add a comment that the lock it not needed because the code is single-threaded. > But I am glad to add lock here, On the one hand, this change makes its use > more consistent and switches it to `__set_bit`. > On the other hand, if we later add support for parallel replay, we’ll need a > lock to handle the race conditions during this stage. > > > > > > * 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). > > #define PCACHE_CACHE_SEGS_MAX (1024 * 1024) /* maximum cache > size for each device is 16T */ > > > kvcalloc(BITS_TO_LONGS(cache_dev->seg_num), sizeof(unsigned long), > GFP_KERNEL); OK. > > * cache->ksets = kcalloc(cache->n_ksets, PCACHE_KSET_SIZE, GFP_KERNEL); > `n_ksets` is equal to the number of online CPUs, so this allocation could > exceed 32768 bytes; I’ll switch it to `kvcalloc`. OK. BTW. how does it handle seting CPUs online or offline while the dm-pcache target is running? > > * kset_onmedia = kzalloc(PCACHE_KSET_ONMEDIA_SIZE_MAX, GFP_KERNEL); > > - could these allocations be larger than 32768 bytes? > > The value is fixed—currently **5144**—and even if we extend it later, it > shouldn’t exceed 32768 bytes for the time being. OK. > > * 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. > > > Correct, it need le32_to_cpu(), I missed it here. > > > > > 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. > > > Sounds a good suggestion, I will try it in V5. Yes. When you implement it, don't forget to test the code path that drops the spinlock and uses mempool_alloc(GFP_NOIO), so that it doesn't bitrot over time. Mikulas
