Thread (106 messages) 106 messages, 6 authors, 2022-01-08

Re: [PATCH 00/48] Folios for 5.17

From: William Kucharski <hidden>
Date: 2021-12-26 22:26:43
Also in: linux-fsdevel

Consolidated multiple review comments into one email, the majority are nits at
best:

[PATCH 04/48]:

An obnoxiously pendantic English grammar nit:

+ * lock).  This can also be called from mark_buffer_dirty(), which I

The period should be inside the paren, e.g.: "lock.)"


[PATCH 05/48]:

+       unsigned char aux[3];

I'd like to see an explanation of why this is "3."

+static inline void folio_batch_init(struct folio_batch *fbatch)
+{
+       fbatch->nr = 0;
+}
+
+static inline unsigned int folio_batch_count(struct folio_batch *fbatch)
+{
+       return fbatch->nr;
+}
+
+static inline unsigned int fbatch_space(struct folio_batch *fbatch)
+{
+       return PAGEVEC_SIZE - fbatch->nr;
+}
+
+/**
+ * folio_batch_add() - Add a folio to a batch.
+ * @fbatch: The folio batch.
+ * @folio: The folio to add.
+ *
+ * The folio is added to the end of the batch.
+ * The batch must have previously been initialised using folio_batch_init().
+ *
+ * Return: The number of slots still available.
+ */
+static inline unsigned folio_batch_add(struct folio_batch *fbatch,
+               struct folio *folio)
+{
+       fbatch->folios[fbatch->nr++] = folio;

Is there any need to validate fbatch in these inlines?

[PATCH 07/48]:

+       xas_for_each(&xas, folio, ULONG_MAX) {                  \
                unsigned left;                                  \
-               if (xas_retry(&xas, head))                      \
+               size_t offset = offset_in_folio(folio, start + __off);  \
+               if (xas_retry(&xas, folio))                     \
                        continue;                               \
-               if (WARN_ON(xa_is_value(head)))                 \
+               if (WARN_ON(xa_is_value(folio)))                \
                        break;                                  \
-               if (WARN_ON(PageHuge(head)))                    \
+               if (WARN_ON(folio_test_hugetlb(folio)))         \
                        break;                                  \
-               for (j = (head->index < index) ? index - head->index : 0; \
-                    j < thp_nr_pages(head); j++) {             \
-                       void *kaddr = kmap_local_page(head + j);        \
-                       base = kaddr + offset;                  \
-                       len = PAGE_SIZE - offset;               \
+               while (offset < folio_size(folio)) {            \

Since offset is not actually used until after a bunch of error conditions
may exit or restart the loop, and isn't used at all in between, defer
its calculation until just before its first use in the "while."

[PATCH 25/48]:

Whether you use your follow-on patch sent to Christop or defer it until later
as mentioned in your followup email, either approach is fine with me.

Otherwise it all looks good.

For the series:

Reviewed-by: William Kucharski <redacted>

On Dec 7, 2021, at 9:22 PM, Matthew Wilcox (Oracle) [off-list ref] wrote:

I was trying to get all the way to adding large folios to the page cache,
but I ran out of time.  I think the changes in this batch of patches
are worth adding, even without large folio support for filesystems other
than tmpfs.

The big change here is the last patch which switches from storing
large folios in the page cache as 2^N identical entries to using the
XArray support for multi-index entries.  As the changelog says, this
fixes a bug that can occur when doing (eg) msync() or sync_file_range().

Some parts of this have been sent before.  The first patch is already
in Andrew's tree, but I included it here so the build bots don't freak
out about not being able to apply this patch series.  The folio_batch
(replacement for pagevec) is new.  I also fixed a few bugs.

This all passes xfstests with no new failures on both xfs and tmpfs.
I intend to put all this into for-next tomorrow.

Matthew Wilcox (Oracle) (48):
 filemap: Remove PageHWPoison check from next_uptodate_page()
 fs/writeback: Convert inode_switch_wbs_work_fn to folios
 mm/doc: Add documentation for folio_test_uptodate
 mm/writeback: Improve __folio_mark_dirty() comment
 pagevec: Add folio_batch
 iov_iter: Add copy_folio_to_iter()
 iov_iter: Convert iter_xarray to use folios
 mm: Add folio_test_pmd_mappable()
 filemap: Add folio_put_wait_locked()
 filemap: Convert page_cache_delete to take a folio
 filemap: Add filemap_unaccount_folio()
 filemap: Convert tracing of page cache operations to folio
 filemap: Add filemap_remove_folio and __filemap_remove_folio
 filemap: Convert find_get_entry to return a folio
 filemap: Remove thp_contains()
 filemap: Convert filemap_get_read_batch to use folios
 filemap: Convert find_get_pages_contig to folios
 filemap: Convert filemap_read_page to take a folio
 filemap: Convert filemap_create_page to folio
 filemap: Convert filemap_range_uptodate to folios
 readahead: Convert page_cache_async_ra() to take a folio
 readahead: Convert page_cache_ra_unbounded to folios
 filemap: Convert do_async_mmap_readahead to take a folio
 filemap: Convert filemap_fault to folio
 filemap: Add read_cache_folio and read_mapping_folio
 filemap: Convert filemap_get_pages to use folios
 filemap: Convert page_cache_delete_batch to folios
 filemap: Use folios in next_uptodate_page
 filemap: Use a folio in filemap_map_pages
 filemap: Use a folio in filemap_page_mkwrite
 filemap: Add filemap_release_folio()
 truncate: Add truncate_cleanup_folio()
 mm: Add unmap_mapping_folio()
 shmem: Convert part of shmem_undo_range() to use a folio
 truncate,shmem: Add truncate_inode_folio()
 truncate: Skip known-truncated indices
 truncate: Convert invalidate_inode_pages2_range() to use a folio
 truncate: Add invalidate_complete_folio2()
 filemap: Convert filemap_read() to use a folio
 filemap: Convert filemap_get_read_batch() to use a folio_batch
 filemap: Return only folios from find_get_entries()
 mm: Convert find_lock_entries() to use a folio_batch
 mm: Remove pagevec_remove_exceptionals()
 fs: Convert vfs_dedupe_file_range_compare to folios
 truncate: Convert invalidate_inode_pages2_range to folios
 truncate,shmem: Handle truncates that split large folios
 XArray: Add xas_advance()
 mm: Use multi-index entries in the page cache

fs/fs-writeback.c              |  24 +-
fs/remap_range.c               | 116 ++--
include/linux/huge_mm.h        |  14 +
include/linux/mm.h             |  68 +--
include/linux/page-flags.h     |  13 +-
include/linux/pagemap.h        |  59 +-
include/linux/pagevec.h        |  61 ++-
include/linux/uio.h            |   7 +
include/linux/xarray.h         |  18 +
include/trace/events/filemap.h |  32 +-
lib/iov_iter.c                 |  29 +-
lib/xarray.c                   |   6 +-
mm/filemap.c                   | 967 ++++++++++++++++-----------------
mm/folio-compat.c              |  11 +
mm/huge_memory.c               |  20 +-
mm/internal.h                  |  35 +-
mm/khugepaged.c                |  12 +-
mm/memory.c                    |  27 +-
mm/migrate.c                   |  29 +-
mm/page-writeback.c            |   6 +-
mm/readahead.c                 |  24 +-
mm/shmem.c                     | 174 +++---
mm/swap.c                      |  26 +-
mm/truncate.c                  | 305 ++++++-----
24 files changed, 1100 insertions(+), 983 deletions(-)

-- 
2.33.0
  
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help