xfs cgroup writeback support

xfs cgroup writeback support

[Christoph Hellwig]

Hi all,

this small series adds cgroup writeback support to XFS.  Unlike
the previous iteration of the support a few years ago it also
ensures that that trailing bios in an ioend inherit the right
cgroup.  It has been tested with the shared/011 xfstests test
that was written to test this functionality in all file systems,
and manually by Stefan Priebe.

This work was funded by Profihost AG.

Note that the first patch was also in my series to move the xfs
writepage code to iomap.c and the second one will conflict with
it.  We'll need to sort out which series to merge first, but given
how simple this one I would suggest to go for this one.

[PATCH 1/2] xfs: simplify xfs_chain_bio

[Christoph Hellwig]

Move setting up operation and write hint to xfs_alloc_ioend, and
then just copy over all needed information from the previous bio
in xfs_chain_bio and stop passing various parameters to it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_aops.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index a6f0f4761a37..9cceb90e77c5 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -665,7 +665,6 @@ xfs_submit_ioend(
 
 	ioend->io_bio->bi_private = ioend;
 	ioend->io_bio->bi_end_io = xfs_end_bio;
-	ioend->io_bio->bi_opf = REQ_OP_WRITE | wbc_to_write_flags(wbc);
 
 	/*
 	 * If we are failing the IO now, just mark the ioend with an
@@ -679,7 +678,6 @@ xfs_submit_ioend(
 		return status;
 	}
 
-	ioend->io_bio->bi_write_hint = ioend->io_inode->i_write_hint;
 	submit_bio(ioend->io_bio);
 	return 0;
 }
@@ -691,7 +689,8 @@ xfs_alloc_ioend(
 	xfs_exntst_t		state,
 	xfs_off_t		offset,
 	struct block_device	*bdev,
-	sector_t		sector)
+	sector_t		sector,
+	struct writeback_control *wbc)
 {
 	struct xfs_ioend	*ioend;
 	struct bio		*bio;
@@ -699,6 +698,8 @@ xfs_alloc_ioend(
 	bio = bio_alloc_bioset(GFP_NOFS, BIO_MAX_PAGES, &xfs_ioend_bioset);
 	bio_set_dev(bio, bdev);
 	bio->bi_iter.bi_sector = sector;
+	bio->bi_opf = REQ_OP_WRITE | wbc_to_write_flags(wbc);
+	bio->bi_write_hint = inode->i_write_hint;
 
 	ioend = container_of(bio, struct xfs_ioend, io_inline_bio);
 	INIT_LIST_HEAD(&ioend->io_list);
@@ -719,24 +720,22 @@ xfs_alloc_ioend(
  * so that the bi_private linkage is set up in the right direction for the
  * traversal in xfs_destroy_ioend().
  */
-static void
+static struct bio *
 xfs_chain_bio(
-	struct xfs_ioend	*ioend,
-	struct writeback_control *wbc,
-	struct block_device	*bdev,
-	sector_t		sector)
+	struct bio		*prev)
 {
 	struct bio *new;
 
 	new = bio_alloc(GFP_NOFS, BIO_MAX_PAGES);
-	bio_set_dev(new, bdev);
-	new->bi_iter.bi_sector = sector;
-	bio_chain(ioend->io_bio, new);
-	bio_get(ioend->io_bio);		/* for xfs_destroy_ioend */
-	ioend->io_bio->bi_opf = REQ_OP_WRITE | wbc_to_write_flags(wbc);
-	ioend->io_bio->bi_write_hint = ioend->io_inode->i_write_hint;
-	submit_bio(ioend->io_bio);
-	ioend->io_bio = new;
+	bio_copy_dev(new, prev);
+	new->bi_iter.bi_sector = bio_end_sector(prev);
+	new->bi_opf = prev->bi_opf;
+	new->bi_write_hint = prev->bi_write_hint;
+
+	bio_chain(prev, new);
+	bio_get(prev);		/* for xfs_destroy_ioend */
+	submit_bio(prev);
+	return new;
 }
 
 /*
@@ -771,14 +770,14 @@ xfs_add_to_ioend(
 		if (wpc->ioend)
 			list_add(&wpc->ioend->io_list, iolist);
 		wpc->ioend = xfs_alloc_ioend(inode, wpc->fork,
-				wpc->imap.br_state, offset, bdev, sector);
+				wpc->imap.br_state, offset, bdev, sector, wbc);
 	}
 
 	if (!__bio_try_merge_page(wpc->ioend->io_bio, page, len, poff, true)) {
 		if (iop)
 			atomic_inc(&iop->write_count);
 		if (bio_full(wpc->ioend->io_bio))
-			xfs_chain_bio(wpc->ioend, wbc, bdev, sector);
+			wpc->ioend->io_bio = xfs_chain_bio(wpc->ioend->io_bio);
 		bio_add_page(wpc->ioend->io_bio, page, len, poff);
 	}
 
-- 
2.20.1

Re: [PATCH 1/2] xfs: simplify xfs_chain_bio

[Darrick J. Wong]

On Mon, Jun 24, 2019 at 03:43:14PM +0200, Christoph Hellwig wrote:
> Move setting up operation and write hint to xfs_alloc_ioend, and
> then just copy over all needed information from the previous bio
> in xfs_chain_bio and stop passing various parameters to it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Uh, is this the same patch with the same name in the previous series?

--D

> ---
>  fs/xfs/xfs_aops.c | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index a6f0f4761a37..9cceb90e77c5 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -665,7 +665,6 @@ xfs_submit_ioend(
>  
>  	ioend->io_bio->bi_private = ioend;
>  	ioend->io_bio->bi_end_io = xfs_end_bio;
> -	ioend->io_bio->bi_opf = REQ_OP_WRITE | wbc_to_write_flags(wbc);
>  
>  	/*
>  	 * If we are failing the IO now, just mark the ioend with an
> @@ -679,7 +678,6 @@ xfs_submit_ioend(
>  		return status;
>  	}
>  
> -	ioend->io_bio->bi_write_hint = ioend->io_inode->i_write_hint;
>  	submit_bio(ioend->io_bio);
>  	return 0;
>  }
> @@ -691,7 +689,8 @@ xfs_alloc_ioend(
>  	xfs_exntst_t		state,
>  	xfs_off_t		offset,
>  	struct block_device	*bdev,
> -	sector_t		sector)
> +	sector_t		sector,
> +	struct writeback_control *wbc)
>  {
>  	struct xfs_ioend	*ioend;
>  	struct bio		*bio;
> @@ -699,6 +698,8 @@ xfs_alloc_ioend(
>  	bio = bio_alloc_bioset(GFP_NOFS, BIO_MAX_PAGES, &xfs_ioend_bioset);
>  	bio_set_dev(bio, bdev);
>  	bio->bi_iter.bi_sector = sector;
> +	bio->bi_opf = REQ_OP_WRITE | wbc_to_write_flags(wbc);
> +	bio->bi_write_hint = inode->i_write_hint;
>  
>  	ioend = container_of(bio, struct xfs_ioend, io_inline_bio);
>  	INIT_LIST_HEAD(&ioend->io_list);
> @@ -719,24 +720,22 @@ xfs_alloc_ioend(
>   * so that the bi_private linkage is set up in the right direction for the
>   * traversal in xfs_destroy_ioend().
>   */
> -static void
> +static struct bio *
>  xfs_chain_bio(
> -	struct xfs_ioend	*ioend,
> -	struct writeback_control *wbc,
> -	struct block_device	*bdev,
> -	sector_t		sector)
> +	struct bio		*prev)
>  {
>  	struct bio *new;
>  
>  	new = bio_alloc(GFP_NOFS, BIO_MAX_PAGES);
> -	bio_set_dev(new, bdev);
> -	new->bi_iter.bi_sector = sector;
> -	bio_chain(ioend->io_bio, new);
> -	bio_get(ioend->io_bio);		/* for xfs_destroy_ioend */
> -	ioend->io_bio->bi_opf = REQ_OP_WRITE | wbc_to_write_flags(wbc);
> -	ioend->io_bio->bi_write_hint = ioend->io_inode->i_write_hint;
> -	submit_bio(ioend->io_bio);
> -	ioend->io_bio = new;
> +	bio_copy_dev(new, prev);
> +	new->bi_iter.bi_sector = bio_end_sector(prev);
> +	new->bi_opf = prev->bi_opf;
> +	new->bi_write_hint = prev->bi_write_hint;
> +
> +	bio_chain(prev, new);
> +	bio_get(prev);		/* for xfs_destroy_ioend */
> +	submit_bio(prev);
> +	return new;
>  }
>  
>  /*
> @@ -771,14 +770,14 @@ xfs_add_to_ioend(
>  		if (wpc->ioend)
>  			list_add(&wpc->ioend->io_list, iolist);
>  		wpc->ioend = xfs_alloc_ioend(inode, wpc->fork,
> -				wpc->imap.br_state, offset, bdev, sector);
> +				wpc->imap.br_state, offset, bdev, sector, wbc);
>  	}
>  
>  	if (!__bio_try_merge_page(wpc->ioend->io_bio, page, len, poff, true)) {
>  		if (iop)
>  			atomic_inc(&iop->write_count);
>  		if (bio_full(wpc->ioend->io_bio))
> -			xfs_chain_bio(wpc->ioend, wbc, bdev, sector);
> +			wpc->ioend->io_bio = xfs_chain_bio(wpc->ioend->io_bio);
>  		bio_add_page(wpc->ioend->io_bio, page, len, poff);
>  	}
>  
> -- 
> 2.20.1
> 

Re: [PATCH 1/2] xfs: simplify xfs_chain_bio

[Christoph Hellwig]

On Mon, Jun 24, 2019 at 09:17:50AM -0700, Darrick J. Wong wrote:
> On Mon, Jun 24, 2019 at 03:43:14PM +0200, Christoph Hellwig wrote:
> > Move setting up operation and write hint to xfs_alloc_ioend, and
> > then just copy over all needed information from the previous bio
> > in xfs_chain_bio and stop passing various parameters to it.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Uh, is this the same patch with the same name in the previous series?

Yes.  See the cover letter for details.

[PATCH 2/2] xfs: implement cgroup aware writeback

[Christoph Hellwig]

Link every newly allocated writeback bio to cgroup pointed to by the
writeback control structure, and charge every byte written back to it.

Tested-by: Stefan Priebe - Profihost AG <s.priebe@profihost.ag>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_aops.c  | 4 +++-
 fs/xfs/xfs_super.c | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 9cceb90e77c5..73c291aeae17 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -700,6 +700,7 @@ xfs_alloc_ioend(
 	bio->bi_iter.bi_sector = sector;
 	bio->bi_opf = REQ_OP_WRITE | wbc_to_write_flags(wbc);
 	bio->bi_write_hint = inode->i_write_hint;
+	wbc_init_bio(wbc, bio);
 
 	ioend = container_of(bio, struct xfs_ioend, io_inline_bio);
 	INIT_LIST_HEAD(&ioend->io_list);
@@ -727,7 +728,7 @@ xfs_chain_bio(
 	struct bio *new;
 
 	new = bio_alloc(GFP_NOFS, BIO_MAX_PAGES);
-	bio_copy_dev(new, prev);
+	bio_copy_dev(new, prev);/* also copies over blkcg information */
 	new->bi_iter.bi_sector = bio_end_sector(prev);
 	new->bi_opf = prev->bi_opf;
 	new->bi_write_hint = prev->bi_write_hint;
