| Message ID | 20230118094329.9553-5-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/9] mm: don't look at xarray value entries in split_huge_pages_in_file | expand |
On Wed, Jan 18, 2023 at 10:43:24AM +0100, Christoph Hellwig wrote: > Add a new SGP_FIND mode for shmem_get_partial_folio that works like > SGP_READ, but does not check i_size. Use that instead of open coding > the page cache lookup in shmem_get_partial_folio. Note that this is > a behavior change in that it reads in swap cache entries for offsets > outside i_size, possibly causing a little bit of extra work. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > include/linux/shmem_fs.h | 1 + > mm/shmem.c | 46 ++++++++++++---------------------------- > 2 files changed, 15 insertions(+), 32 deletions(-) > > diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h > index d09d54be4ffd99..7ba160ac066e5e 100644 > --- a/include/linux/shmem_fs.h > +++ b/include/linux/shmem_fs.h > @@ -105,6 +105,7 @@ enum sgp_type { > SGP_CACHE, /* don't exceed i_size, may allocate page */ > SGP_WRITE, /* may exceed i_size, may allocate !Uptodate page */ > SGP_FALLOC, /* like SGP_WRITE, but make existing page Uptodate */ > + SGP_FIND, /* like SGP_READ, but also read outside i_size */ > }; > > int shmem_get_folio(struct inode *inode, pgoff_t index, struct folio **foliop, > diff --git a/mm/shmem.c b/mm/shmem.c > index 9e1015cbad29f9..e9500fea43a8dc 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -877,27 +877,6 @@ void shmem_unlock_mapping(struct address_space *mapping) > } > } > > -static struct folio *shmem_get_partial_folio(struct inode *inode, pgoff_t index) > -{ > - struct folio *folio; > - > - /* > - * At first avoid shmem_get_folio(,,,SGP_READ): that fails > - * beyond i_size, and reports fallocated pages as holes. > - */ > - folio = __filemap_get_folio(inode->i_mapping, index, > - FGP_ENTRY | FGP_LOCK, 0); This all seems reasonable to me at a glance, FWIW, but I am a little curious why this wouldn't split up into two changes. I.e., switch this over to filemap_get_entry() to minimally remove the FGP_ENTRY dependency without a behavior change, then (perhaps after the next patch) introduce SGP_FIND in a separate patch. That makes it easier to review and potentially undo if it happens to pose a problem in the future. Hm? Brian > - if (!xa_is_value(folio)) > - return folio; > - /* > - * But read a page back from swap if any of it is within i_size > - * (although in some cases this is just a waste of time). > - */ > - folio = NULL; > - shmem_get_folio(inode, index, &folio, SGP_READ); > - return folio; > -} > - > /* > * Remove range of pages and swap entries from page cache, and free them. > * If !unfalloc, truncate or punch hole; if unfalloc, undo failed fallocate. > @@ -957,7 +936,8 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend, > goto whole_folios; > > same_folio = (lstart >> PAGE_SHIFT) == (lend >> PAGE_SHIFT); > - folio = shmem_get_partial_folio(inode, lstart >> PAGE_SHIFT); > + folio = NULL; > + shmem_get_folio(inode, lstart >> PAGE_SHIFT, &folio, SGP_FIND); > if (folio) { > same_folio = lend < folio_pos(folio) + folio_size(folio); > folio_mark_dirty(folio); > @@ -971,14 +951,16 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend, > folio = NULL; > } > > - if (!same_folio) > - folio = shmem_get_partial_folio(inode, lend >> PAGE_SHIFT); > - if (folio) { > - folio_mark_dirty(folio); > - if (!truncate_inode_partial_folio(folio, lstart, lend)) > - end = folio->index; > - folio_unlock(folio); > - folio_put(folio); > + if (!same_folio) { > + folio = NULL; > + shmem_get_folio(inode, lend >> PAGE_SHIFT, &folio, SGP_FIND); > + if (folio) { > + folio_mark_dirty(folio); > + if (!truncate_inode_partial_folio(folio, lstart, lend)) > + end = folio->index; > + folio_unlock(folio); > + folio_put(folio); > + } > } > > whole_folios: > @@ -1900,7 +1882,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index, > if (folio_test_uptodate(folio)) > goto out; > /* fallocated folio */ > - if (sgp != SGP_READ) > + if (sgp != SGP_READ && sgp != SGP_FIND) > goto clear; > folio_unlock(folio); > folio_put(folio); > @@ -1911,7 +1893,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index, > * SGP_NOALLOC: fail on hole, with NULL folio, letting caller fail. > */ > *foliop = NULL; > - if (sgp == SGP_READ) > + if (sgp == SGP_READ || sgp == SGP_FIND) > return 0; > if (sgp == SGP_NOALLOC) > return -ENOENT; > -- > 2.39.0 > >
On Wed, Jan 18, 2023 at 08:57:05AM -0500, Brian Foster wrote: > This all seems reasonable to me at a glance, FWIW, but I am a little > curious why this wouldn't split up into two changes. I.e., switch this > over to filemap_get_entry() to minimally remove the FGP_ENTRY dependency > without a behavior change, then (perhaps after the next patch) introduce > SGP_FIND in a separate patch. That makes it easier to review and > potentially undo if it happens to pose a problem in the future. Hm? The minimal change to filemap_get_entry would require to add the lock, check mapping and retry loop and thus add a fair amount of code. So I looked for ways to avoid that and came up with this version. But if there is a strong preference to first open code the logic and then later consolidate it I could do that.
On Wed, Jan 18, 2023 at 05:43:58PM +0100, Christoph Hellwig wrote: > On Wed, Jan 18, 2023 at 08:57:05AM -0500, Brian Foster wrote: > > This all seems reasonable to me at a glance, FWIW, but I am a little > > curious why this wouldn't split up into two changes. I.e., switch this > > over to filemap_get_entry() to minimally remove the FGP_ENTRY dependency > > without a behavior change, then (perhaps after the next patch) introduce > > SGP_FIND in a separate patch. That makes it easier to review and > > potentially undo if it happens to pose a problem in the future. Hm? > > The minimal change to filemap_get_entry would require to add the > lock, check mapping and retry loop and thus add a fair amount of > code. So I looked for ways to avoid that and came up with this > version. But if there is a strong preference to first open code > the logic and then later consolidate it I could do that. > Ok. Not a strong preference from me. I don't think it's worth complicating that much just to split up. Brian
diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h index d09d54be4ffd99..7ba160ac066e5e 100644 --- a/include/linux/shmem_fs.h +++ b/include/linux/shmem_fs.h @@ -105,6 +105,7 @@ enum sgp_type { SGP_CACHE, /* don't exceed i_size, may allocate page */ SGP_WRITE, /* may exceed i_size, may allocate !Uptodate page */ SGP_FALLOC, /* like SGP_WRITE, but make existing page Uptodate */ + SGP_FIND, /* like SGP_READ, but also read outside i_size */ }; int shmem_get_folio(struct inode *inode, pgoff_t index, struct folio **foliop, diff --git a/mm/shmem.c b/mm/shmem.c index 9e1015cbad29f9..e9500fea43a8dc 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -877,27 +877,6 @@ void shmem_unlock_mapping(struct address_space *mapping) } } -static struct folio *shmem_get_partial_folio(struct inode *inode, pgoff_t index) -{ - struct folio *folio; - - /* - * At first avoid shmem_get_folio(,,,SGP_READ): that fails - * beyond i_size, and reports fallocated pages as holes. - */ - folio = __filemap_get_folio(inode->i_mapping, index, - FGP_ENTRY | FGP_LOCK, 0); - if (!xa_is_value(folio)) - return folio; - /* - * But read a page back from swap if any of it is within i_size - * (although in some cases this is just a waste of time). - */ - folio = NULL; - shmem_get_folio(inode, index, &folio, SGP_READ); - return folio; -} - /* * Remove range of pages and swap entries from page cache, and free them. * If !unfalloc, truncate or punch hole; if unfalloc, undo failed fallocate. @@ -957,7 +936,8 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend, goto whole_folios; same_folio = (lstart >> PAGE_SHIFT) == (lend >> PAGE_SHIFT); - folio = shmem_get_partial_folio(inode, lstart >> PAGE_SHIFT); + folio = NULL; + shmem_get_folio(inode, lstart >> PAGE_SHIFT, &folio, SGP_FIND); if (folio) { same_folio = lend < folio_pos(folio) + folio_size(folio); folio_mark_dirty(folio); @@ -971,14 +951,16 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend, folio = NULL; } - if (!same_folio) - folio = shmem_get_partial_folio(inode, lend >> PAGE_SHIFT); - if (folio) { - folio_mark_dirty(folio); - if (!truncate_inode_partial_folio(folio, lstart, lend)) - end = folio->index; - folio_unlock(folio); - folio_put(folio); + if (!same_folio) { + folio = NULL; + shmem_get_folio(inode, lend >> PAGE_SHIFT, &folio, SGP_FIND); + if (folio) { + folio_mark_dirty(folio); + if (!truncate_inode_partial_folio(folio, lstart, lend)) + end = folio->index; + folio_unlock(folio); + folio_put(folio); + } } whole_folios: @@ -1900,7 +1882,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index, if (folio_test_uptodate(folio)) goto out; /* fallocated folio */ - if (sgp != SGP_READ) + if (sgp != SGP_READ && sgp != SGP_FIND) goto clear; folio_unlock(folio); folio_put(folio); @@ -1911,7 +1893,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index, * SGP_NOALLOC: fail on hole, with NULL folio, letting caller fail. */ *foliop = NULL; - if (sgp == SGP_READ) + if (sgp == SGP_READ || sgp == SGP_FIND) return 0; if (sgp == SGP_NOALLOC) return -ENOENT;
Add a new SGP_FIND mode for shmem_get_partial_folio that works like SGP_READ, but does not check i_size. Use that instead of open coding the page cache lookup in shmem_get_partial_folio. Note that this is a behavior change in that it reads in swap cache entries for offsets outside i_size, possibly causing a little bit of extra work. Signed-off-by: Christoph Hellwig <hch@lst.de> --- include/linux/shmem_fs.h | 1 + mm/shmem.c | 46 ++++++++++++---------------------------- 2 files changed, 15 insertions(+), 32 deletions(-)