| Message ID | 20230307143410.28031-5-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] mm: don't look at xarray value entries in split_huge_pages_in_file | expand |
On Tue, 7 Mar 2023, 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(-) I thought this was fine at first, and of course it's good for all the usual cases; but not for shmem_get_partial_folio()'s awkward cases. Two issues with it. One, as you highlight above, the possibility of reading more swap unnecessarily. I do not mind if partial truncation entails reading a little unnecessary swap; but I don't like the common case of truncation to 0 to entail that; even less eviction; even less unmounting, when eviction of all risks reading lots of swap. The old code behaved well at i_size 0, the new code not so much. Two, truncating a large folio is expected to trim that folio down to the smaller sizei (if splitting allows); but SGP_FIND was coded too much like SGP_READ, in reporting fallocated (!uptodate) folios as NULL, unlike before. Then the following loop of shmem_undo_range() removed that whole folio - removing pages "promised" to the file by the earlier fallocate. Not as seriously bad as deleting data would be, and not very likely to be noticed, but still not right. Replacing shmem_get_partial_folio() by SGP_FIND was a good direction to try, but it hasn't worked out. I tried to get SGPs to work right for it before, when shmem_get_partial_page() was introduced; but I did not manage to do so. I think we have to go back to how this was. Andrew, please replace Christoph's "shmem: remove shmem_get_partial_folio" in mm-unstable by this patch below, which achieves the same object (eliminating FGP_ENTRY) while keeping closer to the old mechanism. [PATCH] shmem: shmem_get_partial_folio use filemap_get_entry To avoid use of the FGP_ENTRY flag, adapt shmem_get_partial_folio() to use filemap_get_entry() and folio_lock() instead of __filemap_get_folio(). Update "page" in the comments there to "folio". Signed-off-by: Hugh Dickins <hughd@google.com> --- mm/shmem.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) --- a/mm/shmem.c +++ b/mm/shmem.c @@ -886,14 +886,21 @@ static struct folio *shmem_get_partial_folio(struct inode *inode, pgoff_t index) /* * At first avoid shmem_get_folio(,,,SGP_READ): that fails - * beyond i_size, and reports fallocated pages as holes. + * beyond i_size, and reports fallocated folios as holes. */ - folio = __filemap_get_folio(inode->i_mapping, index, - FGP_ENTRY | FGP_LOCK, 0); - if (!xa_is_value(folio)) + folio = filemap_get_entry(inode->i_mapping, index); + if (!folio) return folio; + if (!xa_is_value(folio)) { + folio_lock(folio); + if (folio->mapping == inode->i_mapping) + return folio; + /* The folio has been swapped out */ + folio_unlock(folio); + folio_put(folio); + } /* - * But read a page back from swap if any of it is within i_size + * But read a folio back from swap if any of it is within i_size * (although in some cases this is just a waste of time). */ folio = NULL;
On Sun, Mar 19, 2023 at 10:19:21PM -0700, Hugh Dickins wrote: > I thought this was fine at first, and of course it's good for all the > usual cases; but not for shmem_get_partial_folio()'s awkward cases. > > Two issues with it. > > One, as you highlight above, the possibility of reading more swap > unnecessarily. I do not mind if partial truncation entails reading > a little unnecessary swap; but I don't like the common case of > truncation to 0 to entail that; even less eviction; even less > unmounting, when eviction of all risks reading lots of swap. > The old code behaved well at i_size 0, the new code not so much. True. We could restore that by doing the i_size check for SGP_FIND, though. > Replacing shmem_get_partial_folio() by SGP_FIND was a good direction > to try, but it hasn't worked out. I tried to get SGPs to work right > for it before, when shmem_get_partial_page() was introduced; but I > did not manage to do so. I think we have to go back to how this was. Hmm, would be sad to lose this entirely. One thing I though about but didn't manage to do, is to rework how the SGP_* flags works. Right now they are used as en enum, and we actually do numerical comparisms on them, which is highly confusing. To be it seems like having actual flags that can be combined and have useful names would seem much better. But I did run out patience for finding good names and figuring out what would be granular enough behavior for such flags. e.g. one would be for limiting to i_size, one for allocating new folios if none was found, etc.
diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h index 103d1000a5a2e2..b87df1b137d3e5 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 448f393d8ab2b1..3705437c5757ba 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(-)