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