mm: Do not reclaim private data from pinned page

Commit Message

Jan Kara April 28, 2023, 12:41 p.m. UTC

If the page is pinned, there's no point in trying to reclaim it.
Furthermore if the page is from the page cache we don't want to reclaim
fs-private data from the page because the pinning process may be writing
to the page at any time and reclaiming fs private info on a dirty page
can upset the filesystem (see link below).

Link: https://lore.kernel.org/linux-mm/[e-mail]
Signed-off-by: Jan Kara <[e-mail]>
---
 mm/vmscan.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

This was the non-controversial part of my series [1] dealing with pinned pages
in filesystems. It is already a win as it avoids crashes in the filesystem and
we can drop workarounds for this in ext4. Can we merge it please?

[1] https://lore.kernel.org/all/[e-mail]/

Comments

Matthew Wilcox April 28, 2023, 12:58 p.m. UTC | #1

On Fri, Apr 28, 2023 at 02:41:40PM +0200, Jan Kara wrote:
> If the page is pinned, there's no point in trying to reclaim it.
> Furthermore if the page is from the page cache we don't want to reclaim
> fs-private data from the page because the pinning process may be writing
> to the page at any time and reclaiming fs private info on a dirty page
> can upset the filesystem (see link below).
> 
> Link: https://lore.kernel.org/linux-mm/[e-mail]
> Signed-off-by: Jan Kara <[e-mail]>

Reviewed-by: Matthew Wilcox (Oracle) <[e-mail]>

Lorenzo Stoakes April 28, 2023, 1:05 p.m. UTC | #2

On Fri, Apr 28, 2023 at 02:41:40PM +0200, Jan Kara wrote:
> If the page is pinned, there's no point in trying to reclaim it.
> Furthermore if the page is from the page cache we don't want to reclaim
> fs-private data from the page because the pinning process may be writing
> to the page at any time and reclaiming fs private info on a dirty page
> can upset the filesystem (see link below).
>
> Link: https://lore.kernel.org/linux-mm/[e-mail]
> Signed-off-by: Jan Kara <[e-mail]>
> ---
>  mm/vmscan.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> This was the non-controversial part of my series [1] dealing with pinned pages
> in filesystems. It is already a win as it avoids crashes in the filesystem and
> we can drop workarounds for this in ext4. Can we merge it please?
>
> [1] https://lore.kernel.org/all/[e-mail]/
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index bf3eedf0209c..401a379ea99a 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1901,6 +1901,16 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>  			}
>  		}
>
> +		/*
> +		 * Folio is unmapped now so it cannot be newly pinned anymore.
> +		 * No point in trying to reclaim folio if it is pinned.
> +		 * Furthermore we don't want to reclaim underlying fs metadata
> +		 * if the folio is pinned and thus potentially modified by the
> +		 * pinning process as that may upset the filesystem.
> +		 */
> +		if (folio_maybe_dma_pinned(folio))
> +			goto activate_locked;
> +
>  		mapping = folio_mapping(folio);
>  		if (folio_test_dirty(folio)) {
>  			/*
> --
> 2.35.3
>

This seems very sensible and helps ameliorate problematic GUP/file
interactions so this seems a no-brainer.

Reviewed-by: Lorenzo Stoakes <[e-mail]>

Christoph Hellwig April 29, 2023, 4:50 a.m. UTC | #3

On Fri, Apr 28, 2023 at 02:41:40PM +0200, Jan Kara wrote:
> If the page is pinned, there's no point in trying to reclaim it.
> Furthermore if the page is from the page cache we don't want to reclaim
> fs-private data from the page because the pinning process may be writing
> to the page at any time and reclaiming fs private info on a dirty page
> can upset the filesystem (see link below).

Looks good:

Reviewed-by: Christoph Hellwig <[e-mail]>

John Hubbard May 1, 2023, 6:12 p.m. UTC | #4

On 4/28/23 05:41, Jan Kara wrote:
> If the page is pinned, there's no point in trying to reclaim it.
> Furthermore if the page is from the page cache we don't want to reclaim
> fs-private data from the page because the pinning process may be writing
> to the page at any time and reclaiming fs private info on a dirty page
> can upset the filesystem (see link below).
> 
> Link: https://lore.kernel.org/linux-mm/[e-mail]
> Signed-off-by: Jan Kara <[e-mail]>
> ---
>  mm/vmscan.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> This was the non-controversial part of my series [1] dealing with pinned pages
> in filesystems. It is already a win as it avoids crashes in the filesystem and
> we can drop workarounds for this in ext4. Can we merge it please?
> 
> [1] https://lore.kernel.org/all/[e-mail]/
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index bf3eedf0209c..401a379ea99a 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1901,6 +1901,16 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>  			}
>  		}
>  
> +		/*
> +		 * Folio is unmapped now so it cannot be newly pinned anymore.
> +		 * No point in trying to reclaim folio if it is pinned.
> +		 * Furthermore we don't want to reclaim underlying fs metadata
> +		 * if the folio is pinned and thus potentially modified by the
> +		 * pinning process as that may upset the filesystem.
> +		 */
> +		if (folio_maybe_dma_pinned(folio))
> +			goto activate_locked;
> +

This is huge! At long last. In fact, with this in the queue, I'm going to close
out our internal bug report from 2018 that launched this whole maybe-dma-pinned 
odyssey. :)

