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]>

Reviewed-by Darrick J. Wong <[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 +0 -0

index 4b71d581091f..0c51154cdb57 100644

include/linux/pagemap.h +23 -0

 };
 
+#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)

include/linux/pagemap.h +32 -8

+
+	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;
 }
 
 /*

@@ -386,16 +449,13 @@ static inline bool mapping_large_folio_support(struct address_space *mapping)
 	VM_WARN_ONCE((unsigned long)mapping & PAGE_MAPPING_ANON,
 			"Anonymous mapping always supports large folio");
 
-	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 0b8c732bb643..d617c9afca51 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 +0 -0

index c1b23989d9ca..66058ae02f2e 100644

mm/readahead.c +0 -0

 	}
 

include/linux/pagemap.h enum mapping_flags { · line 207

Quoted 1 line

include/linux/pagemap.h +0 -1

-	AS_LARGE_FOLIO_SUPPORT = 6,

nit: this removed enum is still referenced in a comment further down the file.

include/linux/pagemap.h enum mapping_flags { · line 215

Quoted 4 lines

include/linux/pagemap.h +4 -0

+	/* 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,

nit: These 3 new enums seem a bit odd.

Quoted 10 lines

include/linux/pagemap.h +10 -0

+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)

include/linux/pagemap.h static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask) · line 398

Quoted 3 lines

include/linux/pagemap.h +3 -0

+		max = MAX_PAGECACHE_ORDER;
+	if (max < min)
+		max = min;

It seems strange to silently clamp these? Presumably for the bs>ps usecase, whatever values are passed in are a hard requirement? So wouldn't want them to be silently reduced. (Especially given the recent change to reduce the size of MAX_PAGECACHE_ORDER to less then PMD size in some cases).

mm/readahead.c void page_cache_ra_order(struct readahead_control *ractl, · line 509

Quoted 6 lines

mm/readahead.c +2 -2

-	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));

I wonder if its possible that ra->size could ever be less than mapping_min_folio_order()? Do you need to handle that?

Quoted 3 lines · Ryan Roberts

include/linux/pagemap.h enum mapping_flags { · line 207

Quoted 1 line

include/linux/pagemap.h +0 -1

-	AS_LARGE_FOLIO_SUPPORT = 6,

nit: this removed enum is still referenced in a comment further down the file.

Thanks. Pankaj, let me know if you want me to send you a patch or if you'll do it directly.

Quoted 6 lines

include/linux/pagemap.h enum mapping_flags { · line 215

Quoted 4 lines

include/linux/pagemap.h +4 -0

+	/* 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,

nit: These 3 new enums seem a bit odd.

Yes, this is "too many helpful suggestions" syndrome. It made a lot more sense originally.

https://lore.kernel.org/linux-fsdevel/[email protected]/

Quoted 10 lines

Quoted 10 lines

include/linux/pagemap.h +10 -0

+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)

Quoted 8 lines

include/linux/pagemap.h static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask) · line 398

Quoted 3 lines

include/linux/pagemap.h +3 -0

+		max = MAX_PAGECACHE_ORDER;
+	if (max < min)
+		max = min;

It seems strange to silently clamp these? Presumably for the bs>ps usecase, whatever values are passed in are a hard requirement? So wouldn't want them to be silently reduced. (Especially given the recent change to reduce the size of MAX_PAGECACHE_ORDER to less then PMD size in some cases).

Hm, yes. We should probably make this return an errno. Including returning an errno for !IS_ENABLED() and min > 0.

Quoted 10 lines

mm/readahead.c void page_cache_ra_order(struct readahead_control *ractl, · line 509

Quoted 7 lines

mm/readahead.c +2 -2

-	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));

I wonder if its possible that ra->size could ever be less than mapping_min_folio_order()? Do you need to handle that?

I think we take care of that in later patches?

This patch is mostly
about honouring the max properly and putting in the infrastructure for the min, but not doing all the necessary work for min.

Quoted 3 lines · Ryan Roberts

Review

Quoted 1 line

(quoted diff) +0 -1

-	AS_LARGE_FOLIO_SUPPORT = 6,

nit: this removed enum is still referenced in a comment further down the file.

Thanks. Pankaj, let me know if you want me to send you a patch or if you'll do it directly.

Quoted 11 lines

Quoted 11 lines

(quoted diff) +10 -0

+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)

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+		max = MAX_PAGECACHE_ORDER;
+	if (max < min)
+		max = min;

It seems strange to silently clamp these? Presumably for the bs>ps usecase, whatever values are passed in are a hard requirement? So wouldn't want them to be silently reduced. (Especially given the recent change to reduce the size of MAX_PAGECACHE_ORDER to less then PMD size in some cases).

Hm, yes. We should probably make this return an errno. Including returning an errno for !IS_ENABLED() and min > 0.

Quoted 3 lines · Ryan Roberts

include/linux/pagemap.h enum mapping_flags { · line 207

Quoted 1 line

include/linux/pagemap.h +0 -1

-	AS_LARGE_FOLIO_SUPPORT = 6,

nit: this removed enum is still referenced in a comment further down the file.

Thanks. Pankaj, let me know if you want me to send you a patch or if you'll do it directly.

Quoted 6 lines

include/linux/pagemap.h enum mapping_flags { · line 215

Quoted 4 lines

include/linux/pagemap.h +4 -0

+	/* 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,

nit: These 3 new enums seem a bit odd.

Yes, this is "too many helpful suggestions" syndrome. It made a lot more sense originally.

https://lore.kernel.org/linux-fsdevel/[email protected]/

Quoted 10 lines

Quoted 10 lines

include/linux/pagemap.h +10 -0

+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)

Quoted 8 lines

include/linux/pagemap.h static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask) · line 398

Quoted 3 lines

include/linux/pagemap.h +3 -0

+		max = MAX_PAGECACHE_ORDER;
+	if (max < min)
+		max = min;

It seems strange to silently clamp these? Presumably for the bs>ps usecase, whatever values are passed in are a hard requirement? So wouldn't want them to be silently reduced. (Especially given the recent change to reduce the size of MAX_PAGECACHE_ORDER to less then PMD size in some cases).

Hm, yes. We should probably make this return an errno. Including returning an errno for !IS_ENABLED() and min > 0.

Quoted 5 lines

Quoted 4 lines

It seems strange to silently clamp these? Presumably for the bs>ps usecase, whatever values are passed in are a hard requirement? So wouldn't want them to be silently reduced. (Especially given the recent change to reduce the size of MAX_PAGECACHE_ORDER to less then PMD size in some cases).

Quoted 2 lines

Hm, yes. We should probably make this return an errno. Including returning an errno for !IS_ENABLED() and min > 0.

What are callers supposed to do with an error? In the case of setting up a newly allocated inode in XFS, the error would be returned in the middle of a transaction and so this failure would result in a filesystem shutdown.

Quoted 8 lines · Dave Chinner

Quoted 7 lines

Quoted 4 lines

It seems strange to silently clamp these? Presumably for the bs>ps usecase, whatever values are passed in are a hard requirement? So wouldn't want them to be silently reduced. (Especially given the recent change to reduce the size of MAX_PAGECACHE_ORDER to less then PMD size in some cases).

Hm, yes. We should probably make this return an errno. Including returning an errno for !IS_ENABLED() and min > 0.

Quoted 4 lines

What are callers supposed to do with an error? In the case of setting up a newly allocated inode in XFS, the error would be returned in the middle of a transaction and so this failure would result in a filesystem shutdown.

I suggest you handle it better than this. If the device is asking for a blocksize > PMD_SIZE, you should fail to mount it.

If the device is
asking for a blocksize > PAGE_SIZE and CONFIG_TRANSPARENT_HUGEPAGE is not set, you should also decline to mount the filesystem.

Quoted 9 lines · Matthew Wilcox

Quoted 8 lines · Dave Chinner

Quoted 7 lines

Quoted 4 lines

It seems strange to silently clamp these? Presumably for the bs>ps usecase, whatever values are passed in are a hard requirement? So wouldn't want them to be silently reduced. (Especially given the recent change to reduce the size of MAX_PAGECACHE_ORDER to less then PMD size in some cases).

Hm, yes. We should probably make this return an errno. Including returning an errno for !IS_ENABLED() and min > 0.

Quoted 7 lines

Quoted 4 lines

What are callers supposed to do with an error? In the case of setting up a newly allocated inode in XFS, the error would be returned in the middle of a transaction and so this failure would result in a filesystem shutdown.

I suggest you handle it better than this. If the device is asking for a blocksize > PMD_SIZE, you should fail to mount it.

That's my point: we already do that.

The largest block size we support is 64kB and that's way smaller than PMD_SIZE on all platforms and we always check for bs > ps support at mount time when the filesystem bs > ps.

Hence we're never going to set the min value to anything unsupported unless someone makes a massive programming mistake. At which point, we want a *hard, immediate fail* so the developer notices their mistake immediately. All filesystems and block devices need to behave this way so the limits should be encoded as asserts in the function to trigger such behaviour.

Quoted 3 lines

If the device is
asking for a blocksize > PAGE_SIZE and CONFIG_TRANSPARENT_HUGEPAGE is not set, you should also decline to mount the filesystem.

What does CONFIG_TRANSPARENT_HUGEPAGE have to do with filesystems being able to use large folios?

If that's an actual dependency of using large folios, then we're at the point where the mm side of large folios needs to be divorced from CONFIG_TRANSPARENT_HUGEPAGE and always supported. Alternatively, CONFIG_TRANSPARENT_HUGEPAGE needs to selected by the block layer and also every filesystem that wants to support sector/blocks sizes larger than PAGE_SIZE. IOWs, large folio support needs to *always* be enabled on systems that say CONFIG_BLOCK=y.

I'd much prefer the former occurs, because making the block layer and filesystems dependent on an mm feature they don't actually use is kinda weird...

-Dave.

Quoted 10 lines · Dave Chinner

Quoted 9 lines · Matthew Wilcox

Quoted 8 lines · Dave Chinner

Quoted 7 lines

Quoted 4 lines

It seems strange to silently clamp these? Presumably for the bs>ps usecase, whatever values are passed in are a hard requirement? So wouldn't want them to be silently reduced. (Especially given the recent change to reduce the size of MAX_PAGECACHE_ORDER to less then PMD size in some cases).

Hm, yes. We should probably make this return an errno. Including returning an errno for !IS_ENABLED() and min > 0.

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

What are callers supposed to do with an error? In the case of setting up a newly allocated inode in XFS, the error would be returned in the middle of a transaction and so this failure would result in a filesystem shutdown.

I suggest you handle it better than this. If the device is asking for a blocksize > PMD_SIZE, you should fail to mount it.

A detail, but MAX_PAGECACHE_ORDER may be smaller than PMD_SIZE even on systems with CONFIG_TRANSPARENT_HUGEPAGE as of a fix that is currently in mm-unstable:

	#ifdef CONFIG_TRANSPARENT_HUGEPAGE
	#define PREFERRED_MAX_PAGECACHE_ORDER	HPAGE_PMD_ORDER
	#else
	#define PREFERRED_MAX_PAGECACHE_ORDER	8
	#endif

	/*
	 * xas_split_alloc() does not support arbitrary orders. This implies no
	 * 512MB THP on ARM64 with 64KB base page size.
	 */
	#define MAX_XAS_ORDER		(XA_CHUNK_SHIFT * 2 - 1)
	#define MAX_PAGECACHE_ORDER	min(MAX_XAS_ORDER,
					    PREFERRED_MAX_PAGECACHE_ORDER)