@@ -782,6 +783,7 @@ xfs_add_to_ioend(
 	}
 
 	wpc->ioend->io_size += len;
+	wbc_account_io(wbc, page, len);
 }
 
 STATIC void
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 594c119824cc..ee0df8f611ff 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1685,6 +1685,8 @@ xfs_fs_fill_super(
 	sb->s_maxbytes = xfs_max_file_offset(sb->s_blocksize_bits);
 	sb->s_max_links = XFS_MAXLINK;
 	sb->s_time_gran = 1;
+	sb->s_iflags |= SB_I_CGROUPWB;
+
 	set_posix_acl_flag(sb);
 
 	/* version 5 superblocks support inode version counters. */
-- 
2.20.1

Re: [PATCH 2/2] xfs: implement cgroup aware writeback

[Darrick J. Wong]

On Mon, Jun 24, 2019 at 03:43:15PM +0200, Christoph Hellwig wrote:
> Link every newly allocated writeback bio to cgroup pointed to by the
> writeback control structure, and charge every byte written back to it.
> 
> Tested-by: Stefan Priebe - Profihost AG <s.priebe@profihost.ag>

Was this tested by running shared/011?  Or did it involve other checks?

As I mentioned in the thread about shared/011, I think the test needs a
better way of figuring out if the filesystem under test actually
supports cgroup writeback so we don't cause failures that then have to
be put on a known-issue list for an old kernel.

FWIW that test looks like it only is testing the accounting, so that
might be as easy as trying a write and seeing if the numbers jump.

--D

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_aops.c  | 4 +++-
>  fs/xfs/xfs_super.c | 2 ++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 9cceb90e77c5..73c291aeae17 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -700,6 +700,7 @@ xfs_alloc_ioend(
>  	bio->bi_iter.bi_sector = sector;
>  	bio->bi_opf = REQ_OP_WRITE | wbc_to_write_flags(wbc);
>  	bio->bi_write_hint = inode->i_write_hint;
> +	wbc_init_bio(wbc, bio);
>  
>  	ioend = container_of(bio, struct xfs_ioend, io_inline_bio);
>  	INIT_LIST_HEAD(&ioend->io_list);
> @@ -727,7 +728,7 @@ xfs_chain_bio(
>  	struct bio *new;
>  
>  	new = bio_alloc(GFP_NOFS, BIO_MAX_PAGES);
> -	bio_copy_dev(new, prev);
> +	bio_copy_dev(new, prev);/* also copies over blkcg information */
>  	new->bi_iter.bi_sector = bio_end_sector(prev);
>  	new->bi_opf = prev->bi_opf;
>  	new->bi_write_hint = prev->bi_write_hint;
> @@ -782,6 +783,7 @@ xfs_add_to_ioend(
>  	}
>  
>  	wpc->ioend->io_size += len;
> +	wbc_account_io(wbc, page, len);
>  }
>  
>  STATIC void
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 594c119824cc..ee0df8f611ff 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1685,6 +1685,8 @@ xfs_fs_fill_super(
>  	sb->s_maxbytes = xfs_max_file_offset(sb->s_blocksize_bits);
>  	sb->s_max_links = XFS_MAXLINK;
>  	sb->s_time_gran = 1;
> +	sb->s_iflags |= SB_I_CGROUPWB;
> +
>  	set_posix_acl_flag(sb);
>  
>  	/* version 5 superblocks support inode version counters. */
> -- 
> 2.20.1
> 

Re: [PATCH 2/2] xfs: implement cgroup aware writeback

[Christoph Hellwig]

On Mon, Jun 24, 2019 at 09:22:15AM -0700, Darrick J. Wong wrote:
> On Mon, Jun 24, 2019 at 03:43:15PM +0200, Christoph Hellwig wrote:
> > Link every newly allocated writeback bio to cgroup pointed to by the
> > writeback control structure, and charge every byte written back to it.
> > 
> > Tested-by: Stefan Priebe - Profihost AG <s.priebe@profihost.ag>
> 
> Was this tested by running shared/011?  Or did it involve other checks?

I verified it with shared/011 and local frobbing.  Stefan can chime
in on what testing he did.

Re: [PATCH 2/2] xfs: implement cgroup aware writeback

[Stefan Priebe - Profihost AG]

Am 25.06.19 um 12:00 schrieb Christoph Hellwig:
> On Mon, Jun 24, 2019 at 09:22:15AM -0700, Darrick J. Wong wrote:
>> On Mon, Jun 24, 2019 at 03:43:15PM +0200, Christoph Hellwig wrote:
>>> Link every newly allocated writeback bio to cgroup pointed to by the
>>> writeback control structure, and charge every byte written back to it.
>>>
>>> Tested-by: Stefan Priebe - Profihost AG <s.priebe@profihost.ag>
>>
>> Was this tested by running shared/011?  Or did it involve other checks?
> 
> I verified it with shared/011 and local frobbing.  Stefan can chime
> in on what testing he did.

I ran some real life Tests:

1.) using Dovecot for IMAP and limit Disk I/O

2.) Using dd without oflag=direct in systemd slices.

3.) Limit apache slice with various PHP applications in it.

