On 2024/1/25 17:03, Yosry Ahmed wrote: >>>>>> The second difference is the handling of lru entry, which is easy that we >>>>>> just zswap_lru_del() in tree lock. >>>>> >>>>> Why do we need zswap_lru_del() at all? We should have already isolated >>>>> the entry at that point IIUC. >>>> >>>> I was thinking how to handle the "zswap_lru_putback()" if not writeback, >>>> in which case we can't use the entry actually since we haven't got reference >>>> of it. So we can don't isolate at the entry, and only zswap_lru_del() when >>>> we are going to writeback actually. >>> >>> Why not just call zswap_lru_putback() before we unlock the folio? >> >> When early return because __read_swap_cache_async() return NULL or !folio_was_allocated, >> we don't have a locked folio yet. The entry maybe invalidated and freed concurrently. > > Oh, that path, right. > > If we don't isolate the entry straightaway, concurrent reclaimers will > see the same entry, call __read_swap_cache_async(), find the folio > already in the swapcache and stop shrinking. This is because usually > this means we are racing with swapin and hitting the warmer part of > the zswap LRU. > > I am not sure if this would matter in practice, maybe Nhat knows > better. Perhaps we can rotate the entry in the LRU before calling > __read_swap_cache_async() to minimize the chances of such a race? Or > we can serialize the calls to __read_swap_cache_async() but this may > be an overkill. Also, not sure, rotate the entry maybe good IMHO since we will zswap_lru_del() once we checked the invalidate race.