But that also implies that the page cache can handle up to order-8 without CONFIG_TRANSPARENT_HUGEPAGE so sounds like there isn't a dependcy on CONFIG_TRANSPARENT_HUGEPAGE in this respect?

Quoted 2 lines

I suggest you handle it better than this. If the device is asking for a blocksize > PMD_SIZE, you should fail to mount it.

That's my point: we already do that.

The largest block size we support is 64kB and that's way smaller than PMD_SIZE on all platforms and we always check for bs > ps support at mount time when the filesystem bs > ps.

Hence we're never going to set the min value to anything unsupported unless someone makes a massive programming mistake. At which point, we want a *hard, immediate fail* so the developer notices their mistake immediately. All filesystems and block devices need to behave this way so the limits should be encoded as asserts in the function to trigger such behaviour.

Quoted 4 lines

If the device is
asking for a blocksize > PAGE_SIZE and CONFIG_TRANSPARENT_HUGEPAGE is not set, you should also decline to mount the filesystem.

What does CONFIG_TRANSPARENT_HUGEPAGE have to do with filesystems being able to use large folios?

If that's an actual dependency of using large folios, then we're at the point where the mm side of large folios needs to be divorced from CONFIG_TRANSPARENT_HUGEPAGE and always supported. Alternatively, CONFIG_TRANSPARENT_HUGEPAGE needs to selected by the block layer and also every filesystem that wants to support sector/blocks sizes larger than PAGE_SIZE. IOWs, large folio support needs to *always* be enabled on systems that say CONFIG_BLOCK=y.

Quoted 9 lines

Quoted 2 lines

I suggest you handle it better than this. If the device is asking for a blocksize > PMD_SIZE, you should fail to mount it.

That's my point: we already do that.

The largest block size we support is 64kB and that's way smaller than PMD_SIZE on all platforms and we always check for bs > ps support at mount time when the filesystem bs > ps.

Quoted 6 lines

Hence we're never going to set the min value to anything unsupported unless someone makes a massive programming mistake. At which point, we want a *hard, immediate fail* so the developer notices their mistake immediately. All filesystems and block devices need to behave this way so the limits should be encoded as asserts in the function to trigger such behaviour.

I agree, this kind of bug will be encountered only during developement and not during actual production due to the limit we have fs block size in XFS.

Quoted 8 lines

Quoted 4 lines

If the device is
asking for a blocksize > PAGE_SIZE and CONFIG_TRANSPARENT_HUGEPAGE is not set, you should also decline to mount the filesystem.

What does CONFIG_TRANSPARENT_HUGEPAGE have to do with filesystems being able to use large folios?

Quoted 8 lines

If that's an actual dependency of using large folios, then we're at the point where the mm side of large folios needs to be divorced from CONFIG_TRANSPARENT_HUGEPAGE and always supported. Alternatively, CONFIG_TRANSPARENT_HUGEPAGE needs to selected by the block layer and also every filesystem that wants to support sector/blocks sizes larger than PAGE_SIZE. IOWs, large folio support needs to *always* be enabled on systems that say CONFIG_BLOCK=y.

Why CONFIG_BLOCK? I think it is enough if it comes from the FS side right? And for now, the only FS that needs that sort of bs > ps guarantee is XFS with this series. Other filesystems such as bcachefs that call mapping_set_large_folios() only enable it as an optimization and it is not needed for the filesystem to function.

So this is my conclusion from the conversation:
- Add a dependency in Kconfig on THP for XFS until we fix the dependency of large folios on THP

- Add a BUILD_BUG_ON(XFS_MAX_BLOCKSIZE > MAX_PAGECACHE_ORDER)
- Add a WARN_ON_ONCE() and clamp the min and max value in mapping_set_folio_order_range() ?

Let me know what you all think @willy, @dave and @ryan.

--
Pankaj

Quoted 23 lines

Quoted 8 lines

Quoted 4 lines

If the device is
asking for a blocksize > PAGE_SIZE and CONFIG_TRANSPARENT_HUGEPAGE is not set, you should also decline to mount the filesystem.

What does CONFIG_TRANSPARENT_HUGEPAGE have to do with filesystems being able to use large folios?

Quoted 8 lines

If that's an actual dependency of using large folios, then we're at the point where the mm side of large folios needs to be divorced from CONFIG_TRANSPARENT_HUGEPAGE and always supported. Alternatively, CONFIG_TRANSPARENT_HUGEPAGE needs to selected by the block layer and also every filesystem that wants to support sector/blocks sizes larger than PAGE_SIZE. IOWs, large folio support needs to *always* be enabled on systems that say CONFIG_BLOCK=y.

Why CONFIG_BLOCK? I think it is enough if it comes from the FS side right? And for now, the only FS that needs that sort of bs > ps guarantee is XFS with this series. Other filesystems such as bcachefs that call mapping_set_large_folios() only enable it as an optimization and it is not needed for the filesystem to function.

Quoted 3 lines

So this is my conclusion from the conversation:
- Add a dependency in Kconfig on THP for XFS until we fix the dependency of large folios on THP

