On Fri, Jan 31, 2025 at 5:21 PM Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> wrote: > > Change zswap_store_page() to return a boolean value, where true > indicates the page is successfully stored in zswap, and false otherwise. > > Since zswap_store_page() no longer returns the size of the entry, > remove 'bytes' variable from zswap_store(), and jump to the put_pool label > when it fails. > > Move the objcg charging and the incrementing of zswap_store_pages > just after the tree store is successful. > At that point, there's no chance of failure, and the freeing path > will certainly uncharge zswap memory and decrement the counter. > > Suggested-by: Yosry Ahmed <yosry.ahmed@xxxxxxxxx> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx> > --- Uh, I was trying to send it in-reply-to this: https://lore.kernel.org/linux-mm/20250129100844.2935-1-42.hyeyoo@xxxxxxxxx Andrew, could you please fold it into the patch? No major functional difference, just adjusted Yosry's comments. > mm/zswap.c | 31 +++++++++++++++---------------- > 1 file changed, 15 insertions(+), 16 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index f0bd962bffd5..ac9d299e7d0c 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -1445,9 +1445,9 @@ static void shrink_worker(struct work_struct *w) > * main API > **********************************/ > > -static ssize_t zswap_store_page(struct page *page, > - struct obj_cgroup *objcg, > - struct zswap_pool *pool) > +static bool zswap_store_page(struct page *page, > + struct obj_cgroup *objcg, > + struct zswap_pool *pool) > { > swp_entry_t page_swpentry = page_swap_entry(page); > struct zswap_entry *entry, *old; > @@ -1456,7 +1456,7 @@ static ssize_t zswap_store_page(struct page *page, > entry = zswap_entry_cache_alloc(GFP_KERNEL, page_to_nid(page)); > if (!entry) { > zswap_reject_kmemcache_fail++; > - return -EINVAL; > + return false; > } > > if (!zswap_compress(page, entry, pool)) > @@ -1483,13 +1483,17 @@ static ssize_t zswap_store_page(struct page *page, > > /* > * The entry is successfully compressed and stored in the tree, there is > - * no further possibility of failure. Grab refs to the pool and objcg. > - * These refs will be dropped by zswap_entry_free() when the entry is > - * removed from the tree. > + * no further possibility of failure. Grab refs to the pool and objcg, > + * charge zswap memory, and increment zswap_stored_pages. > + * The opposite actions will be performed by zswap_entry_free() > + * when the entry is removed from the tree. > */ > zswap_pool_get(pool); > - if (objcg) > + if (objcg) { > obj_cgroup_get(objcg); > + obj_cgroup_charge_zswap(objcg, entry->length); > + } > + atomic_long_inc(&zswap_stored_pages); > > /* > * We finish initializing the entry while it's already in xarray. > @@ -1504,22 +1508,19 @@ static ssize_t zswap_store_page(struct page *page, > entry->pool = pool; > entry->swpentry = page_swpentry; > entry->objcg = objcg; > - if (objcg) > - obj_cgroup_charge_zswap(objcg, entry->length); > entry->referenced = true; > if (entry->length) { > INIT_LIST_HEAD(&entry->lru); > zswap_lru_add(&zswap_list_lru, entry); > } > - atomic_long_inc(&zswap_stored_pages); > > - return entry->length; > + return true; > > store_failed: > zpool_free(pool->zpool, entry->handle); > compress_failed: > zswap_entry_cache_free(entry); > - return -EINVAL; > + return false; > } > > bool zswap_store(struct folio *folio) > @@ -1566,10 +1567,8 @@ bool zswap_store(struct folio *folio) > > for (index = 0; index < nr_pages; ++index) { > struct page *page = folio_page(folio, index); > - ssize_t bytes; > > - bytes = zswap_store_page(page, objcg, pool); > - if (bytes < 0) > + if (!zswap_store_page(page, objcg, pool)) > goto put_pool; > } > > -- > 2.47.1 >
