Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1231413 > unrolled thread

Re: [PATCH 2/2] ext4: implement cgroup writeback support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] ext4: implement cgroup writeback support Artem Bityutskiy <dedekind1@gmail.com> - 2015-09-23 14:50 +0200
    Re: [PATCH 2/2] ext4: implement cgroup writeback support Artem Bityutskiy <dedekind1@gmail.com> - 2015-09-23 16:00 +0200
      Re: [PATCH 2/2] ext4: implement cgroup writeback support Theodore Ts'o <tytso@mit.edu> - 2015-09-23 19:10 +0200
        Re: [PATCH 2/2] ext4: implement cgroup writeback support Tejun Heo <tj@kernel.org> - 2015-09-23 20:00 +0200
    Re: [PATCH 2/2] ext4: implement cgroup writeback support Chris Mason <clm@fb.com> - 2015-09-23 19:30 +0200
      Re: [PATCH 2/2] ext4: implement cgroup writeback support Chris Mason <clm@fb.com> - 2015-09-23 20:00 +0200
        Re: [PATCH 2/2] ext4: implement cgroup writeback support Theodore Ts'o <tytso@mit.edu> - 2015-09-23 20:30 +0200
          Re: [PATCH 2/2] ext4: implement cgroup writeback support Artem Bityutskiy <dedekind1@gmail.com> - 2015-09-23 21:50 +0200
            Re: [PATCH 2/2] ext4: implement cgroup writeback support Theodore Ts'o <tytso@mit.edu> - 2015-09-23 22:50 +0200
              Re: [PATCH 2/2] ext4: implement cgroup writeback support Artem Bityutskiy <dedekind1@gmail.com> - 2015-09-24 10:20 +0200
      Re: [PATCH 2/2] ext4: implement cgroup writeback support Tejun Heo <tj@kernel.org> - 2015-09-23 21:10 +0200
    Re: [PATCH 2/2] ext4: implement cgroup writeback support Tejun Heo <tj@kernel.org> - 2015-09-23 20:10 +0200
      Re: [PATCH 2/2] ext4: implement cgroup writeback support Tejun Heo <tj@kernel.org> - 2015-09-23 21:00 +0200
        [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable cgroup  writeback on traditional hierarchies Tejun Heo <tj@kernel.org> - 2015-09-23 23:10 +0200
          Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable  cgroup writeback on traditional hierarchies Artem Bityutskiy <dedekind1@gmail.com> - 2015-09-24 10:20 +0200
            Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable  cgroup writeback on traditional hierarchies Tejun Heo <tj@kernel.org> - 2015-09-24 22:50 +0200
              Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable  cgroup writeback on traditional hierarchies Tejun Heo <tj@kernel.org> - 2015-09-28 23:40 +0200
            Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable  cgroup writeback on traditional hierarchies Tejun Heo <tj@kernel.org> - 2015-09-24 22:50 +0200
              Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable  cgroup writeback on traditional hierarchies Artem Bityutskiy <dedekind1@gmail.com> - 2015-09-25 08:50 +0200
                Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable  cgroup writeback on traditional hierarchies Artem Bityutskiy <dedekind1@gmail.com> - 2015-09-25 13:00 +0200
                  Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable  cgroup writeback on traditional hierarchies Tejun Heo <tj@kernel.org> - 2015-09-25 17:50 +0200
                    Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable  cgroup writeback on traditional hierarchies Tejun Heo <tj@kernel.org> - 2015-09-27 00:20 +0200
                      Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable  cgroup writeback on traditional hierarchies Artem Bityutskiy <dedekind1@gmail.com> - 2015-09-29 13:40 +0200
                        Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable  cgroup writeback on traditional hierarchies Tejun Heo <tj@kernel.org> - 2015-09-29 16:30 +0200
          Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable  cgroup writeback on traditional hierarchies Jens Axboe <axboe@fb.com> - 2015-09-24 18:20 +0200
          Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable  cgroup writeback on traditional hierarchies Tejun Heo <tj@kernel.org> - 2015-09-24 22:50 +0200

Page 1 of 2  [1] 2  Next page →

#1231413 — Re: [PATCH 2/2] ext4: implement cgroup writeback support

On Tue, 2015-07-21 at 23:56 -0400, Theodore Ts'o wrote:
> > v2: Updated for MS_CGROUPWB -> SB_I_CGROUPWB.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: "Theodore Ts'o" <tytso@mit.edu>
> > Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> > Cc: linux-ext4@vger.kernel.org
> 
> Thanks, applied.

Hi, this patch introduces a regression - a major one, I'd say.

Symptoms: copy a bunch of file, run sync, then run 'reboot', and after
you boot up the copied files are corrupted. So basically the user
-visible symptom is that 'sync' does not work.

I quite an effort to bisect it, but it led me to this patch.

If I take the latest upstream (v4.3-rc2+), and revert this patch:

001e4a8 ext4: imlpement cgroup writeback support

then the problem goes away - files are not corrupted after reboot.

I use ext4 on top of a "bare" partition, no LVM or dm layers involved.

I use Fedora 22 with all the latest package updates, and I only change
the kernel there.

The corruption seems to be that the start with a bunch of zeroes
instead of the real data, but I did not check carefully, looked only at
one file briefly.

Artem.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1231459

On Wed, 2015-09-23 at 15:49 +0300, Artem Bityutskiy wrote:
> On Tue, 2015-07-21 at 23:56 -0400, Theodore Ts'o wrote:
> > > v2: Updated for MS_CGROUPWB -> SB_I_CGROUPWB.
> > > 
> > > Signed-off-by: Tejun Heo <tj@kernel.org>
> > > Cc: "Theodore Ts'o" <tytso@mit.edu>
> > > Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> > > Cc: linux-ext4@vger.kernel.org
> > 
> > Thanks, applied.
> 
> Hi, this patch introduces a regression - a major one, I'd say.
> 
> Symptoms: copy a bunch of file, run sync, then run 'reboot', and
> after
> you boot up the copied files are corrupted. So basically the user
> -visible symptom is that 'sync' does not work.

Just FYI, this is the issue I briefly reported last Fri:

https://lkml.org/lkml/2015/9/18/640

Artem.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231577

On Wed, Sep 23, 2015 at 04:50:53PM +0300, Artem Bityutskiy wrote:
> On Wed, 2015-09-23 at 15:49 +0300, Artem Bityutskiy wrote:
> > On Tue, 2015-07-21 at 23:56 -0400, Theodore Ts'o wrote:
> > > > v2: Updated for MS_CGROUPWB -> SB_I_CGROUPWB.
> > > > 
> > > > Signed-off-by: Tejun Heo <tj@kernel.org>
> > > > Cc: "Theodore Ts'o" <tytso@mit.edu>
> > > > Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> > > > Cc: linux-ext4@vger.kernel.org
> > > 
> > > Thanks, applied.
> > 
> > Hi, this patch introduces a regression - a major one, I'd say.
> > 
> > Symptoms: copy a bunch of file, run sync, then run 'reboot', and
> > after
> > you boot up the copied files are corrupted. So basically the user
> > -visible symptom is that 'sync' does not work.
> 
> Just FYI, this is the issue I briefly reported last Fri:
> 
> https://lkml.org/lkml/2015/9/18/640

Also note this performance regression reported by Dexuan Cui

	https://lkml.org/lkml/2015/9/23/333

I didn't notice these problems since I my userspace doesn't enable the
writeback cgroup.  (In fact I don't know how to do it using Debian
Jessie.)

Tejun, can you please take a look at this and give me a
recommendation?  I'm willing to wait a day or two while we try to fix
the problem, but past that point I'd like to have a fix or revert this
commit before Linus releases the next -rc release.

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231608

Hello, Ted.

On Wed, Sep 23, 2015 at 01:02:38PM -0400, Theodore Ts'o wrote:
> Also note this performance regression reported by Dexuan Cui
> 
> 	https://lkml.org/lkml/2015/9/23/333
> 
> I didn't notice these problems since I my userspace doesn't enable the
> writeback cgroup.  (In fact I don't know how to do it using Debian
> Jessie.)

The thing is I don't think they're either.

> Tejun, can you please take a look at this and give me a
> recommendation?  I'm willing to wait a day or two while we try to fix
> the problem, but past that point I'd like to have a fix or revert this
> commit before Linus releases the next -rc release.

Yeap, looking at them both.  Will update as soon as I know more.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231588

On Wed, Sep 23, 2015 at 03:49:12PM +0300, Artem Bityutskiy wrote:
> On Tue, 2015-07-21 at 23:56 -0400, Theodore Ts'o wrote:
> > > v2: Updated for MS_CGROUPWB -> SB_I_CGROUPWB.
> > > 
> > > Signed-off-by: Tejun Heo <tj@kernel.org>
> > > Cc: "Theodore Ts'o" <tytso@mit.edu>
> > > Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> > > Cc: linux-ext4@vger.kernel.org
> > 
> > Thanks, applied.
> 
> Hi, this patch introduces a regression - a major one, I'd say.
> 
> Symptoms: copy a bunch of file, run sync, then run 'reboot', and after
> you boot up the copied files are corrupted. So basically the user
> -visible symptom is that 'sync' does not work.

Hi Artem,

Are you doing a hard shutdown (reboot -nf)?  If you're doing a friendly
shutdown, is the FS unmounting cleanly?

> 
> I quite an effort to bisect it, but it led me to this patch.

I bet it was a long bisect.  Trying to see if the same patch to btrfs
has similar impacts.

-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231609

On Wed, Sep 23, 2015 at 08:41:25PM +0300, Artem Bityutskiy wrote:
>    Hi
> 
>    $ sync
>    $ reboot

If this is case, it should be possible to reproduce with:

cp a bunch of stuff to /ext4
unmount /ext4
mount ext4
compare data

If you're not getting a clean unmount of the test FS during the reboot,
its a different test.  Trying to reproduce here, so far its clean.
Could you please double check for failed unmounts?

Thanks,
Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231638

Artem,

Can you (or someone on the cgroups list, perhaps) give more details
about how Fedora 22 sets up groups?

Unfortunately apparently no one has gotten an official Fedora image
for Google Compute Engine so it's a bit of a pain for me to reproduce
the problem.  (I suppose I could use AWS, but all of my test
infrastructure uses GCE, and I'd really rather not have to install a
Java Runtime on my laptop. :-)

Or can you reproduce this problem on Debian Jessie?

Thanks,

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231681

On Wed, Sep 23, 2015 at 9:24 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> Artem,
>
> Can you (or someone on the cgroups list, perhaps) give more details
> about how Fedora 22 sets up groups?
>
> Unfortunately apparently no one has gotten an official Fedora image
> for Google Compute Engine so it's a bit of a pain for me to reproduce
> the problem.  (I suppose I could use AWS, but all of my test
> infrastructure uses GCE, and I'd really rather not have to install a
> Java Runtime on my laptop. :-)

[ My apologies for top posting and for sending HTML e-mails which do
not get through vger.
I am using gmail web interface, and just learned how to send plain
text from here. So re-sending
my longer answer. ]

Hi Ted, Chris, Tejun, all,

quick and probably messy reply before I go to sleep...

I can give more information tomorrow.  But one note - It would be helpful to get
questions like "send us the output of this command" rather than "what are the
cgroups you are in", because I am not fluent with cgroups. IOW, more specific
questions are welcome.

Some more about my setup. I have an number of testboxes, which are 1/2/4-socket
servers. I compile the kernel for them on a separate worker box. Then I copy the
kernel binary to /boot, and the modules to /lib/modules, then run
'sync' and then
reboot to reboot to the new kernel. And vrey often many module files
are corrupted.
They won't load because of majic/crc mismatches.

I copy stuff over scp. Well, this is not exactly scp, but rather a
Python 'scp' module,
which is based on the 'paramiko' module. But I think this should not matter.

Anyway, may be there are some cgroups related with scp/ssh sessions or
/lib/modules
in Fedora 22?

Also note, I tried to be careful during bisecting, I used 4 servers in
parallel, and
did 5 reboot tests on each of them. With this patch reverted all 4
boxes survive 5
reboots just fine. Without this patch reverted, each fail 1-3 reboots.

And, by the way, I forgot this detail - I cut AC power off at the end,
then put it back
on after a 20 seconds delay. I mean, this is a clean reboot, but with
power cut at the
end. So the process is this:

1. I run 'sync' on the box remotely over ssh
2. I run 'reboot'  on the box remotely over ssh, the ssh connection
gets closed at this point
3. I ping the box, and keep doing this until it is stops echoing back
4. I wait several seconds, and then just cut the AC power off. The
wall socket power is off.

So if there was something in, say, SSD cache which was not synced, it
is gone too.

May be this patch reveals an existing issue. My setup has been stable
with 4.2 and many
previous kernels, and it only fails with 4.3-rcX, and my bisecting
lead to this patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231715

On Wed, Sep 23, 2015 at 10:47:16PM +0300, Artem Bityutskiy wrote:
> And, by the way, I forgot this detail - I cut AC power off at the
> end, then put it back on after a 20 seconds delay. I mean, this is a
> clean reboot, but with power cut at the end.

Is this reproducible without the power cut?  And what model SSD are
you using, and are you sure that it has Power Loss Protection (many
SSD vendors use power loss protection as the price discrimination
feature between consumer-grade SSD and enterprise-grade SSD's that
cost $$$).  If this problem wasn't showing up with 4.2, and is only
failing with 4.3-rcX, then it might not be a hardware issue --- but
it's also possible that there was a timing issue which was hiding a
hardware problem.  So for the purposes of debugging, removing the
power cut from the set of variables is a useful thing to do.

      	       	       	  	       	 	- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231939

On Wed, 2015-09-23 at 16:48 -0400, Theodore Ts'o wrote:
> On Wed, Sep 23, 2015 at 10:47:16PM +0300, Artem Bityutskiy wrote:
> > And, by the way, I forgot this detail - I cut AC power off at the
> > end, then put it back on after a 20 seconds delay. I mean, this is
> > a
> > clean reboot, but with power cut at the end.
> 
> Is this reproducible without the power cut?  And what model SSD are
> you using, and are you sure that it has Power Loss Protection (many
> SSD vendors use power loss protection as the price discrimination
> feature between consumer-grade SSD and enterprise-grade SSD's that
> cost $$$).  If this problem wasn't showing up with 4.2, and is only
> failing with 4.3-rcX, then it might not be a hardware issue --- but
> it's also possible that there was a timing issue which was hiding a
> hardware problem.  So for the purposes of debugging, removing the
> power cut from the set of variables is a useful thing to do.

Ted, you are right that the problem may be anywhere, but since Tejun's
patch fixes it, I decided to not spend time on testing without AC power
cuts.

But I checked and on of my boxes uses an HDD, and it shows the same
problem, which makes the theory of imperfect SSD firmware being
involved less likely.

Thanks,
Artem.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231658

Hello, Artem.

On Wed, Sep 23, 2015 at 08:41:25PM +0300, Artem Bityutskiy wrote:
> $ sync
> $ reboot
> 
> is exactly the sequence.

Can you please test with 4.3-rc2 and see whether the issue is
reproducible?  The multi-wb wait logic rewrite was merged during rc1
and I'm wondering whether this is a bug in the old implementation.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231626

Hello, Artem.

On Wed, Sep 23, 2015 at 03:49:12PM +0300, Artem Bityutskiy wrote:
> I use Fedora 22 with all the latest package updates, and I only change
> the kernel there.

What's your cgroup setup like?  Are you trying out the unified
hierarchy?  Given that you didn't mention that, I'm guessing you
aren't which makes me pretty head-scratchy because I don't see how the
actual behavior would change depending on that flag.  I'll dig more.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231653

On Wed, Sep 23, 2015 at 02:09:34PM -0400, Tejun Heo wrote:
> Hello, Artem.
> 
> On Wed, Sep 23, 2015 at 03:49:12PM +0300, Artem Bityutskiy wrote:
> > I use Fedora 22 with all the latest package updates, and I only change
> > the kernel there.
> 
> What's your cgroup setup like?  Are you trying out the unified
> hierarchy?  Given that you didn't mention that, I'm guessing you
> aren't which makes me pretty head-scratchy because I don't see how the
> actual behavior would change depending on that flag.  I'll dig more.

Ugh... never mind.  I spotted what I did wrong.  The sync thing still
needs figuring out (it still should have worked) but fixing the
regression should be easy.  Will update soon.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231732 — [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable cgroup writeback on traditional hierarchies

inode_cgwb_enabled() gates cgroup writeback support.  If it returns
true, each inode is attached to the corresponding memory domain which
gets mapped to io domain.  It currently only tests whether the
filesystem and bdi support cgroup writeback; however, cgroup writeback
support doesn't work on traditional hierarchies and thus it should
also test whether memcg and iocg are on the default hierarchy.

This caused traditional hierarchy setups to hit the cgroup writeback
path inadvertently and ended up creating separate writeback domains
for each memcg and mapping them all to the root iocg uncovering a
couple issues in the cgroup writeback path.

cgroup writeback was never meant to be enabled on traditional
hierarchies.  Make inode_cgwb_enabled() test whether both memcg and
iocg are on the default hierarchy.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Artem Bityutskiy <dedekind1@gmail.com>
Reported-by: Dexuan Cui <decui@microsoft.com>
Link: http://lkml.kernel.org/g/1443012552.19983.209.camel@gmail.com
Link: http://lkml.kernel.org/g/f30d4a6aa8a546ff88f73021d026a453@SIXPR30MB031.064d.mgd.msft.net
---
Hello,

So, this should make the regression go away.  It doesn't fix the
underlying bugs but they shouldn't get triggered by people not
experimenting with cgroup.

I'm gonna keep digging the underlying issues but this should make the
regressions go away.  If it's okay, I think it'd be better to route
this through cgroup/for-4.3-fixes as it's gonna cause a conflict with
for-4.4 branch and handling the merge there is easier.

Thanks.

 include/linux/backing-dev.h |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 5a5d79e..d5eb4ad1 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -13,6 +13,7 @@
 #include <linux/sched.h>
 #include <linux/blkdev.h>
 #include <linux/writeback.h>
+#include <linux/memcontrol.h>
 #include <linux/blk-cgroup.h>
 #include <linux/backing-dev-defs.h>
 #include <linux/slab.h>
@@ -252,13 +253,19 @@ int inode_congested(struct inode *inode, int cong_bits);
  * @inode: inode of interest
  *
  * cgroup writeback requires support from both the bdi and filesystem.
- * Test whether @inode has both.
+ * Also, both memcg and iocg have to be on the default hierarchy.  Test
+ * whether all conditions are met.
+ *
+ * Note that the test result may change dynamically on the same inode
+ * depending on how memcg and iocg are configured.
  */
 static inline bool inode_cgwb_enabled(struct inode *inode)
 {
 	struct backing_dev_info *bdi = inode_to_bdi(inode);
 
-	return bdi_cap_account_dirty(bdi) &&
+	return cgroup_on_dfl(mem_cgroup_root_css->cgroup) &&
+		cgroup_on_dfl(blkcg_root_css->cgroup) &&
+		bdi_cap_account_dirty(bdi) &&
 		(bdi->capabilities & BDI_CAP_CGROUP_WRITEBACK) &&
 		(inode->i_sb->s_iflags & SB_I_CGROUPWB);
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1231934 — Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable cgroup writeback on traditional hierarchies

On Wed, 2015-09-23 at 17:07 -0400, Tejun Heo wrote:
> Hello,
> 
> So, this should make the regression go away.  It doesn't fix the
> underlying bugs but they shouldn't get triggered by people not
> experimenting with cgroup.

Tejun,

this hits the nail on the head and makes the problem go away.

I've tested the tip of Linuses tree (v4.3-rc2+) plus this patch - no
data corruption after reboots.

I've tested just the tip of Linuses tree (v4.3-rc2+) without this
patch, and I do see the data corruption after reboots.

Tested-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Artem.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232424 — Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable cgroup writeback on traditional hierarchies

Hello,

On Thu, Sep 24, 2015 at 08:40:18AM +0000, Dexuan Cui wrote:
> I can confirm the patch fixes my "slow write" issue too.
> 
> Tested-by: Dexuan Cui <decui@microsoft.com>

Yeah, this should make it go away w/o using cgroup writeback
explicitly; however, I think the proper solution for cgroup writeback
is moving bandwidth estimation from memory domain to io domain so that
two separate bw estimations wouldn't interfere with each other leading
to unexpected outcomes.  I'll work on the changes.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234534 — Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable cgroup writeback on traditional hierarchies

Hello,

On Thu, Sep 24, 2015 at 04:47:36PM -0400, Tejun Heo wrote:
> On Thu, Sep 24, 2015 at 08:40:18AM +0000, Dexuan Cui wrote:
> > I can confirm the patch fixes my "slow write" issue too.
> > 
> > Tested-by: Dexuan Cui <decui@microsoft.com>
> 
> Yeah, this should make it go away w/o using cgroup writeback
> explicitly; however, I think the proper solution for cgroup writeback
> is moving bandwidth estimation from memory domain to io domain so that
> two separate bw estimations wouldn't interfere with each other leading
> to unexpected outcomes.  I'll work on the changes.

So, this one actually turns out to be mostly caused by enabling cgroup
writeback when it shouldn't be.  balance_dirty_pages() ended up
looking at a different bdi_writeback from the actual writeback path so
the throttling was completley off, so making sure that cgroup
writeback doesn't get turned on traditional hierarchies is the right
solution here.

While auditing the behavior, I noticed a couple non-critical issues.
Will post patches to fix them soon.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232428 — Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable cgroup writeback on traditional hierarchies

Hello, Artem.

On Thu, Sep 24, 2015 at 11:09:46AM +0300, Artem Bityutskiy wrote:
> On Wed, 2015-09-23 at 17:07 -0400, Tejun Heo wrote:
> > So, this should make the regression go away.  It doesn't fix the
> > underlying bugs but they shouldn't get triggered by people not
> > experimenting with cgroup.
> 
> this hits the nail on the head and makes the problem go away.

Yeah but there still is an underlying problem here.  I've been going
through the sync path today but can't trigger or spot anything wrong.
Can you please apply the patch at the end of this mail, trigger the
failure and report the kernel log?

Thanks a lot.

---
 fs/fs-writeback.c           |  154 ++++++++++++++++++++++++++++++++++++++++++--
 fs/inode.c                  |    1 
 include/linux/backing-dev.h |   20 +++++
 include/linux/fs.h          |    2 
 mm/backing-dev.c            |    2 
 5 files changed, 171 insertions(+), 8 deletions(-)

--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -101,7 +101,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(wbc_writepa
 
 static bool wb_io_lists_populated(struct bdi_writeback *wb)
 {
-	if (wb_has_dirty_io(wb)) {
+	if (test_bit(WB_has_dirty_io, &wb->state)) {
 		return false;
 	} else {
 		set_bit(WB_has_dirty_io, &wb->state);
@@ -763,6 +763,15 @@ static long wb_split_bdi_pages(struct bd
 		return DIV_ROUND_UP_ULL((u64)nr_pages * this_bw, tot_bw);
 }
 
+extern spinlock_t cgwb_lock;
+
+struct split_work_dbg {
+	DECLARE_BITMAP(all_wbs, 8192);
+	DECLARE_BITMAP(iterated_wbs, 8192);
+	DECLARE_BITMAP(written_wbs, 8192);
+	DECLARE_BITMAP(sync_wbs, 8192);
+};
+
 /**
  * bdi_split_work_to_wbs - split a wb_writeback_work to all wb's of a bdi
  * @bdi: target backing_dev_info
@@ -776,11 +785,25 @@ static long wb_split_bdi_pages(struct bd
  */
 static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
 				  struct wb_writeback_work *base_work,
-				  bool skip_if_busy)
+				  bool skip_if_busy, struct split_work_dbg *dbg)
 {
 	int next_memcg_id = 0;
 	struct bdi_writeback *wb;
 	struct wb_iter iter;
+	struct radix_tree_iter riter;
+	void **slot;
+
+	if (dbg) {
+		spin_lock_irq(&cgwb_lock);
+		set_bit(bdi->wb.memcg_css->id, dbg->all_wbs);
+		bdi->wb.last_comp_gen = bdi->wb.comp_gen;
+		radix_tree_for_each_slot(slot, &bdi->cgwb_tree, &riter, 0) {
+			wb = *slot;
+			set_bit(wb->memcg_css->id, dbg->all_wbs);
+			wb->last_comp_gen = wb->comp_gen;
+		}
+		spin_unlock_irq(&cgwb_lock);
+	}
 
 	might_sleep();
 restart:
@@ -791,6 +814,9 @@ restart:
 		struct wb_writeback_work *work;
 		long nr_pages;
 
+		if (dbg)
+			set_bit(wb->memcg_css->id, dbg->iterated_wbs);
+
 		/* SYNC_ALL writes out I_DIRTY_TIME too */
 		if (!wb_has_dirty_io(wb) &&
 		    (base_work->sync_mode == WB_SYNC_NONE ||
@@ -799,6 +825,9 @@ restart:
 		if (skip_if_busy && writeback_in_progress(wb))
 			continue;
 
+		if (dbg)
+			set_bit(wb->memcg_css->id, dbg->written_wbs);
+
 		nr_pages = wb_split_bdi_pages(wb, base_work->nr_pages);
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
@@ -817,6 +846,8 @@ restart:
 		work->auto_free = 0;
 		work->done = &fallback_work_done;
 
+		if (dbg)
+			set_bit(wb->memcg_css->id, dbg->sync_wbs);
 		wb_queue_work(wb, work);
 
 		next_memcg_id = wb->memcg_css->id + 1;
@@ -1425,6 +1456,9 @@ static long writeback_sb_inodes(struct s
 			break;
 		}
 
+		inode->i_dbg_marker = 0;
+		inode->i_dbg_marker2 = 0;
+
 		/*
 		 * Don't bother with new inodes or inodes being freed, first
 		 * kind does not need periodic writeout yet, and for the latter
@@ -1515,6 +1549,7 @@ static long writeback_sb_inodes(struct s
 				break;
 		}
 	}
+
 	return wrote;
 }
 
@@ -1574,6 +1609,28 @@ static long writeback_inodes_wb(struct b
 	return nr_pages - work.nr_pages;
 }
 
+static int inode_which_wb_io_list(struct inode *inode, struct backing_dev_info *bdi)
+{
+	struct bdi_writeback *wb = inode->i_wb ?: &bdi->wb;
+	struct inode *pos;
+
+	if (list_empty(&inode->i_io_list))
+		return 0;
+	list_for_each_entry(pos, &wb->b_dirty, i_io_list)
+		if (pos == inode)
+			return 1;
+	list_for_each_entry(pos, &wb->b_io, i_io_list)
+		if (pos == inode)
+			return 2;
+	list_for_each_entry(pos, &wb->b_more_io, i_io_list)
+		if (pos == inode)
+			return 3;
+	list_for_each_entry(pos, &wb->b_dirty_time, i_io_list)
+		if (pos == inode)
+			return 4;
+	return 5;
+}
+
 /*
  * Explicit flushing or periodic writeback of "old" data.
  *
@@ -1604,6 +1661,16 @@ static long wb_writeback(struct bdi_writ
 
 	blk_start_plug(&plug);
 	spin_lock(&wb->list_lock);
+
+	list_for_each_entry(inode, &wb->b_dirty, i_io_list)
+		inode->i_dbg_marker2 = 1;
+	list_for_each_entry(inode, &wb->b_io, i_io_list)
+		inode->i_dbg_marker2 = 2;
+	list_for_each_entry(inode, &wb->b_more_io, i_io_list)
+		inode->i_dbg_marker2 = 3;
+	list_for_each_entry(inode, &wb->b_dirty_time, i_io_list)
+		inode->i_dbg_marker2 = 4;
+
 	for (;;) {
 		/*
 		 * Stop writeback when nr_pages has been consumed
@@ -1681,6 +1748,24 @@ static long wb_writeback(struct bdi_writ
 			spin_lock(&wb->list_lock);
 		}
 	}
+
+	if (work->sync_mode == WB_SYNC_ALL) {
+		list_for_each_entry(inode, &wb->b_dirty, i_io_list)
+			if (inode->i_dbg_marker2)
+				printk("XXX wb_writeback: inode %lu marker2=%d on b_dirty\n",
+				       inode->i_ino, inode->i_dbg_marker2);
+		list_for_each_entry(inode, &wb->b_io, i_io_list)
+			printk("XXX wb_writeback: inode %lu marker2=%d on b_io\n",
+			       inode->i_ino, inode->i_dbg_marker2);
+		list_for_each_entry(inode, &wb->b_more_io, i_io_list)
+			printk("XXX wb_writeback: inode %lu marker2=%d on b_more_io\n",
+			       inode->i_ino, inode->i_dbg_marker2);
+		list_for_each_entry(inode, &wb->b_dirty_time, i_io_list)
+			if (inode->i_dbg_marker2)
+				printk("XXX wb_writeback: inode %lu marker2=%d on b_dirty_time\n",
+				       inode->i_ino, inode->i_dbg_marker2);
+	}
+
 	spin_unlock(&wb->list_lock);
 	blk_finish_plug(&plug);
 
@@ -1785,8 +1870,11 @@ static long wb_do_writeback(struct bdi_w
 
 		if (work->auto_free)
 			kfree(work);
-		if (done && atomic_dec_and_test(&done->cnt))
-			wake_up_all(&wb->bdi->wb_waitq);
+		if (done) {
+			wb->comp_gen++;
+			if (atomic_dec_and_test(&done->cnt))
+				wake_up_all(&wb->bdi->wb_waitq);
+		}
 	}
 
 	/*
@@ -1976,6 +2064,9 @@ void __mark_inode_dirty(struct inode *in
 
 	trace_writeback_mark_inode_dirty(inode, flags);
 
+	WARN_ON_ONCE(!(sb->s_flags & MS_LAZYTIME) &&
+		     !list_empty(&inode_to_bdi(inode)->wb.b_dirty_time));
+
 	/*
 	 * Don't do this for I_DIRTY_PAGES - that doesn't actually
 	 * dirty the inode itself
@@ -2165,7 +2256,7 @@ static void __writeback_inodes_sb_nr(str
 		return;
 	WARN_ON(!rwsem_is_locked(&sb->s_umount));
 
-	bdi_split_work_to_wbs(sb->s_bdi, &work, skip_if_busy);
+	bdi_split_work_to_wbs(sb->s_bdi, &work, skip_if_busy, NULL);
 	wb_wait_for_completion(bdi, &done);
 }
 
@@ -2257,6 +2348,10 @@ void sync_inodes_sb(struct super_block *
 		.for_sync	= 1,
 	};
 	struct backing_dev_info *bdi = sb->s_bdi;
+	static DEFINE_MUTEX(dbg_mutex);
+	static struct split_work_dbg dbg;
+	static DECLARE_BITMAP(tmp_bitmap, 8192);
+	struct inode *inode;
 
 	/*
 	 * Can't skip on !bdi_has_dirty() because we should wait for !dirty
@@ -2267,9 +2362,56 @@ void sync_inodes_sb(struct super_block *
 		return;
 	WARN_ON(!rwsem_is_locked(&sb->s_umount));
 
-	bdi_split_work_to_wbs(bdi, &work, false);
+	mutex_lock(&dbg_mutex);
+
+	printk("XXX SYNCING %d:%d\n", MAJOR(sb->s_dev), MINOR(sb->s_dev));
+
+	bitmap_zero(dbg.all_wbs, 8192);
+	bitmap_zero(dbg.iterated_wbs, 8192);
+	bitmap_zero(dbg.written_wbs, 8192);
+	bitmap_zero(dbg.sync_wbs, 8192);
+
+	spin_lock(&sb->s_inode_list_lock);
+	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
+		spin_lock(&inode->i_lock);
+		inode->i_dbg_marker = inode_which_wb_io_list(inode, bdi);
+		spin_unlock(&inode->i_lock);
+	}
+	spin_unlock(&sb->s_inode_list_lock);
+
+	bdi_split_work_to_wbs(bdi, &work, false, &dbg);
 	wb_wait_for_completion(bdi, &done);
 
+	spin_lock(&sb->s_inode_list_lock);
+	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
+		struct bdi_writeback *wb = inode->i_wb ?: &bdi->wb;
+
+		if (!inode->i_dbg_marker)
+			continue;
+
+		spin_lock_irq(&wb->list_lock);
+		if (inode->i_state & I_DIRTY_ALL)
+			printk("XXX sync_inodes_sb(%d:%d): dirty inode %lu skipped, wb=%d comp_gen=%d->%d which=%d->%d i_state=0x%lx\n",
+			       MAJOR(sb->s_dev), MINOR(sb->s_dev), inode->i_ino,
+			       wb->memcg_css->id, wb->last_comp_gen, wb->comp_gen,
+			       inode->i_dbg_marker, inode_which_wb_io_list(inode, bdi),
+			       inode->i_state);
+		spin_unlock_irq(&wb->list_lock);
+	}
+	spin_unlock(&sb->s_inode_list_lock);
+
+	bitmap_andnot(tmp_bitmap, dbg.all_wbs, dbg.iterated_wbs, 8192);
+	if (!bitmap_empty(tmp_bitmap, 8192))
+		printk("XXX sync_inodes_sb(%d:%d): iteration skipped %8192pbl\n",
+		       MAJOR(sb->s_dev), MINOR(sb->s_dev), tmp_bitmap);
+
+	printk("XXX all_wbs      = %8192pbl\n", dbg.all_wbs);
+	printk("XXX iterated_wbs = %8192pbl\n", dbg.iterated_wbs);
+	printk("XXX written_wbs  = %8192pbl\n", dbg.written_wbs);
+	printk("XXX sync_wbs     = %8192pbl\n", dbg.sync_wbs);
+
+	mutex_unlock(&dbg_mutex);
+
 	wait_sb_inodes(sb);
 }
 EXPORT_SYMBOL(sync_inodes_sb);
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -183,6 +183,7 @@ int inode_init_always(struct super_block
 #endif
 	inode->i_flctx = NULL;
 	this_cpu_inc(nr_inodes);
+	inode->i_dbg_marker = 0;
 
 	return 0;
 out:
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -38,7 +38,25 @@ extern struct workqueue_struct *bdi_wq;
 
 static inline bool wb_has_dirty_io(struct bdi_writeback *wb)
 {
-	return test_bit(WB_has_dirty_io, &wb->state);
+	bool ret = test_bit(WB_has_dirty_io, &wb->state);
+	long tot_write_bw = atomic_long_read(&wb->bdi->tot_write_bandwidth);
+
+	if (!ret && (!list_empty(&wb->b_dirty) || !list_empty(&wb->b_io) ||
+		     !list_empty(&wb->b_more_io))) {
+		const char *name = wb->bdi->dev ? dev_name(wb->bdi->dev) : "UNK";
+
+		pr_err("wb_has_dirty_io: ERR %s has_dirty=%d b_dirty=%d b_io=%d b_more_io=%d\n",
+		       name, ret, !list_empty(&wb->b_dirty), !list_empty(&wb->b_io), !list_empty(&wb->b_more_io));
+		WARN_ON(1);
+	}
+	if (ret && !tot_write_bw) {
+		const char *name = wb->bdi->dev ? dev_name(wb->bdi->dev) : "UNK";
+
+		pr_err("wb_has_dirty_io: ERR %s has_dirty=%d but tot_write_bw=%ld\n",
+		       name, ret, tot_write_bw);
+		WARN_ON(1);
+	}
+	return ret;
 }
 
 static inline bool bdi_has_dirty_io(struct backing_dev_info *bdi)
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -677,6 +677,8 @@ struct inode {
 #endif
 
 	void			*i_private; /* fs or device private pointer */
+	unsigned		i_dbg_marker;
+	unsigned		i_dbg_marker2;
 };
 
 static inline int inode_unhashed(struct inode *inode)
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -382,7 +382,7 @@ static void wb_exit(struct bdi_writeback
  * protected.  cgwb_release_wait is used to wait for the completion of cgwb
  * releases from bdi destruction path.
  */
-static DEFINE_SPINLOCK(cgwb_lock);
+DEFINE_SPINLOCK(cgwb_lock);
 static DECLARE_WAIT_QUEUE_HEAD(cgwb_release_wait);
 
 /**
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232603 — Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable cgroup writeback on traditional hierarchies

On Thu, 2015-09-24 at 16:45 -0400, Tejun Heo wrote:
> Hello, Artem.
> 
> On Thu, Sep 24, 2015 at 11:09:46AM +0300, Artem Bityutskiy wrote:
> > On Wed, 2015-09-23 at 17:07 -0400, Tejun Heo wrote:
> > > So, this should make the regression go away.  It doesn't fix the
> > > underlying bugs but they shouldn't get triggered by people not
> > > experimenting with cgroup.
> > 
> > this hits the nail on the head and makes the problem go away.
> 
> Yeah but there still is an underlying problem here.  I've been going
> through the sync path today but can't trigger or spot anything wrong.
> Can you please apply the patch at the end of this mail, trigger the
> failure and report the kernel log?
> 
> Thanks a lot.

Does not compile with multiple errors like

linux/fs/fs-writeback.c:799:10: error: ‘struct bdi_writeback’ has no member named ‘last_comp_gen’
   bdi->wb.last_comp_gen = bdi->wb.comp_gen;
          ^



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232711 — Re: [PATCH cgroup/for-4.3-fixes] cgroup, writeback: don't enable cgroup writeback on traditional hierarchies

On Fri, 2015-09-25 at 09:49 +0300, Artem Bityutskiy wrote:
> On Thu, 2015-09-24 at 16:45 -0400, Tejun Heo wrote:
> > Hello, Artem.
> > 
> > On Thu, Sep 24, 2015 at 11:09:46AM +0300, Artem Bityutskiy wrote:
> > > On Wed, 2015-09-23 at 17:07 -0400, Tejun Heo wrote:
> > > > So, this should make the regression go away.  It doesn't fix
> > > > the
> > > > underlying bugs but they shouldn't get triggered by people not
> > > > experimenting with cgroup.
> > > 
> > > this hits the nail on the head and makes the problem go away.
> > 
> > Yeah but there still is an underlying problem here.  I've been
> > going
> > through the sync path today but can't trigger or spot anything
> > wrong.
> > Can you please apply the patch at the end of this mail, trigger the
> > failure and report the kernel log?
> > 
> > Thanks a lot.
> 
> Does not compile with multiple errors like
> 
> linux/fs/fs-writeback.c:799:10: error: ‘struct bdi_writeback’ has no
> member named ‘last_comp_gen’
>    bdi->wb.last_comp_gen = bdi->wb.comp_gen;

I tried to extend your patch with these fields, but I am not sure I got
it right, so please, send a new patch, I'll run the reboot corruption
test with your patch.

Please, note, because this test is about reboots, I'll probably output
everything to the serial console. Therefore, please, do not print too
much data. Otherwise I'd have to modify my scripts to collect dmesg
before restarting, which is more work.

Artem.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web