enable bs > ps in XFS

From: "Matthew Wilcox (Oracle)" <[email protected]>

We need filesystems to be able to communicate acceptable folio sizes to the pagecache for a variety of uses (e.g. large block sizes). Support a range of folio sizes between order-0 and order-31.

Signed-off-by Matthew Wilcox (Oracle) <[email protected]>

Co-developed-by Pankaj Raghav <[email protected]>

Signed-off-by Pankaj Raghav <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

---
 include/linux/pagemap.h | 86 ++++++++++++++++++++++++++++++++++-------
 mm/filemap.c            |  6 +--
 mm/readahead.c          |  4 +-
 3 files changed, 77 insertions(+), 19 deletions(-)

include/linux/pagemap.h +73 -13

index 8f09ed4a4451..228275e7049f 100644
@@ -204,14 +204,21 @@ enum mapping_flags {
 	AS_EXITING	= 4, 	/* final truncate in progress */
 	/* writeback related tags are not used */
 	AS_NO_WRITEBACK_TAGS = 5,
-	AS_LARGE_FOLIO_SUPPORT = 6,
-	AS_RELEASE_ALWAYS,	/* Call ->release_folio(), even if no private data */
-	AS_STABLE_WRITES,	/* must wait for writeback before modifying
+	AS_RELEASE_ALWAYS = 6,	/* Call ->release_folio(), even if no private data */
+	AS_STABLE_WRITES = 7,	/* must wait for writeback before modifying
 				   folio contents */
-	AS_UNMOVABLE,		/* The mapping cannot be moved, ever */
-	AS_INACCESSIBLE,	/* Do not attempt direct R/W access to the mapping */
+	AS_UNMOVABLE = 8,	/* The mapping cannot be moved, ever */
+	AS_INACCESSIBLE = 9,	/* Do not attempt direct R/W access to the mapping */
+	/* Bits 16-25 are used for FOLIO_ORDER */
+	AS_FOLIO_ORDER_BITS = 5,
+	AS_FOLIO_ORDER_MIN = 16,
+	AS_FOLIO_ORDER_MAX = AS_FOLIO_ORDER_MIN + AS_FOLIO_ORDER_BITS,
 };
 
+#define AS_FOLIO_ORDER_MASK     ((1u << AS_FOLIO_ORDER_BITS) - 1)
+#define AS_FOLIO_ORDER_MIN_MASK (AS_FOLIO_ORDER_MASK << AS_FOLIO_ORDER_MIN)
+#define AS_FOLIO_ORDER_MAX_MASK (AS_FOLIO_ORDER_MASK << AS_FOLIO_ORDER_MAX)
+
 /**
  * mapping_set_error - record a writeback error in the address_space
  * @mapping: the mapping in which an error should be set

@@ -360,9 +367,49 @@ static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask)
 #define MAX_PAGECACHE_ORDER	8
 #endif
 
+/*
+ * mapping_set_folio_order_range() - Set the orders supported by a file.
+ * @mapping: The address space of the file.
+ * @min: Minimum folio order (between 0-MAX_PAGECACHE_ORDER inclusive).
+ * @max: Maximum folio order (between @min-MAX_PAGECACHE_ORDER inclusive).
+ *
+ * The filesystem should call this function in its inode constructor to
+ * indicate which base size (min) and maximum size (max) of folio the VFS
+ * can use to cache the contents of the file.  This should only be used
+ * if the filesystem needs special handling of folio sizes (ie there is
+ * something the core cannot know).
+ * Do not tune it based on, eg, i_size.
+ *
+ * Context: This should not be called while the inode is active as it
+ * is non-atomic.
+ */
+static inline void mapping_set_folio_order_range(struct address_space *mapping,
+						 unsigned int min,
+						 unsigned int max)
+{
+	if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
+		return;
+
+	if (min > MAX_PAGECACHE_ORDER)
+		min = MAX_PAGECACHE_ORDER;
+	if (max > MAX_PAGECACHE_ORDER)
+		max = MAX_PAGECACHE_ORDER;
+	if (max < min)
+		max = min;
+
+	mapping->flags = (mapping->flags & ~AS_FOLIO_ORDER_MASK) |
+		(min << AS_FOLIO_ORDER_MIN) | (max << AS_FOLIO_ORDER_MAX);
+}
+
+static inline void mapping_set_folio_min_order(struct address_space *mapping,
+					       unsigned int min)
+{
+	mapping_set_folio_order_range(mapping, min, MAX_PAGECACHE_ORDER);
+}
+
 /**
  * mapping_set_large_folios() - Indicate the file supports large folios.
- * @mapping: The file.
+ * @mapping: The address space of the file.
  *
  * The filesystem should call this function in its inode constructor to
  * indicate that the VFS can use large folios to cache the contents of

@@ -373,7 +420,23 @@ static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask)
  */
 static inline void mapping_set_large_folios(struct address_space *mapping)
 {
-	__set_bit(AS_LARGE_FOLIO_SUPPORT, &mapping->flags);
+	mapping_set_folio_order_range(mapping, 0, MAX_PAGECACHE_ORDER);
+}
+
+static inline
+unsigned int mapping_max_folio_order(const struct address_space *mapping)
+{
+	if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
+		return 0;
+	return (mapping->flags & AS_FOLIO_ORDER_MAX_MASK) >> AS_FOLIO_ORDER_MAX;
+}
+
+static inline
+unsigned int mapping_min_folio_order(const struct address_space *mapping)
+{
+	if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
+		return 0;
+	return (mapping->flags & AS_FOLIO_ORDER_MIN_MASK) >> AS_FOLIO_ORDER_MIN;
 }
 
 /*

@@ -382,16 +445,13 @@ static inline void mapping_set_large_folios(struct address_space *mapping)
  */
 static inline bool mapping_large_folio_support(struct address_space *mapping)
 {
-	return IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
-		test_bit(AS_LARGE_FOLIO_SUPPORT, &mapping->flags);
+	return mapping_max_folio_order(mapping) > 0;
 }
 
 /* Return the maximum folio size for this pagecache mapping, in bytes. */
-static inline size_t mapping_max_folio_size(struct address_space *mapping)
+static inline size_t mapping_max_folio_size(const struct address_space *mapping)
 {
-	if (mapping_large_folio_support(mapping))
-		return PAGE_SIZE << MAX_PAGECACHE_ORDER;
-	return PAGE_SIZE;
+	return PAGE_SIZE << mapping_max_folio_order(mapping);
 }
 
 static inline int filemap_nr_thps(struct address_space *mapping)

mm/filemap.c +2 -4

index 37061aafd191..46c7a6f59788 100644
@@ -1933,10 +1933,8 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
 		if (WARN_ON_ONCE(!(fgp_flags & (FGP_LOCK | FGP_FOR_MMAP))))
 			fgp_flags |= FGP_LOCK;
 
-		if (!mapping_large_folio_support(mapping))
-			order = 0;
-		if (order > MAX_PAGECACHE_ORDER)
-			order = MAX_PAGECACHE_ORDER;
+		if (order > mapping_max_folio_order(mapping))
+			order = mapping_max_folio_order(mapping);
 		/* If we're not aligned, allocate a smaller folio */
 		if (index & ((1UL << order) - 1))
 			order = __ffs(index);

mm/readahead.c +2 -2