THP isn't supported on some arches, so isn't this effectively saying XFS can no longer be used with those arches, even if the bs <= ps? I think while pagecache large folios depend on THP, you need to make this a mount-time check in the FS?

But ideally, MAX_PAGECACHE_ORDER would be set to 0 for !CONFIG_TRANSPARENT_HUGEPAGE so you can just check against that and don't have to worry about THP availability directly.

Willy; Why is MAX_PAGECACHE_ORDER set to 8 when THP is disabled currently?

Quoted 8 lines

- Add a BUILD_BUG_ON(XFS_MAX_BLOCKSIZE > MAX_PAGECACHE_ORDER)
- Add a WARN_ON_ONCE() and clamp the min and max value in mapping_set_folio_order_range() ?

Let me know what you all think @willy, @dave and @ryan.

--
Pankaj

Quoted 43 lines · Pankaj Raghav (Samsung)

Quoted 9 lines

Quoted 2 lines

I suggest you handle it better than this. If the device is asking for a blocksize > PMD_SIZE, you should fail to mount it.

That's my point: we already do that.

The largest block size we support is 64kB and that's way smaller than PMD_SIZE on all platforms and we always check for bs > ps support at mount time when the filesystem bs > ps.

Quoted 6 lines

Hence we're never going to set the min value to anything unsupported unless someone makes a massive programming mistake. At which point, we want a *hard, immediate fail* so the developer notices their mistake immediately. All filesystems and block devices need to behave this way so the limits should be encoded as asserts in the function to trigger such behaviour.

I agree, this kind of bug will be encountered only during developement and not during actual production due to the limit we have fs block size in XFS.

Quoted 8 lines

Quoted 4 lines

If the device is
asking for a blocksize > PAGE_SIZE and CONFIG_TRANSPARENT_HUGEPAGE is not set, you should also decline to mount the filesystem.

What does CONFIG_TRANSPARENT_HUGEPAGE have to do with filesystems being able to use large folios?

Quoted 8 lines

If that's an actual dependency of using large folios, then we're at the point where the mm side of large folios needs to be divorced from CONFIG_TRANSPARENT_HUGEPAGE and always supported. Alternatively, CONFIG_TRANSPARENT_HUGEPAGE needs to selected by the block layer and also every filesystem that wants to support sector/blocks sizes larger than PAGE_SIZE. IOWs, large folio support needs to *always* be enabled on systems that say CONFIG_BLOCK=y.

Why CONFIG_BLOCK? I think it is enough if it comes from the FS side right? And for now, the only FS that needs that sort of bs > ps guarantee is XFS with this series. Other filesystems such as bcachefs that call mapping_set_large_folios() only enable it as an optimization and it is not needed for the filesystem to function.

Quoted 3 lines

So this is my conclusion from the conversation:
- Add a dependency in Kconfig on THP for XFS until we fix the dependency of large folios on THP

THP isn't supported on some arches, so isn't this effectively saying XFS can no longer be used with those arches, even if the bs <= ps?

Quoted 44 lines · Ryan Roberts

Quoted 43 lines · Pankaj Raghav (Samsung)

Quoted 9 lines

Quoted 2 lines

I suggest you handle it better than this. If the device is asking for a blocksize > PMD_SIZE, you should fail to mount it.

That's my point: we already do that.

The largest block size we support is 64kB and that's way smaller than PMD_SIZE on all platforms and we always check for bs > ps support at mount time when the filesystem bs > ps.

Quoted 6 lines

Hence we're never going to set the min value to anything unsupported unless someone makes a massive programming mistake. At which point, we want a *hard, immediate fail* so the developer notices their mistake immediately. All filesystems and block devices need to behave this way so the limits should be encoded as asserts in the function to trigger such behaviour.

I agree, this kind of bug will be encountered only during developement and not during actual production due to the limit we have fs block size in XFS.

Quoted 8 lines

Quoted 4 lines

If the device is
asking for a blocksize > PAGE_SIZE and CONFIG_TRANSPARENT_HUGEPAGE is not set, you should also decline to mount the filesystem.

What does CONFIG_TRANSPARENT_HUGEPAGE have to do with filesystems being able to use large folios?

Quoted 8 lines

If that's an actual dependency of using large folios, then we're at the point where the mm side of large folios needs to be divorced from CONFIG_TRANSPARENT_HUGEPAGE and always supported. Alternatively, CONFIG_TRANSPARENT_HUGEPAGE needs to selected by the block layer and also every filesystem that wants to support sector/blocks sizes larger than PAGE_SIZE. IOWs, large folio support needs to *always* be enabled on systems that say CONFIG_BLOCK=y.

Why CONFIG_BLOCK? I think it is enough if it comes from the FS side right? And for now, the only FS that needs that sort of bs > ps guarantee is XFS with this series. Other filesystems such as bcachefs that call mapping_set_large_folios() only enable it as an optimization and it is not needed for the filesystem to function.

Quoted 6 lines

Quoted 3 lines

So this is my conclusion from the conversation:
- Add a dependency in Kconfig on THP for XFS until we fix the dependency of large folios on THP

THP isn't supported on some arches, so isn't this effectively saying XFS can no longer be used with those arches, even if the bs <= ps?

I'm good with that - we're already long past the point where we try to support XFS on every linux platform. Indeed, we've recent been musing about making XFS depend on 64 bit only - 32 bit systems don't have the memory capacity to run the full xfs tool chain (e.g. xfs_repair) on filesystems over about a TB in size, and they are greatly limited in kernel memory and vmap areas, both of which XFS makes heavy use of. Basically, friends don't let friends use XFS on 32 bit systems, and that's been true for about 20 years now.

Our problem is the test matrix - if we now have to explicitly test XFS both with and without large folios enabled to support these platforms, we've just doubled our test matrix. The test matrix is already far too large to robustly cover, so anything that requires doubling the number of kernel configs we have to test is, IMO, a non-starter.

That's why we really don't support XFS on 32 bit systems anymore and why we're talking about making that official with a config option. If we're at the point where XFS will now depend on large folios (i.e THP), then we need to seriously consider reducing the supported arches to just those that support both 64 bit and THP. If niche arches want to support THP, or enable large folios without the need for THP, then they can do that work and then they get XFS for free.

Just because an arch might run a Linux kernel, it doesn't mean we have to support XFS on it....

-Dave.

Quoted 7 lines

Quoted 7 lines

Why CONFIG_BLOCK? I think it is enough if it comes from the FS side right? And for now, the only FS that needs that sort of bs > ps guarantee is XFS with this series. Other filesystems such as bcachefs that call mapping_set_large_folios() only enable it as an optimization and it is not needed for the filesystem to function.

Quoted 6 lines

Quoted 3 lines

So this is my conclusion from the conversation:
- Add a dependency in Kconfig on THP for XFS until we fix the dependency of large folios on THP

THP isn't supported on some arches, so isn't this effectively saying XFS can no longer be used with those arches, even if the bs <= ps?

I'm good with that - we're already long past the point where we try

to support XFS on every linux platform. Indeed, we've recent been musing about making XFS depend on 64 bit only - 32 bit systems don't have the memory capacity to run the full xfs tool chain (e.g. xfs_repair) on filesystems over about a TB in size, and they are greatly limited in kernel memory and vmap areas, both of which XFS makes heavy use of. Basically, friends don't let friends use XFS on 32 bit systems, and that's been true for about 20 years now.

Our problem is the test matrix - if we now have to explicitly test XFS both with and without large folios enabled to support these platforms, we've just doubled our test matrix. The test matrix is already far too large to robustly cover, so anything that requires doubling the number of kernel configs we have to test is, IMO, a non-starter.

That's why we really don't support XFS on 32 bit systems anymore and why we're talking about making that official with a config option. If we're at the point where XFS will now depend on large folios (i.e THP), then we need to seriously consider reducing the supported arches to just those that support both 64 bit and THP. If niche arches want to support THP, or enable large folios without the need for THP, then they can do that work and then they get XFS for free.

Just because an arch might run a Linux kernel, it doesn't mean we have to support XFS on it....

Quoted 3 lines

If the device is
asking for a blocksize > PAGE_SIZE and CONFIG_TRANSPARENT_HUGEPAGE is not set, you should also decline to mount the filesystem.

What does CONFIG_TRANSPARENT_HUGEPAGE have to do with filesystems being able to use large folios?

Hi,

Here are some drive-by review comments as I'm evaluating whether these patches can help me with what I'm trying to do at
https://lore.kernel.org/linux-mm/[email protected]/...

