From: Kairui Song <kasong@tencent.com>

Currently, none of the folio_contains callers should encounter swap
cache folios.

For fs/ callers, swap cache folios are never part of their workflow.

For filemap and truncate, folio_contains is only used for sanity
checks to verify the folio index matches the expected
lookup / invalidation target.

The swap cache does not utilize filemap or truncate helpers in ways
that would trigger these checks, as it mostly implements its own
cache management.

Shmem won't trigger these sanity checks either unless thing went
wrong, as it would directly trigger a BUG because swap cache index are
unrelated and almost never matches shmem index. Shmem have to handle
mixed values of folios, shadows, and swap entries, so it has its own
way of handling the mapping.

While some filemap helpers works for swap cache space, the swap cache
is different from the page cache in many ways. So this particular helper
will unlikely to work in a helpful way for swap cache folios.

So make it explicit here that folio_contains should not be used for
swap cache folios. This helps to avoid misuse, make swap cache less
exposed and remove the folio_index usage here.

Signed-off-by: Kairui Song <kasong@tencent.com>
---
 include/linux/pagemap.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index af25fb640463..1dc3416a9c0d 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -935,14 +935,14 @@ static inline struct page *folio_file_page(struct folio *folio, pgoff_t index)
  * @folio: The folio.
  * @index: The page index within the file.
  *
