btrfs: try to allocate larger folios for metadata

PATCH Superseded Qu Wenruo 4 participants 9 messages Jul 19, 2024 Btrfs file system

Share & tools

Qu Wenruo

author

2024-07-19 04:49 UTC #0 lore raw

v5 cover btrfs: try to allocate larger folios for metadata

[CHANGELOG]
Changlog:
v5:
- Use root memcgroup to attach folios to btree inode filemap
- Only try higher order folio once without NOFAIL nor extra retry

v4:
- Hide the feature behind CONFIG_BTRFS_DEBUG
So that end users won't be affected (aka, still per-page based allocation) meanwhile we can do more testing on this new behavior.

v3:
- Rebased to the latest for-next branch
- Use PAGE_ALLOC_COSTLY_ORDER to determine whether to use __GFP_NOFAIL
- Add a dependency MM patch "mm/page_alloc: unify the warning on NOFAIL and high order allocation"
This allows us to use NOFAIL up to 32K nodesize, and makes sure for default 16K nodesize, all metadata would go 16K folios

v2:
- Rebased to handle the change in "btrfs: cache folio size and shift in extent_buffer"

This is the latest update on the attempt to utilize larger folios for btrfs metadata.

The previous version exposed a reproducibe hang at btrfs/187, where we hang at filemap_add_folio() around its memcgroup charge code.

Even without the problem, I still believe for btree inode we do not really need all the memcgroup charge, nor using __GFP_NOFAIL to work around the possible memcgroup limits.

So in this update, suggested by the memcgroup people from SUSE, there is a new patch to make btree inode filemap folio attaching to use the root memcgroup, so that we won't be limited by the memcgroup.

Then for the patch enabling the larger folio, I reverted back to the old behavior that we only try larger folio once without extra retry, just to be extra safe.

Qu Wenruo (2):
btrfs: always uses root memcgroup for filemap_add_folio()
btrfs: prefer to allocate larger folio for metadata

 fs/btrfs/extent_io.c | 112 ++++++++++++++++++++++++++++++-------------
 1 file changed, 78 insertions(+), 34 deletions(-)

Signature · 1 line

2.45.2

Qu Wenruo

author

2024-07-19 04:49 UTC #1 lore raw

PATCH v5 1/2 btrfs: always uses root memcgroup for filemap_add_folio()

The function filemap_add_folio() would also charge the memory cgroup, as all filemap folios are controlled by memory cgroup.
This also means, if there is some memory cgroup set, we're only relying on the __GFP_NOFAIL flag to prevent mem cgroup to return -ENOMEM.

However this is not really that suitable for btrfs btree inode, as the btree inode is not accessible out of btrfs module, and we do not want the memory limits on this critical part of btrfs metadata allocation.

So here we just manually set the active memory cgroup to the root one, which has no memory limits or whatever, before calling filemap_add_folio(), then restore to the old memcgroup.

By this we avoid the unnecessary memory limits and can later remove the __GFP_NOFAIL usage.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/extent_io.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

fs/btrfs/extent_io.c +10 -0

