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

[Chi Zhiling <chizhiling@163.com>]

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?


Thanks,

> 
>> Signed-off-by: Chi Zhiling <chizhiling@kylinos.cn>
>> ---
>>   include/linux/shmem_fs.h | 2 +-
>>   mm/khugepaged.c          | 2 +-
>>   mm/shmem.c               | 9 +++------
>>   3 files changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
>> index 93a0ba872ebe..d461713c095b 100644
>> --- a/include/linux/shmem_fs.h
>> +++ b/include/linux/shmem_fs.h
>> @@ -165,7 +165,7 @@ extern unsigned long 
>> shmem_partial_swap_usage(struct address_space *mapping,
>>   /* Flag allocation requirements to shmem_get_folio */
>>   enum sgp_type {
>>       SGP_READ,    /* don't exceed i_size, don't allocate page */
>> -    SGP_NOALLOC,    /* similar, but fail on hole or use fallocated 
>> page */
>> +    SGP_NOALLOC,    /* like SGP_READ, but accept fallocated page */
>>       SGP_CACHE,    /* don't exceed i_size, may allocate page */
>>       SGP_WRITE,    /* may exceed i_size, may allocate !Uptodate page */
>>       SGP_FALLOC,    /* like SGP_WRITE, but make existing page 
>> Uptodate */
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index b8452dbdb043..3309d1c094df 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -1950,7 +1950,7 @@ static enum scan_result collapse_file(struct 
>> mm_struct *mm, unsigned long addr,
>>                   xas_unlock_irq(&xas);
>>                   /* swap in or instantiate fallocated page */
>>                   if (shmem_get_folio(mapping->host, index, 0,
>> -                        &folio, SGP_NOALLOC)) {
>> +                        &folio, SGP_NOALLOC) || !folio) {
>>                       result = SCAN_FAIL;
>>                       goto xa_unlocked;
>>                   }
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 3b5dc21b323c..458853c506ea 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -2524,14 +2524,11 @@ static int shmem_get_folio_gfp(struct inode 
>> *inode, pgoff_t index,
>>       }
>>       /*
>> -     * SGP_READ: succeed on hole, with NULL folio, letting caller zero.
>> -     * SGP_NOALLOC: fail on hole, with NULL folio, letting caller fail.
>> +     * SGP_READ/SGP_NOALLOC: succeed on hole, with NULL folio.
>>        */
>>       *foliop = NULL;
>> -    if (sgp == SGP_READ)
>> +    if (sgp <= SGP_NOALLOC)
>>           return 0;
>> -    if (sgp == SGP_NOALLOC)
>> -        return -ENOENT;
>>       /*
>>        * Fast cache lookup and swap lookup did not find it: allocate.
>> @@ -2657,7 +2654,7 @@ static int shmem_get_folio_gfp(struct inode 
>> *inode, pgoff_t index,
>>    *
>>    * When no folio is found, the behavior depends on @sgp:
>>    *  - for SGP_READ, *@foliop is %NULL and 0 is returned
>> - *  - for SGP_NOALLOC, *@foliop is %NULL and -ENOENT is returned
>> + *  - for SGP_NOALLOC, *@foliop is %NULL and 0 is returned
>>    *  - for all other flags a new folio is allocated, inserted into the
>>    *    page cache and returned locked in @foliop.
>>    *