[PATCH] xfs: implement cgroup writeback support

[PATCH] xfs: implement cgroup writeback support

[Shaohua Li]

From: Shaohua Li <shli@fb.com>

Basically this is a copy of commit 001e4a8775f6(ext4: implement cgroup
writeback support). Tested with a fio test, verified writeback is
throttled against cgroup io.max write bandwidth, also verified moving
the fio test to another cgroup and the writeback is throttled against
new cgroup setting.

Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 fs/xfs/xfs_aops.c  | 2 ++
 fs/xfs/xfs_super.c | 1 +
 2 files changed, 3 insertions(+)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index f18e593..6535054 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -630,8 +630,10 @@ xfs_add_to_ioend(
 		if (wpc->ioend)
 			list_add(&wpc->ioend->io_list, iolist);
 		wpc->ioend = xfs_alloc_ioend(inode, wpc->io_type, offset, bh);
+		wbc_init_bio(wbc, wpc->ioend->io_bio);
 	}
 
+	wbc_account_io(wbc, bh->b_page, bh->b_size);
 	/*
 	 * If the buffer doesn't fit into the bio we need to allocate a new
 	 * one.  This shouldn't happen more than once for a given buffer.
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 584cf2d..aea3bc2 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1634,6 +1634,7 @@ xfs_fs_fill_super(
 	sb->s_max_links = XFS_MAXLINK;
 	sb->s_time_gran = 1;
 	set_posix_acl_flag(sb);
+	sb->s_iflags |= SB_I_CGROUPWB;
 
 	/* version 5 superblocks support inode version counters. */
 	if (XFS_SB_VERSION_NUM(&mp->m_sb) == XFS_SB_VERSION_5)
-- 
2.9.5

Re: [PATCH] xfs: implement cgroup writeback support

[Darrick J. Wong]

On Thu, Oct 12, 2017 at 12:16:48PM -0700, Shaohua Li wrote:
> From: Shaohua Li <shli@fb.com>
> 
> Basically this is a copy of commit 001e4a8775f6(ext4: implement cgroup
> writeback support). Tested with a fio test, verified writeback is
> throttled against cgroup io.max write bandwidth, also verified moving
> the fio test to another cgroup and the writeback is throttled against
> new cgroup setting.

Just out of curiosity, is there an xfstests that covers this?

It /looks/ ok at a first glance, but I'll have to take a closer look to
figure out if the warnings in Documentation/ apply:

"Depending on the configuration, the bio may be executed at a lower
priority and if the writeback session is holding shared resources, e.g.
a journal entry, may lead to priority inversion."

--D

> Cc: Tejun Heo <tj@kernel.org>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  fs/xfs/xfs_aops.c  | 2 ++
>  fs/xfs/xfs_super.c | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index f18e593..6535054 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -630,8 +630,10 @@ xfs_add_to_ioend(
>  		if (wpc->ioend)
>  			list_add(&wpc->ioend->io_list, iolist);
>  		wpc->ioend = xfs_alloc_ioend(inode, wpc->io_type, offset, bh);
> +		wbc_init_bio(wbc, wpc->ioend->io_bio);
>  	}
>  
> +	wbc_account_io(wbc, bh->b_page, bh->b_size);
>  	/*
>  	 * If the buffer doesn't fit into the bio we need to allocate a new
>  	 * one.  This shouldn't happen more than once for a given buffer.
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 584cf2d..aea3bc2 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1634,6 +1634,7 @@ xfs_fs_fill_super(
>  	sb->s_max_links = XFS_MAXLINK;
>  	sb->s_time_gran = 1;
>  	set_posix_acl_flag(sb);
> +	sb->s_iflags |= SB_I_CGROUPWB;
>  
>  	/* version 5 superblocks support inode version counters. */
>  	if (XFS_SB_VERSION_NUM(&mp->m_sb) == XFS_SB_VERSION_5)
> -- 
> 2.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs: implement cgroup writeback support

[Shaohua Li]