Greets,
Stefan

Re: xfs cgroup writeback support

[Darrick J. Wong]

On Mon, Jun 24, 2019 at 03:43:13PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this small series adds cgroup writeback support to XFS.  Unlike
> the previous iteration of the support a few years ago it also
> ensures that that trailing bios in an ioend inherit the right
> cgroup.  It has been tested with the shared/011 xfstests test
> that was written to test this functionality in all file systems,
> and manually by Stefan Priebe.
> 
> This work was funded by Profihost AG.
> 
> Note that the first patch was also in my series to move the xfs
> writepage code to iomap.c and the second one will conflict with
> it.  We'll need to sort out which series to merge first, but given
> how simple this one I would suggest to go for this one.

By the way, did all the things Dave complained about in last year's
attempt[1] to add cgroup writeback support get fixed?  IIRC someone
whose name I didn't recognise complained about log starvation due to
REQ_META bios being charged to the wrong cgroup and other misbehavior.

Also, I remember that in the earlier 2017 discussion[2] we talked about
a fstest to test that writeback throttling actually capped bandwidth
usage correctly.  I haven't been following cgroupwb development since
2017 -- does it not ratelimit bandwidth now, or is there some test for
that?  The only test I could find was shared/011 which only tests the
accounting, not bandwidth.

