[PATCH 0/2] improve buffered write performance with fgf order hint

[PATCH 0/2] improve buffered write performance with fgf order hint

[Pankaj Raghav (Samsung)]

From: Pankaj Raghav <p.raghav@samsung.com>

Filesystems that support large folios can set the fgf order before
buffered write(see XFS iomap_write_begin()) that can provide as a hint
to page cache to allocate large folios, if possible.

The first patch is a minor cleanup.
The second patch sets fgf order before starting the buffered write.

I tested the performance on Samsung SSD 990 pro on a system with 64GB
RAM as follows:

$ bcachefs format -f /dev/nvme0n1;
$ mount -t bcachefs /dev/nvme0n1 /mnt
$ fio --name=bcachefs --filename=/mnt/test  --size=100G \
    --ioengine=io_uring --iodepth=16 --rw=write --bs=128k

I measured the BW(MB/s) across three runs on 6.10-rc3:
Before patches: 2730
After patches: 3430 (1.25x boost)

With -o no_data_io mount option:
Before patches: 2920
After patches: 4630 (1.5x boost)

I was not able to test the patches with ktest due to some issue with
debian(some broken package issue). Maybe Kent can run it in his CI while
I try to fix ktest locally?

Pankaj Raghav (2):
  bcachefs: use FGP_WRITEBEGIN instead of combining individual flags
  bcachefs: set fgf order hint before starting a buffered write

 fs/bcachefs/fs-io-buffered.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)


base-commit: 03d44168cbd7fc57d5de56a3730427db758fc7f6
-- 
2.44.1

[PATCH 1/2] bcachefs: use FGP_WRITEBEGIN instead of combining individual flags

[Pankaj Raghav (Samsung)]

From: Pankaj Raghav <p.raghav@samsung.com>

Use FGP_WRITEBEGIN to avoid repeating the individual FGP flags before
starting a buffered write.

Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
 fs/bcachefs/fs-io-buffered.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/bcachefs/fs-io-buffered.c b/fs/bcachefs/fs-io-buffered.c
index 54873ecc635c..865691dd0173 100644
--- a/fs/bcachefs/fs-io-buffered.c
+++ b/fs/bcachefs/fs-io-buffered.c
@@ -677,9 +677,8 @@ int bch2_write_begin(struct file *file, struct address_space *mapping,
 
 	bch2_pagecache_add_get(inode);
 
-	folio = __filemap_get_folio(mapping, pos >> PAGE_SHIFT,
-				FGP_LOCK|FGP_WRITE|FGP_CREAT|FGP_STABLE,
-				mapping_gfp_mask(mapping));
+	folio = __filemap_get_folio(mapping, pos >> PAGE_SHIFT, FGP_WRITEBEGIN,
+				    mapping_gfp_mask(mapping));
 	if (IS_ERR_OR_NULL(folio))
 		goto err_unlock;
 
@@ -820,9 +819,8 @@ static int __bch2_buffered_write(struct bch_inode_info *inode,
 	darray_init(&fs);
 
 	ret = bch2_filemap_get_contig_folios_d(mapping, pos, end,
-				   FGP_LOCK|FGP_WRITE|FGP_STABLE|FGP_CREAT,
-				   mapping_gfp_mask(mapping),
-				   &fs);
+					       FGP_WRITEBEGIN,
+					       mapping_gfp_mask(mapping), &fs);
 	if (ret)
 		goto out;
 
-- 
2.44.1

Re: [PATCH 1/2] bcachefs: use FGP_WRITEBEGIN instead of combining individual flags

[Matthew Wilcox]

On Fri, Jun 14, 2024 at 10:50:30AM +0000, Pankaj Raghav (Samsung) wrote:
>  	ret = bch2_filemap_get_contig_folios_d(mapping, pos, end,
> -				   FGP_LOCK|FGP_WRITE|FGP_STABLE|FGP_CREAT,
> -				   mapping_gfp_mask(mapping),
> -				   &fs);
> +					       FGP_WRITEBEGIN,
> +					       mapping_gfp_mask(mapping), &fs);

Don't change the indentation here.  In the next patch it makes the
lines too long.

In general, don't change the indentation.  It's been one of the biggest
pains of the folio conversion.  "Oh, you changed the name of this
function and now the arguments don't line up".  Don't line up the
arguments with the opening paren.

Re: [PATCH 1/2] bcachefs: use FGP_WRITEBEGIN instead of combining individual flags

[Pankaj Raghav (Samsung)]