index 75e934a1fd78..da34b28da02c 100644
@@ -504,9 +504,9 @@ void page_cache_ra_order(struct readahead_control *ractl,
 
 	limit = min(limit, index + ra->size - 1);
 
-	if (new_order < MAX_PAGECACHE_ORDER) {
+	if (new_order < mapping_max_folio_order(mapping)) {
 		new_order += 2;
-		new_order = min_t(unsigned int, MAX_PAGECACHE_ORDER, new_order);
+		new_order = min(mapping_max_folio_order(mapping), new_order);
 		new_order = min_t(unsigned int, new_order, ilog2(ra->size));
 	}
 

Signature · 1 line

2.44.1

From: Pankaj Raghav <[email protected]>

filemap_create_folio() and do_read_cache_folio() were always allocating folio of order 0. __filemap_get_folio was trying to allocate higher order folios when fgp_flags had higher order hint set but it will default to order 0 folio if higher order memory allocation fails.

Supporting mapping_min_order implies that we guarantee each folio in the page cache has at least an order of mapping_min_order. When adding new folios to the page cache we must also ensure the index used is aligned to the mapping_min_order as the page cache requires the index to be aligned to the order of the folio.

Co-developed-by Luis Chamberlain <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

Signed-off-by Pankaj Raghav <[email protected]>

---
  include/linux/pagemap.h | 20 ++++++++++++++++++++
  mm/filemap.c            | 26 ++++++++++++++++++--------
  2 files changed, 38 insertions(+), 8 deletions(-)

From: Pankaj Raghav <[email protected]>

filemap_create_folio() and do_read_cache_folio() were always allocating folio of order 0. __filemap_get_folio was trying to allocate higher order folios when fgp_flags had higher order hint set but it will default to order 0 folio if higher order memory allocation fails.

Supporting mapping_min_order implies that we guarantee each folio in the page cache has at least an order of mapping_min_order. When adding new folios to the page cache we must also ensure the index used is aligned to the mapping_min_order as the page cache requires the index to be aligned to the order of the folio.

Co-developed-by Luis Chamberlain <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

Signed-off-by Pankaj Raghav <[email protected]>

---
 include/linux/pagemap.h | 20 ++++++++++++++++++++
 mm/filemap.c            | 26 ++++++++++++++++++--------
 2 files changed, 38 insertions(+), 8 deletions(-)

include/linux/pagemap.h +20 -0

index 228275e7049f..899b8d751768 100644
@@ -439,6 +439,26 @@ unsigned int mapping_min_folio_order(const struct address_space *mapping)
 	return (mapping->flags & AS_FOLIO_ORDER_MIN_MASK) >> AS_FOLIO_ORDER_MIN;
 }
 
+static inline unsigned long mapping_min_folio_nrpages(struct address_space *mapping)
+{
+	return 1UL << mapping_min_folio_order(mapping);
+}
+
+/**
+ * mapping_align_start_index() - Align starting index based on the min
+ * folio order of the page cache.
+ * @mapping: The address_space.
+ *
+ * Ensure the index used is aligned to the minimum folio order when adding
+ * new folios to the page cache by rounding down to the nearest minimum
+ * folio number of pages.
+ */
+static inline pgoff_t mapping_align_start_index(struct address_space *mapping,
+						pgoff_t index)
+{
+	return round_down(index, mapping_min_folio_nrpages(mapping));
+}
+
 /*
  * Large folio support currently depends on THP.  These dependencies are
  * being worked on but are not yet fixed.

mm/filemap.c +18 -8

index 46c7a6f59788..8bb0d2bc93c5 100644
@@ -859,6 +859,8 @@ noinline int __filemap_add_folio(struct address_space *mapping,
 
 	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
 	VM_BUG_ON_FOLIO(folio_test_swapbacked(folio), folio);
+	VM_BUG_ON_FOLIO(folio_order(folio) < mapping_min_folio_order(mapping),
+			folio);
 	mapping_set_update(&xas, mapping);
 
 	VM_BUG_ON_FOLIO(index & (folio_nr_pages(folio) - 1), folio);

@@ -1919,8 +1921,10 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
 		folio_wait_stable(folio);
 no_page:
 	if (!folio && (fgp_flags & FGP_CREAT)) {
-		unsigned order = FGF_GET_ORDER(fgp_flags);
+		unsigned int min_order = mapping_min_folio_order(mapping);
+		unsigned int order = max(min_order, FGF_GET_ORDER(fgp_flags));
 		int err;
+		index = mapping_align_start_index(mapping, index);
 
 		if ((fgp_flags & FGP_WRITE) && mapping_can_writeback(mapping))
 			gfp |= __GFP_WRITE;

@@ -1943,7 +1947,7 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
 			gfp_t alloc_gfp = gfp;
 
 			err = -ENOMEM;
-			if (order > 0)
+			if (order > min_order)
 				alloc_gfp |= __GFP_NORETRY | __GFP_NOWARN;
 			folio = filemap_alloc_folio(alloc_gfp, order);
 			if (!folio)

@@ -1958,7 +1962,7 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
 				break;
 			folio_put(folio);
 			folio = NULL;
-		} while (order-- > 0);
+		} while (order-- > min_order);
 
 		if (err == -EEXIST)
 			goto repeat;

@@ -2447,13 +2451,16 @@ static int filemap_update_page(struct kiocb *iocb,
 }
 
 static int filemap_create_folio(struct file *file,
-		struct address_space *mapping, pgoff_t index,
+		struct address_space *mapping, loff_t pos,
 		struct folio_batch *fbatch)
 {
 	struct folio *folio;
 	int error;
+	unsigned int min_order = mapping_min_folio_order(mapping);
+	pgoff_t index;
 
-	folio = filemap_alloc_folio(mapping_gfp_mask(mapping), 0);
+	folio = filemap_alloc_folio(mapping_gfp_mask(mapping),
+				    min_order);
 	if (!folio)
 		return -ENOMEM;
 

@@ -2471,6 +2478,8 @@ static int filemap_create_folio(struct file *file,
 	 * well to keep locking rules simple.
 	 */
 	filemap_invalidate_lock_shared(mapping);
+	/* index in PAGE units but aligned to min_order number of pages. */
+	index = (pos >> (PAGE_SHIFT + min_order)) << min_order;
 	error = filemap_add_folio(mapping, folio, index,
 			mapping_gfp_constraint(mapping, GFP_KERNEL));
 	if (error == -EEXIST)

@@ -2531,8 +2540,7 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count,
 	if (!folio_batch_count(fbatch)) {
 		if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_WAITQ))
 			return -EAGAIN;
-		err = filemap_create_folio(filp, mapping,
-				iocb->ki_pos >> PAGE_SHIFT, fbatch);
+		err = filemap_create_folio(filp, mapping, iocb->ki_pos, fbatch);
 		if (err == AOP_TRUNCATED_PAGE)
 			goto retry;
 		return err;