index aa7f8148cd0d..cfeed7673009 100644
@@ -2971,6 +2971,7 @@ static int attach_eb_folio_to_filemap(struct extent_buffer *eb, int i,
 
 	struct btrfs_fs_info *fs_info = eb->fs_info;
 	struct address_space *mapping = fs_info->btree_inode->i_mapping;
+	struct mem_cgroup *old_memcg;
 	const unsigned long index = eb->start >> PAGE_SHIFT;
 	struct folio *existing_folio = NULL;
 	int ret;

@@ -2981,8 +2982,17 @@ static int attach_eb_folio_to_filemap(struct extent_buffer *eb, int i,
 	ASSERT(eb->folios[i]);
 
 retry:
+	/*
+	 * Btree inode is a btrfs internal inode, and not exposed to any
+	 * user.
+	 * Furthermore we do not want any cgroup limits on this inode.
+	 * So we always use root_mem_cgroup as our active memcg when attaching
+	 * the folios.
+	 */
+	old_memcg = set_active_memcg(root_mem_cgroup);
 	ret = filemap_add_folio(mapping, eb->folios[i], index + i,
 				GFP_NOFS | __GFP_NOFAIL);
+	set_active_memcg(old_memcg);
 	if (!ret)
 		goto finish;

Signature · 1 line

2.45.2

Michal Hocko

2024-07-19 07:08 UTC #2 lore raw

Re: [PATCH v5 1/2] btrfs: always uses root memcgroup for filemap_add_folio()

On Fri 19-07-24 14:19:05, Qu Wenruo wrote:
[...]

Quoted 5 lines

fs/btrfs/extent_io.c +1 -0

@@ -2981,8 +2982,17 @@ static int attach_eb_folio_to_filemap(struct extent_buffer *eb, int i,
 	ASSERT(eb->folios[i]);
 
 retry:
+	/*

fs/btrfs/extent_io.c static int attach_eb_folio_to_filemap(struct extent_buffer *eb, int i, · line 2995

Quoted 10 lines

fs/btrfs/extent_io.c +8 -0

+	 * Btree inode is a btrfs internal inode, and not exposed to any
+	 * user.
+	 * Furthermore we do not want any cgroup limits on this inode.
+	 * So we always use root_mem_cgroup as our active memcg when attaching
+	 * the folios.
+	 */
+	old_memcg = set_active_memcg(root_mem_cgroup);
 	ret = filemap_add_folio(mapping, eb->folios[i], index + i,
 				GFP_NOFS | __GFP_NOFAIL);
+	set_active_memcg(old_memcg);

I do not think this will compile with CONFIG_MEMCG=n

Quoted 5 lines

fs/btrfs/extent_io.c +0 -0
 	if (!ret)
 		goto finish;
 
Signature · 1 line
2.45.2

Signature · 2 lines

Michal Hocko
SUSE Labs

Qu Wenruo

2024-07-19 07:17 UTC #3 lore raw

Re: [PATCH v5 1/2] btrfs: always uses root memcgroup for filemap_add_folio()

Quoted 5 lines · Michal Hocko

On Fri 19-07-24 14:19:05, Qu Wenruo wrote:
[...]
Quoted 3 lines
fs/btrfs/extent_io.c +0 -0
@@ -2981,8 +2982,17 @@ static int attach_eb_folio_to_filemap(struct extent_buffer *eb, int i,
  	ASSERT(eb->folios[i]);
 

Quoted 14 lines

fs/btrfs/extent_io.c static int attach_eb_folio_to_filemap(struct extent_buffer *eb, int i, · line 2995

Quoted 12 lines

fs/btrfs/extent_io.c +9 -0

  retry:
+	/*
+	 * Btree inode is a btrfs internal inode, and not exposed to any
+	 * user.
+	 * Furthermore we do not want any cgroup limits on this inode.
+	 * So we always use root_mem_cgroup as our active memcg when attaching
+	 * the folios.
+	 */
+	old_memcg = set_active_memcg(root_mem_cgroup);
  	ret = filemap_add_folio(mapping, eb->folios[i], index + i,
  				GFP_NOFS | __GFP_NOFAIL);
+	set_active_memcg(old_memcg);

I do not think this will compile with CONFIG_MEMCG=n

My bad.

I'll fix it from the btrfs part so that @root_mem_cgroup would be changed to NULL for the CONFIG_MEMCG=n case.

Meanwhile, can we just define root_mem_cgroup as NULL globally? That looks more reasonable to me, and if that's fine I can send out an extra patch just for that.

Thanks,
Qu

Quoted 7 lines

Quoted 6 lines
fs/btrfs/extent_io.c +0 -1
  	if (!ret)
  		goto finish;
 
--
2.45.2

Qu Wenruo

author

2024-07-19 07:20 UTC #4 lore raw

Re: [PATCH v5 1/2] btrfs: always uses root memcgroup for filemap_add_folio()

Quoted 25 lines · Qu Wenruo

Quoted 6 lines · Michal Hocko
On Fri 19-07-24 14:19:05, Qu Wenruo wrote:
[...]
Quoted 4 lines
fs/btrfs/extent_io.c +0 -0
@@ -2981,8 +2982,17 @@ static int attach_eb_folio_to_filemap(struct 
 extent_buffer *eb, int i,
       ASSERT(eb->folios[i]);
 
Quoted 15 lines
Quoted 5 lines
retry:
+    /*
+     * Btree inode is a btrfs internal inode, and not exposed to any +     * user.
+     * Furthermore we do not want any cgroup limits on this inode.
Quoted 8 lines
+     * So we always use root_mem_cgroup as our active memcg when attaching
+     * the folios.
+     */
+    old_memcg = set_active_memcg(root_mem_cgroup);
      ret = filemap_add_folio(mapping, eb->folios[i], index + i, GFP_NOFS | __GFP_NOFAIL);
+    set_active_memcg(old_memcg);
I do not think this will compile with CONFIG_MEMCG=n

Quoted 6 lines

My bad.
I'll fix it from the btrfs part so that @root_mem_cgroup would be changed to NULL for the CONFIG_MEMCG=n case.
Meanwhile, can we just define root_mem_cgroup as NULL globally?

Of course, I mean for the CONFIG_MEMCG=n case.

Thanks,
Qu

Quoted 13 lines

That looks more reasonable to me, and if that's fine I can send out an extra patch just for that.
Thanks,
Qu
Quoted 7 lines
Quoted 6 lines
if (!ret)
goto finish;
Signature · 1 line
2.45.2

Michal Hocko

2024-07-19 07:26 UTC #5 lore raw

Re: [PATCH v5 1/2] btrfs: always uses root memcgroup for filemap_add_folio()

In reply to Qu Wenruo

Quoted 3 lines · Qu Wenruo

Meanwhile, can we just define root_mem_cgroup as NULL globally? That looks more reasonable to me, and if that's fine I can send out an extra patch just for that.

I think it would be sufficient to pull root_mem_cgroup declaration out of ifdef in memcontrol.h. We do not really have to define it. Please send the patch and add all memcg maintainers. Ideally with this patch in the series so that the intention is clear.

Signature · 2 lines

Michal Hocko
SUSE Labs

kernel test robot

build bot

2024-07-19 15:42 UTC #6 lore raw

Re: [PATCH v5 1/2] btrfs: always uses root memcgroup for filemap_add_folio()

In reply to Qu Wenruo

kernel test robot noticed the following build errors:

Hi Qu,

kernel test robot noticed the following build errors:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on linus/master v6.10 next-20240719] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Qu-Wenruo/btrfs-always-uses-root-memcgroup-for-filemap_add_folio/20240719-125131
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link:    https://lore.kernel.org/r/d408a1b35307101e82a6845e26af4a122c8e5a25.1721363035.git.wqu%40suse.com
patch subject: [PATCH v5 1/2] btrfs: always uses root memcgroup for filemap_add_folio()
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20240719/[email protected]/config)
compiler: sh4-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240719/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-samsung.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-semitek.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-sjoy.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-sony.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-speedlink.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-steam.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-steelseries.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-sunplus.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-gaff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-tmff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-tivo.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-topseed.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-twinhan.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-uclogic.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-xinmo.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-zpff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-zydacron.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-viewsonic.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-waltop.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-winwing.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/fbtft/fbtft.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-bootrom.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-spilib.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-hid.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-light.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-log.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-loopback.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-power-supply.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-raw.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-vibrator.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-audio-manager.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-gbphy.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-gpio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-i2c.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-pwm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-sdio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-spi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-uart.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-usb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/platform/chrome/cros_kunit_proto_test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mailbox/mtk-cmdq-mailbox.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_simpleondemand.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_performance.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_powersave.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_userspace.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/perf/arm-ccn.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/perf/fsl_imx8_ddr_perf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/perf/arm_cspmu/arm_cspmu_module.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/perf/arm_cspmu/nvidia_cspmu.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/perf/arm_cspmu/ampere_cspmu.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvmem/nvmem-apple-efuses.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvmem/nvmem_brcm_nvram.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvmem/nvmem_u-boot-env.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/interconnect/imx/imx-interconnect.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/interconnect/imx/imx8mm-interconnect.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/interconnect/imx/imx8mq-interconnect.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/interconnect/imx/imx8mn-interconnect.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/interconnect/imx/imx8mp-interconnect.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hte/hte-tegra194-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/vdpa/vdpa.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/parport/parport.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/parsers/brcm_u-boot.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/parsers/tplink_safeloader.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/chips/cfi_util.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/chips/cfi_cmdset_0020.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/maps/map_funcs.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/spmi/hisi-spmi-controller.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/spmi/spmi-pmic-arb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/pcmcia/pcmcia_rsrc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwmon/corsair-cpro.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwmon/mr75203.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/greybus/greybus.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/greybus/gb-es2.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/rpmsg/rpmsg_char.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/iio/adc/ingenic-adc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/iio/adc/xilinx-ams.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/iio/buffer/kfifo_buf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-hub.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-aspeed.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-gpio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-ast-cf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-scom.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/siox/siox-bus-gpio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/counter/ftm-quaddec.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/vfio-mdev/mtty.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/vfio-mdev/mdpy.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/vfio-mdev/mdpy-fb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/vfio-mdev/mbochs.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/configfs/configfs_sample.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/bytestream-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/dma-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/inttype-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/record-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kobject/kobject-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kobject/kset-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kprobes/kprobe_example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kprobes/kretprobe_example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kmemleak/kmemleak-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in arch/sh/drivers/push-switch.o

>> ERROR: modpost: "root_mem_cgroup" [fs/btrfs/btrfs.ko] undefined!

ERROR: modpost: "__delay" [drivers/net/mdio/mdio-cavium.ko] undefined!
ERROR: modpost: "devm_of_clk_add_hw_provider" [drivers/media/i2c/tc358746.ko] undefined!
ERROR: modpost: "devm_clk_hw_register" [drivers/media/i2c/tc358746.ko] undefined!
ERROR: modpost: "of_clk_hw_simple_get" [drivers/media/i2c/tc358746.ko] undefined!

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

kernel test robot

build bot

2024-07-19 15:42 UTC #7 lore raw

Re: [PATCH v5 1/2] btrfs: always uses root memcgroup for filemap_add_folio()

In reply to Qu Wenruo

kernel test robot noticed the following build errors:

Hi Qu,

kernel test robot noticed the following build errors:

url:    https://github.com/intel-lab-lkp/linux/commits/Qu-Wenruo/btrfs-always-uses-root-memcgroup-for-filemap_add_folio/20240719-125131
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link:    https://lore.kernel.org/r/d408a1b35307101e82a6845e26af4a122c8e5a25.1721363035.git.wqu%40suse.com
patch subject: [PATCH v5 1/2] btrfs: always uses root memcgroup for filemap_add_folio()
config: x86_64-buildonly-randconfig-002-20240719 (https://download.01.org/0day-ci/archive/20240719/[email protected]/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240719/[email protected]/reproduce)

All errors (new ones prefixed by >>):

>> fs/btrfs/extent_io.c:2992:31: error: use of undeclared identifier 'root_mem_cgroup'; did you mean 'parent_mem_cgroup'?
    2992 |         old_memcg = set_active_memcg(root_mem_cgroup);
         |                                      ^~~~~~~~~~~~~~~
         |                                      parent_mem_cgroup

include/linux/memcontrol.h:1288:34: note: 'parent_mem_cgroup' declared here
1288 | static inline struct mem_cgroup *parent_mem_cgroup(struct mem_cgroup *memcg)
| ^
1 error generated.

vim +2992 fs/btrfs/extent_io.c

2971
2972 struct btrfs_fs_info *fs_info = eb->fs_info;
2973 struct address_space *mapping = fs_info->btree_inode->i_mapping;
2974 struct mem_cgroup *old_memcg;
2975 const unsigned long index = eb->start >> PAGE_SHIFT;
2976 struct folio *existing_folio = NULL;
2977 int ret;
2978
2979 ASSERT(found_eb_ret);
2980
2981 /* Caller should ensure the folio exists. */
2982 ASSERT(eb->folios[i]);
2983
2984 retry:
2985 /*
2986 * Btree inode is a btrfs internal inode, and not exposed to any
2987 * user.
2988 * Furthermore we do not want any cgroup limits on this inode.
2989 * So we always use root_mem_cgroup as our active memcg when attaching
2990 * the folios.
2991 */

> 2992		old_memcg = set_active_memcg(root_mem_cgroup);

2993 ret = filemap_add_folio(mapping, eb->folios[i], index + i,
2994 GFP_NOFS | __GFP_NOFAIL);
2995 set_active_memcg(old_memcg);
2996 if (!ret)
2997 goto finish;
2998
2999 existing_folio = filemap_lock_folio(mapping, index + i);
3000 /* The page cache only exists for a very short time, just retry. */
3001 if (IS_ERR(existing_folio)) {
3002 existing_folio = NULL;
3003 goto retry;
3004 }
3005
3006 /* For now, we should only have single-page folios for btree inode. */
3007 ASSERT(folio_nr_pages(existing_folio) == 1);
3008
3009 if (folio_size(existing_folio) != eb->folio_size) {
3010 folio_unlock(existing_folio);
3011 folio_put(existing_folio);
3012 return -EAGAIN;
3013 }
3014
3015 finish:
3016 spin_lock(&mapping->i_private_lock);
3017 if (existing_folio && fs_info->nodesize < PAGE_SIZE) {
3018 /* We're going to reuse the existing page, can drop our folio now. */
3019 __free_page(folio_page(eb->folios[i], 0));
3020 eb->folios[i] = existing_folio;
3021 } else if (existing_folio) {
3022 struct extent_buffer *existing_eb;
3023
3024 existing_eb = grab_extent_buffer(fs_info,
3025 folio_page(existing_folio, 0));
3026 if (existing_eb) {
3027 /* The extent buffer still exists, we can use it directly. */
3028 *found_eb_ret = existing_eb;
3029 spin_unlock(&mapping->i_private_lock);
3030 folio_unlock(existing_folio);
3031 folio_put(existing_folio);
3032 return 1;
3033 }
3034 /* The extent buffer no longer exists, we can reuse the folio. */
3035 __free_page(folio_page(eb->folios[i], 0));
3036 eb->folios[i] = existing_folio;
3037 }
3038 eb->folio_size = folio_size(eb->folios[i]);
3039 eb->folio_shift = folio_shift(eb->folios[i]);
3040 /* Should not fail, as we have preallocated the memory. */
3041 ret = attach_extent_buffer_folio(eb, eb->folios[i], prealloc);
3042 ASSERT(!ret);
3043 /*
3044 * To inform we have an extra eb under allocation, so that
3045 * detach_extent_buffer_page() won't release the folio private when the
3046 * eb hasn't been inserted into radix tree yet.
3047 *
3048 * The ref will be decreased when the eb releases the page, in
3049 * detach_extent_buffer_page(). Thus needs no special handling in the
3050 * error path.
3051 */
3052 btrfs_folio_inc_eb_refs(fs_info, eb->folios[i]);
3053 spin_unlock(&mapping->i_private_lock);
3054 return 0;
3055 }
3056

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Qu Wenruo

author

2024-07-19 04:49 UTC #8 lore raw

PATCH v5 2/2 btrfs: prefer to allocate larger folio for metadata

In reply to Qu Wenruo

For btrfs metadata, the high order folios are only utilized when all the following conditions are met:

- The extent buffer start is aligned to nodesize
This should be the common case for any btrfs in the last 5 years.

- The nodesize is larger than page size
Or there is no need to use larger folios at all.

- MM layer can fulfill our folio allocation request

- The larger folio must exactly cover the extent buffer No longer no smaller, must be an exact fit.

This is to make extent buffer accessors much easier.
They only need to check the first slot in eb->folios[], to determine their access unit (need per-page handling or a large folio covering the whole eb).

There is another small blockage, filemap APIs can not guarantee the folio size.
For example, by default we go 16K nodesize on x86_64, meaning a larger folio we expect would be with order 2 (size 16K).
We don't accept 2 order 1 (size 8K) folios, or we fall back to 4 order 0 (page sized) folios.

So here we go a different workaround, allocate a order 2 folio first, then attach them to the filemap of metadata.

Thus here comes several results related to the attach attempt of eb folios:

1) We can attach the pre-allocated eb folio to filemap This is the most simple and hot path, we just continue our work setting up the extent buffer.

2) There is an existing folio in the filemap

2.0) Subpage case
We would reuse the folio no matter what, subpage is doing a different way handling folio->private (a bitmap other than a pointer to an existing eb).

2.1) There is already a live extent buffer attached to the filemap
folio
This should be more or less hot path, we grab the existing eb and free the current one.