Review static inline void mapping_set_folio_order_range(struct address_space *mapping, · line 401

Quoted 11 lines · Pankaj Raghav (Samsung)

+++ b/include/linux/pagemap.h

(quoted diff) +4 -1

@@ -394,13 +394,24 @@ static inline void mapping_set_folio_order_range(struct address_space *mapping,
                                                 unsigned int min,
                                                 unsigned int max)
 {
-       if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
+       if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
+               VM_WARN_ONCE(1, 
+       "THP needs to be enabled to support mapping folio order range");
                return;
+       }

No. Filesystems call mapping_set_folio_order_range() without it being conditional on CONFIG_TRANSPARENT_HUGEPAGE. Usually that takes the form of an unconditional call to mapping_set_large_folios().

For now, this is the only patch that is blocking for the next version.

Based on the discussion, is the following logical @ryan, @dave and @willy?

- We give explicit VM_WARN_ONCE if we try to set folio order range if the THP is disabled, min and max is greater than MAX_PAGECACHE_ORDER.

include/linux/pagemap.h +14 -4

index 14e1415f7dcf4..313c9fad61859 100644
@@ -394,13 +394,24 @@ static inline void mapping_set_folio_order_range(struct address_space *mapping,
                                                 unsigned int min,
                                                 unsigned int max)
 {
-       if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
+       if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
+               VM_WARN_ONCE(1, 
+       "THP needs to be enabled to support mapping folio order range");
                return;
+       }
 
-       if (min > MAX_PAGECACHE_ORDER)
+       if (min > MAX_PAGECACHE_ORDER) {
+               VM_WARN_ONCE(1, 
+       "min order > MAX_PAGECACHE_ORDER. Setting min_order to MAX_PAGECACHE_ORDER");
                min = MAX_PAGECACHE_ORDER;
-       if (max > MAX_PAGECACHE_ORDER)
+       }
+
+       if (max > MAX_PAGECACHE_ORDER) {
+               VM_WARN_ONCE(1, 
+       "max order > MAX_PAGECACHE_ORDER. Setting max_order to MAX_PAGECACHE_ORDER");
                max = MAX_PAGECACHE_ORDER;
+       }
+
        if (max < min)
                max = min;
 
- We make THP an explicit dependency for XFS:

fs/xfs/Kconfig +1 -0

index d41edd30388b7..be2c1c0e9fe8b 100644
@@ -5,6 +5,7 @@ config XFS_FS
        select EXPORTFS
        select LIBCRC32C
        select FS_IOMAP
+       select TRANSPARENT_HUGEPAGE
        help
          XFS is a high performance journaling filesystem which originated
          on the SGI IRIX platform.  It is completely multi-threaded, can

OR

We create a helper in page cache that FSs can use to check if a specific order can be supported at mount time:

include/linux/pagemap.h +1 -0

+
 
 

fs/xfs/xfs_super.c +0 -0

index b8a93a8f35cac..e2be8743c2c20 100644

fs/xfs/xfs_super.c +4 -1

+                       error = -ENOSYS;
+                       goto out_free_sb;
+               }
+
                xfs_warn(mp,
 "EXPERIMENTAL: V5 Filesystem with Large Block Size (%d bytes) enabled.",
                        mp->m_sb.sb_blocksize);
 
 
--

Pankaj

Quoted 16 lines

- We make THP an explicit dependency for XFS:

fs/xfs/Kconfig +1 -0

index d41edd30388b7..be2c1c0e9fe8b 100644
@@ -5,6 +5,7 @@ config XFS_FS
        select EXPORTFS
        select LIBCRC32C
        select FS_IOMAP
+       select TRANSPARENT_HUGEPAGE
        help
          XFS is a high performance journaling filesystem which originated
          on the SGI IRIX platform.  It is completely multi-threaded, can

Quoted 4 lines

OR

We create a helper in page cache that FSs can use to check if a specific order can be supported at mount time:

I like this solution better; if XFS is going to drop support for o[ld]d architectures I think we need /some/ sort of notice period. Or at least a better story than "we want to support 64k fsblocks on x64 so we're withdrawing support even for 4k fsblocks and smallish filesystems on m68k".

You probably don't want bs>ps support to block on some arcane discussion about 32-bit, right? ;)

include/linux/pagemap.h static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask) · line 381

Quoted 13 lines

include/linux/pagemap.h +5 -0

index 14e1415f7dcf..9be775ef11a5 100644
@@ -374,6 +374,14 @@ static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask)
 #define MAX_XAS_ORDER          (XA_CHUNK_SHIFT * 2 - 1)
 #define MAX_PAGECACHE_ORDER    min(MAX_XAS_ORDER, PREFERRED_MAX_PAGECACHE_ORDER)
 
+
+static inline unsigned int mapping_max_folio_order_supported()
+{
+    if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
+      return 0;

Shouldn't this line be indented by two tabs, not six spaces?

include/linux/pagemap.h static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask) · line 383

Quoted 2 lines

include/linux/pagemap.h +2 -0

+    return MAX_PAGECACHE_ORDER;
+}

Alternately, should this return the max folio size in bytes?

static inline size_t mapping_max_folio_size(void)

{
	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
		return 1U << (PAGE_SHIFT + MAX_PAGECACHE_ORDER);
	return PAGE_SIZE;
}

Then the validation looks like:

	const size_t	max_folio_size = mapping_max_folio_size();

	if (mp->m_sb.sb_blocksize > max_folio_size) {
		xfs_warn(mp,
 "block size (%u bytes) not supported; maximum folio size is %u.",
				mp->m_sb.sb_blocksize, max_folio_size);
		error = -ENOSYS;
		goto out_free_sb;
	}

(Don't mind me bikeshedding here.)

Quoted 11 lines

include/linux/pagemap.h +1 -0

+
 
 

fs/xfs/xfs_super.c +0 -0

index b8a93a8f35cac..e2be8743c2c20 100644
@@ -1647,6 +1647,15 @@ xfs_fs_fill_super(
                        goto out_free_sb;
                }
 

fs/xfs/xfs_super.c xfs_fs_fill_super( · line 1654

Quoted 5 lines

fs/xfs/xfs_super.c +5 -0

+               if (mp->m_sb.sb_blocklog - PAGE_SHIFT >
+                   mapping_max_folio_order_supported()) {
+                       xfs_warn(mp,
+"Block Size (%d bytes) is not supported. Check MAX_PAGECACHE_ORDER",
+                       mp->m_sb.sb_blocksize);

You might as well print MAX_PAGECACHE_ORDER here to make analysis easier on less-familiar architectures:

xfs_warn(mp,
"block size (%d bytes) is not supported; max folio size is %u.",

					mp->m_sb.sb_blocksize,
					1U << mapping_max_folio_order_supported());

(I wrote this comment first.)

--D

Quoted 11 lines

fs/xfs/xfs_super.c +4 -1

+                       error = -ENOSYS;
+                       goto out_free_sb;
+               }
+
                xfs_warn(mp,
 "EXPERIMENTAL: V5 Filesystem with Large Block Size (%d bytes) enabled.",
                        mp->m_sb.sb_blocksize);
 
 
--

Pankaj

Quoted 27 lines

Quoted 16 lines

- We make THP an explicit dependency for XFS:

fs/xfs/Kconfig +1 -0

index d41edd30388b7..be2c1c0e9fe8b 100644
@@ -5,6 +5,7 @@ config XFS_FS
        select EXPORTFS
        select LIBCRC32C
        select FS_IOMAP
+       select TRANSPARENT_HUGEPAGE
        help
          XFS is a high performance journaling filesystem which originated
          on the SGI IRIX platform.  It is completely multi-threaded, can

Quoted 4 lines

OR

We create a helper in page cache that FSs can use to check if a specific order can be supported at mount time:

I like this solution better; if XFS is going to drop support for o[ld]d architectures I think we need /some/ sort of notice period. Or at least a better story than "we want to support 64k fsblocks on x64 so we're withdrawing support even for 4k fsblocks and smallish filesystems on m68k".

Quoted 3 lines

You probably don't want bs>ps support to block on some arcane discussion about 32-bit, right? ;)

:)

Quoted 19 lines

include/linux/pagemap.h static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask) · line 381

Quoted 13 lines

include/linux/pagemap.h +5 -0