--D

[1] https://patchwork.kernel.org/comment/21658249/
[2] https://patchwork.kernel.org/comment/21042703/

Re: xfs cgroup writeback support

[Christoph Hellwig]

On Mon, Jun 24, 2019 at 08:25:27PM -0700, Darrick J. Wong wrote:
> By the way, did all the things Dave complained about in last year's
> attempt[1] to add cgroup writeback support get fixed?  IIRC someone
> whose name I didn't recognise complained about log starvation due to
> REQ_META bios being charged to the wrong cgroup and other misbehavior.

As mentioned in the reference thread while the metadata throttling is
an issue, it is in existing one and not one touched by the cgroup
writeback support.  This patch just ensures that writeback takes the
cgroup information from the inode instead of the current task.  The
fact that blkcg should not even look at any cgroup information for
REQ_META is something that should be fixed entirely in core cgroup
code is orthogonal to how we pick the attached cgroup.

> Also, I remember that in the earlier 2017 discussion[2] we talked about
> a fstest to test that writeback throttling actually capped bandwidth
> usage correctly.  I haven't been following cgroupwb development since
> 2017 -- does it not ratelimit bandwidth now, or is there some test for
> that?  The only test I could find was shared/011 which only tests the
> accounting, not bandwidth.

As far as I can tell cfq could limit bandwith, but cgq is done now.
Either way all that is hiddent way below us.

