On Fri, Jun 9, 2023 at 3:23 AM Domenico Cerasuolo
<cerasuolodomenico@xxxxxxxxx> wrote:
>
> On Wed, Jun 7, 2023 at 11:27 AM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
> >
> > On Tue, Jun 6, 2023 at 7:56 AM Domenico Cerasuolo
> > <cerasuolodomenico@xxxxxxxxx> wrote:
> > >
> > > Previously, in zswap, the writeback function was passed to zpool drivers
> > > for their usage in calling the writeback operation. However, since the
> > > drivers did not possess references to entries and the function was
> > > specifically designed to work with handles, the writeback process has
> > > been modified to occur directly within zswap. Consequently, this change
> > > allows for some simplification of the writeback function, taking into
> > > account the updated workflow.
> > >
> > > Signed-off-by: Domenico Cerasuolo <cerasuolodomenico@xxxxxxxxx>
> > > ---
> > > mm/zswap.c | 69 ++++++++++++++----------------------------------------
> > > 1 file changed, 17 insertions(+), 52 deletions(-)
> > >
> > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > index 2831bf56b168..ef8604812352 100644
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > > @@ -250,7 +250,8 @@ static bool zswap_has_pool;
> > > pr_debug("%s pool %s/%s\n", msg, (p)->tfm_name, \
> > > zpool_get_type((p)->zpool))
> > >
> > > -static int zswap_writeback_entry(struct zpool *pool, unsigned long handle);
> > > +static int zswap_writeback_entry(struct zswap_entry *entry, struct zswap_header *zhdr,
> > > + struct zswap_tree *tree);
> > > static int zswap_pool_get(struct zswap_pool *pool);
> > > static void zswap_pool_put(struct zswap_pool *pool);
> > >
> > > @@ -632,15 +633,21 @@ static int zswap_shrink(struct zswap_pool *pool)
> > > }
> > > spin_unlock(&tree->lock);
> > >
> > > - ret = zswap_writeback_entry(pool->zpool, lru_entry->handle);
> > > + ret = zswap_writeback_entry(lru_entry, zhdr, tree);
> > >
> > > spin_lock(&tree->lock);
> > > if (ret) {
> > > spin_lock(&pool->lru_lock);
> > > list_move(&lru_entry->lru, &pool->lru);
> > > spin_unlock(&pool->lru_lock);
> > > + zswap_entry_put(tree, tree_entry);
> > > + } else {
> > > + /* free the local reference */
> > > + zswap_entry_put(tree, tree_entry);
> > > + /* free the entry if it's not been invalidated*/
> > > + if (lru_entry == zswap_rb_search(&tree->rbroot, swpoffset))
> > > + zswap_entry_put(tree, tree_entry);
> >
> > The comment that was here about the 2 possible cases was useful imo,
> > maybe keep it?
>
> I honestly found it brittle in that we're not interested in the refcount there,
> but rather in releasing the base reference that keeps the entry valid.
> There's not way to know which refcount value it should be though, consider
> that throughout this series the values can be 1 or 0, but also 2 or 1,
> depending on where this function is called (zpool_shrink or zswap_shrink).
Yeah looking at it with fresh eyes makes sense, we want to invalidate
the entry, not really caring about the refcount (see below).
>
> >
> > Also, I am not sure why we need to do a tree search vs. just reading
> > the refcount here before the first put. We can even make
> > zswap_entry_put() return the refcount after the put to know if we need
> > the additional put or not.
> >
> > Can anyone think of any reason why we need to explicitly search the tree here?
>
> I think that the reasoning here is that refcount > 0 doesn't necessarily mean
> that the entry is on the tree. I'm not sure if reading refcount directly here
> would cause an issue now, probably not, but I wonder if bugs could be
> introduced if the context in which this function is called changes.
Yeah I agree. As Johannes suggested, using zswap_invalidate_entry()
(from my exclusive loads patch in mm-unstable) would be best here imo.
>
> >
> > > }
> > > - zswap_entry_put(tree, tree_entry);
> > > spin_unlock(&tree->lock);
> > >
> > > return ret ? -EAGAIN : 0;
> > > @@ -1039,16 +1046,14 @@ static int zswap_get_swap_cache_page(swp_entry_t entry,
> > > * the swap cache, the compressed version stored by zswap can be
> > > * freed.
> > > */
> > > -static int zswap_writeback_entry(struct zpool *pool, unsigned long handle)
> > > +static int zswap_writeback_entry(struct zswap_entry *entry, struct zswap_header *zhdr,
> > > + struct zswap_tree *tree)
> > > {
> > > - struct zswap_header *zhdr;
> > > - swp_entry_t swpentry;
> > > - struct zswap_tree *tree;
> > > - pgoff_t offset;
> > > - struct zswap_entry *entry;
> > > + swp_entry_t swpentry = zhdr->swpentry;
> > > struct page *page;
> > > struct scatterlist input, output;
> > > struct crypto_acomp_ctx *acomp_ctx;
> > > + struct zpool *pool = entry->pool->zpool;
> > >
> > > u8 *src, *tmp = NULL;
> > > unsigned int dlen;
> > > @@ -1063,25 +1068,6 @@ static int zswap_writeback_entry(struct zpool *pool, unsigned long handle)
> > > return -ENOMEM;
> > > }
> > >
> > > - /* extract swpentry from data */
> > > - zhdr = zpool_map_handle(pool, handle, ZPOOL_MM_RO);
> > > - swpentry = zhdr->swpentry; /* here */
> > > - tree = zswap_trees[swp_type(swpentry)];
> > > - offset = swp_offset(swpentry);
> > > - zpool_unmap_handle(pool, handle);
> > > -
> > > - /* find and ref zswap entry */
> > > - spin_lock(&tree->lock);
> > > - entry = zswap_entry_find_get(&tree->rbroot, offset);
> > > - if (!entry) {
> > > - /* entry was invalidated */
> > > - spin_unlock(&tree->lock);
> > > - kfree(tmp);
> > > - return 0;
> > > - }
> > > - spin_unlock(&tree->lock);
> > > - BUG_ON(offset != entry->offset);
> > > -
> > > /* try to allocate swap cache page */
> > > switch (zswap_get_swap_cache_page(swpentry, &page)) {
> > > case ZSWAP_SWAPCACHE_FAIL: /* no memory or invalidate happened */
> > > @@ -1115,12 +1101,12 @@ static int zswap_writeback_entry(struct zpool *pool, unsigned long handle)
> > > acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
> > > dlen = PAGE_SIZE;
> > >
> > > - zhdr = zpool_map_handle(pool, handle, ZPOOL_MM_RO);
> > > + zhdr = zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO);
> > > src = (u8 *)zhdr + sizeof(struct zswap_header);
> > > if (!zpool_can_sleep_mapped(pool)) {
> > > memcpy(tmp, src, entry->length);
> > > src = tmp;
> > > - zpool_unmap_handle(pool, handle);
> > > + zpool_unmap_handle(pool, entry->handle);
> > > }
> > >
> > > mutex_lock(acomp_ctx->mutex);
> > > @@ -1135,7 +1121,7 @@ static int zswap_writeback_entry(struct zpool *pool, unsigned long handle)
> > > if (!zpool_can_sleep_mapped(pool))
> > > kfree(tmp);
> > > else
> > > - zpool_unmap_handle(pool, handle);
> > > + zpool_unmap_handle(pool, entry->handle);
> > >
> > > BUG_ON(ret);
> > > BUG_ON(dlen != PAGE_SIZE);
> > > @@ -1152,23 +1138,7 @@ static int zswap_writeback_entry(struct zpool *pool, unsigned long handle)
> > > put_page(page);
> > > zswap_written_back_pages++;
> > >
> > > - spin_lock(&tree->lock);
> > > - /* drop local reference */
> > > - zswap_entry_put(tree, entry);
> > > -
> > > - /*
> > > - * There are two possible situations for entry here:
> > > - * (1) refcount is 1(normal case), entry is valid and on the tree
> > > - * (2) refcount is 0, entry is freed and not on the tree
> > > - * because invalidate happened during writeback
> > > - * search the tree and free the entry if find entry
> > > - */
> > > - if (entry == zswap_rb_search(&tree->rbroot, offset))
> > > - zswap_entry_put(tree, entry);
> > > - spin_unlock(&tree->lock);
> > > -
> > > return ret;
> > > -
> > > fail:
> > > if (!zpool_can_sleep_mapped(pool))
> > > kfree(tmp);
> > > @@ -1177,13 +1147,8 @@ static int zswap_writeback_entry(struct zpool *pool, unsigned long handle)
> > > * if we get here due to ZSWAP_SWAPCACHE_EXIST
> > > * a load may be happening concurrently.
> > > * it is safe and okay to not free the entry.
> > > - * if we free the entry in the following put
> > > * it is also okay to return !0
> > > */
> > > - spin_lock(&tree->lock);
> > > - zswap_entry_put(tree, entry);
> > > - spin_unlock(&tree->lock);
> > > -
> > > return ret;
> > > }
> > >
> > > --
> > > 2.34.1
> > >