2.2) No live eb.
2.2.1) The filemap folio is larger than eb folio
This is a better case, we can reuse the filemap folio, but we need to cleanup all the pre-allocated folios of the new eb before reusing.
Later code should take the folio size change into consideration.

2.2.2) The filemap folio is the same size of eb folio
We just free the current folio, and reuse the filemap one. No other special handling needed.

2.2.3) The filemap folio is smaller than eb folio
This is the most tricky corner case, we can not easily replace the folio in filemap using our eb folio.

Thus here we return -EAGAIN, to inform our caller to re-try with order 0 (of course with our larger folio freed).

Otherwise all the needed infrastructure is already here, we only need to try allocate larger folio as our first try in alloc_eb_folio_array().

For now, the higher order allocation is only a preferable attempt for debug build, before we had enough test coverage and push it to end users.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/extent_io.c | 102 ++++++++++++++++++++++++++++---------------
 1 file changed, 68 insertions(+), 34 deletions(-)

fs/btrfs/extent_io.c +68 ~~-34~~

index cfeed7673009..d7824644d593 100644
@@ -719,12 +719,28 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array,
  *
  * For now, the folios populated are always in order 0 (aka, single page).
  */
-static int alloc_eb_folio_array(struct extent_buffer *eb, bool nofail)
+static int alloc_eb_folio_array(struct extent_buffer *eb, int order,
+				bool nofail)
 {
 	struct page *page_array[INLINE_EXTENT_BUFFER_PAGES] = { 0 };
 	int num_pages = num_extent_pages(eb);
 	int ret;
 
+	if (order) {
+		gfp_t gfp;
+
+		if (order > 0)
+			gfp = GFP_NOFS | __GFP_NORETRY | __GFP_NOWARN;
+		else
+			gfp = nofail ? (GFP_NOFS | __GFP_NOFAIL) : GFP_NOFS;
+		eb->folios[0] = folio_alloc(gfp, order);
+		if (likely(eb->folios[0])) {
+			eb->folio_size = folio_size(eb->folios[0]);
+			eb->folio_shift = folio_shift(eb->folios[0]);
+			return 0;
+		}
+		/* Fallback to 0 order (single page) allocation. */
+	}
 	ret = btrfs_alloc_page_array(num_pages, page_array, nofail);
 	if (ret < 0)
 		return ret;

@@ -2707,7 +2723,7 @@ struct extent_buffer *btrfs_clone_extent_buffer(const struct extent_buffer *src)
 	 */
 	set_bit(EXTENT_BUFFER_UNMAPPED, &new->bflags);
 
-	ret = alloc_eb_folio_array(new, false);
+	ret = alloc_eb_folio_array(new, 0, false);
 	if (ret) {
 		btrfs_release_extent_buffer(new);
 		return NULL;

@@ -2740,7 +2756,7 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
 	if (!eb)
 		return NULL;
 
-	ret = alloc_eb_folio_array(eb, false);
+	ret = alloc_eb_folio_array(eb, 0, false);
 	if (ret)
 		goto err;

@@ -2955,6 +2971,14 @@ static int check_eb_alignment(struct btrfs_fs_info *fs_info, u64 start)
 	return 0;
 }
 
+static void free_all_eb_folios(struct extent_buffer *eb)
+{
+	for (int i = 0; i < INLINE_EXTENT_BUFFER_PAGES; i++) {
+		if (eb->folios[i])
+			folio_put(eb->folios[i]);
+		eb->folios[i] = NULL;
+	}
+}
 
 /*
  * Return 0 if eb->folios[i] is attached to btree inode successfully.

@@ -2974,6 +2998,7 @@ static int attach_eb_folio_to_filemap(struct extent_buffer *eb, int i,
 	struct mem_cgroup *old_memcg;
 	const unsigned long index = eb->start >> PAGE_SHIFT;
 	struct folio *existing_folio = NULL;
+	const int eb_order = folio_order(eb->folios[0]);
 	int ret;
 
 	ASSERT(found_eb_ret);

@@ -3003,15 +3028,6 @@ static int attach_eb_folio_to_filemap(struct extent_buffer *eb, int i,
 		goto retry;
 	}
 
-	/* For now, we should only have single-page folios for btree inode. */
-	ASSERT(folio_nr_pages(existing_folio) == 1);
-
-	if (folio_size(existing_folio) != eb->folio_size) {
-		folio_unlock(existing_folio);
-		folio_put(existing_folio);
-		return -EAGAIN;
-	}
-
 finish:
 	spin_lock(&mapping->i_private_lock);
 	if (existing_folio && fs_info->nodesize < PAGE_SIZE) {

@@ -3020,6 +3036,7 @@ static int attach_eb_folio_to_filemap(struct extent_buffer *eb, int i,
 		eb->folios[i] = existing_folio;
 	} else if (existing_folio) {
 		struct extent_buffer *existing_eb;
+		int existing_order = folio_order(existing_folio);
 
 		existing_eb = grab_extent_buffer(fs_info,
 						 folio_page(existing_folio, 0));

@@ -3031,9 +3048,34 @@ static int attach_eb_folio_to_filemap(struct extent_buffer *eb, int i,
 			folio_put(existing_folio);
 			return 1;
 		}
-		/* The extent buffer no longer exists, we can reuse the folio. */
-		__free_page(folio_page(eb->folios[i], 0));
-		eb->folios[i] = existing_folio;
+		if (existing_order > eb_order) {
+			/*
+			 * The existing one has higher order, we need to drop
+			 * all eb folios before resuing it.
+			 * And this should only happen for the first folio.
+			 */
+			ASSERT(i == 0);
+			free_all_eb_folios(eb);
+			eb->folios[i] = existing_folio;
+		} else if (existing_order == eb_order) {
+			/*
+			 * Can safely reuse the filemap folio, just
+			 * release the eb one.
+			 */
+			folio_put(eb->folios[i]);
+			eb->folios[i] = existing_folio;
+		} else {
+			/*
+			 * The existing one has lower order.
+			 *
+			 * Just retry and fallback to order 0.
+			 */
+			ASSERT(i == 0);
+			folio_unlock(existing_folio);
+			folio_put(existing_folio);
+			spin_unlock(&mapping->i_private_lock);
+			return -EAGAIN;
+		}
 	}
 	eb->folio_size = folio_size(eb->folios[i]);
 	eb->folio_shift = folio_shift(eb->folios[i]);

@@ -3066,6 +3108,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 	u64 lockdep_owner = owner_root;
 	bool page_contig = true;
 	int uptodate = 1;
+	int order = 0;
 	int ret;
 
 	if (check_eb_alignment(fs_info, start))

@@ -3082,6 +3125,10 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 		btrfs_warn_32bit_limit(fs_info);
 #endif
 
+	if (IS_ENABLED(CONFIG_BTRFS_DEBUG) && fs_info->nodesize > PAGE_SIZE &&
+	    IS_ALIGNED(start, fs_info->nodesize))
+		order = ilog2(fs_info->nodesize >> PAGE_SHIFT);
+
 	eb = find_extent_buffer(fs_info, start);
 	if (eb)
 		return eb;

@@ -3116,7 +3163,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 
 reallocate:
 	/* Allocate all pages first. */
-	ret = alloc_eb_folio_array(eb, true);
+	ret = alloc_eb_folio_array(eb, order, true);
 	if (ret < 0) {
 		btrfs_free_subpage(prealloc);
 		goto out;

@@ -3134,26 +3181,12 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 		}
 
 		/*
-		 * TODO: Special handling for a corner case where the order of
-		 * folios mismatch between the new eb and filemap.
-		 *
-		 * This happens when:
-		 *
-		 * - the new eb is using higher order folio
-		 *
-		 * - the filemap is still using 0-order folios for the range
-		 *   This can happen at the previous eb allocation, and we don't
-		 *   have higher order folio for the call.
-		 *
-		 * - the existing eb has already been freed
-		 *
-		 * In this case, we have to free the existing folios first, and
-		 * re-allocate using the same order.
-		 * Thankfully this is not going to happen yet, as we're still
-		 * using 0-order folios.
+		 * Got a corner case where the existing folio is lower order,
+		 * fallback to 0 order and retry.
 		 */
 		if (unlikely(ret == -EAGAIN)) {
-			ASSERT(0);
+			order = 0;
+			free_all_eb_folios(eb);
 			goto reallocate;
 		}
 		attached++;

@@ -3164,6 +3197,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 		 * and free the allocated page.
 		 */
 		folio = eb->folios[i];
+		num_folios = num_extent_folios(eb);
 		WARN_ON(btrfs_folio_test_dirty(fs_info, folio, eb->start, eb->len));
 
 		/*

Signature · 1 line

2.45.2