Re: [PATCH v2 2/2] mm: shmem: make unused huge shrinker memcg aware

[Qi Zheng]

Hi Baolin,

On 7/29/26 11:29 AM, Baolin Wang wrote:
> > > > @@ -2532,28 +2727,6 @@ static int shmem_get_folio_gfp(struct inode 
> > > > *inode, pgoff_t index,
> > > >   alloced:
> > > >       alloced = true;
> > > > -    if (folio_test_large(folio) &&
> > > > -        DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE) <
> > > > -                    folio_next_index(folio)) {
> > > > -        struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb);
> > > > -        struct shmem_inode_info *info = SHMEM_I(inode);
> > > > -        /*
> > > > -         * Part of the large folio is beyond i_size: subject
> > > > -         * to shrink under memory pressure.
> > > > -         */
> > > > -        spin_lock(&sbinfo->shrinklist_lock);
> > > > -        /*
> > > > -         * _careful to defend against unlocked access to
> > > > -         * ->shrink_list in shmem_unused_huge_shrink()
> > > > -         */
> > > > -        if (list_empty_careful(&info->shrinklist)) {
> > > > -            list_add_tail(&info->shrinklist,
> > > > -                      &sbinfo->shrinklist);
> > > > -            sbinfo->shrinklist_len++;
> > > > -        }
> > > > -        spin_unlock(&sbinfo->shrinklist_lock);
> > > > -    }
> > >
> > > Why move this additional logic down?
> >
> > Because under the following condition:
> >
> >          if (sgp <= SGP_CACHE &&
> >          ((loff_t)index << PAGE_SHIFT) >= i_size_read(inode)) {
> >          error = -EINVAL;
> >          goto unlock;
> >      }
> >
> > we will jump to unlock and remove the folio from page cache.
> >
> > With the new memcg-aware design, shmem_unused_huge_add() takes a memcg
> > reference. If the folio is then removed by the error path, the inode
> > sits on the shrinker list holding a stale memcg reference and pointing
> > at an i_size that no longer matches the folio.
> >
> > So we should ensure the inode is only queued when the folio is fully set
> > up and about to be returned successfully.
>
> Right. But I think this deserves a separate preparation patch with above 
> explanation, moving the original shrinklist addition logic to after all 
> checks are completed.

Agree, will split this into a separate preparation patch.

Thanks,
Qi