enable bs > ps in XFS

include/linux/pagemap.h +14 -0

+/*
+ * mapping_set_folio_min_order() - Set the minimum folio order
+ * @mapping: The address_space.
+ * @min: Minimum folio order (between 0-MAX_PAGECACHE_ORDER inclusive).
+ *
+ * The filesystem should call this function in its inode constructor to
+ * indicate which base size 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.
+ */

Quoted 14 lines · Pankaj Raghav (Samsung)

include/linux/pagemap.h +14 -0

+/*
+ * mapping_set_folio_min_order() - Set the minimum folio order
+ * @mapping: The address_space.
+ * @min: Minimum folio order (between 0-MAX_PAGECACHE_ORDER inclusive).
+ *
+ * The filesystem should call this function in its inode constructor to
+ * indicate which base size 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.
+ */

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

Quoted 10 lines

include/linux/pagemap.h +10 -0

+static inline void mapping_set_folio_min_order(struct address_space *mapping,
+					       unsigned int min)
+{
+	if (min > MAX_PAGECACHE_ORDER)
+		min = MAX_PAGECACHE_ORDER;
+
+	mapping->flags = (mapping->flags & ~AS_FOLIO_ORDER_MASK) |
+			 (min << AS_FOLIO_ORDER_MIN) |
+			 (MAX_PAGECACHE_ORDER << AS_FOLIO_ORDER_MAX);
+}

I was surprised when I read this, which indicates it might be surprising for others too. I think it at least needs a comment to say that the maximum will be set to the MAX_PAGECACHE_ORDER, because I was expecting it to set max == min. I guess that isn't what XFS wants, but someone doing this to, eg, ext4 is going to have an unpleasant surprise when they call into block_read_full_folio() and overrun 'arr'.

I'm still not entirely convinced this wouldn't be better to do as mapping_set_folio_order_range() and have

static inline void mapping_set_folio_min_order(struct address_space *mapping,
unsigned int min)

{
	mapping_set_folio_range(mapping, min, MAX_PAGECACHE_ORDER);
}

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.

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

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

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

---
 mm/filemap.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

mm/filemap.c +17 -7

index a1cb3ea55fb6..57889f206829 100644
@@ -849,6 +849,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);
 
 	if (!huge) {

@@ -1886,8 +1888,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;

@@ -1927,7 +1931,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;

@@ -2416,13 +2420,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;
 

@@ -2440,6 +2447,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)

@@ -2500,8 +2509,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;

@@ -3662,9 +3670,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.43.0

mm/readahead.c +2 -2

@@ -239,8 +239,8 @@ 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 += folio_nr_pages(folio);
+			i = ractl->_index + ractl->_nr_pages - index;
 			continue;
 		}
 

Quoted 11 lines · Pankaj Raghav (Samsung)

mm/readahead.c +2 -2

@@ -239,8 +239,8 @@ 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 += folio_nr_pages(folio);
+			i = ractl->_index + ractl->_nr_pages - index;
 			continue;
 		}
 

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

Quoted 8 lines

mm/readahead.c +1 -1

@@ -252,13 +252,14 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 			folio_put(folio);
 			read_pages(ractl);
 			ractl->_index++;
-			i = ractl->_index + ractl->_nr_pages - index - 1;
+			i = ractl->_index + ractl->_nr_pages - index;
 			continue;
 		}

You changed index++ in the first hunk, but not the second hunk. Is that intentional?

Hah, you changed this here. Please move into previous patch.

Quoted 11 lines · Pankaj Raghav (Samsung)

mm/readahead.c +2 -2

@@ -239,8 +239,8 @@ 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 += folio_nr_pages(folio);
+			i = ractl->_index + ractl->_nr_pages - index;
  			continue;
  		}
 

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

Quoted 8 lines

mm/readahead.c +1 -1

@@ -252,13 +252,14 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
  			folio_put(folio);
  			read_pages(ractl);
  			ractl->_index++;
-			i = ractl->_index + ractl->_nr_pages - index - 1;
+			i = ractl->_index + ractl->_nr_pages - index;
  			continue;
  		}

You changed index++ in the first hunk, but not the second hunk. Is that intentional?

Quoted 14 lines · Matthew Wilcox

Quoted 13 lines · Pankaj Raghav (Samsung)

mm/readahead.c +2 -2

@@ -239,8 +239,8 @@ 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 += folio_nr_pages(folio);
+            i = ractl->_index + ractl->_nr_pages - index;
               continue;
           }
   @@ -252,13 +252,14 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,

              folio_put(folio);
              read_pages(ractl);

Quoted 8 lines

