Re: [PATCH 00/48] Folios for 5.17

[William Kucharski <william.kucharski@oracle.com>]

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 <william.kucharski@oracle.com>


> On Dec 7, 2021, at 9:22 PM, Matthew Wilcox (Oracle) <willy@infradead.org> 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
> 
>