On Fri, Jun 14, 2024 at 03:36:22PM +0100, Matthew Wilcox wrote:
> On Fri, Jun 14, 2024 at 10:50:30AM +0000, Pankaj Raghav (Samsung) wrote:
> >  	ret = bch2_filemap_get_contig_folios_d(mapping, pos, end,
> > -				   FGP_LOCK|FGP_WRITE|FGP_STABLE|FGP_CREAT,
> > -				   mapping_gfp_mask(mapping),
> > -				   &fs);
> > +					       FGP_WRITEBEGIN,
> > +					       mapping_gfp_mask(mapping), &fs);
> 
> Don't change the indentation here.  In the next patch it makes the
> lines too long.

I used clangd to do the formatting. I will probably stick without
changing the format in this patch

Thanks.
> 
> In general, don't change the indentation.  It's been one of the biggest
> pains of the folio conversion.  "Oh, you changed the name of this
> function and now the arguments don't line up".  Don't line up the
> arguments with the opening paren.

[PATCH 2/2] bcachefs: set fgf order hint before starting a buffered write

[Pankaj Raghav (Samsung)]

From: Pankaj Raghav <p.raghav@samsung.com>

Set the preferred folio order in the fgp_flags by calling
fgf_set_order(). Page cache will try to allocate large folio of the
preferred order whenever possible instead of allocating multiple 0 order
folios.

This improves the buffered write performance up to 1.25x with default
mount options and up to 1.57x when mounted with no_data_io option with
the following fio workload:

fio --name=bcachefs --filename=/mnt/test  --size=100G \
     --ioengine=io_uring --iodepth=16 --rw=write --bs=128k

Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
 fs/bcachefs/fs-io-buffered.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/bcachefs/fs-io-buffered.c b/fs/bcachefs/fs-io-buffered.c
index 865691dd0173..1355d618f988 100644
--- a/fs/bcachefs/fs-io-buffered.c
+++ b/fs/bcachefs/fs-io-buffered.c
@@ -677,7 +677,8 @@ int bch2_write_begin(struct file *file, struct address_space *mapping,
 
 	bch2_pagecache_add_get(inode);
 
-	folio = __filemap_get_folio(mapping, pos >> PAGE_SHIFT, FGP_WRITEBEGIN,
+	folio = __filemap_get_folio(mapping, pos >> PAGE_SHIFT,
+				    FGP_WRITEBEGIN | fgf_set_order(len),
 				    mapping_gfp_mask(mapping));
 	if (IS_ERR_OR_NULL(folio))
 		goto err_unlock;
@@ -819,7 +820,7 @@ static int __bch2_buffered_write(struct bch_inode_info *inode,
 	darray_init(&fs);
 
 	ret = bch2_filemap_get_contig_folios_d(mapping, pos, end,
-					       FGP_WRITEBEGIN,
+					       FGP_WRITEBEGIN | fgf_set_order(len),
 					       mapping_gfp_mask(mapping), &fs);
 	if (ret)
 		goto out;
-- 
2.44.1

Re: [PATCH 0/2] improve buffered write performance with fgf order hint

[Kent Overstreet]

On Fri, Jun 14, 2024 at 10:50:29AM +0000, Pankaj Raghav (Samsung) wrote:
> From: Pankaj Raghav <p.raghav@samsung.com>
> 
> Filesystems that support large folios can set the fgf order before
> buffered write(see XFS iomap_write_begin()) that can provide as a hint
> to page cache to allocate large folios, if possible.
> 
> The first patch is a minor cleanup.
> The second patch sets fgf order before starting the buffered write.
> 
> I tested the performance on Samsung SSD 990 pro on a system with 64GB
> RAM as follows:
> 
> $ bcachefs format -f /dev/nvme0n1;
> $ mount -t bcachefs /dev/nvme0n1 /mnt
> $ fio --name=bcachefs --filename=/mnt/test  --size=100G \
>     --ioengine=io_uring --iodepth=16 --rw=write --bs=128k
> 
> I measured the BW(MB/s) across three runs on 6.10-rc3:
> Before patches: 2730
> After patches: 3430 (1.25x boost)
> 
> With -o no_data_io mount option:
> Before patches: 2920
> After patches: 4630 (1.5x boost)
> 
> I was not able to test the patches with ktest due to some issue with
> debian(some broken package issue). Maybe Kent can run it in his CI while
> I try to fix ktest locally?

It's in my testing branch, results will be showing up here:
https://evilpiepirate.org/~testdashboard/ci?branch=bcachefs-testing