- * Context: The caller should have the page locked in order to prevent
- * (eg) shmem from moving the page between the page cache and swap cache
- * and changing its index in the middle of the operation.
+ * Context: The caller should have the folio locked and ensure
+ * (e.g.) shmem did not move this folio to swap cache.
  * Return: true or false.
  */
 static inline bool folio_contains(struct folio *folio, pgoff_t index)
 {
-	return index - folio_index(folio) < folio_nr_pages(folio);
+	VM_BUG_ON_FOLIO(folio_test_swapcache(folio), folio);
+	return index - folio->index < folio_nr_pages(folio);
 }
 
 unsigned filemap_get_folios(struct address_space *mapping, pgoff_t *start,
-- 
2.49.0

On 29.04.25 13:49, Kairui Song wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> Currently, none of the folio_contains callers should encounter swap
> cache folios.
> 
> For fs/ callers, swap cache folios are never part of their workflow.
> 
> For filemap and truncate, folio_contains is only used for sanity
> checks to verify the folio index matches the expected
> lookup / invalidation target.
> 
> The swap cache does not utilize filemap or truncate helpers in ways
> that would trigger these checks, as it mostly implements its own
> cache management.
> 
> Shmem won't trigger these sanity checks either unless thing went
> wrong, as it would directly trigger a BUG because swap cache index are
> unrelated and almost never matches shmem index. Shmem have to handle
> mixed values of folios, shadows, and swap entries, so it has its own
> way of handling the mapping.
> 
> While some filemap helpers works for swap cache space, the swap cache
> is different from the page cache in many ways. So this particular helper
> will unlikely to work in a helpful way for swap cache folios.
> 
> So make it explicit here that folio_contains should not be used for
> swap cache folios. This helps to avoid misuse, make swap cache less
> exposed and remove the folio_index usage here.
> 
> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
>   include/linux/pagemap.h | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index af25fb640463..1dc3416a9c0d 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -935,14 +935,14 @@ static inline struct page *folio_file_page(struct folio *folio, pgoff_t index)
>    * @folio: The folio.
>    * @index: The page index within the file.
>    *
> - * Context: The caller should have the page locked in order to prevent
> - * (eg) shmem from moving the page between the page cache and swap cache
> - * and changing its index in the middle of the operation.
> + * Context: The caller should have the folio locked and ensure
> + * (e.g.) shmem did not move this folio to swap cache.

The "(e.g.)" looks weird. Maybe "ensure that e.g., shmem ..."

"to the"

>    * Return: true or false.
>    */
>   static inline bool folio_contains(struct folio *folio, pgoff_t index)
>   {
> -	return index - folio_index(folio) < folio_nr_pages(folio);
> +	VM_BUG_ON_FOLIO(folio_test_swapcache(folio), folio);

Likely you want VM_WARN_ON_ONCE_FOLIO() here.

> +	return index - folio->index < folio_nr_pages(folio);
>   }
>   
>   unsigned filemap_get_folios(struct address_space *mapping, pgoff_t *start,


Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

On Tue, Apr 29, 2025 at 8:22 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 29.04.25 13:49, Kairui Song wrote:
> > From: Kairui Song <kasong@tencent.com>
> >
> > Currently, none of the folio_contains callers should encounter swap
> > cache folios.
> >
> > For fs/ callers, swap cache folios are never part of their workflow.
> >
> > For filemap and truncate, folio_contains is only used for sanity
> > checks to verify the folio index matches the expected
> > lookup / invalidation target.
> >
> > The swap cache does not utilize filemap or truncate helpers in ways
> > that would trigger these checks, as it mostly implements its own
> > cache management.
> >
> > Shmem won't trigger these sanity checks either unless thing went
> > wrong, as it would directly trigger a BUG because swap cache index are
> > unrelated and almost never matches shmem index. Shmem have to handle
> > mixed values of folios, shadows, and swap entries, so it has its own
> > way of handling the mapping.
> >
> > While some filemap helpers works for swap cache space, the swap cache
> > is different from the page cache in many ways. So this particular helper
> > will unlikely to work in a helpful way for swap cache folios.
> >
> > So make it explicit here that folio_contains should not be used for
> > swap cache folios. This helps to avoid misuse, make swap cache less
> > exposed and remove the folio_index usage here.
> >
> > Signed-off-by: Kairui Song <kasong@tencent.com>
> > ---
> >   include/linux/pagemap.h | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> > index af25fb640463..1dc3416a9c0d 100644
> > --- a/include/linux/pagemap.h
> > +++ b/include/linux/pagemap.h
> > @@ -935,14 +935,14 @@ static inline struct page *folio_file_page(struct folio *folio, pgoff_t index)
> >    * @folio: The folio.
> >    * @index: The page index within the file.
> >    *
> > - * Context: The caller should have the page locked in order to prevent
> > - * (eg) shmem from moving the page between the page cache and swap cache
> > - * and changing its index in the middle of the operation.
> > + * Context: The caller should have the folio locked and ensure
> > + * (e.g.) shmem did not move this folio to swap cache.
>
> The "(e.g.)" looks weird. Maybe "ensure that e.g., shmem ..."
>
> "to the"
>
> >    * Return: true or false.
> >    */
> >   static inline bool folio_contains(struct folio *folio, pgoff_t index)
> >   {
> > -     return index - folio_index(folio) < folio_nr_pages(folio);
> > +     VM_BUG_ON_FOLIO(folio_test_swapcache(folio), folio);
>
> Likely you want VM_WARN_ON_ONCE_FOLIO() here.

All its caller will trigger a bug if it encounters a swap cache, so I
kept that behaviour consistent. Let's keep that unchanged for now.

>
> > +     return index - folio->index < folio_nr_pages(folio);
> >   }
> >
> >   unsigned filemap_get_folios(struct address_space *mapping, pgoff_t *start,
>
>
> Acked-by: David Hildenbrand <david@redhat.com>

Thanks!

>
> --
> Cheers,
>
> David / dhildenb
>
>

On 29.04.25 15:32, Kairui Song wrote:
> On Tue, Apr 29, 2025 at 8:22 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 29.04.25 13:49, Kairui Song wrote:
>>> From: Kairui Song <kasong@tencent.com>
>>>
>>> Currently, none of the folio_contains callers should encounter swap
>>> cache folios.
>>>
>>> For fs/ callers, swap cache folios are never part of their workflow.
>>>
>>> For filemap and truncate, folio_contains is only used for sanity
>>> checks to verify the folio index matches the expected
>>> lookup / invalidation target.
>>>
>>> The swap cache does not utilize filemap or truncate helpers in ways
>>> that would trigger these checks, as it mostly implements its own
>>> cache management.
>>>
>>> Shmem won't trigger these sanity checks either unless thing went
>>> wrong, as it would directly trigger a BUG because swap cache index are
>>> unrelated and almost never matches shmem index. Shmem have to handle
>>> mixed values of folios, shadows, and swap entries, so it has its own
>>> way of handling the mapping.
>>>
>>> While some filemap helpers works for swap cache space, the swap cache
>>> is different from the page cache in many ways. So this particular helper
>>> will unlikely to work in a helpful way for swap cache folios.
>>>
>>> So make it explicit here that folio_contains should not be used for
>>> swap cache folios. This helps to avoid misuse, make swap cache less
>>> exposed and remove the folio_index usage here.
>>>
>>> Signed-off-by: Kairui Song <kasong@tencent.com>
>>> ---
>>>    include/linux/pagemap.h | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
>>> index af25fb640463..1dc3416a9c0d 100644
>>> --- a/include/linux/pagemap.h
>>> +++ b/include/linux/pagemap.h
>>> @@ -935,14 +935,14 @@ static inline struct page *folio_file_page(struct folio *folio, pgoff_t index)
>>>     * @folio: The folio.
>>>     * @index: The page index within the file.
>>>     *
>>> - * Context: The caller should have the page locked in order to prevent
>>> - * (eg) shmem from moving the page between the page cache and swap cache
>>> - * and changing its index in the middle of the operation.
>>> + * Context: The caller should have the folio locked and ensure
>>> + * (e.g.) shmem did not move this folio to swap cache.
>>
>> The "(e.g.)" looks weird. Maybe "ensure that e.g., shmem ..."
>>
>> "to the"
>>
>>>     * Return: true or false.
>>>     */
>>>    static inline bool folio_contains(struct folio *folio, pgoff_t index)
>>>    {
>>> -     return index - folio_index(folio) < folio_nr_pages(folio);
>>> +     VM_BUG_ON_FOLIO(folio_test_swapcache(folio), folio);
>>
>> Likely you want VM_WARN_ON_ONCE_FOLIO() here.
> 
> All its caller will trigger a bug if it encounters a swap cache, so I
> kept that behaviour consistent. Let's keep that unchanged for now.

I suggest reading coding-style.rst about "Do not add new code that uses 
any of the BUG() variants, such as BUG(), BUG_ON(), or VM_BUG_ON()".

VM_BUG_ON is particularly stupid.

-- 
Cheers,

David / dhildenb