support large folio swap-out and swap-in for shmem

In the following patches, shmem will support the swap out of large folios, which means the shmem mappings may contain large order swap entries, so using xa_get_order() to get the folio order of the shmem swap entry to update the '*start' correctly.

Signed-off-by Baolin Wang <[email protected]>

---
 mm/filemap.c | 4 ++++
 1 file changed, 4 insertions(+)

mm/filemap.c +2 -0

index 4130be74f6fd..4c312aab8b1f 100644
@@ -2056,6 +2056,8 @@ unsigned find_get_entries(struct address_space *mapping, pgoff_t *start,
 		folio = fbatch->folios[idx];
 		if (!xa_is_value(folio))
 			nr = folio_nr_pages(folio);
+		else
+			nr = 1 << xa_get_order(&mapping->i_pages, indices[idx]);
 		*start = indices[idx] + nr;
 	}
 	return folio_batch_count(fbatch);

To support large folio swapin for shmem in the following patches, add large folio allocation for the new replacement folio in shmem_replace_folio(). Moreover large folios occupy N consecutive entries in the swap cache instead of using multi-index entries like the page cache, therefore we should replace each consecutive entries in the swap cache instead of using the shmem_replace_entry().

As well as updating statistics and folio reference count using the number of pages in the folio.

Signed-off-by Baolin Wang <[email protected]>

---
 mm/shmem.c | 54 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 23 deletions(-)

mm/shmem.c +6 -10