Quoted 5 lines

              ractl->_index++;
-            i = ractl->_index + ractl->_nr_pages - index - 1; +            i = ractl->_index + ractl->_nr_pages - index; continue;
          }

You changed index++ in the first hunk, but not the second hunk.  Is that intentional?

Hmm. Looks you are right; it should be modified, too. Will be fixing it up.

Cheers,

Hannes

Hi Hannes,

Quoted 24 lines · Hannes Reinecke

Quoted 14 lines · Matthew Wilcox

Quoted 13 lines · Pankaj Raghav (Samsung)

mm/readahead.c +2 -2

@@ -239,8 +239,8 @@ 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 += folio_nr_pages(folio);
+            i = ractl->_index + ractl->_nr_pages - index;
               continue;
           }
   @@ -252,13 +252,14 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,

              folio_put(folio);
              read_pages(ractl);

Quoted 8 lines

Quoted 5 lines

              ractl->_index++;
-            i = ractl->_index + ractl->_nr_pages - index - 1; +            i = ractl->_index + ractl->_nr_pages - index;
              continue;
          }

You changed index++ in the first hunk, but not the second hunk.  Is that intentional?

Quoted 3 lines

Hmm. Looks you are right; it should be modified, too. Will be fixing it up.

You initially had also in the second hunk:
ractl->index += folio_nr_pages(folio);

and I changed it to what it is now.

The reason is in my reply to willy:
https://lore.kernel.org/linux-xfs/s4jn4t4betknd3y4ltfccqxyfktzdljiz7klgbqsrccmv3rwrd@orlwjz77oyxo/

Let me know if you agree with it.

Quoted 38 lines · Pankaj Raghav

Hi Hannes,

Quoted 24 lines · Hannes Reinecke

Quoted 14 lines · Matthew Wilcox

Quoted 13 lines · Pankaj Raghav (Samsung)

mm/readahead.c +2 -2

@@ -239,8 +239,8 @@ 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 += folio_nr_pages(folio);
+            i = ractl->_index + ractl->_nr_pages - index;
                continue;
            }
    @@ -252,13 +252,14 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,

               folio_put(folio);
               read_pages(ractl);

Quoted 8 lines

Quoted 5 lines

               ractl->_index++;
-            i = ractl->_index + ractl->_nr_pages - index - 1; +            i = ractl->_index + ractl->_nr_pages - index; continue;
           }

You changed index++ in the first hunk, but not the second hunk.  Is that intentional?

Quoted 3 lines

Hmm. Looks you are right; it should be modified, too. Will be fixing it up.

You initially had also in the second hunk:
ractl->index += folio_nr_pages(folio);

and I changed it to what it is now.

The reason is in my reply to willy:
https://lore.kernel.org/linux-xfs/s4jn4t4betknd3y4ltfccqxyfktzdljiz7klgbqsrccmv3rwrd@orlwjz77oyxo/

Quoted 2 lines

Let me know if you agree with it.

Bah. That really is overly complicated. When we attempt a conversion that conversion should be stand-alone, not rely on some other patch modifications later on.
We definitely need to work on that to make it easier to review, even
without having to read the mail thread.

Cheers,

Hannes

Quoted 39 lines · Hannes Reinecke

Quoted 38 lines · Pankaj Raghav

Hi Hannes,

Quoted 24 lines · Hannes Reinecke

Quoted 14 lines · Matthew Wilcox

Quoted 13 lines · Pankaj Raghav (Samsung)

mm/readahead.c +2 -2

@@ -239,8 +239,8 @@ 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 += folio_nr_pages(folio);
+            i = ractl->_index + ractl->_nr_pages - index;
                continue;
            }
    @@ -252,13 +252,14 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,

               folio_put(folio);
               read_pages(ractl);

Quoted 8 lines

Quoted 5 lines

               ractl->_index++;
-            i = ractl->_index + ractl->_nr_pages - index - 1; +            i = ractl->_index + ractl->_nr_pages - index;
               continue;
           }

You changed index++ in the first hunk, but not the second hunk.  Is that intentional?

Quoted 3 lines

Hmm. Looks you are right; it should be modified, too. Will be fixing it up.

You initially had also in the second hunk:
ractl->index += folio_nr_pages(folio);

and I changed it to what it is now.

The reason is in my reply to willy:
https://lore.kernel.org/linux-xfs/s4jn4t4betknd3y4ltfccqxyfktzdljiz7klgbqsrccmv3rwrd@orlwjz77oyxo/

Quoted 7 lines

Quoted 2 lines

Let me know if you agree with it.

