| Message ID | 20240625090646.1194644-1-gshan@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | mm/filemap: Limit page cache size to that supported by xarray | expand |
On Tue, 25 Jun 2024 19:06:42 +1000 Gavin Shan <gshan@redhat.com> wrote: > Currently, xarray can't support arbitrary page cache size. More details > can be found from the WARN_ON() statement in xas_split_alloc(). In our > test whose code is attached below, we hit the WARN_ON() on ARM64 system > where the base page size is 64KB and huge page size is 512MB. The issue > was reported long time ago and some discussions on it can be found here > [1]. > > [1] https://www.spinics.net/lists/linux-xfs/msg75404.html > > In order to fix the issue, we need to adjust MAX_PAGECACHE_ORDER to one > supported by xarray and avoid PMD-sized page cache if needed. The code > changes are suggested by David Hildenbrand. > > PATCH[1] adjusts MAX_PAGECACHE_ORDER to that supported by xarray > PATCH[2-3] avoids PMD-sized page cache in the synchronous readahead path > PATCH[4] avoids PMD-sized page cache for shmem files if needed Questions on the timing of these. 1&2 are cc:stable whereas 3&4 are not. I could split them and feed 1&2 into 6.10-rcX and 3&4 into 6.11-rc1. A problem with this approach is that we're putting a basically untested combination into -stable: 1&2 might have bugs which were accidentally fixed in 3&4. A way to avoid this is to add cc:stable to all four patches. What are your thoughts on this matter?
On 25.06.24 20:37, Andrew Morton wrote: > On Tue, 25 Jun 2024 19:06:42 +1000 Gavin Shan <gshan@redhat.com> wrote: > >> Currently, xarray can't support arbitrary page cache size. More details >> can be found from the WARN_ON() statement in xas_split_alloc(). In our >> test whose code is attached below, we hit the WARN_ON() on ARM64 system >> where the base page size is 64KB and huge page size is 512MB. The issue >> was reported long time ago and some discussions on it can be found here >> [1]. >> >> [1] https://www.spinics.net/lists/linux-xfs/msg75404.html >> >> In order to fix the issue, we need to adjust MAX_PAGECACHE_ORDER to one >> supported by xarray and avoid PMD-sized page cache if needed. The code >> changes are suggested by David Hildenbrand. >> >> PATCH[1] adjusts MAX_PAGECACHE_ORDER to that supported by xarray >> PATCH[2-3] avoids PMD-sized page cache in the synchronous readahead path >> PATCH[4] avoids PMD-sized page cache for shmem files if needed > > Questions on the timing of these. > > 1&2 are cc:stable whereas 3&4 are not. > > I could split them and feed 1&2 into 6.10-rcX and 3&4 into 6.11-rc1. A > problem with this approach is that we're putting a basically untested > combination into -stable: 1&2 might have bugs which were accidentally > fixed in 3&4. A way to avoid this is to add cc:stable to all four > patches. > > What are your thoughts on this matter? Especially 4 should also be CC stable, so likely we should just do it for all of them.
On Tue, 25 Jun 2024 20:51:13 +0200 David Hildenbrand <david@redhat.com> wrote: > > I could split them and feed 1&2 into 6.10-rcX and 3&4 into 6.11-rc1. A > > problem with this approach is that we're putting a basically untested > > combination into -stable: 1&2 might have bugs which were accidentally > > fixed in 3&4. A way to avoid this is to add cc:stable to all four > > patches. > > > > What are your thoughts on this matter? > > Especially 4 should also be CC stable, so likely we should just do it > for all of them. Fine. A Fixes: for 3 & 4 would be good. Otherwise we're potentially asking for those to be backported further than 1 & 2, which seems wrong. Then again, by having different Fixes: in the various patches we're suggesting that people split the patch series apart as they slot things into the indicated places. In other words, it's not a patch series at all - it's a sprinkle of independent fixes. Are we OK thinking of it in that fashion?
On 25.06.24 20:58, Andrew Morton wrote: > On Tue, 25 Jun 2024 20:51:13 +0200 David Hildenbrand <david@redhat.com> wrote: > >>> I could split them and feed 1&2 into 6.10-rcX and 3&4 into 6.11-rc1. A >>> problem with this approach is that we're putting a basically untested >>> combination into -stable: 1&2 might have bugs which were accidentally >>> fixed in 3&4. A way to avoid this is to add cc:stable to all four >>> patches. >>> >>> What are your thoughts on this matter? >> >> Especially 4 should also be CC stable, so likely we should just do it >> for all of them. > > Fine. A Fixes: for 3 & 4 would be good. Otherwise we're potentially > asking for those to be backported further than 1 & 2, which seems > wrong. 4 is shmem fix, which likely dates back a bit longer. > > Then again, by having different Fixes: in the various patches we're > suggesting that people split the patch series apart as they slot things > into the indicated places. In other words, it's not a patch series at > all - it's a sprinkle of independent fixes. Are we OK thinking of it > in that fashion? The common themes is "pagecache cannot handle > order-11", #1-3 tackle "ordinary" file THP, #4 tackles shmem THP. So I'm not sure we should be splitting it apart. It's just that shmem THP arrived before file THP :)
On 6/26/24 5:05 AM, David Hildenbrand wrote: > On 25.06.24 20:58, Andrew Morton wrote: >> On Tue, 25 Jun 2024 20:51:13 +0200 David Hildenbrand <david@redhat.com> wrote: >> >>>> I could split them and feed 1&2 into 6.10-rcX and 3&4 into 6.11-rc1. A >>>> problem with this approach is that we're putting a basically untested >>>> combination into -stable: 1&2 might have bugs which were accidentally >>>> fixed in 3&4. A way to avoid this is to add cc:stable to all four >>>> patches. >>>> >>>> What are your thoughts on this matter? >>> >>> Especially 4 should also be CC stable, so likely we should just do it >>> for all of them. >> >> Fine. A Fixes: for 3 & 4 would be good. Otherwise we're potentially >> asking for those to be backported further than 1 & 2, which seems >> wrong. > > 4 is shmem fix, which likely dates back a bit longer. > >> >> Then again, by having different Fixes: in the various patches we're >> suggesting that people split the patch series apart as they slot things >> into the indicated places. In other words, it's not a patch series at >> all - it's a sprinkle of independent fixes. Are we OK thinking of it >> in that fashion? > > The common themes is "pagecache cannot handle > order-11", #1-3 tackle "ordinary" file THP, #4 tackles shmem THP. > > So I'm not sure we should be splitting it apart. It's just that shmem THP arrived before file THP :) > I rechecked the history, it's a bit hard to have precise fix tag for PATCH[4]. Please let me know if you have a better one for PATCH[4]. #4 Fixes: 800d8c63b2e9 ("shmem: add huge pages support") Cc: stable@kernel.org # v4.10+ Fixes: 552446a41661 ("shmem: Convert shmem_add_to_page_cache to XArray") Cc: stable@kernel.org # v4.20+ #3 Fixes: 793917d997df ("mm/readahead: Add large folio readahead") Cc: stable@kernel.org # v5.18+ #2 Fixes: 4687fdbb805a ("mm/filemap: Support VM_HUGEPAGE for file mappings") Cc: stable@kernel.org # v5.18+ #1 Fixes: 793917d997df ("mm/readahead: Add large folio readahead") Cc: stable@kernel.org # v5.18+ I probably need to move PATCH[3] before PATCH[2] since PATCH[1] and PATCH[2] point to same commit. Thanks, Gavin
On Wed, 26 Jun 2024 10:37:00 +1000 Gavin Shan <gshan@redhat.com> wrote: > > I rechecked the history, it's a bit hard to have precise fix tag for PATCH[4]. > Please let me know if you have a better one for PATCH[4]. > > #4 > Fixes: 800d8c63b2e9 ("shmem: add huge pages support") > Cc: stable@kernel.org # v4.10+ > Fixes: 552446a41661 ("shmem: Convert shmem_add_to_page_cache to XArray") > Cc: stable@kernel.org # v4.20+ > #3 > Fixes: 793917d997df ("mm/readahead: Add large folio readahead") > Cc: stable@kernel.org # v5.18+ > #2 > Fixes: 4687fdbb805a ("mm/filemap: Support VM_HUGEPAGE for file mappings") > Cc: stable@kernel.org # v5.18+ > #1 > Fixes: 793917d997df ("mm/readahead: Add large folio readahead") > Cc: stable@kernel.org # v5.18+ > > I probably need to move PATCH[3] before PATCH[2] since PATCH[1] and PATCH[2] > point to same commit. OK, thanks. I assume you'll be sending a new revision of the series. And Ryan had comments. Please incorporate the above into the updated changelogs as best you can.
On Wed, Jun 26, 2024 at 10:37:00AM +1000, Gavin Shan wrote: > On 6/26/24 5:05 AM, David Hildenbrand wrote: > > On 25.06.24 20:58, Andrew Morton wrote: > > > On Tue, 25 Jun 2024 20:51:13 +0200 David Hildenbrand <david@redhat.com> wrote: > > > > > > > > I could split them and feed 1&2 into 6.10-rcX and 3&4 into 6.11-rc1. A > > > > > problem with this approach is that we're putting a basically untested > > > > > combination into -stable: 1&2 might have bugs which were accidentally > > > > > fixed in 3&4. A way to avoid this is to add cc:stable to all four > > > > > patches. > > > > > > > > > > What are your thoughts on this matter? > > > > > > > > Especially 4 should also be CC stable, so likely we should just do it > > > > for all of them. > > > > > > Fine. A Fixes: for 3 & 4 would be good. Otherwise we're potentially > > > asking for those to be backported further than 1 & 2, which seems > > > wrong. > > > > 4 is shmem fix, which likely dates back a bit longer. > > > > > > > > Then again, by having different Fixes: in the various patches we're > > > suggesting that people split the patch series apart as they slot things > > > into the indicated places. In other words, it's not a patch series at > > > all - it's a sprinkle of independent fixes. Are we OK thinking of it > > > in that fashion? > > > > The common themes is "pagecache cannot handle > order-11", #1-3 tackle "ordinary" file THP, #4 tackles shmem THP. > > > > So I'm not sure we should be splitting it apart. It's just that shmem THP arrived before file THP :) > > > > I rechecked the history, it's a bit hard to have precise fix tag for PATCH[4]. > Please let me know if you have a better one for PATCH[4]. > > #4 > Fixes: 800d8c63b2e9 ("shmem: add huge pages support") > Cc: stable@kernel.org # v4.10+ > Fixes: 552446a41661 ("shmem: Convert shmem_add_to_page_cache to XArray") > Cc: stable@kernel.org # v4.20+ > #3 > Fixes: 793917d997df ("mm/readahead: Add large folio readahead") > Cc: stable@kernel.org # v5.18+ > #2 > Fixes: 4687fdbb805a ("mm/filemap: Support VM_HUGEPAGE for file mappings") > Cc: stable@kernel.org # v5.18+ > #1 > Fixes: 793917d997df ("mm/readahead: Add large folio readahead") > Cc: stable@kernel.org # v5.18+ I actually think it's this: commit 6b24ca4a1a8d Author: Matthew Wilcox (Oracle) <willy@infradead.org> Date: Sat Jun 27 22:19:08 2020 -0400 mm: Use multi-index entries in the page cache We currently store large folios as 2^N consecutive entries. While this consumes rather more memory than necessary, it also turns out to be buggy. A writeback operation which starts within a tail page of a dirty folio will not write back the folio as the xarray's dirty bit is only set on the head index. With multi-index entries, the dirty bit will be found no matter where in the folio the operation starts. This does end up simplifying the page cache slightly, although not as much as I had hoped. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Reviewed-by: William Kucharski <william.kucharski@oracle.com> Before this, we could split an arbitrary size folio to order 0. After it, we're limited to whatever the xarray allows us to split.
On 6/27/24 6:38 AM, Andrew Morton wrote: > On Wed, 26 Jun 2024 10:37:00 +1000 Gavin Shan <gshan@redhat.com> wrote: >> >> I rechecked the history, it's a bit hard to have precise fix tag for PATCH[4]. >> Please let me know if you have a better one for PATCH[4]. >> >> #4 >> Fixes: 800d8c63b2e9 ("shmem: add huge pages support") >> Cc: stable@kernel.org # v4.10+ >> Fixes: 552446a41661 ("shmem: Convert shmem_add_to_page_cache to XArray") >> Cc: stable@kernel.org # v4.20+ >> #3 >> Fixes: 793917d997df ("mm/readahead: Add large folio readahead") >> Cc: stable@kernel.org # v5.18+ >> #2 >> Fixes: 4687fdbb805a ("mm/filemap: Support VM_HUGEPAGE for file mappings") >> Cc: stable@kernel.org # v5.18+ >> #1 >> Fixes: 793917d997df ("mm/readahead: Add large folio readahead") >> Cc: stable@kernel.org # v5.18+ >> >> I probably need to move PATCH[3] before PATCH[2] since PATCH[1] and PATCH[2] >> point to same commit. > > OK, thanks. > > I assume you'll be sending a new revision of the series. And Ryan had > comments. Please incorporate the above into the updated changelogs as > best you can. > Yes, I will post a new revision where all pending comments will be addressed. Thanks, Gavin
On 6/27/24 6:54 AM, Matthew Wilcox wrote: > On Wed, Jun 26, 2024 at 10:37:00AM +1000, Gavin Shan wrote: >> On 6/26/24 5:05 AM, David Hildenbrand wrote: >>> On 25.06.24 20:58, Andrew Morton wrote: >>>> On Tue, 25 Jun 2024 20:51:13 +0200 David Hildenbrand <david@redhat.com> wrote: >>>> >>>>>> I could split them and feed 1&2 into 6.10-rcX and 3&4 into 6.11-rc1. A >>>>>> problem with this approach is that we're putting a basically untested >>>>>> combination into -stable: 1&2 might have bugs which were accidentally >>>>>> fixed in 3&4. A way to avoid this is to add cc:stable to all four >>>>>> patches. >>>>>> >>>>>> What are your thoughts on this matter? >>>>> >>>>> Especially 4 should also be CC stable, so likely we should just do it >>>>> for all of them. >>>> >>>> Fine. A Fixes: for 3 & 4 would be good. Otherwise we're potentially >>>> asking for those to be backported further than 1 & 2, which seems >>>> wrong. >>> >>> 4 is shmem fix, which likely dates back a bit longer. >>> >>>> >>>> Then again, by having different Fixes: in the various patches we're >>>> suggesting that people split the patch series apart as they slot things >>>> into the indicated places. In other words, it's not a patch series at >>>> all - it's a sprinkle of independent fixes. Are we OK thinking of it >>>> in that fashion? >>> >>> The common themes is "pagecache cannot handle > order-11", #1-3 tackle "ordinary" file THP, #4 tackles shmem THP. >>> >>> So I'm not sure we should be splitting it apart. It's just that shmem THP arrived before file THP :) >>> >> >> I rechecked the history, it's a bit hard to have precise fix tag for PATCH[4]. >> Please let me know if you have a better one for PATCH[4]. >> >> #4 >> Fixes: 800d8c63b2e9 ("shmem: add huge pages support") >> Cc: stable@kernel.org # v4.10+ >> Fixes: 552446a41661 ("shmem: Convert shmem_add_to_page_cache to XArray") >> Cc: stable@kernel.org # v4.20+ >> #3 >> Fixes: 793917d997df ("mm/readahead: Add large folio readahead") >> Cc: stable@kernel.org # v5.18+ >> #2 >> Fixes: 4687fdbb805a ("mm/filemap: Support VM_HUGEPAGE for file mappings") >> Cc: stable@kernel.org # v5.18+ >> #1 >> Fixes: 793917d997df ("mm/readahead: Add large folio readahead") >> Cc: stable@kernel.org # v5.18+ > > I actually think it's this: > > commit 6b24ca4a1a8d > Author: Matthew Wilcox (Oracle) <willy@infradead.org> > Date: Sat Jun 27 22:19:08 2020 -0400 > > mm: Use multi-index entries in the page cache > > We currently store large folios as 2^N consecutive entries. While this > consumes rather more memory than necessary, it also turns out to be buggy. > A writeback operation which starts within a tail page of a dirty folio will > not write back the folio as the xarray's dirty bit is only set on the > head index. With multi-index entries, the dirty bit will be found no > matter where in the folio the operation starts. > > This does end up simplifying the page cache slightly, although not as > much as I had hoped. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > Reviewed-by: William Kucharski <william.kucharski@oracle.com> > > Before this, we could split an arbitrary size folio to order 0. After > it, we're limited to whatever the xarray allows us to split. > Thanks, PATCH[4]'s fix tag will point to 6b24ca4a1a8d ("mm: Use multi-index entries in the page cache"), which was merged to v5.17. The fix tags for other patches are correct Thanks, Gavin
Currently, xarray can't support arbitrary page cache size. More details can be found from the WARN_ON() statement in xas_split_alloc(). In our test whose code is attached below, we hit the WARN_ON() on ARM64 system where the base page size is 64KB and huge page size is 512MB. The issue was reported long time ago and some discussions on it can be found here [1]. [1] https://www.spinics.net/lists/linux-xfs/msg75404.html In order to fix the issue, we need to adjust MAX_PAGECACHE_ORDER to one supported by xarray and avoid PMD-sized page cache if needed. The code changes are suggested by David Hildenbrand. PATCH[1] adjusts MAX_PAGECACHE_ORDER to that supported by xarray PATCH[2-3] avoids PMD-sized page cache in the synchronous readahead path PATCH[4] avoids PMD-sized page cache for shmem files if needed Test program ============ # cat test.c #define _GNU_SOURCE #include <stdio.h> #include <stdlib.h> #include <unistd.h> #include <string.h> #include <fcntl.h> #include <errno.h> #include <sys/syscall.h> #include <sys/mman.h> #define TEST_XFS_FILENAME "/tmp/data" #define TEST_SHMEM_FILENAME "/dev/shm/data" #define TEST_MEM_SIZE 0x20000000 int main(int argc, char **argv) { const char *filename; int fd = 0; void *buf = (void *)-1, *p; int pgsize = getpagesize(); int ret; if (pgsize != 0x10000) { fprintf(stderr, "64KB base page size is required\n"); return -EPERM; } system("echo force > /sys/kernel/mm/transparent_hugepage/shmem_enabled"); system("rm -fr /tmp/data"); system("rm -fr /dev/shm/data"); system("echo 1 > /proc/sys/vm/drop_caches"); /* Open xfs or shmem file */ filename = TEST_XFS_FILENAME; if (argc > 1 && !strcmp(argv[1], "shmem")) filename = TEST_SHMEM_FILENAME; fd = open(filename, O_CREAT | O_RDWR | O_TRUNC); if (fd < 0) { fprintf(stderr, "Unable to open <%s>\n", filename); return -EIO; } /* Extend file size */ ret = ftruncate(fd, TEST_MEM_SIZE); if (ret) { fprintf(stderr, "Error %d to ftruncate()\n", ret); goto cleanup; } /* Create VMA */ buf = mmap(NULL, TEST_MEM_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); if (buf == (void *)-1) { fprintf(stderr, "Unable to mmap <%s>\n", filename); goto cleanup; } fprintf(stdout, "mapped buffer at 0x%p\n", buf); ret = madvise(buf, TEST_MEM_SIZE, MADV_HUGEPAGE); if (ret) { fprintf(stderr, "Unable to madvise(MADV_HUGEPAGE)\n"); goto cleanup; } /* Populate VMA */ ret = madvise(buf, TEST_MEM_SIZE, MADV_POPULATE_WRITE); if (ret) { fprintf(stderr, "Error %d to madvise(MADV_POPULATE_WRITE)\n", ret); goto cleanup; } /* Punch the file to enforce xarray split */ ret = fallocate(fd, FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE, TEST_MEM_SIZE - pgsize, pgsize); if (ret) fprintf(stderr, "Error %d to fallocate()\n", ret); cleanup: if (buf != (void *)-1) munmap(buf, TEST_MEM_SIZE); if (fd > 0) close(fd); return 0; } # gcc test.c -o test # cat /proc/1/smaps | grep KernelPageSize | head -n 1 KernelPageSize: 64 kB # ./test shmem : ------------[ cut here ]------------ WARNING: CPU: 17 PID: 5253 at lib/xarray.c:1025 xas_split_alloc+0xf8/0x128 Modules linked in: nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib \ nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct \ nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 \ ip_set nf_tables rfkill nfnetlink vfat fat virtio_balloon \ drm fuse xfs libcrc32c crct10dif_ce ghash_ce sha2_ce sha256_arm64 \ virtio_net sha1_ce net_failover failover virtio_console virtio_blk \ dimlib virtio_mmio CPU: 17 PID: 5253 Comm: test Kdump: loaded Tainted: G W 6.10.0-rc5-gavin+ #12 Hardware name: QEMU KVM Virtual Machine, BIOS edk2-20240524-1.el9 05/24/2024 pstate: 83400005 (Nzcv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--) pc : xas_split_alloc+0xf8/0x128 lr : split_huge_page_to_list_to_order+0x1c4/0x720 sp : ffff80008a92f5b0 x29: ffff80008a92f5b0 x28: ffff80008a92f610 x27: ffff80008a92f728 x26: 0000000000000cc0 x25: 000000000000000d x24: ffff0000cf00c858 x23: ffff80008a92f610 x22: ffffffdfc0600000 x21: 0000000000000000 x20: 0000000000000000 x19: ffffffdfc0600000 x18: 0000000000000000 x17: 0000000000000000 x16: 0000018000000000 x15: 3374004000000000 x14: 0000e00000000000 x13: 0000000000002000 x12: 0000000000000020 x11: 3374000000000000 x10: 3374e1c0ffff6000 x9 : ffffb463a84c681c x8 : 0000000000000003 x7 : 0000000000000000 x6 : ffff00011c976ce0 x5 : ffffb463aa47e378 x4 : 0000000000000000 x3 : 0000000000000cc0 x2 : 000000000000000d x1 : 000000000000000c x0 : 0000000000000000 Call trace: xas_split_alloc+0xf8/0x128 split_huge_page_to_list_to_order+0x1c4/0x720 truncate_inode_partial_folio+0xdc/0x160 shmem_undo_range+0x2bc/0x6a8 shmem_fallocate+0x134/0x430 vfs_fallocate+0x124/0x2e8 ksys_fallocate+0x4c/0xa0 __arm64_sys_fallocate+0x24/0x38 invoke_syscall.constprop.0+0x7c/0xd8 do_el0_svc+0xb4/0xd0 el0_svc+0x44/0x1d8 el0t_64_sync_handler+0x134/0x150 el0t_64_sync+0x17c/0x180 Gavin Shan (4): mm/filemap: Make MAX_PAGECACHE_ORDER acceptable to xarray mm/filemap: Skip to allocate PMD-sized folios if needed mm/readahead: Limit page cache size in page_cache_ra_order() mm/shmem: Disable PMD-sized page cache if needed include/linux/pagemap.h | 11 +++++++++-- mm/filemap.c | 2 +- mm/readahead.c | 8 ++++---- mm/shmem.c | 15 +++++++++++++-- 4 files changed, 27 insertions(+), 9 deletions(-)