Re: xfs cgroup writeback support

[Darrick J. Wong]

On Tue, Jun 25, 2019 at 12:05:32PM +0200, Christoph Hellwig wrote:
> On Mon, Jun 24, 2019 at 08:25:27PM -0700, Darrick J. Wong wrote:
> > By the way, did all the things Dave complained about in last year's
> > attempt[1] to add cgroup writeback support get fixed?  IIRC someone
> > whose name I didn't recognise complained about log starvation due to
> > REQ_META bios being charged to the wrong cgroup and other misbehavior.
> 
> As mentioned in the reference thread while the metadata throttling is
> an issue, it is in existing one and not one touched by the cgroup
> writeback support.  This patch just ensures that writeback takes the
> cgroup information from the inode instead of the current task.  The
> fact that blkcg should not even look at any cgroup information for
> REQ_META is something that should be fixed entirely in core cgroup
> code is orthogonal to how we pick the attached cgroup.

That may be, but I don't want to merge this patchset only to find out
I've unleashed Pandora's box of untested cgroupwb hell... I /think/ they
fixed all those problems, but it didn't take all that long tracing the
blkg/blkcg object relationships for my brain to fall out. :/

[Oh well I guess I'll try to turn all that on in my test vm and see if
its brain falls out overnight too...]

> > Also, I remember that in the earlier 2017 discussion[2] we talked about
> > a fstest to test that writeback throttling actually capped bandwidth
> > usage correctly.  I haven't been following cgroupwb development since
> > 2017 -- does it not ratelimit bandwidth now, or is there some test for
> > that?  The only test I could find was shared/011 which only tests the
> > accounting, not bandwidth.
> 
> As far as I can tell cfq could limit bandwith, but cgq is done now.
> Either way all that is hiddent way below us.

<shrug> ok?  I mean, if bandwidth limits died as a feature it'd be nice
to know that outright. :)

--D

Re: xfs cgroup writeback support

[Christoph Hellwig]

On Tue, Jun 25, 2019 at 10:57:01PM -0700, Darrick J. Wong wrote:
> That may be, but I don't want to merge this patchset only to find out
> I've unleashed Pandora's box of untested cgroupwb hell... I /think/ they
> fixed all those problems, but it didn't take all that long tracing the
> blkg/blkcg object relationships for my brain to fall out. :/

c'mon.  We are adding handful of line patch to finally bring XFS in
line with everyone else.  That doesn't mean we want to take over
cgroup maintenance.

Re: xfs cgroup writeback support

[Darrick J. Wong]

On Wed, Jun 26, 2019 at 07:57:32AM +0200, Christoph Hellwig wrote:
> On Tue, Jun 25, 2019 at 10:57:01PM -0700, Darrick J. Wong wrote:
> > That may be, but I don't want to merge this patchset only to find out
> > I've unleashed Pandora's box of untested cgroupwb hell... I /think/ they
> > fixed all those problems, but it didn't take all that long tracing the
> > blkg/blkcg object relationships for my brain to fall out. :/
> 
> c'mon.  We are adding handful of line patch to finally bring XFS in
> line with everyone else.  That doesn't mean we want to take over
> cgroup maintenance.

I didn't want us to take over maintenance of copy_file_range and the
remap ioctls either, but now look.  /me burns out on cleaning up new
features that haven't been adequately specified and tested and having to
retrofit sane behavior into existing userspace ABI; and I wouldn't be
shocked to hear Dave and Amir probably feel similarly.

<frown>

That said, I did go digging further into how cgroup writeback accounting
works and AFAICT the things people were complaining about in the last
three attempts to add cgroupwb support to xfs have been solved, so I
guess I'm fine with the series:

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D