Bah. That really is overly complicated. When we attempt a conversion that conversion should be stand-alone, not rely on some other patch modifications later on.
We definitely need to work on that to make it easier to review, even
without having to read the mail thread.

I don't know understand what you mean by overly complicated. This conversion is standalone and it is wrong to use folio_nr_pages after we put the folio. This patch just reworks the loop and in the next patch I add min order support to readahead.

This patch doesn't depend on the next patch.

Quoted 6 lines

Quoted 5 lines

Bah. That really is overly complicated. When we attempt a conversion that conversion should be stand-alone, not rely on some other patch modifications later on.
We definitely need to work on that to make it easier to review, even
without having to read the mail thread.

Quoted 6 lines

I don't know understand what you mean by overly complicated. This conversion is standalone and it is wrong to use folio_nr_pages after we put the folio. This patch just reworks the loop and in the next patch I add min order support to readahead.

This patch doesn't depend on the next patch.

Let me rephrase: what does 'ractl->_index' signify? From my understanding it should be the index of the first folio/page in ractl, right?

If so I find it hard to understand how we _could_ increase it by one; _index should _always_ in units of the minimal pagemap size.

And if we don't have it here (as you suggested in the mailthread) I'd rather move this patch _after_ the minimal pagesize is introduced to ensure that _index is always incremented by the right amount.

Quoted 11 lines · Pankaj Raghav (Samsung)

mm/readahead.c +2 -2

@@ -239,8 +239,8 @@ 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 += folio_nr_pages(folio);
+			i = ractl->_index + ractl->_nr_pages - index;
 			continue;
 		}
 

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

Quoted 8 lines

mm/readahead.c +1 -1

@@ -252,13 +252,14 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
 			folio_put(folio);
 			read_pages(ractl);
 			ractl->_index++;
-			i = ractl->_index + ractl->_nr_pages - index - 1;
+			i = ractl->_index + ractl->_nr_pages - index;
 			continue;
 		}

You changed index++ in the first hunk, but not the second hunk. Is that intentional?

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.
When read_pages() is triggered and if a page is already present, check for truncation and move the ractl->_index by mapping_min_nrpages if that folio was truncated. This is done to ensure we keep the alignment requirement while adding a folio to the page cache.

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 min_order requirement of the page cache.

mm/readahead.c +12 -2

