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

[Matthew Wilcox]

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@xxxxxxxxxx>
> ---
>  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
>