diff mbox series

[RFC,1/2] shmem: set shmem_writepage() variables early

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

Commit Message

Luis Chamberlain Feb. 7, 2023, 2:52 a.m. UTC
shmem_writepage() sets up variables typically used *after* a possible
huge page split. However even if that does happen the address space
mapping should not change. So it should be safe to set that from
the beginning.

The folio should always be locked from the start as well. It however
was not clear if the folio address can / should change, as well as
the first inode.

This commit makes no functional changes other a double check on the
folio locking which might be superflous. This change should help make
the subsequent patch easier to review.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 mm/shmem.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Matthew Wilcox Feb. 7, 2023, 3:52 a.m. UTC | #1
On Mon, Feb 06, 2023 at 06:52:58PM -0800, Luis Chamberlain wrote:
> shmem_writepage() sets up variables typically used *after* a possible
> huge page split. However even if that does happen the address space
> mapping should not change. So it should be safe to set that from
> the beginning.

Yes, we can get mapping from the folio early.  It doesn't change
on split.

> The folio should always be locked from the start as well. It however
> was not clear if the folio address can / should change, as well as
> the first inode.
> 
> This commit makes no functional changes other a double check on the
> folio locking which might be superflous. This change should help make
> the subsequent patch easier to review.

You don't need to check that the folio's locked, and you don't
need to reassign inode after the split.

> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  mm/shmem.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 28f3c699c8ce..a2c6aa11aab8 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1332,11 +1332,13 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>  {
>  	struct folio *folio = page_folio(page);
>  	struct shmem_inode_info *info;
> -	struct address_space *mapping;
> -	struct inode *inode;
> +	struct address_space *mapping = folio->mapping;
> +	struct inode *inode = mapping->host;
>  	swp_entry_t swap;
>  	pgoff_t index;
>  
> +	BUG_ON(!folio_test_locked(folio));
> +
>  	/*
>  	 * If /sys/kernel/mm/transparent_hugepage/shmem_enabled is "always" or
>  	 * "force", drivers/gpu/drm/i915/gem/i915_gem_shmem.c gets huge pages,
> @@ -1351,8 +1353,8 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>  		folio_clear_dirty(folio);
>  	}
>  
> +	/* Can the folio or first inode change on after a split? */
>  	BUG_ON(!folio_test_locked(folio));
> -	mapping = folio->mapping;
>  	index = folio->index;
>  	inode = mapping->host;
>  	info = SHMEM_I(inode);
> -- 
> 2.39.0
>
Luis Chamberlain Feb. 8, 2023, 4:08 p.m. UTC | #2
On Tue, Feb 07, 2023 at 03:52:53AM +0000, Matthew Wilcox wrote:
> On Mon, Feb 06, 2023 at 06:52:58PM -0800, Luis Chamberlain wrote:
> > shmem_writepage() sets up variables typically used *after* a possible
> > huge page split. However even if that does happen the address space
> > mapping should not change. So it should be safe to set that from
> > the beginning.
> 
> Yes, we can get mapping from the folio early.  It doesn't change
> on split.

Great.

> > The folio should always be locked from the start as well. It however
> > was not clear if the folio address can / should change, as well as
> > the first inode.
> > 
> > This commit makes no functional changes other a double check on the
> > folio locking which might be superflous. This change should help make
> > the subsequent patch easier to review.
> 
> You don't need to check that the folio's locked,

That BUG_ON() has been on shmem since linux-history days it's probably
best as a separate patch.

> and you don't
> need to reassign inode after the split.

Great! Thanks for this confirmation!

  Luis
diff mbox series

Patch

diff --git a/mm/shmem.c b/mm/shmem.c
index 28f3c699c8ce..a2c6aa11aab8 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1332,11 +1332,13 @@  static int shmem_writepage(struct page *page, struct writeback_control *wbc)
 {
 	struct folio *folio = page_folio(page);
 	struct shmem_inode_info *info;
-	struct address_space *mapping;
-	struct inode *inode;
+	struct address_space *mapping = folio->mapping;
+	struct inode *inode = mapping->host;
 	swp_entry_t swap;
 	pgoff_t index;
 
+	BUG_ON(!folio_test_locked(folio));
+
 	/*
 	 * If /sys/kernel/mm/transparent_hugepage/shmem_enabled is "always" or
 	 * "force", drivers/gpu/drm/i915/gem/i915_gem_shmem.c gets huge pages,
@@ -1351,8 +1353,8 @@  static int shmem_writepage(struct page *page, struct writeback_control *wbc)
 		folio_clear_dirty(folio);
 	}
 
+	/* Can the folio or first inode change on after a split? */
 	BUG_ON(!folio_test_locked(folio));
-	mapping = folio->mapping;
 	index = folio->index;
 	inode = mapping->host;
 	info = SHMEM_I(inode);