index 14e1415f7dcf..9be775ef11a5 100644
@@ -374,6 +374,14 @@ static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask)
 #define MAX_XAS_ORDER          (XA_CHUNK_SHIFT * 2 - 1)
 #define MAX_PAGECACHE_ORDER    min(MAX_XAS_ORDER, PREFERRED_MAX_PAGECACHE_ORDER)
 
+
+static inline unsigned int mapping_max_folio_order_supported()
+{
+    if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
+      return 0;

Shouldn't this line be indented by two tabs, not six spaces?

include/linux/pagemap.h static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask) · line 383

Quoted 2 lines

include/linux/pagemap.h +2 -0

+    return MAX_PAGECACHE_ORDER;
+}

Quoted 8 lines

Alternately, should this return the max folio size in bytes?

static inline size_t mapping_max_folio_size(void)

{
	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
		return 1U << (PAGE_SHIFT + MAX_PAGECACHE_ORDER);
	return PAGE_SIZE;
}

We already have mapping_max_folio_size(mapping) which returns the maximum folio order set for that mapping. So this could be called as mapping_max_folio_size_supported().

So we could just have mapping_max_folio_size_supported() instead of having mapping_max_folio_order_supported as you suggest.

Quoted 26 lines

Then the validation looks like:

	const size_t	max_folio_size = mapping_max_folio_size();

	if (mp->m_sb.sb_blocksize > max_folio_size) {
		xfs_warn(mp,
 "block size (%u bytes) not supported; maximum folio size is %u.",
				mp->m_sb.sb_blocksize, max_folio_size);
		error = -ENOSYS;
		goto out_free_sb;
	}