On Thu, Oct 12, 2017 at 01:16:18PM -0700, Darrick J. Wong wrote:
> On Thu, Oct 12, 2017 at 12:16:48PM -0700, Shaohua Li wrote:
> > From: Shaohua Li <shli@fb.com>
> > 
> > Basically this is a copy of commit 001e4a8775f6(ext4: implement cgroup
> > writeback support). Tested with a fio test, verified writeback is
> > throttled against cgroup io.max write bandwidth, also verified moving
> > the fio test to another cgroup and the writeback is throttled against
> > new cgroup setting.
> 
> Just out of curiosity, is there an xfstests that covers this?

No as far as I know
 
> It /looks/ ok at a first glance, but I'll have to take a closer look to
> figure out if the warnings in Documentation/ apply:
> 
> "Depending on the configuration, the bio may be executed at a lower
> priority and if the writeback session is holding shared resources, e.g.
> a journal entry, may lead to priority inversion."

The writepages code path is for file data, which I think should not cause
priority inversion because there is dependence between writeback to different
files. For metadata write, the priority inversion could happen. This happens to
other filesystems too, like ext4 and btrfs. Tejun submitted patches recently to
force metadata write from root cgroup, which could fix the issue. We might need
to do similar thing for xfs later.

Thanks,
Shaohua
> > Cc: Tejun Heo <tj@kernel.org>
> > Signed-off-by: Shaohua Li <shli@fb.com>
> > ---
> >  fs/xfs/xfs_aops.c  | 2 ++
> >  fs/xfs/xfs_super.c | 1 +
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> > index f18e593..6535054 100644
> > --- a/fs/xfs/xfs_aops.c
> > +++ b/fs/xfs/xfs_aops.c
> > @@ -630,8 +630,10 @@ xfs_add_to_ioend(
> >  		if (wpc->ioend)
> >  			list_add(&wpc->ioend->io_list, iolist);
> >  		wpc->ioend = xfs_alloc_ioend(inode, wpc->io_type, offset, bh);
> > +		wbc_init_bio(wbc, wpc->ioend->io_bio);
> >  	}
> >  
> > +	wbc_account_io(wbc, bh->b_page, bh->b_size);
> >  	/*
> >  	 * If the buffer doesn't fit into the bio we need to allocate a new
> >  	 * one.  This shouldn't happen more than once for a given buffer.
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 584cf2d..aea3bc2 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1634,6 +1634,7 @@ xfs_fs_fill_super(
> >  	sb->s_max_links = XFS_MAXLINK;
> >  	sb->s_time_gran = 1;
> >  	set_posix_acl_flag(sb);
> > +	sb->s_iflags |= SB_I_CGROUPWB;
> >  
> >  	/* version 5 superblocks support inode version counters. */
> >  	if (XFS_SB_VERSION_NUM(&mp->m_sb) == XFS_SB_VERSION_5)
> > -- 
> > 2.9.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs: implement cgroup writeback support

[Darrick J. Wong]

On Thu, Oct 12, 2017 at 12:49:13PM -0700, Shaohua Li wrote:
> On Thu, Oct 12, 2017 at 01:16:18PM -0700, Darrick J. Wong wrote:
> > On Thu, Oct 12, 2017 at 12:16:48PM -0700, Shaohua Li wrote:
> > > From: Shaohua Li <shli@fb.com>
> > > 
> > > Basically this is a copy of commit 001e4a8775f6(ext4: implement cgroup
> > > writeback support). Tested with a fio test, verified writeback is
> > > throttled against cgroup io.max write bandwidth, also verified moving
> > > the fio test to another cgroup and the writeback is throttled against
> > > new cgroup setting.
> > 
> > Just out of curiosity, is there an xfstests that covers this?
> 
> No as far as I know

I think we had better have some bare minimum functionality tests before
merging so that I can figure out if this really works (without having to
figure out how to test this manually) and everyone else can make sure
they don't cause regressions when they go modifying the writeback code.

