On Tue, Jun 11, 2024 at 11:47 AM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote: > > [..] > > > @@ -1336,6 +1347,7 @@ static void swap_entry_free(struct swap_info_struct *p, swp_entry_t entry) > > > count = p->swap_map[offset]; > > > VM_BUG_ON(count != SWAP_HAS_CACHE); > > > p->swap_map[offset] = 0; > > > + clear_bit(offset, p->zeromap); > > > > Hmm so clear_bit() is done at the swap_entry_free() point. I wonder if > > we can have a problem, where: > > > > 1. The swap entry has its zeromap bit set, and is freed to the swap > > slot cache (free_swap_slot() in mm/swap_slots.c). For instance, it is > > reclaimed from the swap cache, and all the processes referring to it > > are terminated, which decrements the swap count to 0 (swap_free() -> > > __swap_entry_free() -> free_swap_slots()) > > > > 2. The swap slot is then re-used in swap space allocation > > (add_to_swap()) - its zeromap bit is never cleared. > > I do not think this can happen before swap_entry_free() is called. > Note that when a swap entry is freed to the swap slot cache in > free_swap_slot(), it is added to cache->slots_ret, not cache->slots. > The former are swap entries cached to be later freed using > swap_entry_free(). Ahhh I see. Good point. Then yeah this should be safe from this POV. > > > > > 3. swap_writepage() writes that non-zero page to swap > > > > 4. swap_read_folio() checks the bitmap, sees that the zeromap bit for > > the entry is set, so populates a zero page for it. > > > > zswap in the past has to carefully invalidate these leftover entries > > quite carefully. Chengming then move the invalidation point to > > free_swap_slot(), massively simplifying the logic. > > I think the main benefit of moving the invalidation point was avoiding > leaving the compressed page in zswap until swap_entry_free() is > called, which will happen only when the swap slot caches are drained. > This is true. In this case yeah there's probably not much difference between clearing the bit here vs in swap_entry_free(). > > > > I wonder if we need to do the same here?