(Don't mind me bikeshedding here.)

Quoted 11 lines

include/linux/pagemap.h +1 -0

+
 
 

fs/xfs/xfs_super.c +0 -0

index b8a93a8f35cac..e2be8743c2c20 100644
@@ -1647,6 +1647,15 @@ xfs_fs_fill_super(
                        goto out_free_sb;
                }
 

Quoted 8 lines

fs/xfs/xfs_super.c xfs_fs_fill_super( · line 1654

Quoted 5 lines

fs/xfs/xfs_super.c +5 -0

+               if (mp->m_sb.sb_blocklog - PAGE_SHIFT >
+                   mapping_max_folio_order_supported()) {
+                       xfs_warn(mp,
+"Block Size (%d bytes) is not supported. Check MAX_PAGECACHE_ORDER",
+                       mp->m_sb.sb_blocksize);

You might as well print MAX_PAGECACHE_ORDER here to make analysis easier on less-familiar architectures:

Yes!

Quoted 22 lines

xfs_warn(mp,
"block size (%d bytes) is not supported; max folio size is %u.",

					mp->m_sb.sb_blocksize,
					1U << mapping_max_folio_order_supported());

(I wrote this comment first.)

--D

Quoted 11 lines

fs/xfs/xfs_super.c +4 -1

+                       error = -ENOSYS;
+                       goto out_free_sb;
+               }
+
                xfs_warn(mp,
 "EXPERIMENTAL: V5 Filesystem with Large Block Size (%d bytes) enabled.",
                        mp->m_sb.sb_blocksize);
 
 
--

Pankaj

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

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

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

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Darrick J. Wong <[email protected]>

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

Quoted 5 lines · Pankaj Raghav (Samsung)

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

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

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

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Darrick J. Wong <[email protected]>

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

after fixing the nits below

Quoted 3 lines

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

+ * mapping_align_index - Align index for this mapping.

include/linux/pagemap.h static inline vm_fault_t folio_lock_or_retry(struct folio *folio, · line 1192

Quoted 9 lines

include/linux/pagemap.h +1 -1

@@ -1165,7 +1186,7 @@ static inline vm_fault_t folio_lock_or_retry(struct folio *folio,
 void folio_wait_bit(struct folio *folio, int bit_nr);
 int folio_wait_bit_killable(struct folio *folio, int bit_nr);
 
-/* 
+/*
  * Wait for a folio to be unlocked.
  *
  * This must be called with the caller "holding" the folio,

Unnecessary whitespace change

From: Pankaj Raghav <[email protected]>

page_cache_ra_unbounded() was allocating single pages (0 order folios) if there was no folio found in an index. Allocate mapping_min_order folios as we need to guarantee the minimum order if it is set.
While we are at it, rework the loop in page_cache_ra_unbounded() to advance with the number of pages in a folio instead of just one page at a time.

page_cache_ra_order() tries to allocate folio to the page cache with a higher order if the index aligns with that order. Modify it so that the order does not go below the mapping_min_order requirement of the page cache. This function will do the right thing even if the new_order passed is less than the 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.

readahead_expand() is called from readahead aops to extend the range of the readahead so this function can assume ractl->_index to be aligned with min_order.

mm/readahead.c +19 -9

+
 	/*
 	 * Preallocate as many pages as we will need.
 	 */
-	for (i = 0; i < nr_to_read; i++) {
+	while (i < nr_to_read) {
 		struct folio *folio = xa_load(&mapping->i_pages, index + i);
 		int ret;
 

@@ -240,12 +257,13 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 			 * not worth getting one just for that.
 			 */
 			read_pages(ractl);
-			ractl->_index++;
-			i = ractl->_index + ractl->_nr_pages - index - 1;
+			ractl->_index += min_nrpages;
+			i = ractl->_index + ractl->_nr_pages - index;
 			continue;
 		}
 
-		folio = filemap_alloc_folio(gfp_mask, 0);
+		folio = filemap_alloc_folio(gfp_mask,
+					    mapping_min_folio_order(mapping));
 		if (!folio)
 			break;
 

@@ -255,14 +273,15 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 			if (ret == -ENOMEM)
 				break;
 			read_pages(ractl);
-			ractl->_index++;
-			i = ractl->_index + ractl->_nr_pages - index - 1;
+			ractl->_index += min_nrpages;
+			i = ractl->_index + ractl->_nr_pages - index;
 			continue;
 		}
-		if (i == nr_to_read - lookahead_size)
+		if (i == mark)
 			folio_set_readahead(folio);
 		ractl->_workingset |= folio_test_workingset(folio);
-		ractl->_nr_pages++;
+		ractl->_nr_pages += min_nrpages;
+		i += min_nrpages;
 	}
 
 	/*

@@ -492,13 +511,19 @@ void page_cache_ra_order(struct readahead_control *ractl,
 {
 	struct address_space *mapping = ractl->mapping;
 	pgoff_t index = readahead_index(ractl);
+	unsigned int min_order = mapping_min_folio_order(mapping);
 	pgoff_t limit = (i_size_read(mapping->host) - 1) >> PAGE_SHIFT;
 	pgoff_t mark = index + ra->size - ra->async_size;
 	unsigned int nofs;
 	int err = 0;
 	gfp_t gfp = readahead_gfp_mask(mapping);
+	unsigned int min_ra_size = max(4, mapping_min_folio_nrpages(mapping));
 
-	if (!mapping_large_folio_support(mapping) || ra->size < 4)
+	/*
+	 * Fallback when size < min_nrpages as each folio should be
+	 * at least min_nrpages anyway.
+	 */
+	if (!mapping_large_folio_support(mapping) || ra->size < min_ra_size)
 		goto fallback;
 
 	limit = min(limit, index + ra->size - 1);

@@ -507,11 +532,20 @@ void page_cache_ra_order(struct readahead_control *ractl,
 		new_order += 2;
 		new_order = min(mapping_max_folio_order(mapping), new_order);
 		new_order = min_t(unsigned int, new_order, ilog2(ra->size));
+		new_order = max(new_order, min_order);
 	}
 

mm/readahead.c +14 -3

 		err = ra_alloc_folio(ractl, index, mark, order, gfp);
 		if (err)

@@ -783,8 +817,15 @@ void readahead_expand(struct readahead_control *ractl,
 	struct file_ra_state *ra = ractl->ra;
 	pgoff_t new_index, new_nr_pages;
 	gfp_t gfp_mask = readahead_gfp_mask(mapping);
+	unsigned long min_nrpages = mapping_min_folio_nrpages(mapping);
+	unsigned int min_order = mapping_min_folio_order(mapping);
 
 	new_index = new_start / PAGE_SIZE;
+	/*
+	 * Readahead code should have aligned the ractl->_index to
+	 * min_nrpages before calling readahead aops.
+	 */
+	VM_BUG_ON(!IS_ALIGNED(ractl->_index, min_nrpages));
 
 	/* Expand the leading edge downwards */
 	while (ractl->_index > new_index) {

@@ -794,9 +835,11 @@ void readahead_expand(struct readahead_control *ractl,
 		if (folio && !xa_is_value(folio))
 			return; /* Folio apparently present */
 
-		folio = filemap_alloc_folio(gfp_mask, 0);
+		folio = filemap_alloc_folio(gfp_mask, min_order);
 		if (!folio)
 			return;
+
+		index = mapping_align_index(mapping, index);
 		if (filemap_add_folio(mapping, folio, index, gfp_mask) < 0) {
 			folio_put(folio);
 			return;

@@ -806,7 +849,7 @@ void readahead_expand(struct readahead_control *ractl,
 			ractl->_workingset = true;
 			psi_memstall_enter(&ractl->_pflags);
 		}
-		ractl->_nr_pages++;
+		ractl->_nr_pages += min_nrpages;
 		ractl->_index = folio->index;
 	}
 

@@ -821,9 +864,11 @@ void readahead_expand(struct readahead_control *ractl,
 		if (folio && !xa_is_value(folio))
 			return; /* Folio apparently present */
 
-		folio = filemap_alloc_folio(gfp_mask, 0);
+		folio = filemap_alloc_folio(gfp_mask, min_order);
 		if (!folio)
 			return;
+
+		index = mapping_align_index(mapping, index);
 		if (filemap_add_folio(mapping, folio, index, gfp_mask) < 0) {
 			folio_put(folio);
 			return;

mm/readahead.c +0 -0

 		}
 	}
 }

Signature · 1 line

2.44.1

Quoted 8 lines · Pankaj Raghav (Samsung)

From: Pankaj Raghav <[email protected]>

page_cache_ra_unbounded() was allocating single pages (0 order folios) if there was no folio found in an index. Allocate mapping_min_order folios as we need to guarantee the minimum order if it is set.
While we are at it, rework the loop in page_cache_ra_unbounded() to advance with the number of pages in a folio instead of just one page at a time.

Ok, sounds pretty straightforward so far.

Quoted 5 lines

page_cache_ra_order() tries to allocate folio to the page cache with a higher order if the index aligns with that order. Modify it so that the order does not go below the mapping_min_order requirement of the page cache. This function will do the right thing even if the new_order passed is less than the mapping_min_order.

Hmm. So if I'm understanding this correctly: Currently, page_cache_ra_order tries to allocate higher order folios if the readahead index happens to be aligned to one of those higher orders. With the minimum mapping order requirement, it now expands the readahead range upwards and downwards to maintain the mapping_min_order requirement. Right?

Quoted 7 lines

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.

readahead_expand() is called from readahead aops to extend the range of the readahead so this function can assume ractl->_index to be aligned with min_order.

...and I guess this function also has to be modified to expand the ra range even further if necessary to align with mapping_min_order. Right?

Quoted 23 lines

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

Co-developed-by Hannes Reinecke <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 mm/readahead.c | 81 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 63 insertions(+), 18 deletions(-)

mm/readahead.c +3 -2

index 66058ae02f2e..2acfd6447d7b 100644
@@ -206,9 +206,10 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 		unsigned long nr_to_read, unsigned long lookahead_size)
 {
 	struct address_space *mapping = ractl->mapping;
-	unsigned long index = readahead_index(ractl);
+	unsigned long ra_folio_index, index = readahead_index(ractl);
 	gfp_t gfp_mask = readahead_gfp_mask(mapping);
-	unsigned long i;
+	unsigned long mark, i = 0;
+	unsigned int min_nrpages = mapping_min_folio_nrpages(mapping);
 

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

Quoted 3 lines

mm/readahead.c +0 -0

 	/*
 	 * Partway through the readahead operation, we will have added

@@ -223,10 +224,26 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,

I'm not as familiar with this function since xfs/iomap don't use it.

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

Quoted 13 lines

mm/readahead.c +10 -0

 	unsigned int nofs = memalloc_nofs_save();
 
 	filemap_invalidate_lock_shared(mapping);
+	index = mapping_align_index(mapping, index);
+
+	/*
+	 * As iterator `i` is aligned to min_nrpages, round_up the
+	 * difference between nr_to_read and lookahead_size to mark the
+	 * index that only has lookahead or "async_region" to set the
+	 * readahead flag.
+	 */
+	ra_folio_index = round_up(readahead_index(ractl) + nr_to_read - lookahead_size,
+				  min_nrpages);

So at this point we've rounded index down and the readahead region up to fit the min_nrpages requirement. I'm not sure what the lookahead region does, since nobody passes nonzero. Judging from the other functions, I guess that's the region that we're allowed to do asynchronously?

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

Quoted 1 line

mm/readahead.c +1 -0

+	mark = ra_folio_index - index;

Ah, ok, yes. We mark the first folio in the "async" region so that we (re)start readahead when someone accesses that folio.

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

Quoted 5 lines

mm/readahead.c +4 -0

+	if (index != readahead_index(ractl)) {
+		nr_to_read += readahead_index(ractl) - index;
+		ractl->_index = index;
+	}

So then if we rounded inded down, now we have to add that to the ra region.

Quoted 14 lines

mm/readahead.c +3 -0

+
 	/*
+	if (!mapping_large_folio_support(mapping) || ra->size < min_ra_size)
 		goto fallback;
 
 	limit = min(limit, index + ra->size - 1);

@@ -507,11 +532,20 @@ void page_cache_ra_order(struct readahead_control *ractl,
 		new_order += 2;
 		new_order = min(mapping_max_folio_order(mapping), new_order);
 		new_order = min_t(unsigned int, new_order, ilog2(ra->size));
+		new_order = max(new_order, min_order);
 	}
 

mm/readahead.c void page_cache_ra_order(struct readahead_control *ractl, · line 547

Quoted 10 lines

mm/readahead.c +7 -0

 	/* See comment in page_cache_ra_unbounded() */
 	nofs = memalloc_nofs_save();
 	filemap_invalidate_lock_shared(mapping);
+	/*
+	 * If the new_order is greater than min_order and index is
+	 * already aligned to new_order, then this will be noop as index
+	 * aligned to new_order should also be aligned to min_order.
+	 */
+	ractl->_index = mapping_align_index(mapping, index);
+	index = readahead_index(ractl);

I guess this also rounds index down to mapping_min_order...

Quoted 5 lines

mm/readahead.c +1 -0

+
 	while (index <= limit) {
 		unsigned int order = new_order;
 

mm/readahead.c void page_cache_ra_order(struct readahead_control *ractl, · line 557

Quoted 7 lines

mm/readahead.c +1 -1

@@ -519,7 +553,7 @@ void page_cache_ra_order(struct readahead_control *ractl,
 		if (index & ((1UL << order) - 1))
 			order = __ffs(index);
 		/* Don't allocate pages past EOF */
-		while (index + (1UL << order) - 1 > limit)
+		while (order > min_order && index + (1UL << order) - 1 > limit)
 			order--;

...and then we try to find an order that works and doesn't go below min_order. We already rounded index down to mapping_min_order, so that will always succeed. Right?

Quoted 24 lines

mm/readahead.c +4 -2

 		err = ra_alloc_folio(ractl, index, mark, order, gfp);
 		if (err)

@@ -821,9 +864,11 @@ void readahead_expand(struct readahead_control *ractl,
 		if (folio && !xa_is_value(folio))
 			return; /* Folio apparently present */
 
-		folio = filemap_alloc_folio(gfp_mask, 0);
+		folio = filemap_alloc_folio(gfp_mask, min_order);
 		if (!folio)
 			return;
+
+		index = mapping_align_index(mapping, index);
 		if (filemap_add_folio(mapping, folio, index, gfp_mask) < 0) {
 			folio_put(folio);
 			return;

@@ -833,10 +878,10 @@ void readahead_expand(struct readahead_control *ractl,
 			ractl->_workingset = true;
 			psi_memstall_enter(&ractl->_pflags);
 		}
-		ractl->_nr_pages++;
+		ractl->_nr_pages += min_nrpages;
 		if (ra) {

mm/readahead.c void readahead_expand(struct readahead_control *ractl, · line 884

Quoted 4 lines

mm/readahead.c +2 -2

-			ra->size++;
-			ra->async_size++;
+			ra->size += min_nrpages;
+			ra->async_size += min_nrpages;

...and here we are expanding the ra window yet again, only now taking min order into account. Right? Looks ok to me, though again, iomap/xfs don't use this function so I'm not that familiar with it.

If the answer to /all/ the questions is 'yes' then

Acked-by Darrick J. Wong <[email protected]>

From: Pankaj Raghav <[email protected]>

page_cache_ra_unbounded() was allocating single pages (0 order folios) if there was no folio found in an index. Allocate mapping_min_order folios as we need to guarantee the minimum order if it is set.
While we are at it, rework the loop in page_cache_ra_unbounded() to advance with the number of pages in a folio instead of just one page at a time.

page_cache_ra_order() tries to allocate folio to the page cache with a higher order if the index aligns with that order. Modify it so that the order does not go below the mapping_min_order requirement of the page cache. This function will do the right thing even if the new_order passed is less than the 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.

readahead_expand() is called from readahead aops to extend the range of the readahead so this function can assume ractl->_index to be aligned with min_order.

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

Co-developed-by Hannes Reinecke <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 mm/readahead.c | 81 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 63 insertions(+), 18 deletions(-)

mm/readahead.c +20 -3

index 66058ae02f2e..2acfd6447d7b 100644
@@ -206,9 +206,10 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 		unsigned long nr_to_read, unsigned long lookahead_size)
 {
 	struct address_space *mapping = ractl->mapping;
-	unsigned long index = readahead_index(ractl);
+	unsigned long ra_folio_index, index = readahead_index(ractl);
 	gfp_t gfp_mask = readahead_gfp_mask(mapping);
-	unsigned long i;
+	unsigned long mark, i = 0;
+	unsigned int min_nrpages = mapping_min_folio_nrpages(mapping);
 
 	/*
 	 * Partway through the readahead operation, we will have added

@@ -223,10 +224,26 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 	unsigned int nofs = memalloc_nofs_save();
 
 	filemap_invalidate_lock_shared(mapping);
+	index = mapping_align_index(mapping, index);
+
+	/*
+	 * As iterator `i` is aligned to min_nrpages, round_up the
+	 * difference between nr_to_read and lookahead_size to mark the
+	 * index that only has lookahead or "async_region" to set the
+	 * readahead flag.
+	 */
+	ra_folio_index = round_up(readahead_index(ractl) + nr_to_read - lookahead_size,
+				  min_nrpages);
+	mark = ra_folio_index - index;
+	if (index != readahead_index(ractl)) {
+		nr_to_read += readahead_index(ractl) - index;
+		ractl->_index = index;
+	}
+
 	/*
 	 * Preallocate as many pages as we will need.
 	 */
-	for (i = 0; i < nr_to_read; i++) {
+	while (i < nr_to_read) {
 		struct folio *folio = xa_load(&mapping->i_pages, index + i);
 		int ret;
 

mm/readahead.c +43 -15

 			read_pages(ractl);
-			ractl->_index++;
-			i = ractl->_index + ractl->_nr_pages - index - 1;
+			ractl->_index += min_nrpages;
+			i = ractl->_index + ractl->_nr_pages - index;
 			continue;
 		}
 
-		folio = filemap_alloc_folio(gfp_mask, 0);
+		folio = filemap_alloc_folio(gfp_mask,
+					    mapping_min_folio_order(mapping));
 		if (!folio)
 			break;
 

@@ -255,14 +273,15 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 			if (ret == -ENOMEM)
 				break;
 			read_pages(ractl);
-			ractl->_index++;
-			i = ractl->_index + ractl->_nr_pages - index - 1;
+			ractl->_index += min_nrpages;
+			i = ractl->_index + ractl->_nr_pages - index;
 			continue;
 		}
-		if (i == nr_to_read - lookahead_size)
+		if (i == mark)
 			folio_set_readahead(folio);
 		ractl->_workingset |= folio_test_workingset(folio);
-		ractl->_nr_pages++;
+		ractl->_nr_pages += min_nrpages;
+		i += min_nrpages;
 	}
 
 	/*

@@ -492,13 +511,19 @@ void page_cache_ra_order(struct readahead_control *ractl,
 {
 	struct address_space *mapping = ractl->mapping;
 	pgoff_t index = readahead_index(ractl);
+	unsigned int min_order = mapping_min_folio_order(mapping);
 	pgoff_t limit = (i_size_read(mapping->host) - 1) >> PAGE_SHIFT;
 	pgoff_t mark = index + ra->size - ra->async_size;
 	unsigned int nofs;
 	int err = 0;
 	gfp_t gfp = readahead_gfp_mask(mapping);
+	unsigned int min_ra_size = max(4, mapping_min_folio_nrpages(mapping));
 
-	if (!mapping_large_folio_support(mapping) || ra->size < 4)
+	/*
+	 * Fallback when size < min_nrpages as each folio should be
+	 * at least min_nrpages anyway.
+	 */
+	if (!mapping_large_folio_support(mapping) || ra->size < min_ra_size)
 		goto fallback;
 
 	limit = min(limit, index + ra->size - 1);

@@ -507,11 +532,20 @@ void page_cache_ra_order(struct readahead_control *ractl,
 		new_order += 2;
 		new_order = min(mapping_max_folio_order(mapping), new_order);
 		new_order = min_t(unsigned int, new_order, ilog2(ra->size));
+		new_order = max(new_order, min_order);
 	}
 
 	/* See comment in page_cache_ra_unbounded() */
 	nofs = memalloc_nofs_save();
 	filemap_invalidate_lock_shared(mapping);
+	/*
+	 * If the new_order is greater than min_order and index is
+	 * already aligned to new_order, then this will be noop as index
+	 * aligned to new_order should also be aligned to min_order.
+	 */
+	ractl->_index = mapping_align_index(mapping, index);
+	index = readahead_index(ractl);
+
 	while (index <= limit) {
 		unsigned int order = new_order;
 

@@ -519,7 +553,7 @@ void page_cache_ra_order(struct readahead_control *ractl,
 		if (index & ((1UL << order) - 1))
 			order = __ffs(index);
 		/* Don't allocate pages past EOF */
-		while (index + (1UL << order) - 1 > limit)
+		while (order > min_order && index + (1UL << order) - 1 > limit)
 			order--;
 		err = ra_alloc_folio(ractl, index, mark, order, gfp);
 		if (err)

@@ -783,8 +817,15 @@ void readahead_expand(struct readahead_control *ractl,
 	struct file_ra_state *ra = ractl->ra;
 	pgoff_t new_index, new_nr_pages;
 	gfp_t gfp_mask = readahead_gfp_mask(mapping);
+	unsigned long min_nrpages = mapping_min_folio_nrpages(mapping);
+	unsigned int min_order = mapping_min_folio_order(mapping);
 
 	new_index = new_start / PAGE_SIZE;
+	/*
+	 * Readahead code should have aligned the ractl->_index to
+	 * min_nrpages before calling readahead aops.
+	 */
+	VM_BUG_ON(!IS_ALIGNED(ractl->_index, min_nrpages));
 
 	/* Expand the leading edge downwards */
 	while (ractl->_index > new_index) {

@@ -794,9 +835,11 @@ void readahead_expand(struct readahead_control *ractl,
 		if (folio && !xa_is_value(folio))
 			return; /* Folio apparently present */
 
-		folio = filemap_alloc_folio(gfp_mask, 0);
+		folio = filemap_alloc_folio(gfp_mask, min_order);
 		if (!folio)
 			return;
+
+		index = mapping_align_index(mapping, index);
 		if (filemap_add_folio(mapping, folio, index, gfp_mask) < 0) {
 			folio_put(folio);
 			return;

@@ -806,7 +849,7 @@ void readahead_expand(struct readahead_control *ractl,
 			ractl->_workingset = true;
 			psi_memstall_enter(&ractl->_pflags);
 		}
-		ractl->_nr_pages++;
+		ractl->_nr_pages += min_nrpages;
 		ractl->_index = folio->index;
 	}
 

@@ -821,9 +864,11 @@ void readahead_expand(struct readahead_control *ractl,
 		if (folio && !xa_is_value(folio))
 			return; /* Folio apparently present */
 
-		folio = filemap_alloc_folio(gfp_mask, 0);
+		folio = filemap_alloc_folio(gfp_mask, min_order);
 		if (!folio)
 			return;
+
+		index = mapping_align_index(mapping, index);
 		if (filemap_add_folio(mapping, folio, index, gfp_mask) < 0) {
 			folio_put(folio);
 			return;

@@ -833,10 +878,10 @@ void readahead_expand(struct readahead_control *ractl,
 			ractl->_workingset = true;
 			psi_memstall_enter(&ractl->_pflags);
 		}
-		ractl->_nr_pages++;
+		ractl->_nr_pages += min_nrpages;
 		if (ra) {
-			ra->size++;
-			ra->async_size++;
+			ra->size += min_nrpages;
+			ra->async_size += min_nrpages;
 		}
 	}
 }

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

Quoted 3 lines

mm/readahead.c +0 -0

@@ -240,12 +257,13 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 			 * not worth getting one just for that.
 			 */

For the case that the folio is already in the xarray, perhaps its worth asserting that the folio is at least min_nrpages?

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]>

Reviewed-by Hannes Reinecke <[email protected]>

---
There was a discussion about whether we need to consider truncation of folio to be considered a split failure or not [1]. The new code has retained the existing behaviour of returning a failure if the folio was truncated. I think we need to have a separate discussion whethere or not to consider it as a failure.

mm/huge_memory.c +8 -0

+			return -EBUSY;
+		}
+		min_order = mapping_min_folio_order(folio->mapping);
+	}
+
+	return split_huge_page_to_list_to_order(&folio->page, list, min_order);
+}
+

Quoted 23 lines · Pankaj Raghav (Samsung)

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]>

Reviewed-by Hannes Reinecke <[email protected]>

---
There was a discussion about whether we need to consider truncation of folio to be considered a split failure or not [1]. The new code has retained the existing behaviour of returning a failure if the folio was truncated. I think we need to have a separate discussion whethere or not to consider it as a failure.

<snip>

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

Quoted 8 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);

Regardless this folio split is from a truncation or not, you should not count every folio split as a THP_SPLIT_PAGE_FAILED. Since not every folio is a THP. You need to do:

if (folio_test_pmd_mappable(folio))

	count_vm_event(THP_SPLIT_PAGE_FAILED);

See commit 835c3a25aa37 ("mm: huge_memory: add the missing folio_test_pmd_mappable() for THP split statistics")

Quoted 9 lines

mm/huge_memory.c +8 -0

+			return -EBUSY;
+		}
+		min_order = mapping_min_folio_order(folio->mapping);
+	}
+
+	return split_huge_page_to_list_to_order(&folio->page, list, min_order);
+}
+

Signature · 2 lines

Best Regards,
Yan, Zi

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. While doing a read, do_fault_around() can create PTEs for pages that lie beyond the EOF leading to incorrect error return when accessing a page beyond the mapped file.

Cap the PTE range to be created for the page cache up to the end of file(EOF) in filemap_map_pages() so that return error codes are consistent with POSIX[1] for LBS configurations.

generic/749(currently in xfstest-dev patches-in-queue branch [0]) has been created to trigger this edge case. This also fixes generic/749 for tmpfs with huge=always on systems with 4k base page size.

[0] https://lore.kernel.org/all/[email protected]/
[1](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.

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

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

Reviewed-by Hannes Reinecke <[email protected]>

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

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

mm/filemap.c +5 -1

index 8eafbd4a4d0c..56ff1d936aa8 100644
@@ -3612,7 +3612,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;

@@ -3638,6 +3638,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

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/iomap/direct-io.c +6 -0

+/*
+ * Used for sub block zeroing in iomap_dio_zero()
+ */
+#define ZERO_PAGE_64K_SIZE (65536)
+#define ZERO_PAGE_64K_ORDER (get_order(ZERO_PAGE_64K_SIZE))
+static struct page *zero_page_64k;

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

@@ -753,3 +765,17 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	return iomap_dio_complete(dio);
 }
 EXPORT_SYMBOL_GPL(iomap_dio_rw);
+

Quoted 13 lines · Pankaj Raghav (Samsung)

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.

Quoted 2 lines

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

Reviewed-by Hannes Reinecke <[email protected]>

Looks fine, so:

Reviewed-by Dave Chinner <[email protected]>

but....

fs/iomap/direct-io.c iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, · line 778

Quoted 6 lines

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

+
+	if (!zero_page_64k)
+		return -ENOMEM;
+
+	set_memory_ro((unsigned long)page_address(zero_page_64k),
+		      1U << ZERO_PAGE_64K_ORDER);

                      ^^^^^^^^^^^^^^^^^^^^^^^^^

isn't that just ZERO_PAGE_64K_SIZE?

-Dave.

Signature · 2 lines

Dave Chinner
[email protected]

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/iomap/buffered-io.c |  4 ++--
 fs/iomap/direct-io.c   | 30 ++++++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 4 deletions(-)

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

index f420c53d86ac..9a9e94c7ed1d 100644
@@ -2007,10 +2007,10 @@ iomap_writepages(struct address_space *mapping, struct writeback_control *wbc,
 }
 EXPORT_SYMBOL_GPL(iomap_writepages);
 
-static int __init iomap_init(void)
+static int __init iomap_pagecache_init(void)
 {
 	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
 			   offsetof(struct iomap_ioend, io_bio),
 			   BIOSET_NEED_BVECS);
 }
-fs_initcall(iomap_init);
+fs_initcall(iomap_pagecache_init);

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

index f3b43d223a46..61d09d2364f7 100644
@@ -11,6 +11,7 @@
 #include <linux/iomap.h>
 #include <linux/backing-dev.h>
 #include <linux/uio.h>
+#include <linux/set_memory.h>
 #include <linux/task_io_accounting_ops.h>
 #include "trace.h"
 

@@ -27,6 +28,13 @@
 #define IOMAP_DIO_WRITE		(1U << 30)
 #define IOMAP_DIO_DIRTY		(1U << 31)
 
+/*
+ * Used for sub block zeroing in iomap_dio_zero()
+ */
+#define ZERO_PAGE_64K_SIZE (65536)
+#define ZERO_PAGE_64K_ORDER (get_order(ZERO_PAGE_64K_SIZE))
+static struct page *zero_page_64k;
+
 struct iomap_dio {
 	struct kiocb		*iocb;
 	const struct iomap_dio_ops *dops;

@@ -236,9 +244,13 @@ 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_PAGE_64K_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);

@@ -246,7 +258,7 @@ static void iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio,
 	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_page_64k, len, 0);
 	iomap_dio_submit_bio(iter, dio, bio, pos);
 }
 

@@ -753,3 +765,17 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	return iomap_dio_complete(dio);
 }
 EXPORT_SYMBOL_GPL(iomap_dio_rw);
+
+static int __init iomap_dio_init(void)
+{
+	zero_page_64k = alloc_pages(GFP_KERNEL | __GFP_ZERO,
+				    ZERO_PAGE_64K_ORDER);
+
+	if (!zero_page_64k)
+		return -ENOMEM;
+
+	set_memory_ro((unsigned long)page_address(zero_page_64k),
+		      1U << ZERO_PAGE_64K_ORDER);
+	return 0;
+}
+fs_initcall(iomap_dio_init);

Signature · 1 line

2.44.1

+#define ZERO_PAGE_64K_ORDER (get_order(ZERO_PAGE_64K_SIZE))

+static struct page *zero_page_64k;

Quoted 1 line

+fs_initcall(iomap_pagecache_init);

s/iomap_pagecache_init/iomap_buffered_init/

We don't use pagecache naming anywhere else in the file.

fs/iomap/direct-io.c line 34

Quoted 5 lines

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

+/*
+ * Used for sub block zeroing in iomap_dio_zero()
+ */
+#define ZERO_PAGE_64K_SIZE (65536)

just use SZ_64K

Quoted 1 line

+#define ZERO_PAGE_64K_ORDER (get_order(ZERO_PAGE_64K_SIZE))

No really point in having this.

Quoted 2 lines

+static struct page *zero_page_64k;

This should be a folio. Encoding the size in the name is also really weird and just creates churn when we have to increase it.

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

Quoted 6 lines

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

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

A WARN_ON without actually erroring out here is highly dangerous.

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

Quoted 3 lines

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

+
 	bio = iomap_dio_alloc_bio(iter, dio, 1, REQ_OP_WRITE | REQ_SYNC | REQ_IDLE);

Overly long line here.