@@ -239,23 +258,35 @@ 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);
+
+			/*
+			 * 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;
 			i = ractl->_index + ractl->_nr_pages - index;
 			continue;
 		}
 
-		folio = filemap_alloc_folio(gfp_mask, 0);

mm/readahead.c +8 -2

 			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 += folio_nr_pages(folio);

@@ -489,12 +520,18 @@ 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;
 	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);

Quoted 9 lines

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.
When read_pages() is triggered and if a page is already present, check for truncation and move the ractl->_index by mapping_min_nrpages if that folio was truncated. This is done to ensure we keep the alignment requirement while adding a folio to the page cache.

Quoted 3 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 min_order requirement of the page cache.

This paragraph doesn't make sense. We have an assertion that there's no folio in the page cache with a lower order than the minimum, so this seems to be describing a situation that can't happen. Does it need to be rephrased (because you're actually describing something else) or is it just stale?

Quoted 11 lines

mm/readahead.c +2 -0

@@ -489,12 +520,18 @@ 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;
 	int err = 0;
 	gfp_t gfp = readahead_gfp_mask(mapping);
+	unsigned int min_ra_size = max(4, mapping_min_folio_nrpages(mapping));
 

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

Quoted 1 line

mm/readahead.c +0 -1

-	if (!mapping_large_folio_support(mapping) || ra->size < 4)

Quoted 7 lines

mm/readahead.c +5 -0

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

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

Quoted 7 lines

mm/readahead.c +2 -0

 	limit = min(limit, index + ra->size - 1);

@@ -505,9 +542,19 @@ void page_cache_ra_order(struct readahead_control *ractl,
 			new_order = MAX_PAGECACHE_ORDER;
 		while ((1 << new_order) > ra->size)
 			new_order--;
+		if (new_order < min_order)
+			new_order = min_order;

I think these are the two lines you're describing in the paragraph that

doesn't make sense to me? And if so, I think they're useless?

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

Quoted 8 lines

mm/readahead.c +1 -1

@@ -515,7 +562,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--;

This raises an interesting question that I don't know if we have a test for. POSIX says that if we mmap, let's say, the first 16kB of a 10kB file, then we can store into offset 0-12287, but stores to offsets 12288-16383 get a signal (I forget if it's SEGV or BUS). Thus far, we've declined to even create folios in the page cache that would let us create PTEs for offset 12288-16383, so I haven't paid too much attention to this. Now we're going to have folios that extend into that range, so we need to be sure that when we mmap(), we only create PTEs that go as far as 12287.

Can you check that we have such an fstest, and that we still pass it with your patches applied and a suitably large block size?

Review void page_cache_ra_order(struct readahead_control *ractl, · line 566

Quoted 7 lines

(quoted diff) +1 -1

@@ -515,7 +562,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--;

This raises an interesting question that I don't know if we have a test for. POSIX says that if we mmap, let's say, the first 16kB of a 10kB file, then we can store into offset 0-12287, but stores to offsets 12288-16383 get a signal (I forget if it's SEGV or BUS). Thus far, we've declined to even create folios in the page cache that would let us create PTEs for offset 12288-16383, so I haven't paid too much attention to this. Now we're going to have folios that extend into that range, so we need to be sure that when we mmap(), we only create PTEs that go as far as 12287.

Can you check that we have such an fstest, and that we still pass it with your patches applied and a suitably large block size?

From: Pankaj Raghav <[email protected]>

As we don't have a way to split a folio to a any given lower folio order yet, avoid splitting the folio in split_huge_page_to_list() if it has a minimum folio order requirement.

Quoted 5 lines · Pankaj Raghav (Samsung)

From: Pankaj Raghav <[email protected]>

As we don't have a way to split a folio to a any given lower folio order yet, avoid splitting the folio in split_huge_page_to_list() if it has a minimum folio order requirement.

FYI, Zi Yan's patch to do that is now in Andrew's tree. c010d47f107f609b9f4d6a103b6dfc53889049e9 in current linux-next (dated Feb 26)

Quoted 10 lines · Matthew Wilcox

Quoted 5 lines · Pankaj Raghav (Samsung)

From: Pankaj Raghav <[email protected]>

As we don't have a way to split a folio to a any given lower folio order yet, avoid splitting the folio in split_huge_page_to_list() if it has a minimum folio order requirement.

FYI, Zi Yan's patch to do that is now in Andrew's tree. c010d47f107f609b9f4d6a103b6dfc53889049e9 in current linux-next (dated Feb 26)

Yes, I started playing with the patches but I am getting a race condition resulting in a null-ptr-deref for which I don't have a good answer for yet.

@zi yan Did you encounter any issue like this when you were testing?

I did the following change (just a prototype) instead of this patch:

mm/huge_memory.c +0 -0

index 9859aa4f7553..63ee7b6ed03d 100644

mm/huge_memory.c +5 -0

+
+       if (!folio_test_anon(folio))
+               new_order = max_t(unsigned int, mapping_min_order, new_order);
        /* reset xarray order to new order after split */
        XA_STATE_ORDER(xas, &folio->mapping->i_pages, folio->index, new_order);
        struct anon_vma *anon_vma = NULL;

@@ -3117,6 +3121,8 @@ int split_huge_page_to_list_to_order(struct page *page, struct list_head *list,
                        goto out;
                }
 
+               // XXX: Remove it later
+               VM_WARN_ON_FOLIO((new_order < mapping_min_order), folio);
                gfp = current_gfp_context(mapping_gfp_mask(mapping) &
                                                        GFP_RECLAIM_MASK);
 

I am getting a random null-ptr deref when I run generic/176 multiple times with different blksizes. I still don't have a minimal reproducer for this issue. Race condition during writeback:

filemap_get_folios_tag+0x171/0x5c0:
arch_atomic_read at arch/x86/include/asm/atomic.h:23
(inlined by) raw_atomic_read at include/linux/atomic/atomic-arch-fallback.h:457 (inlined by) raw_atomic_fetch_add_unless at include/linux/atomic/atomic-arch-fallback.h:2426 (inlined by) raw_atomic_add_unless at include/linux/atomic/atomic-arch-fallback.h:2456 (inlined by) atomic_add_unless at include/linux/atomic/atomic-instrumented.h:1518 (inlined by) page_ref_add_unless at include/linux/page_ref.h:238
(inlined by) folio_ref_add_unless at include/linux/page_ref.h:247
(inlined by) folio_ref_try_add_rcu at include/linux/page_ref.h:280
(inlined by) folio_try_get_rcu at include/linux/page_ref.h:313
(inlined by) find_get_entry at mm/filemap.c:1984
(inlined by) filemap_get_folios_tag at mm/filemap.c:2222