> > It /looks/ ok at a first glance, but I'll have to take a closer look to
> > figure out if the warnings in Documentation/ apply:
> > 
> > "Depending on the configuration, the bio may be executed at a lower
> > priority and if the writeback session is holding shared resources, e.g.
> > a journal entry, may lead to priority inversion."
> 
> The writepages code path is for file data, which I think should not cause
> priority inversion because there is dependence between writeback to different
> files. For metadata write, the priority inversion could happen. This happens to
> other filesystems too, like ext4 and btrfs. Tejun submitted patches recently to
> force metadata write from root cgroup, which could fix the issue. We might need
> to do similar thing for xfs later.

<nod>

--D

> 
> Thanks,
> Shaohua
> > > Cc: Tejun Heo <tj@kernel.org>
> > > Signed-off-by: Shaohua Li <shli@fb.com>
> > > ---
> > >  fs/xfs/xfs_aops.c  | 2 ++
> > >  fs/xfs/xfs_super.c | 1 +
> > >  2 files changed, 3 insertions(+)
> > > 
> > > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> > > index f18e593..6535054 100644
> > > --- a/fs/xfs/xfs_aops.c
> > > +++ b/fs/xfs/xfs_aops.c
> > > @@ -630,8 +630,10 @@ xfs_add_to_ioend(
> > >  		if (wpc->ioend)
> > >  			list_add(&wpc->ioend->io_list, iolist);
> > >  		wpc->ioend = xfs_alloc_ioend(inode, wpc->io_type, offset, bh);
> > > +		wbc_init_bio(wbc, wpc->ioend->io_bio);
> > >  	}
> > >  
> > > +	wbc_account_io(wbc, bh->b_page, bh->b_size);
> > >  	/*
> > >  	 * If the buffer doesn't fit into the bio we need to allocate a new
> > >  	 * one.  This shouldn't happen more than once for a given buffer.
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index 584cf2d..aea3bc2 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > > @@ -1634,6 +1634,7 @@ xfs_fs_fill_super(
> > >  	sb->s_max_links = XFS_MAXLINK;
> > >  	sb->s_time_gran = 1;
> > >  	set_posix_acl_flag(sb);
> > > +	sb->s_iflags |= SB_I_CGROUPWB;
> > >  
> > >  	/* version 5 superblocks support inode version counters. */
> > >  	if (XFS_SB_VERSION_NUM(&mp->m_sb) == XFS_SB_VERSION_5)
> > > -- 
> > > 2.9.5
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs: implement cgroup writeback support

[Dave Chinner]

On Thu, Oct 12, 2017 at 12:16:48PM -0700, Shaohua Li wrote:
> From: Shaohua Li <shli@fb.com>
> 
> Basically this is a copy of commit 001e4a8775f6(ext4: implement cgroup
> writeback support). Tested with a fio test, verified writeback is
> throttled against cgroup io.max write bandwidth, also verified moving
> the fio test to another cgroup and the writeback is throttled against
> new cgroup setting.

So, I pushed back on this originally because there is no test
infrastructure for it. IOWs, we can't actually validate this code
works as intended. It appears you have some method of validating it
- please turn that into some fstests so the filesystem
developers know when we break it. Because we will break it, and if
there's regression tests in fstests we'll also notice when somebody
else breaks it, too.

> ---
>  fs/xfs/xfs_aops.c  | 2 ++
>  fs/xfs/xfs_super.c | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index f18e593..6535054 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -630,8 +630,10 @@ xfs_add_to_ioend(
>  		if (wpc->ioend)
>  			list_add(&wpc->ioend->io_list, iolist);
>  		wpc->ioend = xfs_alloc_ioend(inode, wpc->io_type, offset, bh);
> +		wbc_init_bio(wbc, wpc->ioend->io_bio);
>  	}
>  
> +	wbc_account_io(wbc, bh->b_page, bh->b_size);

A comment, please. I'm going to look at this in a couple of months
and not have a clue why this is here...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com