diff mbox series

[v2,4/6] shmem: skip page split if we're not reclaiming

Message ID 20230309230545.2930737-5-mcgrof@kernel.org (mailing list archive)
State New
Headers show
Series tmpfs: add the option to disable swap | expand

Commit Message

Luis Chamberlain March 9, 2023, 11:05 p.m. UTC 
In theory when info->flags & VM_LOCKED we should not be getting
shem_writepage() called so we should be verifying this with a
WARN_ON_ONCE(). Since we should not be swapping then best to ensure
we also don't do the folio split earlier too. So just move the check
early to avoid folio splits in case its a dubious call.

We also have a similar early bail when !total_swap_pages so just move
that earlier to avoid the possible folio split in the same situation.

Acked-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 mm/shmem.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Yosry Ahmed March 9, 2023, 11:09 p.m. UTC | #1 
On Thu, Mar 9, 2023 at 3:05 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> In theory when info->flags & VM_LOCKED we should not be getting
> shem_writepage() called so we should be verifying this with a
> WARN_ON_ONCE(). Since we should not be swapping then best to ensure
> we also don't do the folio split earlier too. So just move the check
> early to avoid folio splits in case its a dubious call.
>
> We also have a similar early bail when !total_swap_pages so just move
> that earlier to avoid the possible folio split in the same situation.
>
> Acked-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Christian Brauner <brauner@kernel.org>
Reviewed-by: Yosry Ahmed <yosryahmed@google.com>

> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  mm/shmem.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 68e9970baf1e..dfd995da77b4 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1350,6 +1350,12 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>         if (WARN_ON_ONCE(!wbc->for_reclaim))
>                 goto redirty;
>
> +       if (WARN_ON_ONCE(info->flags & VM_LOCKED))
> +               goto redirty;
> +
> +       if (!total_swap_pages)
> +               goto redirty;
> +
>         /*
>          * If /sys/kernel/mm/transparent_hugepage/shmem_enabled is "always" or
>          * "force", drivers/gpu/drm/i915/gem/i915_gem_shmem.c gets huge pages,
> @@ -1365,10 +1371,6 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>         }
>
>         index = folio->index;
> -       if (info->flags & VM_LOCKED)
> -               goto redirty;
> -       if (!total_swap_pages)
> -               goto redirty;
>
>         /*
>          * This is somewhat ridiculous, but without plumbing a SWAP_MAP_FALLOC
> --
> 2.39.1
>
Hugh Dickins April 18, 2023, 4:41 a.m. UTC | #2 
On Thu, 9 Mar 2023, Luis Chamberlain wrote:

> In theory when info->flags & VM_LOCKED we should not be getting
> shem_writepage() called so we should be verifying this with a
> WARN_ON_ONCE(). Since we should not be swapping then best to ensure
> we also don't do the folio split earlier too. So just move the check
> early to avoid folio splits in case its a dubious call.
> 
> We also have a similar early bail when !total_swap_pages so just move
> that earlier to avoid the possible folio split in the same situation.
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  mm/shmem.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 68e9970baf1e..dfd995da77b4 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1350,6 +1350,12 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>  	if (WARN_ON_ONCE(!wbc->for_reclaim))
>  		goto redirty;
>  
> +	if (WARN_ON_ONCE(info->flags & VM_LOCKED))
> +		goto redirty;

Well, okay, I don't mind that.  But shall we take bets on how soon syzbot
(hope it's not watching) will try flipping SHM_LOCK on while swapping out
pages from a SHM segment, and hit that warning?  Perhaps I'm wrong, but I
don't think any serialization prevents that.

Hugh

> +
> +	if (!total_swap_pages)
> +		goto redirty;
> +
>  	/*
>  	 * If /sys/kernel/mm/transparent_hugepage/shmem_enabled is "always" or
>  	 * "force", drivers/gpu/drm/i915/gem/i915_gem_shmem.c gets huge pages,
> @@ -1365,10 +1371,6 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>  	}
>  
>  	index = folio->index;
> -	if (info->flags & VM_LOCKED)
> -		goto redirty;
> -	if (!total_swap_pages)
> -		goto redirty;
>  
>  	/*
>  	 * This is somewhat ridiculous, but without plumbing a SWAP_MAP_FALLOC
> -- 
> 2.39.1
Luis Chamberlain April 18, 2023, 9:11 p.m. UTC | #3 
On Mon, Apr 17, 2023 at 09:41:41PM -0700, Hugh Dickins wrote:
> On Thu, 9 Mar 2023, Luis Chamberlain wrote:
> 
> > In theory when info->flags & VM_LOCKED we should not be getting
> > shem_writepage() called so we should be verifying this with a
> > WARN_ON_ONCE(). Since we should not be swapping then best to ensure
> > we also don't do the folio split earlier too. So just move the check
> > early to avoid folio splits in case its a dubious call.
> > 
> > We also have a similar early bail when !total_swap_pages so just move
> > that earlier to avoid the possible folio split in the same situation.
> > 
> > Acked-by: David Hildenbrand <david@redhat.com>
> > Reviewed-by: Christian Brauner <brauner@kernel.org>
> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > ---
> >  mm/shmem.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 68e9970baf1e..dfd995da77b4 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1350,6 +1350,12 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
> >  	if (WARN_ON_ONCE(!wbc->for_reclaim))
> >  		goto redirty;
> >  
> > +	if (WARN_ON_ONCE(info->flags & VM_LOCKED))
> > +		goto redirty;
> 
> Well, okay, I don't mind that.  But shall we take bets on how soon syzbot
> (hope it's not watching) will try flipping SHM_LOCK on while swapping out
> pages from a SHM segment, and hit that warning?  Perhaps I'm wrong, but I
> don't think any serialization prevents that.

I though that may be the case. Would such serialization be welcomed?

  Luis
Hugh Dickins April 18, 2023, 9:20 p.m. UTC | #4 
On Tue, 18 Apr 2023, Luis Chamberlain wrote:
> On Mon, Apr 17, 2023 at 09:41:41PM -0700, Hugh Dickins wrote:
> > On Thu, 9 Mar 2023, Luis Chamberlain wrote:
> > 
> > > +	if (WARN_ON_ONCE(info->flags & VM_LOCKED))
> > > +		goto redirty;
> > 
> > Well, okay, I don't mind that.  But shall we take bets on how soon syzbot
> > (hope it's not watching) will try flipping SHM_LOCK on while swapping out
> > pages from a SHM segment, and hit that warning?  Perhaps I'm wrong, but I
> > don't think any serialization prevents that.
> 
> I though that may be the case. Would such serialization be welcomed?

Absolutely not!  We don't insert slowdowns just to avoid warnings,
unless the warning is of something that really matters.  This one
does not matter, the situation is correctly handled, so the warning
would be better reverted.  Though I personally don't mind you leaving
it in until the first report of it arrives.

Hugh
diff mbox series

Patch

diff --git a/mm/shmem.c b/mm/shmem.c
index 68e9970baf1e..dfd995da77b4 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1350,6 +1350,12 @@  static int shmem_writepage(struct page *page, struct writeback_control *wbc)
 	if (WARN_ON_ONCE(!wbc->for_reclaim))
 		goto redirty;
 
+	if (WARN_ON_ONCE(info->flags & VM_LOCKED))
+		goto redirty;
+
+	if (!total_swap_pages)
+		goto redirty;
+
 	/*
 	 * If /sys/kernel/mm/transparent_hugepage/shmem_enabled is "always" or
 	 * "force", drivers/gpu/drm/i915/gem/i915_gem_shmem.c gets huge pages,
@@ -1365,10 +1371,6 @@  static int shmem_writepage(struct page *page, struct writeback_control *wbc)
 	}
 
 	index = folio->index;
-	if (info->flags & VM_LOCKED)
-		goto redirty;
-	if (!total_swap_pages)
-		goto redirty;
 
 	/*
 	 * This is somewhat ridiculous, but without plumbing a SWAP_MAP_FALLOC