[  537.863105] ==================================================================
[  537.863968] BUG: KASAN: null-ptr-deref in filemap_get_folios_tag+0x171/0x5c0
[  537.864581] Write of size 4 at addr 0000000000000036 by task kworker/u32:5/366
[  537.865123]
[  537.865293] CPU: 6 PID: 366 Comm: kworker/u32:5 Not tainted 6.8.0-11739-g7d0c6e7b5a7d-dirty #795
[  537.867201] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[  537.868444] Workqueue: writeback wb_workfn (flush-254:32)
[  537.869055] Call Trace:
[  537.869341]  <TASK>
[  537.869569]  dump_stack_lvl+0x4f/0x70
[  537.869938]  kasan_report+0xbd/0xf0
[  537.870293]  ? filemap_get_folios_tag+0x171/0x5c0
[  537.870767]  ? filemap_get_folios_tag+0x171/0x5c0
[  537.871578]  kasan_check_range+0x101/0x1b0
[  537.871893]  filemap_get_folios_tag+0x171/0x5c0
[  537.872269]  ? __pfx_filemap_get_folios_tag+0x10/0x10
[  537.872857]  ? __pfx___submit_bio+0x10/0x10
[  537.873326]  ? mlock_drain_local+0x234/0x3f0
[  537.873938]  writeback_iter+0x59a/0xe00
[  537.874477]  ? __pfx_iomap_do_writepage+0x10/0x10
[  537.874969]  write_cache_pages+0x7f/0x100
[  537.875396]  ? __pfx_write_cache_pages+0x10/0x10
[  537.875892]  ? do_raw_spin_lock+0x12d/0x270
[  537.876345]  ? __pfx_do_raw_spin_lock+0x10/0x10
[  537.876804]  iomap_writepages+0x88/0xf0
[  537.877186]  xfs_vm_writepages+0x120/0x190
[  537.877705]  ? __pfx_xfs_vm_writepages+0x10/0x10
[  537.878161]  ? lock_release+0x36f/0x670
[  537.878521]  ? __wb_calc_thresh+0xe5/0x3b0
[  537.878892]  ? __pfx_lock_release+0x10/0x10
[  537.879308]  do_writepages+0x170/0x7a0
[  537.879676]  ? __pfx_do_writepages+0x10/0x10
[  537.880182]  ? writeback_sb_inodes+0x312/0xe40
[  537.880689]  ? reacquire_held_locks+0x1f1/0x4a0
[  537.881193]  ? writeback_sb_inodes+0x312/0xe40
[  537.881665]  ? find_held_lock+0x2d/0x110
[  537.882104]  ? lock_release+0x36f/0x670
[  537.883344]  ? wbc_attach_and_unlock_inode+0x3b8/0x710
[  537.883853]  ? __pfx_lock_release+0x10/0x10
[  537.884229]  ? __pfx_lock_release+0x10/0x10
[  537.884604]  ? lock_acquire+0x138/0x2f0
[  537.884952]  __writeback_single_inode+0xd4/0xa60
[  537.885369]  writeback_sb_inodes+0x4cf/0xe40
[  537.885760]  ? __pfx_writeback_sb_inodes+0x10/0x10
[  537.886208]  ? __pfx_move_expired_inodes+0x10/0x10
[  537.886640]  __writeback_inodes_wb+0xb4/0x200
[  537.887037]  wb_writeback+0x55b/0x7c0
[  537.887372]  ? __pfx_wb_writeback+0x10/0x10
[  537.887750]  ? lock_acquire+0x138/0x2f0
[  537.888094]  ? __pfx_register_lock_class+0x10/0x10
[  537.888521]  wb_workfn+0x648/0xbb0
[  537.888835]  ? __pfx_wb_workfn+0x10/0x10
[  537.889192]  ? lock_acquire+0x128/0x2f0
[  537.889539]  ? lock_acquire+0x138/0x2f0
[  537.889890]  process_one_work+0x7ff/0x1710
[  537.890272]  ? __pfx_process_one_work+0x10/0x10
[  537.890685]  ? assign_work+0x16c/0x240

[  537.891026]  worker_thread+0x6e8/0x12b0
[  537.891381]  ? __pfx_worker_thread+0x10/0x10
[  537.891768]  kthread+0x2ad/0x380
[  537.892064]  ? __pfx_kthread+0x10/0x10
[  537.892403]  ret_from_fork+0x2d/0x70
[  537.892728]  ? __pfx_kthread+0x10/0x10
[  537.893068]  ret_from_fork_asm+0x1a/0x30
[  537.893434]  </TASK>

Quoted 20 lines · Pankaj Raghav (Samsung)

Quoted 10 lines · Matthew Wilcox

Quoted 5 lines · Pankaj Raghav (Samsung)

