The critical patch to review here is patch 11, "Handle truncates that
split THPs". This code is shared with shmem, and while xfstests passes
(both with the tmpfs filesystem and with THPs enabled for XFS), it is
terribly subtle.
I posted a similar patch series a few weeks ago [1], but this goes a few
steps further than that one did. In addition to the unification of
find_get_entries() and pagevec_lookup_entries(), this patch series
includes:
- Only return the head pages from tagged lookups
- Factor a lot of common code out of the various batch lookup routines
- Add mapping_seek_hole_data()
- Only return head pages from find_get_entries
I also have a patch to iomap to use mapping_seek_hole_data(), but I'm
not including that as part of this batch of patches -- I'll send it
through the iomap tree once mapping_seek_hole_data() lands upstream.
[1] https://lore.kernel.org/linux-mm/[email protected]/
Matthew Wilcox (Oracle) (12):
mm: Make pagecache tagged lookups return only head pages
mm/shmem: Use pagevec_lookup in shmem_unlock_mapping
mm/filemap: Add helper for finding pages
mm/filemap: Add mapping_seek_hole_data
mm: Add and use find_lock_entries
mm: Add an 'end' parameter to find_get_entries
mm: Add an 'end' parameter to pagevec_lookup_entries
mm: Remove nr_entries parameter from pagevec_lookup_entries
mm: Pass pvec directly to find_get_entries
mm: Remove pagevec_lookup_entries
mm/truncate,shmem: Handle truncates that split THPs
mm/filemap: Return only head pages from find_get_entries
include/linux/pagemap.h | 5 +-
include/linux/pagevec.h | 4 -
mm/filemap.c | 267 +++++++++++++++++++++++++++-------------
mm/internal.h | 5 +
mm/shmem.c | 214 +++++++-------------------------
mm/swap.c | 38 +-----
mm/truncate.c | 249 ++++++++++++++-----------------------
7 files changed, 329 insertions(+), 453 deletions(-)
--
2.28.0
Simplifies the callers and uses the existing functionality
in find_get_entries(). We can also drop the final argument of
truncate_exceptional_pvec_entries() and simplify the logic in that
function.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
include/linux/pagevec.h | 5 ++---
mm/swap.c | 8 ++++----
mm/truncate.c | 41 ++++++++++-------------------------------
3 files changed, 16 insertions(+), 38 deletions(-)
diff --git a/include/linux/pagevec.h b/include/linux/pagevec.h
index 081d934eda64..4b245592262c 100644
--- a/include/linux/pagevec.h
+++ b/include/linux/pagevec.h
@@ -26,9 +26,8 @@ struct pagevec {
void __pagevec_release(struct pagevec *pvec);
void __pagevec_lru_add(struct pagevec *pvec);
unsigned pagevec_lookup_entries(struct pagevec *pvec,
- struct address_space *mapping,
- pgoff_t start, unsigned nr_entries,
- pgoff_t *indices);
+ struct address_space *mapping, pgoff_t start, pgoff_t end,
+ unsigned nr_entries, pgoff_t *indices);
void pagevec_remove_exceptionals(struct pagevec *pvec);
unsigned pagevec_lookup_range(struct pagevec *pvec,
struct address_space *mapping,
diff --git a/mm/swap.c b/mm/swap.c
index fcf6ccb94b09..b6e56a84b466 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -1036,6 +1036,7 @@ void __pagevec_lru_add(struct pagevec *pvec)
* @pvec: Where the resulting entries are placed
* @mapping: The address_space to search
* @start: The starting entry index
+ * @end: The highest index to return (inclusive).
* @nr_entries: The maximum number of pages
* @indices: The cache indices corresponding to the entries in @pvec
*
@@ -1056,11 +1057,10 @@ void __pagevec_lru_add(struct pagevec *pvec)
* found.
*/
unsigned pagevec_lookup_entries(struct pagevec *pvec,
- struct address_space *mapping,
- pgoff_t start, unsigned nr_entries,
- pgoff_t *indices)
+ struct address_space *mapping, pgoff_t start, pgoff_t end,
+ unsigned nr_entries, pgoff_t *indices)
{
- pvec->nr = find_get_entries(mapping, start, ULONG_MAX, nr_entries,
+ pvec->nr = find_get_entries(mapping, start, end, nr_entries,
pvec->pages, indices);
return pagevec_count(pvec);
}
diff --git a/mm/truncate.c b/mm/truncate.c
index 5dbe0c77b5ac..69ea72e7fc1c 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -57,11 +57,10 @@ static void clear_shadow_entry(struct address_space *mapping, pgoff_t index,
* exceptional entries similar to what pagevec_remove_exceptionals does.
*/
static void truncate_exceptional_pvec_entries(struct address_space *mapping,
- struct pagevec *pvec, pgoff_t *indices,
- pgoff_t end)
+ struct pagevec *pvec, pgoff_t *indices)
{
int i, j;
- bool dax, lock;
+ bool dax;
/* Handled by shmem itself */
if (shmem_mapping(mapping))
@@ -75,8 +74,7 @@ static void truncate_exceptional_pvec_entries(struct address_space *mapping,
return;
dax = dax_mapping(mapping);
- lock = !dax && indices[j] < end;
- if (lock)
+ if (!dax)
xa_lock_irq(&mapping->i_pages);
for (i = j; i < pagevec_count(pvec); i++) {
@@ -88,9 +86,6 @@ static void truncate_exceptional_pvec_entries(struct address_space *mapping,
continue;
}
- if (index >= end)
- continue;
-
if (unlikely(dax)) {
dax_delete_mapping_entry(mapping, index);
continue;
@@ -99,7 +94,7 @@ static void truncate_exceptional_pvec_entries(struct address_space *mapping,
__clear_shadow_entry(mapping, index, page);
}
- if (lock)
+ if (!dax)
xa_unlock_irq(&mapping->i_pages);
pvec->nr = j;
}
@@ -329,7 +324,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
while (index < end && find_lock_entries(mapping, index, end - 1,
&pvec, indices)) {
index = indices[pagevec_count(&pvec) - 1] + 1;
- truncate_exceptional_pvec_entries(mapping, &pvec, indices, end);
+ truncate_exceptional_pvec_entries(mapping, &pvec, indices);
for (i = 0; i < pagevec_count(&pvec); i++)
truncate_cleanup_page(mapping, pvec.pages[i]);
delete_from_page_cache_batch(mapping, &pvec);
@@ -381,8 +376,8 @@ void truncate_inode_pages_range(struct address_space *mapping,
index = start;
for ( ; ; ) {
cond_resched();
- if (!pagevec_lookup_entries(&pvec, mapping, index,
- min(end - index, (pgoff_t)PAGEVEC_SIZE), indices)) {
+ if (!pagevec_lookup_entries(&pvec, mapping, index, end - 1,
+ PAGEVEC_SIZE, indices)) {
/* If all gone from start onwards, we're done */
if (index == start)
break;
@@ -390,23 +385,12 @@ void truncate_inode_pages_range(struct address_space *mapping,
index = start;
continue;
}
- if (index == start && indices[0] >= end) {
- /* All gone out of hole to be punched, we're done */
- pagevec_remove_exceptionals(&pvec);
- pagevec_release(&pvec);
- break;
- }
for (i = 0; i < pagevec_count(&pvec); i++) {
struct page *page = pvec.pages[i];
/* We rely upon deletion not changing page->index */
index = indices[i];
- if (index >= end) {
- /* Restart punch to make sure all gone */
- index = start - 1;
- break;
- }
if (xa_is_value(page))
continue;
@@ -417,7 +401,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
truncate_inode_page(mapping, page);
unlock_page(page);
}
- truncate_exceptional_pvec_entries(mapping, &pvec, indices, end);
+ truncate_exceptional_pvec_entries(mapping, &pvec, indices);
pagevec_release(&pvec);
index++;
}
@@ -528,8 +512,6 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
/* We rely upon deletion not changing page->index */
index = indices[i];
- if (index > end)
- break;
if (xa_is_value(page)) {
invalidate_exceptional_entry(mapping, index,
@@ -629,16 +611,13 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
pagevec_init(&pvec);
index = start;
- while (index <= end && pagevec_lookup_entries(&pvec, mapping, index,
- min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1,
- indices)) {
+ while (pagevec_lookup_entries(&pvec, mapping, index, end,
+ PAGEVEC_SIZE, indices)) {
for (i = 0; i < pagevec_count(&pvec); i++) {
struct page *page = pvec.pages[i];
/* We rely upon deletion not changing page->index */
index = indices[i];
- if (index > end)
- break;
if (xa_is_value(page)) {
if (!invalidate_exceptional_entry2(mapping,
--
2.28.0
Rewrite shmem_seek_hole_data() and move it to filemap.c.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
include/linux/pagemap.h | 2 ++
mm/filemap.c | 68 ++++++++++++++++++++++++++++++++++++++
mm/shmem.c | 72 +++--------------------------------------
3 files changed, 74 insertions(+), 68 deletions(-)
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 4e52a3ff92fb..869dc371b800 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -724,6 +724,8 @@ extern void __delete_from_page_cache(struct page *page, void *shadow);
int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask);
void delete_from_page_cache_batch(struct address_space *mapping,
struct pagevec *pvec);
+loff_t mapping_seek_hole_data(struct address_space *, loff_t start, loff_t end,
+ int whence);
/*
* Like add_to_page_cache_locked, but used to add newly allocated pages:
diff --git a/mm/filemap.c b/mm/filemap.c
index d8f5ff07eb9c..5662f932b85b 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2416,6 +2416,74 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
}
EXPORT_SYMBOL(generic_file_read_iter);
+static inline
+unsigned int seek_page_size(struct xa_state *xas, struct page *page)
+{
+ if (xa_is_value(page))
+ return PAGE_SIZE << xa_get_order(xas->xa, xas->xa_index);
+ return thp_size(page);
+}
+
+/**
+ * mapping_seek_hole_data - Seek for SEEK_DATA / SEEK_HOLE in the page cache.
+ * @mapping: Address space to search.
+ * @start: First byte to consider.
+ * @end: Limit of search (exclusive).
+ * @whence: Either SEEK_HOLE or SEEK_DATA.
+ *
+ * If the page cache knows which blocks contain holes and which blocks
+ * contain data, your filesystem can use this function to implement
+ * SEEK_HOLE and SEEK_DATA. This is useful for filesystems which are
+ * entirely memory-based such as tmpfs, and filesystems which support
+ * unwritten extents.
+ *
+ * Return: The requested offset on successs, or -ENXIO if @whence specifies
+ * SEEK_DATA and there is no data after @start. There is an implicit hole
+ * after @end - 1, so SEEK_HOLE returns @end if all the bytes between @start
+ * and @end contain data.
+ */
+loff_t mapping_seek_hole_data(struct address_space *mapping, loff_t start,
+ loff_t end, int whence)
+{
+ XA_STATE(xas, &mapping->i_pages, start >> PAGE_SHIFT);
+ pgoff_t max = (end - 1) / PAGE_SIZE;
+ bool seek_data = (whence == SEEK_DATA);
+ struct page *page;
+
+ if (end <= start)
+ return -ENXIO;
+
+ rcu_read_lock();
+ while ((page = xas_find_get_entry(&xas, max, XA_PRESENT))) {
+ loff_t pos = xas.xa_index * PAGE_SIZE;
+
+ if (start < pos) {
+ if (!seek_data)
+ goto unlock;
+ start = pos;
+ }
+
+ if (seek_data)
+ goto unlock;
+
+ start = pos + seek_page_size(&xas, page);
+ }
+ rcu_read_unlock();
+
+ if (seek_data)
+ return -ENXIO;
+ goto out;
+
+unlock:
+ rcu_read_unlock();
+ if (!xa_is_value(page))
+ put_page(page);
+out:
+ if (start > end)
+ return end;
+ return start;
+}
+
#ifdef CONFIG_MMU
#define MMAP_LOTSAMISS (100)
/*
diff --git a/mm/shmem.c b/mm/shmem.c
index 108931a6cc43..b65263d9bb67 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2659,85 +2659,21 @@ static ssize_t shmem_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
return retval ? retval : error;
}
-/*
- * llseek SEEK_DATA or SEEK_HOLE through the page cache.
- */
-static pgoff_t shmem_seek_hole_data(struct address_space *mapping,
- pgoff_t index, pgoff_t end, int whence)
-{
- struct page *page;
- struct pagevec pvec;
- pgoff_t indices[PAGEVEC_SIZE];
- bool done = false;
- int i;
-
- pagevec_init(&pvec);
- pvec.nr = 1; /* start small: we may be there already */
- while (!done) {
- pvec.nr = find_get_entries(mapping, index,
- pvec.nr, pvec.pages, indices);
- if (!pvec.nr) {
- if (whence == SEEK_DATA)
- index = end;
- break;
- }
- for (i = 0; i < pvec.nr; i++, index++) {
- if (index < indices[i]) {
- if (whence == SEEK_HOLE) {
- done = true;
- break;
- }
- index = indices[i];
- }
- page = pvec.pages[i];
- if (page && !xa_is_value(page)) {
- if (!PageUptodate(page))
- page = NULL;
- }
- if (index >= end ||
- (page && whence == SEEK_DATA) ||
- (!page && whence == SEEK_HOLE)) {
- done = true;
- break;
- }
- }
- pagevec_remove_exceptionals(&pvec);
- pagevec_release(&pvec);
- pvec.nr = PAGEVEC_SIZE;
- cond_resched();
- }
- return index;
-}
-
static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence)
{
struct address_space *mapping = file->f_mapping;
struct inode *inode = mapping->host;
- pgoff_t start, end;
- loff_t new_offset;
if (whence != SEEK_DATA && whence != SEEK_HOLE)
return generic_file_llseek_size(file, offset, whence,
MAX_LFS_FILESIZE, i_size_read(inode));
+ if (offset < 0)
+ return -ENXIO;
+
inode_lock(inode);
/* We're holding i_mutex so we can access i_size directly */
- if (offset < 0 || offset >= inode->i_size)
- offset = -ENXIO;
- else {
- start = offset >> PAGE_SHIFT;
- end = (inode->i_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
- new_offset = shmem_seek_hole_data(mapping, start, end, whence);
- new_offset <<= PAGE_SHIFT;
- if (new_offset > offset) {
- if (new_offset < inode->i_size)
- offset = new_offset;
- else if (whence == SEEK_DATA)
- offset = -ENXIO;
- else
- offset = inode->i_size;
- }
- }
+ offset = mapping_seek_hole_data(mapping, offset, inode->i_size, whence);
if (offset >= 0)
offset = vfs_setpos(file, offset, MAX_LFS_FILESIZE);
--
2.28.0
This simplifies the callers and leads to a more efficient implementation
since the XArray has this functionality already.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
Reviewed-by: William Kucharski <[email protected]>
---
include/linux/pagemap.h | 4 ++--
mm/filemap.c | 9 +++++----
mm/shmem.c | 10 ++--------
mm/swap.c | 2 +-
4 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 869dc371b800..d440c6750757 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -414,8 +414,8 @@ static inline struct page *find_subpage(struct page *head, pgoff_t index)
}
unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
- unsigned int nr_entries, struct page **entries,
- pgoff_t *indices);
+ pgoff_t end, unsigned int nr_entries, struct page **entries,
+ pgoff_t *indices);
unsigned find_get_pages_range(struct address_space *mapping, pgoff_t *start,
pgoff_t end, unsigned int nr_pages,
struct page **pages);
diff --git a/mm/filemap.c b/mm/filemap.c
index 9f7f6b46aee4..6dc0a9b8c0fa 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1799,6 +1799,7 @@ static inline struct page *xas_find_get_entry(struct xa_state *xas,
* find_get_entries - gang pagecache lookup
* @mapping: The address_space to search
* @start: The starting page cache index
+ * @end: The final page index (inclusive).
* @nr_entries: The maximum number of entries
* @entries: Where the resulting entries are placed
* @indices: The cache indices corresponding to the entries in @entries
@@ -1822,9 +1823,9 @@ static inline struct page *xas_find_get_entry(struct xa_state *xas,
*
* Return: the number of pages and shadow entries which were found.
*/
-unsigned find_get_entries(struct address_space *mapping,
- pgoff_t start, unsigned int nr_entries,
- struct page **entries, pgoff_t *indices)
+unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
+ pgoff_t end, unsigned int nr_entries, struct page **entries,
+ pgoff_t *indices)
{
XA_STATE(xas, &mapping->i_pages, start);
struct page *page;
@@ -1834,7 +1835,7 @@ unsigned find_get_entries(struct address_space *mapping,
return 0;
rcu_read_lock();
- while ((page = xas_find_get_entry(&xas, ULONG_MAX, XA_PRESENT))) {
+ while ((page = xas_find_get_entry(&xas, end, XA_PRESENT))) {
/*
* Terminate early on finding a THP, to allow the caller to
* handle it all at once; but continue if this is hugetlbfs.
diff --git a/mm/shmem.c b/mm/shmem.c
index a73ce8ce28e3..404e45c285ca 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -911,8 +911,6 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
struct page *page = pvec.pages[i];
index = indices[i];
- if (index >= end)
- break;
if (xa_is_value(page)) {
if (unfalloc)
@@ -965,9 +963,8 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
while (index < end) {
cond_resched();
- pvec.nr = find_get_entries(mapping, index,
- min(end - index, (pgoff_t)PAGEVEC_SIZE),
- pvec.pages, indices);
+ pvec.nr = find_get_entries(mapping, index, end - 1,
+ PAGEVEC_SIZE, pvec.pages, indices);
if (!pvec.nr) {
/* If all gone or hole-punch or unfalloc, we're done */
if (index == start || end != -1)
@@ -980,9 +977,6 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
struct page *page = pvec.pages[i];
index = indices[i];
- if (index >= end)
- break;
-
if (xa_is_value(page)) {
if (unfalloc)
continue;
diff --git a/mm/swap.c b/mm/swap.c
index d16d65d9b4e0..fcf6ccb94b09 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -1060,7 +1060,7 @@ unsigned pagevec_lookup_entries(struct pagevec *pvec,
pgoff_t start, unsigned nr_entries,
pgoff_t *indices)
{
- pvec->nr = find_get_entries(mapping, start, nr_entries,
+ pvec->nr = find_get_entries(mapping, start, ULONG_MAX, nr_entries,
pvec->pages, indices);
return pagevec_count(pvec);
}
--
2.28.0
All callers of find_get_entries() use a pvec, so pass it directly
instead of manipulating it in the caller.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
include/linux/pagemap.h | 3 +--
mm/filemap.c | 21 +++++++++------------
mm/shmem.c | 5 ++---
mm/swap.c | 4 +---
4 files changed, 13 insertions(+), 20 deletions(-)
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index d440c6750757..788c280737e0 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -414,8 +414,7 @@ static inline struct page *find_subpage(struct page *head, pgoff_t index)
}
unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
- pgoff_t end, unsigned int nr_entries, struct page **entries,
- pgoff_t *indices);
+ pgoff_t end, struct pagevec *pvec, pgoff_t *indices);
unsigned find_get_pages_range(struct address_space *mapping, pgoff_t *start,
pgoff_t end, unsigned int nr_pages,
struct page **pages);
diff --git a/mm/filemap.c b/mm/filemap.c
index 6dc0a9b8c0fa..a4ad294a34dc 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1800,14 +1800,12 @@ static inline struct page *xas_find_get_entry(struct xa_state *xas,
* @mapping: The address_space to search
* @start: The starting page cache index
* @end: The final page index (inclusive).
- * @nr_entries: The maximum number of entries
- * @entries: Where the resulting entries are placed
+ * @pvec: Where the resulting entries are placed.
* @indices: The cache indices corresponding to the entries in @entries
*
- * find_get_entries() will search for and return a group of up to
- * @nr_entries entries in the mapping. The entries are placed at
- * @entries. find_get_entries() takes a reference against any actual
- * pages it returns.
+ * find_get_entries() will search for and return a batch of entries in
+ * the mapping. The entries are placed in @pvec. find_get_entries()
+ * takes a reference on any actual pages it returns.
*
* The search returns a group of mapping-contiguous page cache entries
* with ascending indexes. There may be holes in the indices due to
@@ -1824,15 +1822,12 @@ static inline struct page *xas_find_get_entry(struct xa_state *xas,
* Return: the number of pages and shadow entries which were found.
*/
unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
- pgoff_t end, unsigned int nr_entries, struct page **entries,
- pgoff_t *indices)
+ pgoff_t end, struct pagevec *pvec, pgoff_t *indices)
{
XA_STATE(xas, &mapping->i_pages, start);
struct page *page;
unsigned int ret = 0;
-
- if (!nr_entries)
- return 0;
+ unsigned nr_entries = PAGEVEC_SIZE;
rcu_read_lock();
while ((page = xas_find_get_entry(&xas, end, XA_PRESENT))) {
@@ -1847,11 +1842,13 @@ unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
}
indices[ret] = xas.xa_index;
- entries[ret] = page;
+ pvec->pages[ret] = page;
if (++ret == nr_entries)
break;
}
rcu_read_unlock();
+
+ pvec->nr = ret;
return ret;
}
diff --git a/mm/shmem.c b/mm/shmem.c
index 404e45c285ca..b2b94025841a 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -963,9 +963,8 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
while (index < end) {
cond_resched();
- pvec.nr = find_get_entries(mapping, index, end - 1,
- PAGEVEC_SIZE, pvec.pages, indices);
- if (!pvec.nr) {
+ if (!find_get_entries(mapping, index, end - 1, &pvec,
+ indices)) {
/* If all gone or hole-punch or unfalloc, we're done */
if (index == start || end != -1)
break;
diff --git a/mm/swap.c b/mm/swap.c
index d4e3ba4c967c..40b23300d353 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -1060,9 +1060,7 @@ unsigned pagevec_lookup_entries(struct pagevec *pvec,
struct address_space *mapping, pgoff_t start, pgoff_t end,
pgoff_t *indices)
{
- pvec->nr = find_get_entries(mapping, start, end, PAGEVEC_SIZE,
- pvec->pages, indices);
- return pagevec_count(pvec);
+ return find_get_entries(mapping, start, end, pvec, indices);
}
/**
--
2.28.0
ping
On Mon, Sep 14, 2020 at 02:00:30PM +0100, Matthew Wilcox (Oracle) wrote:
> The critical patch to review here is patch 11, "Handle truncates that
> split THPs". This code is shared with shmem, and while xfstests passes
> (both with the tmpfs filesystem and with THPs enabled for XFS), it is
> terribly subtle.
>
> I posted a similar patch series a few weeks ago [1], but this goes a few
> steps further than that one did. In addition to the unification of
> find_get_entries() and pagevec_lookup_entries(), this patch series
> includes:
>
> - Only return the head pages from tagged lookups
> - Factor a lot of common code out of the various batch lookup routines
> - Add mapping_seek_hole_data()
> - Only return head pages from find_get_entries
>
> I also have a patch to iomap to use mapping_seek_hole_data(), but I'm
> not including that as part of this batch of patches -- I'll send it
> through the iomap tree once mapping_seek_hole_data() lands upstream.
>
> [1] https://lore.kernel.org/linux-mm/[email protected]/
>
> Matthew Wilcox (Oracle) (12):
> mm: Make pagecache tagged lookups return only head pages
> mm/shmem: Use pagevec_lookup in shmem_unlock_mapping
> mm/filemap: Add helper for finding pages
> mm/filemap: Add mapping_seek_hole_data
> mm: Add and use find_lock_entries
> mm: Add an 'end' parameter to find_get_entries
> mm: Add an 'end' parameter to pagevec_lookup_entries
> mm: Remove nr_entries parameter from pagevec_lookup_entries
> mm: Pass pvec directly to find_get_entries
> mm: Remove pagevec_lookup_entries
> mm/truncate,shmem: Handle truncates that split THPs
> mm/filemap: Return only head pages from find_get_entries
>
> include/linux/pagemap.h | 5 +-
> include/linux/pagevec.h | 4 -
> mm/filemap.c | 267 +++++++++++++++++++++++++++-------------
> mm/internal.h | 5 +
> mm/shmem.c | 214 +++++++-------------------------
> mm/swap.c | 38 +-----
> mm/truncate.c | 249 ++++++++++++++-----------------------
> 7 files changed, 329 insertions(+), 453 deletions(-)
>
> --
> 2.28.0
>
On Mon 14-09-20 14:00:34, Matthew Wilcox (Oracle) wrote:
> Rewrite shmem_seek_hole_data() and move it to filemap.c.
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
...
> +/**
> + * mapping_seek_hole_data - Seek for SEEK_DATA / SEEK_HOLE in the page cache.
> + * @mapping: Address space to search.
> + * @start: First byte to consider.
> + * @end: Limit of search (exclusive).
> + * @whence: Either SEEK_HOLE or SEEK_DATA.
> + *
> + * If the page cache knows which blocks contain holes and which blocks
> + * contain data, your filesystem can use this function to implement
> + * SEEK_HOLE and SEEK_DATA. This is useful for filesystems which are
> + * entirely memory-based such as tmpfs, and filesystems which support
> + * unwritten extents.
> + *
> + * Return: The requested offset on successs, or -ENXIO if @whence specifies
> + * SEEK_DATA and there is no data after @start. There is an implicit hole
> + * after @end - 1, so SEEK_HOLE returns @end if all the bytes between @start
> + * and @end contain data.
> + */
> +loff_t mapping_seek_hole_data(struct address_space *mapping, loff_t start,
> + loff_t end, int whence)
> +{
> + XA_STATE(xas, &mapping->i_pages, start >> PAGE_SHIFT);
> + pgoff_t max = (end - 1) / PAGE_SIZE;
> + bool seek_data = (whence == SEEK_DATA);
> + struct page *page;
> +
> + if (end <= start)
> + return -ENXIO;
> +
> + rcu_read_lock();
> + while ((page = xas_find_get_entry(&xas, max, XA_PRESENT))) {
> + loff_t pos = xas.xa_index * PAGE_SIZE;
OK, but for ordinary filesystems this could be problematic because of
exceptional entries?
Also for shmem you've dropped the PageUptodate check which I'm not sure is
safe?
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
Looks good to me; I really like the addition of the "end" parameter to
find_get_entries() and the conversion of pagevec_lookup_entries().
For the series:
Reviewed-by: William Kucharski <[email protected]>
> On Sep 14, 2020, at 7:00 AM, Matthew Wilcox (Oracle) <[email protected]> wrote:
>
> The critical patch to review here is patch 11, "Handle truncates that
> split THPs". This code is shared with shmem, and while xfstests passes
> (both with the tmpfs filesystem and with THPs enabled for XFS), it is
> terribly subtle.
>
> I posted a similar patch series a few weeks ago [1], but this goes a few
> steps further than that one did. In addition to the unification of
> find_get_entries() and pagevec_lookup_entries(), this patch series
> includes:
>
> - Only return the head pages from tagged lookups
> - Factor a lot of common code out of the various batch lookup routines
> - Add mapping_seek_hole_data()
> - Only return head pages from find_get_entries
>
> I also have a patch to iomap to use mapping_seek_hole_data(), but I'm
> not including that as part of this batch of patches -- I'll send it
> through the iomap tree once mapping_seek_hole_data() lands upstream.
>
> [1] https://lore.kernel.org/linux-mm/[email protected]/
>
> Matthew Wilcox (Oracle) (12):
> mm: Make pagecache tagged lookups return only head pages
> mm/shmem: Use pagevec_lookup in shmem_unlock_mapping
> mm/filemap: Add helper for finding pages
> mm/filemap: Add mapping_seek_hole_data
> mm: Add and use find_lock_entries
> mm: Add an 'end' parameter to find_get_entries
> mm: Add an 'end' parameter to pagevec_lookup_entries
> mm: Remove nr_entries parameter from pagevec_lookup_entries
> mm: Pass pvec directly to find_get_entries
> mm: Remove pagevec_lookup_entries
> mm/truncate,shmem: Handle truncates that split THPs
> mm/filemap: Return only head pages from find_get_entries
>
> include/linux/pagemap.h | 5 +-
> include/linux/pagevec.h | 4 -
> mm/filemap.c | 267 +++++++++++++++++++++++++++-------------
> mm/internal.h | 5 +
> mm/shmem.c | 214 +++++++-------------------------
> mm/swap.c | 38 +-----
> mm/truncate.c | 249 ++++++++++++++-----------------------
> 7 files changed, 329 insertions(+), 453 deletions(-)
>
> --
> 2.28.0
>
On Mon 14-09-20 14:00:37, Matthew Wilcox (Oracle) wrote:
> Simplifies the callers and uses the existing functionality
> in find_get_entries(). We can also drop the final argument of
> truncate_exceptional_pvec_entries() and simplify the logic in that
> function.
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
Looks good to me. You can add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> include/linux/pagevec.h | 5 ++---
> mm/swap.c | 8 ++++----
> mm/truncate.c | 41 ++++++++++-------------------------------
> 3 files changed, 16 insertions(+), 38 deletions(-)
>
> diff --git a/include/linux/pagevec.h b/include/linux/pagevec.h
> index 081d934eda64..4b245592262c 100644
> --- a/include/linux/pagevec.h
> +++ b/include/linux/pagevec.h
> @@ -26,9 +26,8 @@ struct pagevec {
> void __pagevec_release(struct pagevec *pvec);
> void __pagevec_lru_add(struct pagevec *pvec);
> unsigned pagevec_lookup_entries(struct pagevec *pvec,
> - struct address_space *mapping,
> - pgoff_t start, unsigned nr_entries,
> - pgoff_t *indices);
> + struct address_space *mapping, pgoff_t start, pgoff_t end,
> + unsigned nr_entries, pgoff_t *indices);
> void pagevec_remove_exceptionals(struct pagevec *pvec);
> unsigned pagevec_lookup_range(struct pagevec *pvec,
> struct address_space *mapping,
> diff --git a/mm/swap.c b/mm/swap.c
> index fcf6ccb94b09..b6e56a84b466 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -1036,6 +1036,7 @@ void __pagevec_lru_add(struct pagevec *pvec)
> * @pvec: Where the resulting entries are placed
> * @mapping: The address_space to search
> * @start: The starting entry index
> + * @end: The highest index to return (inclusive).
> * @nr_entries: The maximum number of pages
> * @indices: The cache indices corresponding to the entries in @pvec
> *
> @@ -1056,11 +1057,10 @@ void __pagevec_lru_add(struct pagevec *pvec)
> * found.
> */
> unsigned pagevec_lookup_entries(struct pagevec *pvec,
> - struct address_space *mapping,
> - pgoff_t start, unsigned nr_entries,
> - pgoff_t *indices)
> + struct address_space *mapping, pgoff_t start, pgoff_t end,
> + unsigned nr_entries, pgoff_t *indices)
> {
> - pvec->nr = find_get_entries(mapping, start, ULONG_MAX, nr_entries,
> + pvec->nr = find_get_entries(mapping, start, end, nr_entries,
> pvec->pages, indices);
> return pagevec_count(pvec);
> }
> diff --git a/mm/truncate.c b/mm/truncate.c
> index 5dbe0c77b5ac..69ea72e7fc1c 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -57,11 +57,10 @@ static void clear_shadow_entry(struct address_space *mapping, pgoff_t index,
> * exceptional entries similar to what pagevec_remove_exceptionals does.
> */
> static void truncate_exceptional_pvec_entries(struct address_space *mapping,
> - struct pagevec *pvec, pgoff_t *indices,
> - pgoff_t end)
> + struct pagevec *pvec, pgoff_t *indices)
> {
> int i, j;
> - bool dax, lock;
> + bool dax;
>
> /* Handled by shmem itself */
> if (shmem_mapping(mapping))
> @@ -75,8 +74,7 @@ static void truncate_exceptional_pvec_entries(struct address_space *mapping,
> return;
>
> dax = dax_mapping(mapping);
> - lock = !dax && indices[j] < end;
> - if (lock)
> + if (!dax)
> xa_lock_irq(&mapping->i_pages);
>
> for (i = j; i < pagevec_count(pvec); i++) {
> @@ -88,9 +86,6 @@ static void truncate_exceptional_pvec_entries(struct address_space *mapping,
> continue;
> }
>
> - if (index >= end)
> - continue;
> -
> if (unlikely(dax)) {
> dax_delete_mapping_entry(mapping, index);
> continue;
> @@ -99,7 +94,7 @@ static void truncate_exceptional_pvec_entries(struct address_space *mapping,
> __clear_shadow_entry(mapping, index, page);
> }
>
> - if (lock)
> + if (!dax)
> xa_unlock_irq(&mapping->i_pages);
> pvec->nr = j;
> }
> @@ -329,7 +324,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
> while (index < end && find_lock_entries(mapping, index, end - 1,
> &pvec, indices)) {
> index = indices[pagevec_count(&pvec) - 1] + 1;
> - truncate_exceptional_pvec_entries(mapping, &pvec, indices, end);
> + truncate_exceptional_pvec_entries(mapping, &pvec, indices);
> for (i = 0; i < pagevec_count(&pvec); i++)
> truncate_cleanup_page(mapping, pvec.pages[i]);
> delete_from_page_cache_batch(mapping, &pvec);
> @@ -381,8 +376,8 @@ void truncate_inode_pages_range(struct address_space *mapping,
> index = start;
> for ( ; ; ) {
> cond_resched();
> - if (!pagevec_lookup_entries(&pvec, mapping, index,
> - min(end - index, (pgoff_t)PAGEVEC_SIZE), indices)) {
> + if (!pagevec_lookup_entries(&pvec, mapping, index, end - 1,
> + PAGEVEC_SIZE, indices)) {
> /* If all gone from start onwards, we're done */
> if (index == start)
> break;
> @@ -390,23 +385,12 @@ void truncate_inode_pages_range(struct address_space *mapping,
> index = start;
> continue;
> }
> - if (index == start && indices[0] >= end) {
> - /* All gone out of hole to be punched, we're done */
> - pagevec_remove_exceptionals(&pvec);
> - pagevec_release(&pvec);
> - break;
> - }
>
> for (i = 0; i < pagevec_count(&pvec); i++) {
> struct page *page = pvec.pages[i];
>
> /* We rely upon deletion not changing page->index */
> index = indices[i];
> - if (index >= end) {
> - /* Restart punch to make sure all gone */
> - index = start - 1;
> - break;
> - }
>
> if (xa_is_value(page))
> continue;
> @@ -417,7 +401,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
> truncate_inode_page(mapping, page);
> unlock_page(page);
> }
> - truncate_exceptional_pvec_entries(mapping, &pvec, indices, end);
> + truncate_exceptional_pvec_entries(mapping, &pvec, indices);
> pagevec_release(&pvec);
> index++;
> }
> @@ -528,8 +512,6 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
>
> /* We rely upon deletion not changing page->index */
> index = indices[i];
> - if (index > end)
> - break;
>
> if (xa_is_value(page)) {
> invalidate_exceptional_entry(mapping, index,
> @@ -629,16 +611,13 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
>
> pagevec_init(&pvec);
> index = start;
> - while (index <= end && pagevec_lookup_entries(&pvec, mapping, index,
> - min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1,
> - indices)) {
> + while (pagevec_lookup_entries(&pvec, mapping, index, end,
> + PAGEVEC_SIZE, indices)) {
> for (i = 0; i < pagevec_count(&pvec); i++) {
> struct page *page = pvec.pages[i];
>
> /* We rely upon deletion not changing page->index */
> index = indices[i];
> - if (index > end)
> - break;
>
> if (xa_is_value(page)) {
> if (!invalidate_exceptional_entry2(mapping,
> --
> 2.28.0
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon 14-09-20 14:00:39, Matthew Wilcox (Oracle) wrote:
> All callers of find_get_entries() use a pvec, so pass it directly
> instead of manipulating it in the caller.
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
Looks good. You can add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> include/linux/pagemap.h | 3 +--
> mm/filemap.c | 21 +++++++++------------
> mm/shmem.c | 5 ++---
> mm/swap.c | 4 +---
> 4 files changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index d440c6750757..788c280737e0 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -414,8 +414,7 @@ static inline struct page *find_subpage(struct page *head, pgoff_t index)
> }
>
> unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
> - pgoff_t end, unsigned int nr_entries, struct page **entries,
> - pgoff_t *indices);
> + pgoff_t end, struct pagevec *pvec, pgoff_t *indices);
> unsigned find_get_pages_range(struct address_space *mapping, pgoff_t *start,
> pgoff_t end, unsigned int nr_pages,
> struct page **pages);
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 6dc0a9b8c0fa..a4ad294a34dc 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1800,14 +1800,12 @@ static inline struct page *xas_find_get_entry(struct xa_state *xas,
> * @mapping: The address_space to search
> * @start: The starting page cache index
> * @end: The final page index (inclusive).
> - * @nr_entries: The maximum number of entries
> - * @entries: Where the resulting entries are placed
> + * @pvec: Where the resulting entries are placed.
> * @indices: The cache indices corresponding to the entries in @entries
> *
> - * find_get_entries() will search for and return a group of up to
> - * @nr_entries entries in the mapping. The entries are placed at
> - * @entries. find_get_entries() takes a reference against any actual
> - * pages it returns.
> + * find_get_entries() will search for and return a batch of entries in
> + * the mapping. The entries are placed in @pvec. find_get_entries()
> + * takes a reference on any actual pages it returns.
> *
> * The search returns a group of mapping-contiguous page cache entries
> * with ascending indexes. There may be holes in the indices due to
> @@ -1824,15 +1822,12 @@ static inline struct page *xas_find_get_entry(struct xa_state *xas,
> * Return: the number of pages and shadow entries which were found.
> */
> unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
> - pgoff_t end, unsigned int nr_entries, struct page **entries,
> - pgoff_t *indices)
> + pgoff_t end, struct pagevec *pvec, pgoff_t *indices)
> {
> XA_STATE(xas, &mapping->i_pages, start);
> struct page *page;
> unsigned int ret = 0;
> -
> - if (!nr_entries)
> - return 0;
> + unsigned nr_entries = PAGEVEC_SIZE;
>
> rcu_read_lock();
> while ((page = xas_find_get_entry(&xas, end, XA_PRESENT))) {
> @@ -1847,11 +1842,13 @@ unsigned find_get_entries(struct address_space *mapping, pgoff_t start,
> }
>
> indices[ret] = xas.xa_index;
> - entries[ret] = page;
> + pvec->pages[ret] = page;
> if (++ret == nr_entries)
> break;
> }
> rcu_read_unlock();
> +
> + pvec->nr = ret;
> return ret;
> }
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 404e45c285ca..b2b94025841a 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -963,9 +963,8 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
> while (index < end) {
> cond_resched();
>
> - pvec.nr = find_get_entries(mapping, index, end - 1,
> - PAGEVEC_SIZE, pvec.pages, indices);
> - if (!pvec.nr) {
> + if (!find_get_entries(mapping, index, end - 1, &pvec,
> + indices)) {
> /* If all gone or hole-punch or unfalloc, we're done */
> if (index == start || end != -1)
> break;
> diff --git a/mm/swap.c b/mm/swap.c
> index d4e3ba4c967c..40b23300d353 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -1060,9 +1060,7 @@ unsigned pagevec_lookup_entries(struct pagevec *pvec,
> struct address_space *mapping, pgoff_t start, pgoff_t end,
> pgoff_t *indices)
> {
> - pvec->nr = find_get_entries(mapping, start, end, PAGEVEC_SIZE,
> - pvec->pages, indices);
> - return pagevec_count(pvec);
> + return find_get_entries(mapping, start, end, pvec, indices);
> }
>
> /**
> --
> 2.28.0
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue, Sep 29, 2020 at 10:46:53AM +0200, Jan Kara wrote:
> On Mon 14-09-20 14:00:34, Matthew Wilcox (Oracle) wrote:
> > Rewrite shmem_seek_hole_data() and move it to filemap.c.
> >
> > + rcu_read_lock();
> > + while ((page = xas_find_get_entry(&xas, max, XA_PRESENT))) {
> > + loff_t pos = xas.xa_index * PAGE_SIZE;
>
> OK, but for ordinary filesystems this could be problematic because of
> exceptional entries?
For ordinary filesystems, I have this queued up on top:
http://git.infradead.org/users/willy/pagecache.git/commitdiff/02c740b215bab901f95a560759b3bd906648da08
which handles exceptional entries. It treats shadow/swap/DAX entries
the same -- there's definitely data there, it's just not in a struct
page right now.
> Also for shmem you've dropped the PageUptodate check which I'm not sure is
> safe?
That was unintentional. I did run xfstests against this patch (just did
it again ... it passes), so I suspect it doesn't create a !Uptodate page.
I'll see if I can enhance the existing xfstests to catch this case.
The patch I link to above also doesn't handle !Uptodate pages on shmem
filesystems the same way that the current code does. So ... on top of
this patch, I propose doing this:
@@ -2416,6 +2416,14 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_ite
r *iter)
}
EXPORT_SYMBOL(generic_file_read_iter);
+static inline loff_t page_seek_hole_data(struct page *page,
+ loff_t start, loff_t end, bool seek_data)
+{
+ if (xa_is_value(page) || PageUptodate(page))
+ return seek_data ? start : end;
+ return seek_data ? end : start;
+}
+
static inline
unsigned int seek_page_size(struct xa_state *xas, struct page *page)
{
@@ -2463,10 +2471,10 @@ loff_t mapping_seek_hole_data(struct address_space *mapping, loff_t start,
start = pos;
}
- if (seek_data)
+ pos += seek_page_size(&xas, page);
+ start = page_seek_hole_data(page, start, pos, seek_data);
+ if (start < pos)
goto unlock;
-
- start = pos + seek_page_size(&xas, page);
}
rcu_read_unlock();
... and then rebasing the other patch on top of this works out nicely.
Here's the result:
http://git.infradead.org/users/willy/pagecache.git/commitdiff/9eb3f496b7cdcdcae83026e861e148f46921c367
http://git.infradead.org/users/willy/pagecache.git/commitdiff/7d93274088f0872d849a906d783dc260bee106b9
On Tue, Sep 29, 2020 at 01:42:51PM +0100, Matthew Wilcox wrote:
> On Tue, Sep 29, 2020 at 10:46:53AM +0200, Jan Kara wrote:
> > Also for shmem you've dropped the PageUptodate check which I'm not sure is
> > safe?
>
> That was unintentional. I did run xfstests against this patch (just did
> it again ... it passes), so I suspect it doesn't create a !Uptodate page.
> I'll see if I can enhance the existing xfstests to catch this case.
Ah. Diff'ing the output between before and after ...
-Test skipped as fs doesn't support unwritten extents.
+07.01 SEEK_HOLE expected 0 or 45056, got 0. succ
+07.02 SEEK_HOLE expected 1 or 45056, got 1. succ
+07.03 SEEK_DATA expected 40960 or 40960, got 40960. succ
+07.04 SEEK_DATA expected 40960 or 40960, got 40960. succ
so, er, the tests didn't report that I'd broken it because it was just an
automatically skipped test. Not sure what to do about that; obviously
we should skip tests that aren't applicable, but it'd be nice to see a
warning that tmpfs used to support this and now doesn't.