Re: [PATCH v1 3/5] mm/shmem: make SGP_NOALLOC succeed on hole like SGP_READ

[Baolin Wang]

On 5/25/26 4:14 PM, Chi Zhiling wrote:
> On 5/25/26 3:10 PM, Baolin Wang wrote:
> > On 5/20/26 6:15 PM, Chi Zhiling wrote:
> > > From: Chi Zhiling <chizhiling@xxxxxxxxxx>
> > >
> > > Change SGP_NOALLOC to return 0 with NULL folio on hole, matching
> > > SGP_READ behavior. This simplifies the sgp_type handling by unifying
> > > hole semantics across these types.
> > >
> > > Previously, SGP_NOALLOC returned -ENOENT on hole, while SGP_READ
> > > returned 0. This inconsistency required special handling in callers
> > > like khugepaged and userfaultfd.
> >
> > This patch doesn't seem to be a performance optimization, and I'm not 
> > convinced.
>
> Hi, baolin
>
> This is not an optimization patch, and it is not related to this patch 
> series either.
>
> > > After this change:
> > > - khugepaged: behavior unchanged (checks both error and NULL folio)
> >
> > But this adds an extra check to khugepaged, and I'm not sure it's 
> > worth it.
>
> You are right, I will drop this patch in v2.
>
> > > - userfaultfd: behavior unchanged (both -ENOENT and NULL are converted
> > >    to -EFAULT before returning to userspace)
> >
> > No, this will break userfaultfd, cause userfaultfd currently returns 
> > an error code directly in this case (please update to the latest 
> > codebase.). See:
> >
> > static struct folio *shmem_get_folio_noalloc(struct inode *inode, 
> > pgoff_t pgoff)
> > {
> >      struct folio *folio;
> >      int err;
> >
> >      err = shmem_get_folio(inode, pgoff, 0, &folio, SGP_NOALLOC);
> >      if (err)
> >          return ERR_PTR(err);
> >
> >      return folio;
> > }
>
> The return value of shmem_get_folio_noalloc() will change, but it seems 
> this does not affect the logic in mfill_atomic_pte_continue().
>
> ```
>      folio = ops->get_folio_noalloc(inode, pgoff);
>      /* Our caller expects us to return -EFAULT if we failed to find 
> folio */
>      if (IS_ERR_OR_NULL(folio))
>          return -EFAULT;
> ```
>
> No matter whether get_folio_noalloc() returns an error or NULL, the 
> error code is converted to -EFAULT here, and mfill_atomic_pte_continue() 
> is currently the only caller of get_folio_noalloc().
>
> Did I miss something?

OK.

But I still wonder if it's worth changing the definition of a flag, 
especially when there's no obvious bug here and this is merely a 
cleanup. Yet this cleanup requires adding an extra check for khugepaged, 
which makes me feel it is not worth it. Unless Hugh prefers to change 
the meaning of this flag:).