From: Pankaj Raghav <[email protected]>

As we don't have a way to split a folio to a any given lower folio order yet, avoid splitting the folio in split_huge_page_to_list() if it has a minimum folio order requirement.

FYI, Zi Yan's patch to do that is now in Andrew's tree. c010d47f107f609b9f4d6a103b6dfc53889049e9 in current linux-next (dated Feb 26)

Yes, I started playing with the patches but I am getting a race condition resulting in a null-ptr-deref for which I don't have a good answer for yet.

@zi yan Did you encounter any issue like this when you were testing?

I did the following change (just a prototype) instead of this patch:

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

Quoted 9 lines

mm/huge_memory.c +1 -0

index 9859aa4f7553..63ee7b6ed03d 100644
@@ -3041,6 +3041,10 @@ int split_huge_page_to_list_to_order(struct page *page, struct list_head *list,
 {
        struct folio *folio = page_folio(page);
        struct deferred_split *ds_queue = get_deferred_split_queue(folio);
+       unsigned int mapping_min_order = mapping_min_folio_order(folio->mapping);

I am not sure if this is right. Since folio can be anonymous and folio->mapping does not point to struct address_space.

Quoted 2 lines

mm/huge_memory.c +2 -0

+
+       if (!folio_test_anon(folio))

Quoted 89 lines

mm/huge_memory.c +3 -0

+               new_order = max_t(unsigned int, mapping_min_order, new_order);
        /* reset xarray order to new order after split */
        XA_STATE_ORDER(xas, &folio->mapping->i_pages, folio->index, new_order);
        struct anon_vma *anon_vma = NULL;

@@ -3117,6 +3121,8 @@ int split_huge_page_to_list_to_order(struct page *page, struct list_head *list,
                        goto out;
                }
 
+               // XXX: Remove it later
+               VM_WARN_ON_FOLIO((new_order < mapping_min_order), folio);
                gfp = current_gfp_context(mapping_gfp_mask(mapping) &
                                                        GFP_RECLAIM_MASK);
 

I am getting a random null-ptr deref when I run generic/176 multiple times with different blksizes. I still don't have a minimal reproducer for this issue. Race condition during writeback:

filemap_get_folios_tag+0x171/0x5c0:
arch_atomic_read at arch/x86/include/asm/atomic.h:23
(inlined by) raw_atomic_read at include/linux/atomic/atomic-arch-fallback.h:457 (inlined by) raw_atomic_fetch_add_unless at include/linux/atomic/atomic-arch-fallback.h:2426 (inlined by) raw_atomic_add_unless at include/linux/atomic/atomic-arch-fallback.h:2456 (inlined by) atomic_add_unless at include/linux/atomic/atomic-instrumented.h:1518 (inlined by) page_ref_add_unless at include/linux/page_ref.h:238
(inlined by) folio_ref_add_unless at include/linux/page_ref.h:247
(inlined by) folio_ref_try_add_rcu at include/linux/page_ref.h:280
(inlined by) folio_try_get_rcu at include/linux/page_ref.h:313
(inlined by) find_get_entry at mm/filemap.c:1984
(inlined by) filemap_get_folios_tag at mm/filemap.c:2222

[  537.863105] ==================================================================
[  537.863968] BUG: KASAN: null-ptr-deref in filemap_get_folios_tag+0x171/0x5c0
[  537.864581] Write of size 4 at addr 0000000000000036 by task kworker/u32:5/366
[  537.865123]
[  537.865293] CPU: 6 PID: 366 Comm: kworker/u32:5 Not tainted 6.8.0-11739-g7d0c6e7b5a7d-dirty #795
[  537.867201] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[  537.868444] Workqueue: writeback wb_workfn (flush-254:32)
[  537.869055] Call Trace:
[  537.869341]  <TASK>
[  537.869569]  dump_stack_lvl+0x4f/0x70
[  537.869938]  kasan_report+0xbd/0xf0
[  537.870293]  ? filemap_get_folios_tag+0x171/0x5c0
[  537.870767]  ? filemap_get_folios_tag+0x171/0x5c0
[  537.871578]  kasan_check_range+0x101/0x1b0
[  537.871893]  filemap_get_folios_tag+0x171/0x5c0
[  537.872269]  ? __pfx_filemap_get_folios_tag+0x10/0x10
[  537.872857]  ? __pfx___submit_bio+0x10/0x10
[  537.873326]  ? mlock_drain_local+0x234/0x3f0
[  537.873938]  writeback_iter+0x59a/0xe00
[  537.874477]  ? __pfx_iomap_do_writepage+0x10/0x10
[  537.874969]  write_cache_pages+0x7f/0x100
[  537.875396]  ? __pfx_write_cache_pages+0x10/0x10
[  537.875892]  ? do_raw_spin_lock+0x12d/0x270
[  537.876345]  ? __pfx_do_raw_spin_lock+0x10/0x10
[  537.876804]  iomap_writepages+0x88/0xf0
[  537.877186]  xfs_vm_writepages+0x120/0x190
[  537.877705]  ? __pfx_xfs_vm_writepages+0x10/0x10
[  537.878161]  ? lock_release+0x36f/0x670
[  537.878521]  ? __wb_calc_thresh+0xe5/0x3b0
[  537.878892]  ? __pfx_lock_release+0x10/0x10
[  537.879308]  do_writepages+0x170/0x7a0
[  537.879676]  ? __pfx_do_writepages+0x10/0x10
[  537.880182]  ? writeback_sb_inodes+0x312/0xe40
[  537.880689]  ? reacquire_held_locks+0x1f1/0x4a0
[  537.881193]  ? writeback_sb_inodes+0x312/0xe40
[  537.881665]  ? find_held_lock+0x2d/0x110
[  537.882104]  ? lock_release+0x36f/0x670
[  537.883344]  ? wbc_attach_and_unlock_inode+0x3b8/0x710
[  537.883853]  ? __pfx_lock_release+0x10/0x10
[  537.884229]  ? __pfx_lock_release+0x10/0x10
[  537.884604]  ? lock_acquire+0x138/0x2f0
[  537.884952]  __writeback_single_inode+0xd4/0xa60
[  537.885369]  writeback_sb_inodes+0x4cf/0xe40
[  537.885760]  ? __pfx_writeback_sb_inodes+0x10/0x10
[  537.886208]  ? __pfx_move_expired_inodes+0x10/0x10
[  537.886640]  __writeback_inodes_wb+0xb4/0x200
[  537.887037]  wb_writeback+0x55b/0x7c0
[  537.887372]  ? __pfx_wb_writeback+0x10/0x10
[  537.887750]  ? lock_acquire+0x138/0x2f0
[  537.888094]  ? __pfx_register_lock_class+0x10/0x10
[  537.888521]  wb_workfn+0x648/0xbb0
[  537.888835]  ? __pfx_wb_workfn+0x10/0x10
[  537.889192]  ? lock_acquire+0x128/0x2f0
[  537.889539]  ? lock_acquire+0x138/0x2f0
[  537.889890]  process_one_work+0x7ff/0x1710
[  537.890272]  ? __pfx_process_one_work+0x10/0x10
[  537.890685]  ? assign_work+0x16c/0x240

Quoted 8 lines

[  537.891026]  worker_thread+0x6e8/0x12b0
[  537.891381]  ? __pfx_worker_thread+0x10/0x10
[  537.891768]  kthread+0x2ad/0x380
[  537.892064]  ? __pfx_kthread+0x10/0x10
[  537.892403]  ret_from_fork+0x2d/0x70
[  537.892728]  ? __pfx_kthread+0x10/0x10
[  537.893068]  ret_from_fork_asm+0x1a/0x30
[  537.893434]  </TASK>

--
Best Regards,
Yan, Zi

Quoted 35 lines · Zi Yan

Quoted 25 lines · Pankaj Raghav (Samsung)

Quoted 10 lines · Matthew Wilcox

Quoted 5 lines · Pankaj Raghav (Samsung)

From: Pankaj Raghav <[email protected]>

As we don't have a way to split a folio to a any given lower folio order yet, avoid splitting the folio in split_huge_page_to_list() if it has a minimum folio order requirement.

FYI, Zi Yan's patch to do that is now in Andrew's tree. c010d47f107f609b9f4d6a103b6dfc53889049e9 in current linux-next (dated Feb 26)

Yes, I started playing with the patches but I am getting a race condition resulting in a null-ptr-deref for which I don't have a good answer for yet.

@zi yan Did you encounter any issue like this when you were testing?

I did the following change (just a prototype) instead of this patch:

mm/huge_memory.c +0 -0

index 9859aa4f7553..63ee7b6ed03d 100644

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

Quoted 4 lines

mm/huge_memory.c +1 -0

@@ -3041,6 +3041,10 @@ int split_huge_page_to_list_to_order(struct page *page, struct list_head *list,
 {
        struct folio *folio = page_folio(page);
        struct deferred_split *ds_queue = get_deferred_split_queue(folio);
+       unsigned int mapping_min_order = mapping_min_folio_order(folio->mapping);

I am not sure if this is right. Since folio can be anonymous and folio->mapping does not point to struct address_space.

Quoted 2 lines

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

Quoted 2 lines

mm/huge_memory.c +2 -0

+
+       if (!folio_test_anon(folio))

Hmm, but I update the new_order only if it is not anonymous. Do you think it is still wrong?

From: Pankaj Raghav <[email protected]>

Instead of assuming that PAGE_SHIFT is always higher than the blocklog, make the calculation generic so that page cache count can be calculated correctly for LBS.

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

---
 fs/xfs/xfs_mount.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

fs/xfs/xfs_mount.c +0 -0

index aabb25dc3efa..9cf800586da7 100644

Quoted 11 lines · Pankaj Raghav (Samsung)

From: Pankaj Raghav <[email protected]>

Instead of assuming that PAGE_SHIFT is always higher than the blocklog, make the calculation generic so that page cache count can be calculated correctly for LBS.

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

---
 fs/xfs/xfs_mount.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

fs/xfs/xfs_mount.c xfs_sb_validate_fsb_count( · line 134

Quoted 7 lines

fs/xfs/xfs_mount.c +0 -0

index aabb25dc3efa..9cf800586da7 100644
@@ -133,9 +133,16 @@ xfs_sb_validate_fsb_count(
 {
 	ASSERT(PAGE_SHIFT >= sbp->sb_blocklog);

but ... you're still asserting that PAGE_SHIFT is larger than blocklog. Shouldn't you delete that assertion?

Quoted 8 lines

fs/xfs/xfs_mount.c +5 -0

 	ASSERT(sbp->sb_blocklog >= BBSHIFT);
+	uint64_t max_index;
+	uint64_t max_bytes;
+
+	if (check_shl_overflow(nblocks, sbp->sb_blocklog, &max_bytes))
+		return -EFBIG;
 

fs/xfs/xfs_mount.c xfs_sb_validate_fsb_count( · line 146

Quoted 6 lines

fs/xfs/xfs_mount.c +3 -1

 	/* 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)
 		return -EFBIG;

This kind of depends on the implementation details of the page cache. We have MAX_LFS_FILESIZE to abstract that; maybe that should be used here?

This is the third version of the series that enables block size > page size (Large Block Size) in XFS. The context and motivation can be seen in cover letter of the RFC v1[1]. We also recorded a talk about this effort at LPC [3], if someone would like more context on this effort.

Quoted 4 lines · Pankaj Raghav (Samsung)

This is the third version of the series that enables block size > page size (Large Block Size) in XFS. The context and motivation can be seen in cover letter of the RFC v1[1]. We also recorded a talk about this effort at LPC [3], if someone would like more context on this effort.

Thank you. This is a lot better.

I'm still trying to understand your opinion on the contents of the file_ra_state. Is it supposed to be properly aligned at all times, or do we work with it in the terms of "desired number of pages" and then force it to conform to the minimum-block-size reality right at the end? Because you seem to be doing both at various points.

Quoted 6 lines · Matthew Wilcox

Quoted 4 lines · Pankaj Raghav (Samsung)

This is the third version of the series that enables block size > page size (Large Block Size) in XFS. The context and motivation can be seen in cover letter of the RFC v1[1]. We also recorded a talk about this effort at LPC [3], if someone would like more context on this effort.

Quoted 7 lines

Thank you.  This is a lot better.

I'm still trying to understand your opinion on the contents of the file_ra_state.  Is it supposed to be properly aligned at all times, or do we work with it in the terms of "desired number of pages" and then force it to conform to the minimum-block-size reality right at the end? Because you seem to be doing both at various points.

Guess what, that's what I had been pondering, too. Each way has its benefits, I guess.

Question really is do we keep the readahead iterator in units of pages, and convert the result, or do we modify the readahead iterator to work on folios, and convert the inputs.

Doesn't really matter much, but we need to decide. The former is probably easier on the caller, and the latter is easier on the consumer.
Take your pick; I really don't mind.

But we should document the result :-)

Cheers,

Hannes

Quoted 4 lines · Pankaj Raghav (Samsung)

This is the third version of the series that enables block size > page size (Large Block Size) in XFS. The context and motivation can be seen in cover letter of the RFC v1[1]. We also recorded a talk about this effort at LPC [3], if someone would like more context on this effort.

Thank you. This is a lot better.

I'm still trying to understand your opinion on the contents of the file_ra_state. Is it supposed to be properly aligned at all times, or do we work with it in the terms of "desired number of pages" and then force it to conform to the minimum-block-size reality right at the end?

Because you seem to be doing both at various points.