@@ -3748,9 +3756,11 @@ static struct folio *do_read_cache_folio(struct address_space *mapping,
 repeat:
 	folio = filemap_get_folio(mapping, index);
 	if (IS_ERR(folio)) {
-		folio = filemap_alloc_folio(gfp, 0);
+		folio = filemap_alloc_folio(gfp,
+					    mapping_min_folio_order(mapping));
 		if (!folio)
 			return ERR_PTR(-ENOMEM);
+		index = mapping_align_start_index(mapping, index);
 		err = filemap_add_folio(mapping, folio, index, gfp);
 		if (unlikely(err)) {
 			folio_put(folio);

Signature · 1 line

2.44.1

+/**
+ * mapping_align_start_index() - Align starting index based on the min
+ * folio order of the page cache.

+ * @mapping: The address_space.
+ *
+ * Ensure the index used is aligned to the minimum folio order when adding
+ * new folios to the page cache by rounding down to the nearest minimum
+ * folio number of pages.

include/linux/pagemap.h unsigned int mapping_min_folio_order(const struct address_space *mapping) · line 445

Quoted 4 lines · Pankaj Raghav (Samsung)

include/linux/pagemap.h +4 -0

+static inline unsigned long mapping_min_folio_nrpages(struct address_space *mapping)
+{
+	return 1UL << mapping_min_folio_order(mapping);
+}

Overly long line here, just line break after the return type.

Then again it only has a single user just below and no documentation so maybe just fold it into the caller?

mm/filemap.c struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index, · line 1927

Quoted 8 lines

no_page:

mm/filemap.c +3 -1

 	if (!folio && (fgp_flags & FGP_CREAT)) {
-		unsigned order = FGF_GET_ORDER(fgp_flags);
+		unsigned int min_order = mapping_min_folio_order(mapping);
+		unsigned int order = max(min_order, FGF_GET_ORDER(fgp_flags));
 		int err;
+		index = mapping_align_start_index(mapping, index);

I wonder if at some point splitting this block that actually allocates a new folio into a separate helper would be nice. It just keep growing in size and complexity.

mm/filemap.c static int filemap_update_page(struct kiocb *iocb, · line 2463

Quoted 3 lines

mm/filemap.c +2 -1

-	folio = filemap_alloc_folio(mapping_gfp_mask(mapping), 0);
+	folio = filemap_alloc_folio(mapping_gfp_mask(mapping),
+				    min_order);

Nit: no need to split this into multiple lines.

mm/filemap.c static int filemap_create_folio(struct file *file, · line 2481

Quoted 9 lines

mm/filemap.c +1 -0

 	if (!folio)
 		return -ENOMEM;

@@ -2471,6 +2478,8 @@ static int filemap_create_folio(struct file *file,
 	 * well to keep locking rules simple.
 	 */
 	filemap_invalidate_lock_shared(mapping);
+	/* index in PAGE units but aligned to min_order number of pages. */

in PAGE_SIZE units? Maybe also make this a complete sentence?

Quoted 7 lines

mm/filemap.c static int filemap_create_folio(struct file *file, · line 2481

Quoted 5 lines

mm/filemap.c +1 -0

@@ -2471,6 +2478,8 @@ static int filemap_create_folio(struct file *file,
 	 * well to keep locking rules simple.
 	 */
 	filemap_invalidate_lock_shared(mapping);
+	/* index in PAGE units but aligned to min_order number of pages. */

in PAGE_SIZE units? Maybe also make this a complete sentence?

Yes, will do.

mm/readahead.c +2 -1

@@ -240,12 +259,24 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 			 * not worth getting one just for that.
 			 */
 			read_pages(ractl);
-			ractl->_index += folio_nr_pages(folio);
+
+			/*

Review void page_cache_ra_unbounded(struct readahead_control *ractl, · line 250

Quoted 5 lines · Pankaj Raghav (Samsung)

(quoted diff) +1 -0

@@ -230,7 +247,9 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 		struct folio *folio = xa_load(&mapping->i_pages, index + i);
 		int ret;
 
+

Spurious newline

Review void page_cache_ra_unbounded(struct readahead_control *ractl, · line 252

Quoted 3 lines

(quoted diff) +1 -0

 		if (folio && !xa_is_value(folio)) {
+			long nr_pages = folio_nr_pages(folio);

Hm, but we don't have a reference on this folio. So this isn't safe.

Quoted 7 lines

(quoted diff) +2 -1

@@ -240,12 +259,24 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 			 * not worth getting one just for that.
 			 */
 			read_pages(ractl);
-			ractl->_index += folio_nr_pages(folio);
+
+			/*

Review void page_cache_ra_unbounded(struct readahead_control *ractl, · line 273

Quoted 10 lines

(quoted diff) +10 -0

+			 * Move the ractl->_index by at least min_pages
+			 * if the folio got truncated to respect the
+			 * alignment constraint in the page cache.
+			 *
+			 */
+			if (mapping != folio->mapping)
+				nr_pages = min_nrpages;
+
+			VM_BUG_ON_FOLIO(nr_pages < min_nrpages, folio);
+			ractl->_index += nr_pages;

Why not just:

			ractl->_index += min_nrpages;

like you do below?

Quoted 1 line

Hm, but we don't have a reference on this folio. So this isn't safe.

That is why I added a check for mapping after read_pages(). You are right, we can make it better.

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+			if (mapping != folio->mapping)
+				nr_pages = min_nrpages;
+
+			VM_BUG_ON_FOLIO(nr_pages < min_nrpages, folio);
+			ractl->_index += nr_pages;

Why not just:

			ractl->_index += min_nrpages;

Then we will only move min_nrpages even if the folio we found had a bigger order. Hannes patches (first patch) made sure we move the ractl->index by folio_nr_pages instead of 1 and making this change will defeat the purpose because without mapping order set, min_nrpages will be 1.

Now we will still move respecting the min order constraint but if we had a bigger folio and we do have a reference, then we move folio_nr_pages.

Quoted 4 lines · Pankaj Raghav (Samsung)

Quoted 1 line

Hm, but we don't have a reference on this folio. So this isn't safe.

That is why I added a check for mapping after read_pages(). You are right, we can make it better.

That's not enoughh.

Quoted 9 lines

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+			if (mapping != folio->mapping)
+				nr_pages = min_nrpages;
+
+			VM_BUG_ON_FOLIO(nr_pages < min_nrpages, folio);
+			ractl->_index += nr_pages;

Why not just:

			ractl->_index += min_nrpages;

Quoted 5 lines

Then we will only move min_nrpages even if the folio we found had a bigger order. Hannes patches (first patch) made sure we move the ractl->index by folio_nr_pages instead of 1 and making this change will defeat the purpose because without mapping order set, min_nrpages will be 1.

Hannes' patch is wrong. It's not safe to call folio_nr_pages() unless you have a reference to the folio.

mm/readahead.c void page_cache_ra_unbounded(struct readahead_control *ractl, · line 269

Quoted 7 lines

mm/readahead.c +1 -2

@@ -266,10 +266,8 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
                         * alignment constraint in the page cache.
                         *
                         */
-                       if (mapping != folio->mapping)
-                               nr_pages = min_nrpages;
+                       nr_pages = max(folio_nr_pages(folio), (long)min_nrpages);

No.

Quoted 2 lines

Now we will still move respecting the min order constraint but if we had a bigger folio and we do have a reference, then we move folio_nr_pages.

You don't have a reference, so it's never safe.

	ractl->_index += min_nrpages;

Quoted 4 lines · Pankaj Raghav (Samsung)

On Mon, Jun 17, 2024 at 01:32:42PM +0100, Matthew Wilcox wrote:
So the following can still be there from Hannes patch as we have a stable reference:

Review

Quoted 5 lines

(quoted diff) +2 -1

 		ractl->_workingset |= folio_test_workingset(folio);
-		ractl->_nr_pages++;
+		ractl->_nr_pages += folio_nr_pages(folio);
+		i += folio_nr_pages(folio);
 	}

We _can_, but we just allocated it, so we know what size it is already.

I'm starting to feel that Hannes' patch should be combined with this one.

Quoted 5 lines · Matthew Wilcox

Quoted 4 lines · Pankaj Raghav (Samsung)

On Mon, Jun 17, 2024 at 01:32:42PM +0100, Matthew Wilcox wrote:
So the following can still be there from Hannes patch as we have a stable reference:

Quoted 7 lines

Review

Quoted 5 lines

  		ractl->_workingset |= folio_test_workingset(folio);

(quoted diff) +2 -1

-		ractl->_nr_pages++;
+		ractl->_nr_pages += folio_nr_pages(folio);
+		i += folio_nr_pages(folio);
  	}

We _can_, but we just allocated it, so we know what size it is already.

Yes.

Quoted 2 lines

I'm starting to feel that Hannes' patch should be combined with this one.

Fine by me. @Hannes, is that ok with you?

Quoted 13 lines · Pankaj Raghav (Samsung)

Quoted 12 lines · Matthew Wilcox

Review

Quoted 9 lines · Pankaj Raghav (Samsung)

On Mon, Jun 17, 2024 at 01:32:42PM +0100, Matthew Wilcox wrote:
So the following can still be there from Hannes patch as we have a stable reference:

  		ractl->_workingset |= folio_test_workingset(folio);

(quoted diff) +2 -1

-		ractl->_nr_pages++;
+		ractl->_nr_pages += folio_nr_pages(folio);
+		i += folio_nr_pages(folio);
  	}

We _can_, but we just allocated it, so we know what size it is already.

Quoted 6 lines

Yes.

Quoted 2 lines

I'm starting to feel that Hannes' patch should be combined with this one.

Fine by me. @Hannes, is that ok with you?

Sure. I was about to re-send my patchset anyway, so feel free to wrap it in.

Quoted 14 lines · Hannes Reinecke

Quoted 13 lines · Pankaj Raghav (Samsung)

Quoted 12 lines · Matthew Wilcox

Review

Quoted 9 lines · Pankaj Raghav (Samsung)

On Mon, Jun 17, 2024 at 01:32:42PM +0100, Matthew Wilcox wrote:
So the following can still be there from Hannes patch as we have a stable reference:

   		ractl->_workingset |= folio_test_workingset(folio);

(quoted diff) +2 -1

-		ractl->_nr_pages++;
+		ractl->_nr_pages += folio_nr_pages(folio);
+		i += folio_nr_pages(folio);
   	}

We _can_, but we just allocated it, so we know what size it is already.

Quoted 8 lines

Quoted 6 lines

Yes.

Quoted 2 lines

I'm starting to feel that Hannes' patch should be combined with this one.

Fine by me. @Hannes, is that ok with you?

Sure. I was about to re-send my patchset anyway, so feel free to wrap it in.

Is it ok if I add your Co-developed and Signed-off tag? This is what I have combining your patch with mine and making willy's changes:

mm/readahead.c +1 -14

index 389cd802da63..f56da953c130 100644
@@ -247,9 +247,7 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
                 struct folio *folio = xa_load(&mapping->i_pages, index + i);
                 int ret;
 
-
                 if (folio && !xa_is_value(folio)) {
-                       long nr_pages = folio_nr_pages(folio);
                         /*
                          * Page already present?  Kick off the current batch
                          * of contiguous pages before continuing with the

@@ -259,18 +257,7 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
                          * not worth getting one just for that.
                          */
                         read_pages(ractl);
-
-                       /*
-                        * Move the ractl->_index by at least min_pages
-                        * if the folio got truncated to respect the
-                        * alignment constraint in the page cache.
-                        *
-                        */
-                       if (mapping != folio->mapping)
-                               nr_pages = min_nrpages;
-
-                       VM_BUG_ON_FOLIO(nr_pages < min_nrpages, folio);
-                       ractl->_index += nr_pages;
+                       ractl->_index += min_nrpages;
                         i = ractl->_index + ractl->_nr_pages - index;
                         continue;
                 }

Quoted 4 lines · Pankaj Raghav (Samsung)

On Mon, Jun 17, 2024 at 01:32:42PM +0100, Matthew Wilcox wrote:
So the following can still be there from Hannes patch as we have a stable reference:

Review

Quoted 5 lines

  		ractl->_workingset |= folio_test_workingset(folio);

(quoted diff) +2 -1

-		ractl->_nr_pages++;
+		ractl->_nr_pages += folio_nr_pages(folio);
+		i += folio_nr_pages(folio);
  	}

We _can_, but we just allocated it, so we know what size it is already. I'm starting to feel that Hannes' patch should be combined with this one.

From: Luis Chamberlain <[email protected]>

split_folio() and split_folio_to_list() assume order 0, to support minorder for non-anonymous folios, we must expand these to check the folio mapping order and use that.

Set new_order to be at least minimum folio order if it is set in split_huge_page_to_list() so that we can maintain minimum folio order requirement in the page cache.

Update the debugfs write files used for testing to ensure the order is respected as well. We simply enforce the min order when a file mapping is used.

Signed-off-by Luis Chamberlain <[email protected]>

Signed-off-by Pankaj Raghav <[email protected]>

---
 include/linux/huge_mm.h | 14 ++++++++---
 mm/huge_memory.c        | 55 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 61 insertions(+), 8 deletions(-)

Quoted 4 lines

mm/huge_memory.c +4 -0

+int split_folio_to_list(struct folio *folio, struct list_head *list)
+{
+	unsigned int min_order = 0;
+

mm/huge_memory.c int split_huge_page_to_list_to_order(struct page *page, struct list_head *list, · line 3285

Quoted 3 lines

mm/huge_memory.c +3 -0

+	if (!folio_test_anon(folio)) {
+		if (!folio->mapping) {
+			count_vm_event(THP_SPLIT_PAGE_FAILED);

You should only increase this counter when the input folio is a THP, namely folio_test_pmd_mappable(folio) is true. For other large folios, we will need a separate counter. Something like MTHP_STAT_FILE_SPLIT_FAILED. See enum mthp_stat_item in include/linux/huge_mm.h.

Quoted 4 lines · Zi Yan

Quoted 4 lines

(quoted diff) +4 -0

+int split_folio_to_list(struct folio *folio, struct list_head *list)
+{
+	unsigned int min_order = 0;
+

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	if (!folio_test_anon(folio)) {
+		if (!folio->mapping) {
+			count_vm_event(THP_SPLIT_PAGE_FAILED);

You should only increase this counter when the input folio is a THP, namely folio_test_pmd_mappable(folio) is true. For other large folios, we will need a separate counter. Something like MTHP_STAT_FILE_SPLIT_FAILED. See enum mthp_stat_item in include/linux/huge_mm.h.

Also, why should this count as a split failure? If we see a NULL mapping, the folio has been truncated and so no longer needs to be split. I understand we currently count it as a failure, but I don't think we should.

Hi Pankaj,

Can you use [email protected] instead of [email protected]? Since I just use the latter to send patches. Thanks.

Quoted 22 lines

From: Luis Chamberlain <[email protected]>

split_folio() and split_folio_to_list() assume order 0, to support minorder for non-anonymous folios, we must expand these to check the folio mapping order and use that.

Set new_order to be at least minimum folio order if it is set in split_huge_page_to_list() so that we can maintain minimum folio order requirement in the page cache.

Update the debugfs write files used for testing to ensure the order is respected as well. We simply enforce the min order when a file mapping is used.

Signed-off-by Luis Chamberlain <[email protected]>

Signed-off-by Pankaj Raghav <[email protected]>

---
 include/linux/huge_mm.h | 14 ++++++++---
 mm/huge_memory.c        | 55 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 61 insertions(+), 8 deletions(-)

<snip>

Quoted 1 line

mm/huge_memory.c int split_huge_page_to_list_to_order(struct page *page, struct list_head *list, · line 3285

Quoted 7 lines

mm/huge_memory.c +7 -0

+int split_folio_to_list(struct folio *folio, struct list_head *list)
+{
+	unsigned int min_order = 0;
+
+	if (!folio_test_anon(folio)) {
+		if (!folio->mapping) {
+			count_vm_event(THP_SPLIT_PAGE_FAILED);

You should only increase this counter when the input folio is a THP, namely folio_test_pmd_mappable(folio) is true. For other large folios, we will need a separate counter. Something like MTHP_STAT_FILE_SPLIT_FAILED. See enum mthp_stat_item in include/linux/huge_mm.h.

Quoted 4 lines · Zi Yan

Hi Pankaj,

Can you use [email protected] instead of [email protected]? Since I just use the latter to send patches. Thanks.

Got it!

Quoted 28 lines · Pankaj Raghav (Samsung)

Quoted 22 lines

From: Luis Chamberlain <[email protected]>

split_folio() and split_folio_to_list() assume order 0, to support minorder for non-anonymous folios, we must expand these to check the folio mapping order and use that.

Set new_order to be at least minimum folio order if it is set in split_huge_page_to_list() so that we can maintain minimum folio order requirement in the page cache.

Update the debugfs write files used for testing to ensure the order is respected as well. We simply enforce the min order when a file mapping is used.

Signed-off-by Luis Chamberlain <[email protected]>

Signed-off-by Pankaj Raghav <[email protected]>

---
 include/linux/huge_mm.h | 14 ++++++++---
 mm/huge_memory.c        | 55 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 61 insertions(+), 8 deletions(-)

<snip>

Quoted 1 line

Quoted 13 lines

mm/huge_memory.c int split_huge_page_to_list_to_order(struct page *page, struct list_head *list, · line 3285

Quoted 7 lines

mm/huge_memory.c +7 -0

+int split_folio_to_list(struct folio *folio, struct list_head *list)
+{
+	unsigned int min_order = 0;
+
+	if (!folio_test_anon(folio)) {
+		if (!folio->mapping) {
+			count_vm_event(THP_SPLIT_PAGE_FAILED);

You should only increase this counter when the input folio is a THP, namely folio_test_pmd_mappable(folio) is true. For other large folios, we will need a separate counter. Something like MTHP_STAT_FILE_SPLIT_FAILED. See enum mthp_stat_item in include/linux/huge_mm.h.

Hmm, but we don't have mTHP support for non-anonymous memory right? In that case it won't be applicable for file backed memory?

Quoted 8 lines · Pankaj Raghav (Samsung)

Quoted 8 lines

mm/huge_memory.c int split_huge_page_to_list_to_order(struct page *page, struct list_head *list, · line 3285

Quoted 2 lines

mm/huge_memory.c +2 -0

+		if (!folio->mapping) {
+			count_vm_event(THP_SPLIT_PAGE_FAILED);

You should only increase this counter when the input folio is a THP, namely folio_test_pmd_mappable(folio) is true. For other large folios, we will need a separate counter. Something like MTHP_STAT_FILE_SPLIT_FAILED. See enum mthp_stat_item in include/linux/huge_mm.h.

Quoted 2 lines

Hmm, but we don't have mTHP support for non-anonymous memory right? In that case it won't be applicable for file backed memory?

Large folio support in page cache precedes mTHP (large anonymous folio), thanks to willy's work. mTHP is more like a subset of large folio. There is no specific counters for page cache large folio. If you think it is worth tracking folios with orders between 0 and 9 (exclusive), you can add counters. Matthew, what is your take on this?

From: Luis Chamberlain <[email protected]>

split_folio() and split_folio_to_list() assume order 0, to support minorder for non-anonymous folios, we must expand these to check the folio mapping order and use that.

Set new_order to be at least minimum folio order if it is set in split_huge_page_to_list() so that we can maintain minimum folio order requirement in the page cache.

Update the debugfs write files used for testing to ensure the order is respected as well. We simply enforce the min order when a file mapping is used.

Signed-off-by Luis Chamberlain <[email protected]>

Signed-off-by Pankaj Raghav <[email protected]>

---
  include/linux/huge_mm.h | 14 ++++++++---
  mm/huge_memory.c        | 55 ++++++++++++++++++++++++++++++++++++++---
  2 files changed, 61 insertions(+), 8 deletions(-)

From: Pankaj Raghav <[email protected]>

Usually the page cache does not extend beyond the size of the inode, therefore, no PTEs are created for folios that extend beyond the size.

But with LBS support, we might extend page cache beyond the size of the inode as we need to guarantee folios of minimum order. Cap the PTE range to be created for the page cache up to the max allowed zero-fill file end, which is aligned to the PAGE_SIZE.

An fstests test has been created to trigger this edge case [0].

[0] https://lore.kernel.org/fstests/[email protected]/

Signed-off-by Luis Chamberlain <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Signed-off-by Pankaj Raghav <[email protected]>

---
 mm/filemap.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/filemap.c +5 -1

index 8bb0d2bc93c5..0e48491b3d10 100644
@@ -3610,7 +3610,7 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,
 	struct vm_area_struct *vma = vmf->vma;
 	struct file *file = vma->vm_file;
 	struct address_space *mapping = file->f_mapping;
-	pgoff_t last_pgoff = start_pgoff;
+	pgoff_t file_end, last_pgoff = start_pgoff;
 	unsigned long addr;
 	XA_STATE(xas, &mapping->i_pages, start_pgoff);
 	struct folio *folio;

@@ -3636,6 +3636,10 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,
 		goto out;
 	}
 
+	file_end = DIV_ROUND_UP(i_size_read(mapping->host), PAGE_SIZE) - 1;
+	if (end_pgoff > file_end)
+		end_pgoff = file_end;
+
 	folio_type = mm_counter_file(folio);
 	do {
 		unsigned long end;

Signature · 1 line

2.44.1

Quoted 9 lines · Pankaj Raghav (Samsung)

From: Pankaj Raghav <[email protected]>

Usually the page cache does not extend beyond the size of the inode, therefore, no PTEs are created for folios that extend beyond the size.

But with LBS support, we might extend page cache beyond the size of the inode as we need to guarantee folios of minimum order. Cap the PTE range to be created for the page cache up to the max allowed zero-fill file end, which is aligned to the PAGE_SIZE.

I think this is slightly misleading because we might well zero-fill to the end of the folio. The issue is that we're supposed to SIGBUS if userspace accesses pages which lie entirely beyond the end of this file. Can you rephrase this?

(from mmap(2))
SIGBUS Attempted access to a page of the buffer that lies beyond the end
of the mapped file. For an explanation of the treatment of the bytes in the page that corresponds to the end of a mapped file that is not a multiple of the page size, see NOTES.

The code is good though.

Reviewed-by Matthew Wilcox (Oracle) <[email protected]>

Quoted 25 lines · Matthew Wilcox

Quoted 9 lines · Pankaj Raghav (Samsung)

From: Pankaj Raghav <[email protected]>

Usually the page cache does not extend beyond the size of the inode, therefore, no PTEs are created for folios that extend beyond the size.

But with LBS support, we might extend page cache beyond the size of the inode as we need to guarantee folios of minimum order. Cap the PTE range to be created for the page cache up to the max allowed zero-fill file end, which is aligned to the PAGE_SIZE.

I think this is slightly misleading because we might well zero-fill to the end of the folio. The issue is that we're supposed to SIGBUS if userspace accesses pages which lie entirely beyond the end of this file. Can you rephrase this?

(from mmap(2))
SIGBUS Attempted access to a page of the buffer that lies beyond the end
of the mapped file. For an explanation of the treatment of the bytes in the page that corresponds to the end of a mapped file that is not a multiple of the page size, see NOTES.

The code is good though.

Quoted 1 line

Reviewed-by Matthew Wilcox (Oracle) <[email protected]>

Since I've been curating the respective fstests test to test for this POSIX corner case [0] I wanted to enable the test for tmpfs instead of skipping it as I originally had it, and that meant also realizing mmap(2) specifically says this now:

Huge page (Huge TLB) mappings

...
For mmap(), offset must be a multiple of the underlying huge page size. The system automatically aligns length to be a multiple of the underlying huge page size.

So do we need to adjust this patch with this:

mm/filemap.c +5 -1

index ea78963f0956..9c8897ba90ff 100644
@@ -3617,6 +3617,7 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,
  	vm_fault_t ret = 0;
  	unsigned long rss = 0;
  	unsigned int nr_pages = 0, mmap_miss = 0, mmap_miss_saved, folio_type;
+	unsigned int align = PAGE_SIZE;
 
  	rcu_read_lock();
  	folio = next_uptodate_folio(&xas, mapping, end_pgoff);

@@ -3636,7 +3637,10 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,
  		goto out;
  	}
 
-	file_end = DIV_ROUND_UP(i_size_read(mapping->host), PAGE_SIZE) - 1;
+	if (folio_test_pmd_mappable(folio))
+		align = 1 << folio_order(folio);
+
+	file_end = DIV_ROUND_UP(i_size_read(mapping->host), align) - 1;
  	if (end_pgoff > file_end)
  		end_pgoff = file_end;
 

[0] https://lore.kernel.org/all/[email protected]/

Luis

Quoted 33 lines · Luis Chamberlain

Quoted 23 lines · Matthew Wilcox

Quoted 9 lines · Pankaj Raghav (Samsung)

From: Pankaj Raghav <[email protected]>

Usually the page cache does not extend beyond the size of the inode, therefore, no PTEs are created for folios that extend beyond the size.

But with LBS support, we might extend page cache beyond the size of the inode as we need to guarantee folios of minimum order. Cap the PTE range to be created for the page cache up to the max allowed zero-fill file end, which is aligned to the PAGE_SIZE.

I think this is slightly misleading because we might well zero-fill to the end of the folio. The issue is that we're supposed to SIGBUS if userspace accesses pages which lie entirely beyond the end of this file. Can you rephrase this?

(from mmap(2))
SIGBUS Attempted access to a page of the buffer that lies beyond the end
of the mapped file. For an explanation of the treatment of the bytes in the page that corresponds to the end of a mapped file that is not a multiple of the page size, see NOTES.

Quoted 3 lines

The code is good though.

Reviewed-by Matthew Wilcox (Oracle) <[email protected]>

Since I've been curating the respective fstests test to test for this POSIX corner case [0] I wanted to enable the test for tmpfs instead of skipping it as I originally had it, and that meant also realizing mmap(2) specifically says this now:

Quoted 1 line

Huge page (Huge TLB) mappings

Confusion alert: this likely talks about hugetlb (MAP_HUGETLB), not THP and friends.

So it might not be required for below changes.

Quoted 39 lines · David Hildenbrand

Quoted 26 lines · Luis Chamberlain

Quoted 25 lines · Matthew Wilcox

Quoted 9 lines · Pankaj Raghav (Samsung)

From: Pankaj Raghav <[email protected]>

Usually the page cache does not extend beyond the size of the inode, therefore, no PTEs are created for folios that extend beyond the size.

But with LBS support, we might extend page cache beyond the size of the inode as we need to guarantee folios of minimum order. Cap the PTE range to be created for the page cache up to the max allowed zero-fill file end, which is aligned to the PAGE_SIZE.

I think this is slightly misleading because we might well zero-fill to the end of the folio. The issue is that we're supposed to SIGBUS if userspace accesses pages which lie entirely beyond the end of this file. Can you rephrase this?

(from mmap(2))
SIGBUS Attempted access to a page of the buffer that lies beyond the end
of the mapped file. For an explanation of the treatment of the bytes in the page that corresponds to the end of a mapped file that is not a multiple of the page size, see NOTES.

The code is good though.

Quoted 8 lines

Quoted 1 line

Reviewed-by Matthew Wilcox (Oracle) <[email protected]>

Since I've been curating the respective fstests test to test for this POSIX corner case [0] I wanted to enable the test for tmpfs instead of skipping it as I originally had it, and that meant also realizing mmap(2) specifically says this now:

Huge page (Huge TLB) mappings

Confusion alert: this likely talks about hugetlb (MAP_HUGETLB), not THP and friends.

Quoted 1 line

So it might not be required for below changes.

Thanks, I had to ask as we're dusting off this little obscure corner of the universe. Reason I ask, is the test fails for tmpfs with huge pages, and this patch fixes it, but it got me wondering the above applies also to tmpfs with huge pages.

Quoted 42 lines · Luis Chamberlain

Quoted 40 lines · David Hildenbrand

Quoted 26 lines · Luis Chamberlain

Quoted 25 lines · Matthew Wilcox

Quoted 9 lines · Pankaj Raghav (Samsung)

From: Pankaj Raghav <[email protected]>

Usually the page cache does not extend beyond the size of the inode, therefore, no PTEs are created for folios that extend beyond the size.

But with LBS support, we might extend page cache beyond the size of the inode as we need to guarantee folios of minimum order. Cap the PTE range to be created for the page cache up to the max allowed zero-fill file end, which is aligned to the PAGE_SIZE.

I think this is slightly misleading because we might well zero-fill to the end of the folio. The issue is that we're supposed to SIGBUS if userspace accesses pages which lie entirely beyond the end of this file. Can you rephrase this?

(from mmap(2))
SIGBUS Attempted access to a page of the buffer that lies beyond the end
of the mapped file. For an explanation of the treatment of the bytes in the page that corresponds to the end of a mapped file that is not a multiple of the page size, see NOTES.

The code is good though.

Quoted 8 lines

Quoted 1 line

Reviewed-by Matthew Wilcox (Oracle) <[email protected]>

Since I've been curating the respective fstests test to test for this POSIX corner case [0] I wanted to enable the test for tmpfs instead of skipping it as I originally had it, and that meant also realizing mmap(2) specifically says this now:

Huge page (Huge TLB) mappings

Confusion alert: this likely talks about hugetlb (MAP_HUGETLB), not THP and friends.

So it might not be required for below changes.

Quoted 4 lines

Thanks, I had to ask as we're dusting off this little obscure corner of the universe. Reason I ask, is the test fails for tmpfs with huge pages, and this patch fixes it, but it got me wondering the above applies also to tmpfs with huge pages.

Is it tmpfs with THP/large folios or shmem with hugetlb? I assume the tmpfs with THP. There are not really mmap/munmap restrictions to THP and friends (because it's supposed to be "transparent" :) ).

The case I tested that failed the test was tmpfs with huge pages (not large folios). So should we then have this:

Quoted 2 lines · Luis Chamberlain

The case I tested that failed the test was tmpfs with huge pages (not large folios). So should we then have this:

No.

Quoted 5 lines · Matthew Wilcox

Quoted 2 lines · Luis Chamberlain

The case I tested that failed the test was tmpfs with huge pages (not large folios). So should we then have this:

No.

OK so this does have a change for tmpfs with huge pages enabled, do we take the position then this is a fix for that?

Quoted 7 lines · Luis Chamberlain

Quoted 5 lines · Matthew Wilcox

Quoted 2 lines · Luis Chamberlain

The case I tested that failed the test was tmpfs with huge pages (not large folios). So should we then have this:

No.

Quoted 2 lines

OK so this does have a change for tmpfs with huge pages enabled, do we take the position then this is a fix for that?

You literally said it was a fix just a few messages up thread?

Besides, the behaviour changes (currently) depending on whether you specify "within_size" or "always". This patch makes it consistent.

From: Pankaj Raghav <[email protected]>

iomap_dio_zero() will pad a fs block with zeroes if the direct IO size < fs block size. iomap_dio_zero() has an implicit assumption that fs block size < page_size. This is true for most filesystems at the moment.

If the block size > page size, this will send the contents of the page next to zero page(as len > PAGE_SIZE) to the underlying block device, causing FS corruption.

iomap is a generic infrastructure and it should not make any assumptions about the fs block size and the page size of the system.

Signed-off-by Pankaj Raghav <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

---
  fs/internal.h          |  5 +++++
  fs/iomap/buffered-io.c |  6 ++++++
  fs/iomap/direct-io.c   | 26 ++++++++++++++++++++++++--
  3 files changed, 35 insertions(+), 2 deletions(-)

fs/internal.h +5 -0

index 84f371193f74..30217f0ff4c6 100644
@@ -35,6 +35,11 @@ static inline void bdev_cache_init(void)
  int __block_write_begin_int(struct folio *folio, loff_t pos, unsigned len,
  		get_block_t *get_block, const struct iomap *iomap);
 
+/*
+ * iomap/direct-io.c
+ */
+int iomap_dio_init(void);
+
  /*
   * char_dev.c
   */

fs/iomap/buffered-io.c +0 -0

index 49938419fcc7..9f791db473e4 100644
@@ -1990,6 +1990,12 @@ EXPORT_SYMBOL_GPL(iomap_writepages);
 
  static int __init iomap_init(void)

fs/iomap/direct-io.c +17 -0

index f3b43d223a46..b95600b254a3 100644
@@ -27,6 +27,13 @@
  #define IOMAP_DIO_WRITE		(1U << 30)
  #define IOMAP_DIO_DIRTY		(1U << 31)
 
+/*
+ * Used for sub block zeroing in iomap_dio_zero()
+ */
+#define ZERO_FSB_SIZE (65536)
+#define ZERO_FSB_ORDER (get_order(ZERO_FSB_SIZE))
+static struct page *zero_fs_block;
+
  struct iomap_dio {
  	struct kiocb		*iocb;
  	const struct iomap_dio_ops *dops;

@@ -52,6 +59,16 @@ struct iomap_dio {
  	};
  };
 
+int iomap_dio_init(void)
+{
+	zero_fs_block = alloc_pages(GFP_KERNEL | __GFP_ZERO, ZERO_FSB_ORDER);
+
+	if (!zero_fs_block)
+		return -ENOMEM;
+
+	return 0;
+}
+
  static struct bio *iomap_dio_alloc_bio(const struct iomap_iter *iter,
  		struct iomap_dio *dio, unsigned short nr_vecs, blk_opf_t opf)
  {

fs/iomap/direct-io.c +3 -1

+
  	bio = iomap_dio_alloc_bio(iter, dio, 1, REQ_OP_WRITE | REQ_SYNC | REQ_IDLE);
  	fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits,
  				  GFP_KERNEL);
+
  	bio->bi_iter.bi_sector = iomap_sector(&iter->iomap, pos);
  	bio->bi_private = dio;
  	bio->bi_end_io = iomap_dio_bio_end_io;
 
-	__bio_add_page(bio, page, len, 0);
+	__bio_add_page(bio, zero_fs_block, len, 0);
  	iomap_dio_submit_bio(iter, dio, bio, pos);
  }
 

Quoted 11 lines

index 49938419fcc7..9f791db473e4 100644

fs/iomap/buffered-io.c +5 -0

@@ -1990,6 +1990,12 @@ EXPORT_SYMBOL_GPL(iomap_writepages);
  static int __init iomap_init(void)
  {
+	int ret;
+
+	ret = iomap_dio_init();
+	if (ret)
+		return ret;

fs/iomap/buffered-io.c EXPORT_SYMBOL_GPL(iomap_writepages); · line 2000

Quoted 4 lines

fs/iomap/buffered-io.c +1 -0

+
  	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
  			   offsetof(struct iomap_ioend, io_bio),
  			   BIOSET_NEED_BVECS);

I suppose that it does not matter that zero_fs_block is leaked if this fails (or is it even leaked?), as I don't think that failing that bioset_init() call is handled at all.

Quoted 11 lines

fs/iomap/buffered-io.c +1 -1

+
  static struct bio *iomap_dio_alloc_bio(const struct iomap_iter *iter,
  		struct iomap_dio *dio, unsigned short nr_vecs, blk_opf_t opf)
  {

@@ -236,17 +253,22 @@ static void iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio,
  		loff_t pos, unsigned len)
  {
  	struct inode *inode = file_inode(dio->iocb->ki_filp);
-	struct page *page = ZERO_PAGE(0);
  	struct bio *bio;

fs/iomap/buffered-io.c static void iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio, · line 260

Quoted 4 lines

fs/iomap/buffered-io.c +4 -0

+	/*
+	 * Max block size supported is 64k
+	 */
+	WARN_ON_ONCE(len > ZERO_FSB_SIZE);

JFYI, As mentioned in https://lore.kernel.org/linux-xfs/[email protected]/T/#m5354e2b2531a5552a8b8acd4a95342ed4d7500f2,
we would like to support an arbitrary size. Maybe I will need to loop for
zeroing sizes > 64K.

Quoted 10 lines

Quoted 10 lines

8419fcc7..9f791db473e4 100644

fs/iomap/buffered-io.c +4 -0

@@ -1990,6 +1990,12 @@ EXPORT_SYMBOL_GPL(iomap_writepages);
   static int __init iomap_init(void)
   {
+	int ret;
+
+	ret = iomap_dio_init();
+	if (ret)

Quoted 8 lines

fs/iomap/buffered-io.c EXPORT_SYMBOL_GPL(iomap_writepages); · line 2000

Quoted 5 lines

fs/iomap/buffered-io.c +2 -0

+		return ret;
+
   	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
   			   offsetof(struct iomap_ioend, io_bio),
   			   BIOSET_NEED_BVECS);

I suppose that it does not matter that zero_fs_block is leaked if this fails (or is it even leaked?), as I don't think that failing that bioset_init() call is handled at all.

If bioset_init fails, then we have even more problems than just a leaked 64k memory? 😉

Do you have something like this in mind?

static struct bio *iomap_dio_alloc_bio(const struct iomap_iter *iter,
struct iomap_dio *dio, unsigned short nr_vecs, blk_opf_t opf)
{

Quoted 9 lines

Quoted 9 lines

fs/iomap/buffered-io.c +1 -1

+
   static struct bio *iomap_dio_alloc_bio(const struct iomap_iter *iter,
   		struct iomap_dio *dio, unsigned short nr_vecs, blk_opf_t opf)
   {

@@ -236,17 +253,22 @@ static void iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio,
   		loff_t pos, unsigned len)
   {
   	struct inode *inode = file_inode(dio->iocb->ki_filp);
-	struct page *page = ZERO_PAGE(0);

Quoted 8 lines

fs/iomap/buffered-io.c static void iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio, · line 260

Quoted 5 lines

fs/iomap/buffered-io.c +4 -0

   	struct bio *bio;
+	/*
+	 * Max block size supported is 64k
+	 */
+	WARN_ON_ONCE(len > ZERO_FSB_SIZE);

JFYI, As mentioned inhttps://urldefense.com/v3/__https://lore.kernel.org/linux-xfs/[email protected]/T/*m5354e2b2531a5552a8b8acd4a95342ed4d7500f2__;Iw!!ACWV5N9M2RV99hQ!MTwVaC6oueHR_vgmDfOvgBX8bPdeTSRPcRcw5-CqtHnFEH-Ya1sUeZwaF-xrBF5XZ_8lJw5l-riq4t8IkfBhf2Q$ , we would like to support an arbitrary size. Maybe I will need to loop for
zeroing sizes > 64K.

The initial patches were looping with a ZERO_PAGE(0), but the initial feedback was to use a huge zero page. But when I discussed that at LSF, the people thought we will be using a lot of memory for sub-block memory, especially on architectures with 64k base page size.

So for now a good tradeoff between memory usage and efficiency was to use a 64k buffer as that is the maximum FSB we support.[1]

IIUC, you will be using this function also to zero out the extent and not just a FSB?

I think we could resort to looping until we have a way to request arbitrary zero folios without having to allocate at it in iomap_dio_alloc_bio() for every IO.

[1]https://urldefense.com/v3/__https://lore.kernel.org/linux-xfs/[email protected]/__;!!ACWV5N9M2RV99hQ!MTwVaC6oueHR_vgmDfOvgBX8bPdeTSRPcRcw5-CqtHnFEH-Ya1sUeZwaF-xrBF5XZ_8lJw5l-riq4t8Ij2hl9yU$  

From: Pankaj Raghav <[email protected]>

iomap_dio_zero() will pad a fs block with zeroes if the direct IO size < fs block size. iomap_dio_zero() has an implicit assumption that fs block size < page_size. This is true for most filesystems at the moment.

If the block size > page size, this will send the contents of the page next to zero page(as len > PAGE_SIZE) to the underlying block device, causing FS corruption.

iomap is a generic infrastructure and it should not make any assumptions about the fs block size and the page size of the system.

Signed-off-by Pankaj Raghav <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

---
 fs/internal.h          |  5 +++++
 fs/iomap/buffered-io.c |  6 ++++++
 fs/iomap/direct-io.c   | 26 ++++++++++++++++++++++++--
 3 files changed, 35 insertions(+), 2 deletions(-)

fs/internal.h +5 -0

index 84f371193f74..30217f0ff4c6 100644
@@ -35,6 +35,11 @@ static inline void bdev_cache_init(void)
 int __block_write_begin_int(struct folio *folio, loff_t pos, unsigned len,
 		get_block_t *get_block, const struct iomap *iomap);
 
+/*
+ * iomap/direct-io.c
+ */
+int iomap_dio_init(void);
+
 /*
  * char_dev.c
  */

fs/iomap/buffered-io.c +6 -0

index 49938419fcc7..9f791db473e4 100644
@@ -1990,6 +1990,12 @@ EXPORT_SYMBOL_GPL(iomap_writepages);
 
 static int __init iomap_init(void)
 {
+	int ret;
+
+	ret = iomap_dio_init();
+	if (ret)
+		return ret;
+
 	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
 			   offsetof(struct iomap_ioend, io_bio),
 			   BIOSET_NEED_BVECS);

fs/iomap/direct-io.c +0 -0

index f3b43d223a46..b95600b254a3 100644

fs/iomap/direct-io.c +1 -0

+
 struct iomap_dio {
 	struct kiocb		*iocb;
 	const struct iomap_dio_ops *dops;

@@ -52,6 +59,16 @@ struct iomap_dio {
 	};
 };

fs/iomap/direct-io.c +8 -2

+
 static struct bio *iomap_dio_alloc_bio(const struct iomap_iter *iter,
 		struct iomap_dio *dio, unsigned short nr_vecs, blk_opf_t opf)
 {

@@ -236,17 +253,22 @@ static void iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio,
 		loff_t pos, unsigned len)
 {
 	struct inode *inode = file_inode(dio->iocb->ki_filp);
-	struct page *page = ZERO_PAGE(0);
 	struct bio *bio;
 
+	/*
+	 * Max block size supported is 64k
+	 */
+	WARN_ON_ONCE(len > ZERO_FSB_SIZE);
+
 	bio = iomap_dio_alloc_bio(iter, dio, 1, REQ_OP_WRITE | REQ_SYNC | REQ_IDLE);
 	fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits,
 				  GFP_KERNEL);
+
 	bio->bi_iter.bi_sector = iomap_sector(&iter->iomap, pos);
 	bio->bi_private = dio;
 	bio->bi_end_io = iomap_dio_bio_end_io;
 
-	__bio_add_page(bio, page, len, 0);
+	__bio_add_page(bio, zero_fs_block, len, 0);
 	iomap_dio_submit_bio(iter, dio, bio, pos);
 }
 

Signature · 1 line

2.44.1

Quoted 7 lines · Pankaj Raghav (Samsung)

From: Pankaj Raghav <[email protected]>

fs/iomap/direct-io.c +0 -0

index f3b43d223a46..b95600b254a3 100644

fs/iomap/direct-io.c line 35

Quoted 9 lines

fs/iomap/direct-io.c +6 -0

@@ -27,6 +27,13 @@
 #define IOMAP_DIO_WRITE		(1U << 30)
 #define IOMAP_DIO_DIRTY		(1U << 31)
 
+/*
+ * Used for sub block zeroing in iomap_dio_zero()
+ */
+#define ZERO_FSB_SIZE (65536)
+#define ZERO_FSB_ORDER (get_order(ZERO_FSB_SIZE))
+static struct page *zero_fs_block;

Er... zero_page_64k ?

Since it's a permanent allocation, can we also mark the memory ro?

Quoted 9 lines

fs/iomap/direct-io.c +1 -0

+
 struct iomap_dio {
 	struct kiocb		*iocb;
 	const struct iomap_dio_ops *dops;

@@ -52,6 +59,16 @@ struct iomap_dio {
 	};
 };
 

fs/iomap/direct-io.c struct iomap_dio { · line 70

Quoted 9 lines

fs/iomap/direct-io.c +9 -0

+int iomap_dio_init(void)
+{
+	zero_fs_block = alloc_pages(GFP_KERNEL | __GFP_ZERO, ZERO_FSB_ORDER);
+
+	if (!zero_fs_block)
+		return -ENOMEM;
+
+	return 0;
+}

Can't we just turn this into another fs_initcall() instead of exporting it just so we can call it from iomap_init? And maybe rename the existing iomap_init to iomap_pagecache_init or something, for clarity's sake?

--D

Quoted 1 line

Quoted 1 line

Quoted 1 line

   WARN_ON_ONCE((gfp_flags & __GFP_NOFAIL) && (order > 1));

fs/xfs/xfs_mount.c +3 -0

+	uint64_t		max_index;
+	uint64_t		max_bytes;
+
 	ASSERT(PAGE_SHIFT >= sbp->sb_blocklog);

Quoted 6 lines

(quoted diff) +3 -0

+	uint64_t		max_index;
+	uint64_t		max_bytes;
+
 	ASSERT(PAGE_SHIFT >= sbp->sb_blocklog);
 	ASSERT(sbp->sb_blocklog >= BBSHIFT);

Review

Quoted 8 lines

(quoted diff) +6 -1

+	if (check_shl_overflow(nblocks, sbp->sb_blocklog, &max_bytes))
+		return -EFBIG;
+
 	/* Limited by ULONG_MAX of page cache index */
-	if (nblocks >> (PAGE_SHIFT - sbp->sb_blocklog) > ULONG_MAX)
+	max_index = max_bytes >> PAGE_SHIFT;
+
+	if (max_index > ULONG_MAX)

Do we really need the max_index variable for a single user here? Or do you plan to add more uses of it later (can't really think of one though)?

fs/xfs/libxfs/xfs_ialloc.c xfs_ialloc_setup_geometry( · line 3027

Quoted 12 lines · Pankaj Raghav (Samsung)

fs/xfs/libxfs/xfs_ialloc.c +5 -0

@@ -3019,6 +3019,11 @@ xfs_ialloc_setup_geometry(
 		igeo->ialloc_align = mp->m_dalign;
 	else
 		igeo->ialloc_align = 0;
+
+	if (mp->m_sb.sb_blocksize > PAGE_SIZE)
+		igeo->min_folio_order = mp->m_sb.sb_blocklog - PAGE_SHIFT;
+	else
+		igeo->min_folio_order = 0;
 }

The minimum folio order isn't really part of the inode (allocation) geometry, is it?

Quoted 8 lines

fs/xfs/libxfs/xfs_ialloc.c xfs_ialloc_setup_geometry( · line 3027

Quoted 5 lines

fs/xfs/libxfs/xfs_ialloc.c +4 -0

+	if (mp->m_sb.sb_blocksize > PAGE_SIZE)
+		igeo->min_folio_order = mp->m_sb.sb_blocklog - PAGE_SHIFT;
+	else
+		igeo->min_folio_order = 0;
 }

The minimum folio order isn't really part of the inode (allocation) geometry, is it?

I suggested it last time around instead of calculating the same constant on every inode allocation. We're already storing in-memory strunct xfs_inode allocation init values in this structure. e.g. in xfs_inode_alloc() we see things like this:

The only other place we might store it is the struct xfs_mount, but given all the inode allocation constants are already in the embedded mp->m_ino_geo structure, it just seems like a much better idea to put it will all the other inode allocation constants than dump it randomly into the struct xfs_mount....

Quoted 9 lines · Dave Chinner

Quoted 8 lines

fs/xfs/libxfs/xfs_ialloc.c xfs_ialloc_setup_geometry( · line 3027

Quoted 5 lines

fs/xfs/libxfs/xfs_ialloc.c +4 -0

+	if (mp->m_sb.sb_blocksize > PAGE_SIZE)
+		igeo->min_folio_order = mp->m_sb.sb_blocklog - PAGE_SHIFT;
+	else
+		igeo->min_folio_order = 0;
 }

The minimum folio order isn't really part of the inode (allocation) geometry, is it?

Quoted 4 lines

I suggested it last time around instead of calculating the same constant on every inode allocation. We're already storing in-memory strunct xfs_inode allocation init values in this structure. e.g. in xfs_inode_alloc() we see things like this:

While new_diflags2 isn't exactly inode geometry, it at least is part of the inode allocation. Folio min order for file data has nothing to do with this at all.

Quoted 5 lines

The only other place we might store it is the struct xfs_mount, but given all the inode allocation constants are already in the embedded mp->m_ino_geo structure, it just seems like a much better idea to put it will all the other inode allocation constants than dump it randomly into the struct xfs_mount....

Well, it is very closely elated to say the m_blockmask field in struct xfs_mount. The again modern CPUs tend to get a you simple subtraction for free in most pipelines doing other things, so I'm not really sure it's worth caching for use in inode allocation to start with, but I don't care strongly about that.

Quoted 9 lines · Dave Chinner

Quoted 8 lines

fs/xfs/libxfs/xfs_ialloc.c xfs_ialloc_setup_geometry( · line 3027

Quoted 5 lines

fs/xfs/libxfs/xfs_ialloc.c +4 -0

+	if (mp->m_sb.sb_blocksize > PAGE_SIZE)
+		igeo->min_folio_order = mp->m_sb.sb_blocklog - PAGE_SHIFT;
+	else
+		igeo->min_folio_order = 0;
 }

The minimum folio order isn't really part of the inode (allocation) geometry, is it?

Quoted 4 lines

I suggested it last time around instead of calculating the same constant on every inode allocation. We're already storing in-memory strunct xfs_inode allocation init values in this structure. e.g. in xfs_inode_alloc() we see things like this:

While new_diflags2 isn't exactly inode geometry, it at least is part of the inode allocation. Folio min order for file data has nothing to do with this at all.

Quoted 5 lines

The only other place we might store it is the struct xfs_mount, but given all the inode allocation constants are already in the embedded mp->m_ino_geo structure, it just seems like a much better idea to put it will all the other inode allocation constants than dump it randomly into the struct xfs_mount....

Well, it is very closely elated to say the m_blockmask field in struct xfs_mount.