index f6bab42180ea..d94f02ad7bd1 100644
@@ -1889,28 +1889,24 @@ static bool shmem_should_replace_folio(struct folio *folio, gfp_t gfp)
 static int shmem_replace_folio(struct folio **foliop, gfp_t gfp,
 				struct shmem_inode_info *info, pgoff_t index)
 {
-	struct folio *old, *new;
-	struct address_space *swap_mapping;
-	swp_entry_t entry;
-	pgoff_t swap_index;
-	int error;
-
-	old = *foliop;
-	entry = old->swap;
-	swap_index = swap_cache_index(entry);
-	swap_mapping = swap_address_space(entry);
+	struct folio *new, *old = *foliop;
+	swp_entry_t entry = old->swap;
+	struct address_space *swap_mapping = swap_address_space(entry);
+	pgoff_t swap_index = swap_cache_index(entry);
+	XA_STATE(xas, &swap_mapping->i_pages, swap_index);
+	int nr_pages = folio_nr_pages(old);

Now the swap device can only swap-in order 0 folio, even though a large folio is swapped out. This requires us to split the large entry previously saved in the shmem pagecache to support the swap in of small folios.

Signed-off-by Baolin Wang <[email protected]>

---
 mm/shmem.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

mm/shmem.c +0 -0

index 345e25425e37..996062dc196b 100644

mm/shmem.c +13 -0

+
+	for (;;) {
+		int order = -1, split_order = 0;
+		void *old = NULL;
+
+		xas_lock_irq(&xas);
+		old = xas_load(&xas);
+		if (!xa_is_value(old) || swp_to_radix_entry(swap) != old) {
+			xas_set_err(&xas, -EEXIST);
+			goto unlock;
+		}
+
+		order = xas_get_order(&xas);

mm/shmem.c +8 -0

+			if (!alloced_order) {
+				split_order = order;
+				goto unlock;
+			}
+			xas_split(&xas, old, order);
+
+			/*
+			 * Re-set the swap entry after splitting, and the swap

mm/shmem.c +45 -0

+		}
+
+unlock:
+		xas_unlock_irq(&xas);
+
+		/* split needed, alloc here and retry. */
+		if (split_order) {
+			xas_split_alloc(&xas, old, split_order, gfp);
+			if (xas_error(&xas))
+				goto error;
+			alloced_shadow = old;
+			alloced_order = split_order;
+			xas_reset(&xas);
+			continue;
+		}
+
+		if (!xas_nomem(&xas, gfp))
+			break;
+	}
+
+error:
+	if (xas_error(&xas))
+		return xas_error(&xas);
+
+	return alloced_order;
+}
+
 /*
  * Swap in the folio pointed to by *foliop.
  * Caller has to make sure that *foliop contains a valid swapped folio.

@@ -2026,12 +2101,37 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
 	/* Look it up and read it in.. */
 	folio = swap_cache_get_folio(swap, NULL, 0);
 	if (!folio) {
+		int split_order;
+
 		/* Or update major stats only when swapin succeeds?? */
 		if (fault_type) {
 			*fault_type |= VM_FAULT_MAJOR;
 			count_vm_event(PGMAJFAULT);
 			count_memcg_event_mm(fault_mm, PGMAJFAULT);
 		}
+
+		/*
+		 * Now swap device can only swap in order 0 folio, then we
+		 * should split the large swap entry stored in the pagecache
+		 * if necessary.
+		 */
+		split_order = shmem_split_large_entry(inode, index, swap, 0, gfp);
+		if (split_order < 0) {
+			error = split_order;
+			goto failed;
+		}
+
+		/*
+		 * If the large swap entry has already been split, it is
+		 * necessary to recalculate the new swap entry based on
+		 * the old order alignment.

Shmem will support large folio allocation [1] [2] to get a better performance, however, the memory reclaim still splits the precious large folios when trying to swap out shmem, which may lead to the memory fragmentation issue and can not take advantage of the large folio for shmeme.

Moreover, the swap code already supports for swapping out large folio without split, hence this patch set supports the large folio swap out for shmem.

Note the i915_gem_shmem driver still need to be split when swapping, thus add a new flag 'split_large_folio' for writeback_control to indicate spliting the large folio.

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

Signed-off-by Baolin Wang <[email protected]>

---
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c |  1 +
 include/linux/writeback.h                 |  4 +++
 mm/shmem.c                                | 12 ++++++---
 mm/vmscan.c                               | 32 ++++++++++++++++++-----
 4 files changed, 38 insertions(+), 11 deletions(-)

The second issue is that swap is more slippery to work with than folios or pages: in the folio_nr_pages() case, that number is stable because we hold a refcount (which stops a THP from being split), and later we'll be taking folio lock too. None of that in the swap case, so (depending on how a large entry gets split) the xa_get_order() result is not reliable. Likewise for other uses of xa_get_order() in this series.

There needs to be some kind of locking or retry to make the order usable, and to avoid shmem_free_swap() occasionally freeing more than it ought. I'll give it thought after.

Quoted 41 lines · Baolin Wang

To support large folio swapin for shmem in the following patches, add large folio allocation for the new replacement folio in shmem_replace_folio(). Moreover large folios occupy N consecutive entries in the swap cache instead of using multi-index entries like the page cache, therefore we should replace each consecutive entries in the swap cache instead of using the shmem_replace_entry().

As well as updating statistics and folio reference count using the number of pages in the folio.

Signed-off-by Baolin Wang <[email protected]>

---
  mm/shmem.c | 54 +++++++++++++++++++++++++++++++-----------------------
  1 file changed, 31 insertions(+), 23 deletions(-)

mm/shmem.c +7 -10

index f6bab42180ea..d94f02ad7bd1 100644
@@ -1889,28 +1889,24 @@ static bool shmem_should_replace_folio(struct folio *folio, gfp_t gfp)
  static int shmem_replace_folio(struct folio **foliop, gfp_t gfp,
  				struct shmem_inode_info *info, pgoff_t index)
  {
-	struct folio *old, *new;
-	struct address_space *swap_mapping;
-	swp_entry_t entry;
-	pgoff_t swap_index;
-	int error;
-
-	old = *foliop;
-	entry = old->swap;
-	swap_index = swap_cache_index(entry);
-	swap_mapping = swap_address_space(entry);
+	struct folio *new, *old = *foliop;
+	swp_entry_t entry = old->swap;
+	struct address_space *swap_mapping = swap_address_space(entry);
+	pgoff_t swap_index = swap_cache_index(entry);
+	XA_STATE(xas, &swap_mapping->i_pages, swap_index);
+	int nr_pages = folio_nr_pages(old);
+	int error = 0, i;
 

mm/shmem.c static bool shmem_should_replace_folio(struct folio *folio, gfp_t gfp) · line 1905

Quoted 8 lines

mm/shmem.c +1 -2

  	/*
  	 * We have arrived here because our zones are constrained, so don't
  	 * limit chance of success by further cpuset and node constraints.
  	 */
  	gfp &= ~GFP_CONSTRAINT_MASK;
-	VM_BUG_ON_FOLIO(folio_test_large(old), old);
-	new = shmem_alloc_folio(gfp, 0, info, index);
+	new = shmem_alloc_folio(gfp, folio_order(old), info, index);

It is not clear to me whether folio_order(old) will ever be more than 0
here: but if it can be, then care will need to be taken over the gfp flags,

that they are suited to allocating the large folio; and there will need to be (could be awkward!) fallback to order 0 when that allocation fails.

My own testing never comes to shmem_replace_folio(): it was originally for one lowend graphics driver; but IIRC there's now a more common case for it.

Quoted 17 lines · Baolin Wang

Now the swap device can only swap-in order 0 folio, even though a large folio is swapped out. This requires us to split the large entry previously saved in the shmem pagecache to support the swap in of small folios.

Signed-off-by Baolin Wang <[email protected]>

---
  mm/shmem.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 100 insertions(+)

mm/shmem.c +0 -0

index 345e25425e37..996062dc196b 100644
@@ -1990,6 +1990,81 @@ static void shmem_set_folio_swapin_error(struct inode *inode, pgoff_t index,
  	swap_free_nr(swap, nr_pages);
  }
 

mm/shmem.c static void shmem_set_folio_swapin_error(struct inode *inode, pgoff_t index, · line 1999

Quoted 7 lines

mm/shmem.c +7 -0

+static int shmem_split_large_entry(struct inode *inode, pgoff_t index,
+				   swp_entry_t swap, int new_order, gfp_t gfp)
+{
+	struct address_space *mapping = inode->i_mapping;
+	XA_STATE_ORDER(xas, &mapping->i_pages, index, new_order);
+	void *alloced_shadow = NULL;
+	int alloced_order = 0, i;

gfp needs to be adjusted: see fix patch below.

Quoted 13 lines

mm/shmem.c +13 -0

+
+	for (;;) {
+		int order = -1, split_order = 0;
+		void *old = NULL;
+
+		xas_lock_irq(&xas);
+		old = xas_load(&xas);
+		if (!xa_is_value(old) || swp_to_radix_entry(swap) != old) {
+			xas_set_err(&xas, -EEXIST);
+			goto unlock;
+		}
+
+		order = xas_get_order(&xas);

mm/shmem.c static void shmem_set_folio_swapin_error(struct inode *inode, pgoff_t index, · line 2022

Quoted 10 lines

mm/shmem.c +10 -0

+
+		/* Swap entry may have changed before we re-acquire the lock */
+		if (alloced_order &&
+		    (old != alloced_shadow || order != alloced_order)) {
+			xas_destroy(&xas);
+			alloced_order = 0;
+		}
+
+		/* Try to split large swap entry in pagecache */
+		if (order > 0 && order > new_order) {

I have not even attempted to understand all the manipulations of order and new_order and alloced_order and split_order. And further down it turns out that this is only ever called with new_order 0.

You may be wanting to cater for more generality in future, but for now please cut this down to the new_order 0 case, and omit that parameter. It will be easier for us to think about the xa_get_order() races if the possibilities are more limited.

Quoted 14 lines

mm/shmem.c +13 -0

+			if (!alloced_order) {
+				split_order = order;
+				goto unlock;
+			}
+			xas_split(&xas, old, order);
+
+			/*
+			 * Re-set the swap entry after splitting, and the swap
+			 * offset of the original large entry must be continuous.
+			 */
+			for (i = 0; i < 1 << order; i += (1 << new_order)) {
+				pgoff_t aligned_index = round_down(index, 1 << order);
+				swp_entry_t tmp;

mm/shmem.c static void shmem_set_folio_swapin_error(struct inode *inode, pgoff_t index, · line 2040

Quoted 5 lines

mm/shmem.c +5 -0

+
+				tmp = swp_entry(swp_type(swap), swp_offset(swap) + i);
+				__xa_store(&mapping->i_pages, aligned_index + i,
+					   swp_to_radix_entry(tmp), 0);
+			}

So that is done under xas lock: good. But is the intermediate state visible to RCU readers, and could that be a problem?

Quoted 11 lines · Baolin Wang

Shmem will support large folio allocation [1] [2] to get a better performance, however, the memory reclaim still splits the precious large folios when trying to swap out shmem, which may lead to the memory fragmentation issue and can not take advantage of the large folio for shmeme.

Moreover, the swap code already supports for swapping out large folio without split, hence this patch set supports the large folio swap out for shmem.

Note the i915_gem_shmem driver still need to be split when swapping, thus add a new flag 'split_large_folio' for writeback_control to indicate spliting the large folio.

Is that last paragraph a misunderstanding? The i915 THP splitting in shmem_writepage() was to avoid mm VM_BUG_ONs and crashes when shmem.c did not support huge page swapout: but now you are enabling that support, and such VM_BUG_ONs and crashes are gone (so far as I can see: and this is written on a laptop using the i915 driver).

I cannot think of why i915 itself would care how mm implements swapout (beyond enjoying faster): I think all the wbc->split_large_folio you introduce here should be reverted. But you may know better!

I do need a further change to shmem_writepage() here: see fixup patch
below: that's written to apply on top of this 9/9, but I'd be glad to see a replacement with wbc->split_large_folio gone, and just one !IS_ENABLED(CONFIG_THP_SWAP) instead.

Quoted 2 lines

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

I get "Not found" for that [2] link.

Quoted 8 lines

Signed-off-by Baolin Wang <[email protected]>

---
  drivers/gpu/drm/i915/gem/i915_gem_shmem.c |  1 +
  include/linux/writeback.h                 |  4 +++
  mm/shmem.c                                | 12 ++++++---
  mm/vmscan.c                               | 32 ++++++++++++++++++-----
  4 files changed, 38 insertions(+), 11 deletions(-)

[PATCH] mm: shmem: shmem_writepage() split folio at EOF before swapout

Working in a constrained (size= or nr_blocks=) huge=always tmpfs relies on swapout to split a large folio at EOF, to trim off its excess before hitting premature ENOSPC: shmem_unused_huge_shrink() contains no code to handle splitting huge swap blocks, and nobody would want that to be added.

Signed-off-by Hugh Dickins <[email protected]>

---
  mm/shmem.c | 15 ++++++++++++++-
  1 file changed, 14 insertions(+), 1 deletion(-)

mm/shmem.c +7 -1

index 37c300f69baf..4dd0570962fa 100644
@@ -1459,6 +1459,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
  	swp_entry_t swap;
  	pgoff_t index;
  	int nr_pages;
+	bool split = false;
 
  	/*
  	 * Our capabilities prevent regular writeback or sync from ever calling

@@ -1480,8 +1481,20 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
  	 * If /sys/kernel/mm/transparent_hugepage/shmem_enabled is "always" or
  	 * "force", drivers/gpu/drm/i915/gem/i915_gem_shmem.c gets huge pages,
  	 * and its shmem_writeback() needs them to be split when swapping.
+	 *
+	 * And shrinkage of pages beyond i_size does not split swap, so
+	 * swapout of a large folio crossing i_size needs to split too
+	 * (unless fallocate has been used to preallocate beyond EOF).
  	 */
-	if (wbc->split_large_folio && folio_test_large(folio)) {
+	if (folio_test_large(folio)) {
+		split = wbc->split_large_folio;

Quoted 29 lines · Baolin Wang

In the following patches, shmem will support the swap out of large folios, which means the shmem mappings may contain large order swap entries, so using xa_get_order() to get the folio order of the shmem swap entry to update the '*start' correctly.

Signed-off-by Baolin Wang <[email protected]>

---
  mm/filemap.c | 4 ++++
  1 file changed, 4 insertions(+)

mm/filemap.c +4 -0

index 4130be74f6fd..4c312aab8b1f 100644
@@ -2056,6 +2056,8 @@ unsigned find_get_entries(struct address_space *mapping, pgoff_t *start,
  		folio = fbatch->folios[idx];
  		if (!xa_is_value(folio))
  			nr = folio_nr_pages(folio);
+		else
+			nr = 1 << xa_get_order(&mapping->i_pages, indices[idx]);
  		*start = indices[idx] + nr;
  	}
  	return folio_batch_count(fbatch);

@@ -2120,6 +2122,8 @@ unsigned find_lock_entries(struct address_space *mapping, pgoff_t *start,
  		folio = fbatch->folios[idx];
  		if (!xa_is_value(folio))
  			nr = folio_nr_pages(folio);
+		else
+			nr = 1 << xa_get_order(&mapping->i_pages, indices[idx]);

mm/filemap.c unsigned find_get_entries(struct address_space *mapping, pgoff_t *start, · line 2063

Quoted 4 lines

mm/filemap.c +0 -0

  		*start = indices[idx] + nr;
  	}
  	return folio_batch_count(fbatch);

Here we have a problem, but I'm not suggesting a fix for it yet: I need to get other fixes out first, then turn to thinking about this - it's not easy.

That code is almost always right, so it works well enough for most people not to have noticed, but there are two issues with it.

The first issue is that it's assuming indices[idx] is already aligned to nr: not necessarily so. I believe it was already wrong in the folio_nr_pages() case, but the time I caught it wrong with a printk was in the swap (value) case. (I may not be stating this correctly:
again more thought needed but I can't spend so long writing.)

And immediately afterwards that kernel build failed with a corrupted (all 0s) .o file - I'm building on ext4 on /dev/loop0 on huge tmpfs while swapping, and happen to be using the "-o discard" option to ext4 mount.

I've been chasing these failures (maybe a few minutes in, maybe half an hour) for days, then had the idea of trying without the "-o discard":
whereupon these builds can be repeated successfully for many hours. IIRC ext4 discard to /dev/loop0 entails hole-punch to the tmpfs.

The alignment issue can easily be corrected, but that might not help. (And those two functions would look better with the rcu_read_unlock() moved down to just before returning: but again, wouldn't really help.)

The second issue is that swap is more slippery to work with than folios or pages: in the folio_nr_pages() case, that number is stable because we hold a refcount (which stops a THP from being split), and later we'll be taking folio lock too. None of that in the swap case, so (depending on how a large entry gets split) the xa_get_order() result is not reliable. Likewise for other uses of xa_get_order() in this series.

There needs to be some kind of locking or retry to make the order usable, and to avoid shmem_free_swap() occasionally freeing more than it ought. I'll give it thought after.

Quoted 35 lines · Hugh Dickins

Quoted 33 lines · Baolin Wang

In the following patches, shmem will support the swap out of large folios, which means the shmem mappings may contain large order swap entries, so using xa_get_order() to get the folio order of the shmem swap entry to update the '*start' correctly.

Signed-off-by Baolin Wang <[email protected]>

---
  mm/filemap.c | 4 ++++
  1 file changed, 4 insertions(+)

mm/filemap.c +2 -0

index 4130be74f6fd..4c312aab8b1f 100644
@@ -2056,6 +2056,8 @@ unsigned find_get_entries(struct address_space
 *mapping, pgoff_t *start,
    folio = fbatch->folios[idx];
    if (!xa_is_value(folio))
  			nr = folio_nr_pages(folio);
+		else
+			nr = 1 << xa_get_order(&mapping->i_pages,
 indices[idx]);
   	*start = indices[idx] + nr;

   }
   return folio_batch_count(fbatch);

mm/filemap.c +2 -0

@@ -2120,6 +2122,8 @@ unsigned find_lock_entries(struct address_space
 *mapping, pgoff_t *start,
    folio = fbatch->folios[idx];
    if (!xa_is_value(folio))
  			nr = folio_nr_pages(folio);
+		else
+			nr = 1 << xa_get_order(&mapping->i_pages,
 indices[idx]);

Quoted 8 lines

Quoted 4 lines

mm/filemap.c +0 -0

   	*start = indices[idx] + nr;

   }
   return folio_batch_count(fbatch);

Here we have a problem, but I'm not suggesting a fix for it yet: I need to get other fixes out first, then turn to thinking about this - it's not easy.

Thanks for raising the issues.

Quoted 23 lines

That code is almost always right, so it works well enough for most people not to have noticed, but there are two issues with it.

The first issue is that it's assuming indices[idx] is already aligned to nr: not necessarily so. I believe it was already wrong in the folio_nr_pages() case, but the time I caught it wrong with a printk was in the swap (value) case. (I may not be stating this correctly:
again more thought needed but I can't spend so long writing.)

And immediately afterwards that kernel build failed with a corrupted (all 0s) .o file - I'm building on ext4 on /dev/loop0 on huge tmpfs while swapping, and happen to be using the "-o discard" option to ext4 mount.

I've been chasing these failures (maybe a few minutes in, maybe half an hour) for days, then had the idea of trying without the "-o discard":
whereupon these builds can be repeated successfully for many hours. IIRC ext4 discard to /dev/loop0 entails hole-punch to the tmpfs.

The alignment issue can easily be corrected, but that might not help. (And those two functions would look better with the rcu_read_unlock() moved down to just before returning: but again, wouldn't really help.)

Quoted 6 lines

The second issue is that swap is more slippery to work with than folios or pages: in the folio_nr_pages() case, that number is stable because we hold a refcount (which stops a THP from being split), and later we'll be taking folio lock too. None of that in the swap case, so (depending on how a large entry gets split) the xa_get_order() result is not reliable. Likewise for other uses of xa_get_order() in this series.

Now we have 2 users of xa_get_order() in this series:

1) shmem_partial_swap_usage(): this is acceptable, since racy results are not a problem for the swap statistics.

2) shmem_undo_range(): when freeing a large swap entry, it will use xa_cmpxchg_irq() to make sure the swap value is not changed (in case the large swap entry is split). If failed to cmpxchg, then it will use current index to retry in shmem_undo_range(). So seems not too bad?

Quoted 77 lines · Baolin Wang

Quoted 35 lines · Hugh Dickins

Quoted 33 lines · Baolin Wang

In the following patches, shmem will support the swap out of large folios, which means the shmem mappings may contain large order swap entries, so using xa_get_order() to get the folio order of the shmem swap entry to update the '*start' correctly.

Signed-off-by Baolin Wang <[email protected]>

---
   mm/filemap.c | 4 ++++
   1 file changed, 4 insertions(+)

mm/filemap.c +2 -0

index 4130be74f6fd..4c312aab8b1f 100644
@@ -2056,6 +2056,8 @@ unsigned find_get_entries(struct address_space
 *mapping, pgoff_t *start,
     folio = fbatch->folios[idx];
     if (!xa_is_value(folio))
   			nr = folio_nr_pages(folio);
+		else
+			nr = 1 << xa_get_order(&mapping->i_pages,
 indices[idx]);
    	*start = indices[idx] + nr;

    }
    return folio_batch_count(fbatch);

mm/filemap.c +2 -0

@@ -2120,6 +2122,8 @@ unsigned find_lock_entries(struct address_space
 *mapping, pgoff_t *start,
     folio = fbatch->folios[idx];
     if (!xa_is_value(folio))
   			nr = folio_nr_pages(folio);
+		else
+			nr = 1 << xa_get_order(&mapping->i_pages,
 indices[idx]);

Quoted 8 lines

Quoted 4 lines

mm/filemap.c +0 -0

    	*start = indices[idx] + nr;

    }
    return folio_batch_count(fbatch);

Here we have a problem, but I'm not suggesting a fix for it yet: I need to get other fixes out first, then turn to thinking about this - it's not easy.

Thanks for raising the issues.

Quoted 23 lines

That code is almost always right, so it works well enough for most people not to have noticed, but there are two issues with it.

The first issue is that it's assuming indices[idx] is already aligned to nr: not necessarily so. I believe it was already wrong in the folio_nr_pages() case, but the time I caught it wrong with a printk was in the swap (value) case. (I may not be stating this correctly:
again more thought needed but I can't spend so long writing.)

And immediately afterwards that kernel build failed with a corrupted (all 0s) .o file - I'm building on ext4 on /dev/loop0 on huge tmpfs while swapping, and happen to be using the "-o discard" option to ext4 mount.

I've been chasing these failures (maybe a few minutes in, maybe half an hour) for days, then had the idea of trying without the "-o discard":
whereupon these builds can be repeated successfully for many hours. IIRC ext4 discard to /dev/loop0 entails hole-punch to the tmpfs.

The alignment issue can easily be corrected, but that might not help. (And those two functions would look better with the rcu_read_unlock() moved down to just before returning: but again, wouldn't really help.)

Quoted 6 lines

The second issue is that swap is more slippery to work with than folios or pages: in the folio_nr_pages() case, that number is stable because we hold a refcount (which stops a THP from being split), and later we'll be taking folio lock too. None of that in the swap case, so (depending on how a large entry gets split) the xa_get_order() result is not reliable. Likewise for other uses of xa_get_order() in this series.

Quoted 4 lines

Now we have 2 users of xa_get_order() in this series:

1) shmem_partial_swap_usage(): this is acceptable, since racy results are not a problem for the swap statistics.

Yes: there might be room for improvement, but no big deal there.

Quoted 5 lines

2) shmem_undo_range(): when freeing a large swap entry, it will use xa_cmpxchg_irq() to make sure the swap value is not changed (in case the large swap entry is split). If failed to cmpxchg, then it will use current index to retry in shmem_undo_range(). So seems not too bad?

Right, I was missing the cmpxchg aspect. I am not entirely convinced of the safety in proceeding in this way, but I shouldn't spread FUD without justification. Today, no yesterday, I realized what might be the actual problem, and it's not at all these entry splitting races I had suspected.

Fix below. Successful testing on mm-everything-2024-08-24-07-21 (well, that minus the commit which spewed warnings from bootup) confirmed it. But testing on mm-everything-2024-08-28-21-38 very quickly failed:
unrelated to this series, presumably caused by patch or patches added since 08-24, one kind of crash on one machine (some memcg thing called from isolate_migratepages_block), another kind of crash on another (some memcg thing called from __read_swap_cache_async), I'm exhausted by now but will investigate later in the day (or hope someone else has).

[PATCH] mm: filemap: use xa_get_order() to get the swap entry order: fix

find_lock_entries(), used in the first pass of shmem_undo_range() and truncate_inode_pages_range() before partial folios are dealt with, has to be careful to avoid those partial folios: as its doc helpfully says, "Folios which are partially outside the range are not returned". Of course, the same must be true of any value entries returned, otherwise truncation and hole-punch risk erasing swapped areas - as has been seen.

Rewrite find_lock_entries() to emphasize that, following the same pattern for folios and for value entries.

Adjust find_get_entries() slightly, to get order while still holding rcu_read_lock(), and to round down the updated start: good changes, like find_lock_entries() now does, but it's unclear if either is ever important.

Signed-off-by Hugh Dickins <[email protected]>

---
  mm/filemap.c | 41 +++++++++++++++++++++++++----------------
  1 file changed, 25 insertions(+), 16 deletions(-)

mm/filemap.c +25 -16

index 885a8ed9d00d..88a2ed008474 100644
@@ -2047,10 +2047,9 @@ unsigned find_get_entries(struct address_space *mapping, pgoff_t *start,
  		if (!folio_batch_add(fbatch, folio))
  			break;
  	}
-	rcu_read_unlock();
 
  	if (folio_batch_count(fbatch)) {
-		unsigned long nr = 1;
+		unsigned long nr;
  		int idx = folio_batch_count(fbatch) - 1;
 
  		folio = fbatch->folios[idx];

@@ -2058,8 +2057,10 @@ unsigned find_get_entries(struct address_space *mapping, pgoff_t *start,
  			nr = folio_nr_pages(folio);
  		else
  			nr = 1 << xa_get_order(&mapping->i_pages, indices[idx]);
-		*start = indices[idx] + nr;
+		*start = round_down(indices[idx] + nr, nr);
  	}
+	rcu_read_unlock();
+
  	return folio_batch_count(fbatch);
  }
 

@@ -2091,10 +2092,17 @@ unsigned find_lock_entries(struct address_space *mapping, pgoff_t *start,
 
  	rcu_read_lock();
  	while ((folio = find_get_entry(&xas, end, XA_PRESENT))) {
+		unsigned long base;
+		unsigned long nr;
+
  		if (!xa_is_value(folio)) {
-			if (folio->index < *start)
+			nr = folio_nr_pages(folio);
+			base = folio->index;
+			/* Omit large folio which begins before the start */
+			if (base < *start)
  				goto put;
-			if (folio_next_index(folio) - 1 > end)
+			/* Omit large folio which extends beyond the end */
+			if (base + nr - 1 > end)
  				goto put;
  			if (!folio_trylock(folio))
  				goto put;

@@ -2103,7 +2111,19 @@ unsigned find_lock_entries(struct address_space *mapping, pgoff_t *start,
  				goto unlock;
  			VM_BUG_ON_FOLIO(!folio_contains(folio, xas.xa_index),
  					folio);
+		} else {
+			nr = 1 << xa_get_order(&mapping->i_pages, xas.xa_index);
+			base = xas.xa_index & ~(nr - 1);
+			/* Omit order>0 value which begins before the start */
+			if (base < *start)
+				continue;
+			/* Omit order>0 value which extends beyond the end */
+			if (base + nr - 1 > end)
+				break;
  		}
+
+		/* Update start now so that last update is correct on return */
+		*start = base + nr;
  		indices[fbatch->nr] = xas.xa_index;
  		if (!folio_batch_add(fbatch, folio))
  			break;

@@ -2115,17 +2135,6 @@ unsigned find_lock_entries(struct address_space *mapping, pgoff_t *start,
  	}
  	rcu_read_unlock();
 
-	if (folio_batch_count(fbatch)) {
-		unsigned long nr = 1;
-		int idx = folio_batch_count(fbatch) - 1;
-
-		folio = fbatch->folios[idx];
-		if (!xa_is_value(folio))
-			nr = folio_nr_pages(folio);
-		else
-			nr = 1 << xa_get_order(&mapping->i_pages, indices[idx]);
-		*start = indices[idx] + nr;
-	}
  	return folio_batch_count(fbatch);
  }
 

On 2024/8/29 16:07, Hugh Dickins wrote:

Quoted 9 lines

Fix below. Successful testing on mm-everything-2024-08-24-07-21 (well, that minus the commit which spewed warnings from bootup) confirmed it. But testing on mm-everything-2024-08-28-21-38 very quickly failed:
unrelated to this series, presumably caused by patch or patches added since 08-24, one kind of crash on one machine (some memcg thing called from isolate_migratepages_block), another kind of crash on another (some memcg thing called from __read_swap_cache_async), I'm exhausted by now but will investigate later in the day (or hope someone else has).

I saw the isolate_migratepages_block crash issue on mm-everything-2024-08-28-09-32, and I reverted Kefeng's series "[PATCH 0/4]
mm: convert to folio_isolate_movable()", the isolate_migratepages_block issue seems to be resolved (at least I can not reproduce it).

And I have already pointed out some potential issues in Kefeng’s series[1]. Andrew has dropped this series from mm-everything-2024-08-28-21-38. However, you can still encounter the isolate_migratepages_block issue on mm-everything-2024-08-28-21-38, while I cannot, weird.

Quoted 13 lines

[PATCH] mm: filemap: use xa_get_order() to get the swap entry order: fix

find_lock_entries(), used in the first pass of shmem_undo_range() and truncate_inode_pages_range() before partial folios are dealt with, has to be careful to avoid those partial folios: as its doc helpfully says, "Folios which are partially outside the range are not returned". Of course, the same must be true of any value entries returned, otherwise truncation and hole-punch risk erasing swapped areas - as has been seen.

Rewrite find_lock_entries() to emphasize that, following the same pattern for folios and for value entries.

Quoted 5 lines

Adjust find_get_entries() slightly, to get order while still holding rcu_read_lock(), and to round down the updated start: good changes, like find_lock_entries() now does, but it's unclear if either is ever important.

Signed-off-by Hugh Dickins <[email protected]>

Thanks Hugh. The changes make sense to me.