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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

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@kylinos.cn>
>>>
>>> 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:).