Reviewed-by: John Hubbard <[e-mail]>

thanks,

David Hildenbrand May 2, 2023, 2:45 p.m. UTC | #5

On 28.04.23 14:41, Jan Kara wrote:
> If the page is pinned, there's no point in trying to reclaim it.
> Furthermore if the page is from the page cache we don't want to reclaim
> fs-private data from the page because the pinning process may be writing
> to the page at any time and reclaiming fs private info on a dirty page
> can upset the filesystem (see link below).
> 
> Link: https://lore.kernel.org/linux-mm/[e-mail]
> Signed-off-by: Jan Kara <[e-mail]>
> ---
>   mm/vmscan.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> This was the non-controversial part of my series [1] dealing with pinned pages
> in filesystems. It is already a win as it avoids crashes in the filesystem and
> we can drop workarounds for this in ext4. Can we merge it please?
> 
> [1] https://lore.kernel.org/all/[e-mail]/
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index bf3eedf0209c..401a379ea99a 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1901,6 +1901,16 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>   			}
>   		}
>   
> +		/*
> +		 * Folio is unmapped now so it cannot be newly pinned anymore.
> +		 * No point in trying to reclaim folio if it is pinned.
> +		 * Furthermore we don't want to reclaim underlying fs metadata
> +		 * if the folio is pinned and thus potentially modified by the
> +		 * pinning process as that may upset the filesystem.
> +		 */
> +		if (folio_maybe_dma_pinned(folio))
> +			goto activate_locked;
> +
>   		mapping = folio_mapping(folio);
>   		if (folio_test_dirty(folio)) {
>   			/*

Acked-by: David Hildenbrand <[e-mail]>

Thanks!

Peter Xu May 2, 2023, 3:26 p.m. UTC | #6

On Fri, Apr 28, 2023 at 02:41:40PM +0200, Jan Kara wrote:
> If the page is pinned, there's no point in trying to reclaim it.
> Furthermore if the page is from the page cache we don't want to reclaim
> fs-private data from the page because the pinning process may be writing
> to the page at any time and reclaiming fs private info on a dirty page
> can upset the filesystem (see link below).
> 
> Link: https://lore.kernel.org/linux-mm/[e-mail]
> Signed-off-by: Jan Kara <[e-mail]>
> ---
>  mm/vmscan.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> This was the non-controversial part of my series [1] dealing with pinned pages
> in filesystems. It is already a win as it avoids crashes in the filesystem and
> we can drop workarounds for this in ext4. Can we merge it please?
> 
> [1] https://lore.kernel.org/all/[e-mail]/
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index bf3eedf0209c..401a379ea99a 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1901,6 +1901,16 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>  			}
>  		}
>  
> +		/*
> +		 * Folio is unmapped now so it cannot be newly pinned anymore.
> +		 * No point in trying to reclaim folio if it is pinned.
> +		 * Furthermore we don't want to reclaim underlying fs metadata
> +		 * if the folio is pinned and thus potentially modified by the
> +		 * pinning process as that may upset the filesystem.
> +		 */
> +		if (folio_maybe_dma_pinned(folio))
> +			goto activate_locked;
> +
>  		mapping = folio_mapping(folio);
>  		if (folio_test_dirty(folio)) {
>  			/*
> -- 
> 2.35.3
> 
> 

IIUC we have similar handling for anon (feb889fb40fafc).  Should we merge
the two sites and just move the check earlier?  Thanks,

David Hildenbrand May 2, 2023, 3:33 p.m. UTC | #7

On 02.05.23 17:26, Peter Xu wrote:
> On Fri, Apr 28, 2023 at 02:41:40PM +0200, Jan Kara wrote:
>> If the page is pinned, there's no point in trying to reclaim it.
>> Furthermore if the page is from the page cache we don't want to reclaim
>> fs-private data from the page because the pinning process may be writing
>> to the page at any time and reclaiming fs private info on a dirty page
>> can upset the filesystem (see link below).
>>
>> Link: https://lore.kernel.org/linux-mm/[e-mail]
>> Signed-off-by: Jan Kara <[e-mail]>
>> ---
>>   mm/vmscan.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> This was the non-controversial part of my series [1] dealing with pinned pages
>> in filesystems. It is already a win as it avoids crashes in the filesystem and
>> we can drop workarounds for this in ext4. Can we merge it please?
>>
>> [1] https://lore.kernel.org/all/[e-mail]/
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index bf3eedf0209c..401a379ea99a 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1901,6 +1901,16 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>>   			}
>>   		}
>>   
>> +		/*
>> +		 * Folio is unmapped now so it cannot be newly pinned anymore.
>> +		 * No point in trying to reclaim folio if it is pinned.
>> +		 * Furthermore we don't want to reclaim underlying fs metadata
>> +		 * if the folio is pinned and thus potentially modified by the
>> +		 * pinning process as that may upset the filesystem.
>> +		 */
>> +		if (folio_maybe_dma_pinned(folio))
>> +			goto activate_locked;
>> +
>>   		mapping = folio_mapping(folio);
>>   		if (folio_test_dirty(folio)) {
>>   			/*
>> -- 
>> 2.35.3
>>
>>
> 
> IIUC we have similar handling for anon (feb889fb40fafc).  Should we merge
> the two sites and just move the check earlier?  Thanks,
> 

feb889fb40fafc introduced a best-effort check that is racy, as the page 
is still mapped (can still get pinned). Further, we get false positives 
most only if a page is shared very often (1024 times), which happens 
rarely with anon pages. Now that we handle COW+pinning correctly using 
PageAnonExclusive, that check only optimizes for the "already pinned" 
case. But it's not required for correctness anymore (so it can be racy).

Here, however, we want more precision, and not false positives simply 
because a page is mapped many times (which can happen easily) or can 
still get pinned while mapped.

Peter Xu May 2, 2023, 3:48 p.m. UTC | #8

On Tue, May 02, 2023 at 05:33:22PM +0200, David Hildenbrand wrote:
> On 02.05.23 17:26, Peter Xu wrote:
> > On Fri, Apr 28, 2023 at 02:41:40PM +0200, Jan Kara wrote:
> > > If the page is pinned, there's no point in trying to reclaim it.
> > > Furthermore if the page is from the page cache we don't want to reclaim
> > > fs-private data from the page because the pinning process may be writing
> > > to the page at any time and reclaiming fs private info on a dirty page
> > > can upset the filesystem (see link below).
> > > 
> > > Link: https://lore.kernel.org/linux-mm/[e-mail]
> > > Signed-off-by: Jan Kara <[e-mail]>
> > > ---
> > >   mm/vmscan.c | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > > 
> > > This was the non-controversial part of my series [1] dealing with pinned pages
> > > in filesystems. It is already a win as it avoids crashes in the filesystem and
> > > we can drop workarounds for this in ext4. Can we merge it please?
> > > 
> > > [1] https://lore.kernel.org/all/[e-mail]/
> > > 
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index bf3eedf0209c..401a379ea99a 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -1901,6 +1901,16 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
> > >   			}
> > >   		}
> > > +		/*
> > > +		 * Folio is unmapped now so it cannot be newly pinned anymore.
> > > +		 * No point in trying to reclaim folio if it is pinned.
> > > +		 * Furthermore we don't want to reclaim underlying fs metadata
> > > +		 * if the folio is pinned and thus potentially modified by the
> > > +		 * pinning process as that may upset the filesystem.
> > > +		 */
> > > +		if (folio_maybe_dma_pinned(folio))
> > > +			goto activate_locked;
> > > +
> > >   		mapping = folio_mapping(folio);
> > >   		if (folio_test_dirty(folio)) {
> > >   			/*
> > > -- 
> > > 2.35.3
> > > 
> > > 
> > 
> > IIUC we have similar handling for anon (feb889fb40fafc).  Should we merge
> > the two sites and just move the check earlier?  Thanks,
> > 
> 
> feb889fb40fafc introduced a best-effort check that is racy, as the page is
> still mapped (can still get pinned). Further, we get false positives most
> only if a page is shared very often (1024 times), which happens rarely with
> anon pages. Now that we handle COW+pinning correctly using
> PageAnonExclusive, that check only optimizes for the "already pinned" case.
> But it's not required for correctness anymore (so it can be racy).
> 
> Here, however, we want more precision, and not false positives simply
> because a page is mapped many times (which can happen easily) or can still
> get pinned while mapped.

Ah makes sense, thanks.

Acked-by: Peter Xu <[e-mail]>

This seems not obvious, though, if we simply read the two commits. It'll be
great if we mention it somewhere in either comment or commit message on the
relationship of the two checks.

David Hildenbrand May 2, 2023, 3:53 p.m. UTC | #9

On 02.05.23 17:48, Peter Xu wrote:
> On Tue, May 02, 2023 at 05:33:22PM +0200, David Hildenbrand wrote:
>> On 02.05.23 17:26, Peter Xu wrote:
>>> On Fri, Apr 28, 2023 at 02:41:40PM +0200, Jan Kara wrote:
>>>> If the page is pinned, there's no point in trying to reclaim it.
>>>> Furthermore if the page is from the page cache we don't want to reclaim
>>>> fs-private data from the page because the pinning process may be writing
>>>> to the page at any time and reclaiming fs private info on a dirty page
>>>> can upset the filesystem (see link below).
>>>>
>>>> Link: https://lore.kernel.org/linux-mm/[e-mail]
>>>> Signed-off-by: Jan Kara <[e-mail]>
>>>> ---
>>>>    mm/vmscan.c | 10 ++++++++++
>>>>    1 file changed, 10 insertions(+)
>>>>
>>>> This was the non-controversial part of my series [1] dealing with pinned pages
>>>> in filesystems. It is already a win as it avoids crashes in the filesystem and
>>>> we can drop workarounds for this in ext4. Can we merge it please?
>>>>
>>>> [1] https://lore.kernel.org/all/[e-mail]/
>>>>
>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>>> index bf3eedf0209c..401a379ea99a 100644
>>>> --- a/mm/vmscan.c
>>>> +++ b/mm/vmscan.c
>>>> @@ -1901,6 +1901,16 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>>>>    			}
>>>>    		}
>>>> +		/*
>>>> +		 * Folio is unmapped now so it cannot be newly pinned anymore.
>>>> +		 * No point in trying to reclaim folio if it is pinned.
>>>> +		 * Furthermore we don't want to reclaim underlying fs metadata
>>>> +		 * if the folio is pinned and thus potentially modified by the
>>>> +		 * pinning process as that may upset the filesystem.
>>>> +		 */
>>>> +		if (folio_maybe_dma_pinned(folio))
>>>> +			goto activate_locked;
>>>> +
>>>>    		mapping = folio_mapping(folio);
>>>>    		if (folio_test_dirty(folio)) {
>>>>    			/*
>>>> -- 
>>>> 2.35.3
>>>>
>>>>
>>>
>>> IIUC we have similar handling for anon (feb889fb40fafc).  Should we merge
>>> the two sites and just move the check earlier?  Thanks,
>>>
>>
>> feb889fb40fafc introduced a best-effort check that is racy, as the page is
>> still mapped (can still get pinned). Further, we get false positives most
>> only if a page is shared very often (1024 times), which happens rarely with
>> anon pages. Now that we handle COW+pinning correctly using
>> PageAnonExclusive, that check only optimizes for the "already pinned" case.
>> But it's not required for correctness anymore (so it can be racy).
>>
>> Here, however, we want more precision, and not false positives simply
>> because a page is mapped many times (which can happen easily) or can still
>> get pinned while mapped.
> 
> Ah makes sense, thanks.
> 
> Acked-by: Peter Xu <[e-mail]>
> 
> This seems not obvious, though, if we simply read the two commits. It'll be
> great if we mention it somewhere in either comment or commit message on the
> relationship of the two checks.

I once had a patch lying around to document the existing check:

https://github.com/davidhildenbrand/linux/commit/abb01d42a99b56e2c5e707ba80ddc8b05ad7d618

Andrew Morton May 2, 2023, 8:20 p.m. UTC | #10

On Fri, 28 Apr 2023 14:41:40 +0200 Jan Kara <[e-mail]> wrote:

> If the page is pinned, there's no point in trying to reclaim it.
> Furthermore if the page is from the page cache we don't want to reclaim
> fs-private data from the page because the pinning process may be writing
> to the page at any time and reclaiming fs private info on a dirty page
> can upset the filesystem (see link below).

Obviously I'll add a cc:stable here.  I'm suspecting it's so old that
there's no real Fixes: target that makes sense?

> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1901,6 +1901,16 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>  			}
>  		}
>  
> +		/*
> +		 * Folio is unmapped now so it cannot be newly pinned anymore.
> +		 * No point in trying to reclaim folio if it is pinned.
> +		 * Furthermore we don't want to reclaim underlying fs metadata
> +		 * if the folio is pinned and thus potentially modified by the
> +		 * pinning process as that may upset the filesystem.
> +		 */
> +		if (folio_maybe_dma_pinned(folio))
> +			goto activate_locked;
> +

So I expect the -stable maintainers will be looking for a pre-folios
version of this when the time comes.

Jan Kara May 3, 2023, 9:51 a.m. UTC | #11

On Tue 02-05-23 13:20:20, Andrew Morton wrote:
> On Fri, 28 Apr 2023 14:41:40 +0200 Jan Kara <[e-mail]> wrote:
> 
> > If the page is pinned, there's no point in trying to reclaim it.
> > Furthermore if the page is from the page cache we don't want to reclaim
> > fs-private data from the page because the pinning process may be writing
> > to the page at any time and reclaiming fs private info on a dirty page
> > can upset the filesystem (see link below).
> 
> Obviously I'll add a cc:stable here.  I'm suspecting it's so old that
> there's no real Fixes: target that makes sense?

In principle the problem is there ever since MM started to track dirty
shared pages and filesystems started to use .page_mkwrite callbacks. So
for very long, yes. That being said the fix makes sense only since we've
added page pinning infrastructure and started using it in various places
which is not that long ago (in 2020, first patches in this direction have
been merged to 5.7). So we could mark it for stable with 5.7+.

> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -1901,6 +1901,16 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
> >  			}
> >  		}
> >  
> > +		/*
> > +		 * Folio is unmapped now so it cannot be newly pinned anymore.
> > +		 * No point in trying to reclaim folio if it is pinned.
> > +		 * Furthermore we don't want to reclaim underlying fs metadata
> > +		 * if the folio is pinned and thus potentially modified by the
> > +		 * pinning process as that may upset the filesystem.
> > +		 */
> > +		if (folio_maybe_dma_pinned(folio))
> > +			goto activate_locked;
> > +
> 
> So I expect the -stable maintainers will be looking for a pre-folios
> version of this when the time comes.

Yeah, right. Luckily that's going to be pretty easy :).

								Honza

Patch

diff --git a/mm/vmscan.c b/mm/vmscan.c
index bf3eedf0209c..401a379ea99a 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1901,6 +1901,16 @@  static unsigned int shrink_folio_list(struct list_head *folio_list,
 			}
 		}
 
+		/*
+		 * Folio is unmapped now so it cannot be newly pinned anymore.
+		 * No point in trying to reclaim folio if it is pinned.
+		 * Furthermore we don't want to reclaim underlying fs metadata
+		 * if the folio is pinned and thus potentially modified by the
+		 * pinning process as that may upset the filesystem.
+		 */
+		if (folio_maybe_dma_pinned(folio))
+			goto activate_locked;
+
 		mapping = folio_mapping(folio);
 		if (folio_test_dirty(folio)) {
 			/*