| Message ID | 20191004221104.646711-1-guro@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cgroup, blkcg: prevent dirty inodes to pin dying memory cgroups | expand |
On 10/5/19 12:11 AM, Roman Gushchin wrote: > > One possible approach to this problem is to switch inodes associated > with dying wbs to the root wb. Switching is a best effort operation > which can fail silently, so unfortunately we can't run once over a > list of associated inodes (even if we'd have such a list). So we > really have to scan all inodes. > > In the proposed patch I schedule a work on each memory cgroup > deletion, which is probably too often. Alternatively, we can do it > periodically under some conditions (e.g. the number of dying memory > cgroups is larger than X). So it's basically a gc run. > > I wonder if there are any better ideas? I don't know this area, so this will be likely easily shown impossible, but perhaps it's useful to do that explicitly. What if instead of reparenting each inode, we "reparent" the wb? But I see it's not a small object either. Could we then add some bias for inode switching conditions so that anyone else touching the inode from dead wb would get it immediately? And what would happen if we reused the reparented wb's for newly created cgroups? Would it "punish" them for the old inodes?
On Fri, Oct 04, 2019 at 03:11:04PM -0700, Roman Gushchin <guro@fb.com> wrote: > An inode which is getting dirty for the first time is associated > with the wb structure (look at __inode_attach_wb()). It can later > be switched to another wb under some conditions (e.g. some other > cgroup is writing a lot of data to the same inode), but generally > stays associated up to the end of life of the inode structure. What about dissociating the wb structure from the charged cgroup after the particular writeback finished? (I understand from your description that wb structure outlives the dirtier and is kept due to other inode (read) users, not sure if that's correct assumption.) Michal
On Mon, Oct 07, 2019 at 06:19:26PM +0200, Michal Koutný wrote: > On Fri, Oct 04, 2019 at 03:11:04PM -0700, Roman Gushchin <guro@fb.com> wrote: > > An inode which is getting dirty for the first time is associated > > with the wb structure (look at __inode_attach_wb()). It can later > > be switched to another wb under some conditions (e.g. some other > > cgroup is writing a lot of data to the same inode), but generally > > stays associated up to the end of life of the inode structure. > What about dissociating the wb structure from the charged cgroup after > the particular writeback finished? (I understand from your description > that wb structure outlives the dirtier and is kept due to other inode > (read) users, not sure if that's correct assumption.) Well, that sounds nice, and I thought into this direction, but I've no idea how to implement it :) First, it's hard to find a good moment for dissociation. It seems that the good moment is after the cgroup has been removed by the user and most of the dirty memory has been written back, but it's hard to formalize, given that other cgroups may write to the same inode concurrently. Second, the current code assumes that wb->memcg association never breaks, and it's not that trivial to get rid of this assumption without introducing new locks, etc. Thanks!
On Mon, Oct 07, 2019 at 04:57:15PM +0200, Vlastimil Babka wrote: > On 10/5/19 12:11 AM, Roman Gushchin wrote: > > > > One possible approach to this problem is to switch inodes associated > > with dying wbs to the root wb. Switching is a best effort operation > > which can fail silently, so unfortunately we can't run once over a > > list of associated inodes (even if we'd have such a list). So we > > really have to scan all inodes. > > > > In the proposed patch I schedule a work on each memory cgroup > > deletion, which is probably too often. Alternatively, we can do it > > periodically under some conditions (e.g. the number of dying memory > > cgroups is larger than X). So it's basically a gc run. > > > > I wonder if there are any better ideas? > > I don't know this area, so this will be likely easily shown impossible, > but perhaps it's useful to do that explicitly. > > What if instead of reparenting each inode, we "reparent" the wb? It seems to be an arguable idea, at least at the offlining moment. Dirty memory left after a cgroup should be written back using corresponding limits, and reparenting can easily break them. Also, it's not clear to me, how to reparent dirty stats? > But I see it's not a small object either. Could we then add some bias > for inode switching conditions so that anyone else touching the inode > from dead wb would get it immediately? You mean touching for writing? That's doable, but doesn't solve the case when there are only readers. And the case is quite common. > And what would happen if we reused the reparented wb's for newly created > cgroups? Would it "punish" them for the old inodes? > No idea, to be honest. Thank you!
On Fri, Oct 04, 2019 at 03:11:04PM -0700, Roman Gushchin wrote: > This is a RFC patch, which is not intended to be merged as is, > but hopefully will start a discussion which can result in a good > solution for the described problem. > > -- > > We've noticed that the number of dying cgroups on our production hosts > tends to grow with the uptime. This time it's caused by the writeback > code. > > An inode which is getting dirty for the first time is associated > with the wb structure (look at __inode_attach_wb()). It can later > be switched to another wb under some conditions (e.g. some other > cgroup is writing a lot of data to the same inode), but generally > stays associated up to the end of life of the inode structure. > > The problem is that the wb structure holds a reference to the original > memory cgroup. So if the inode was dirty once, it has a good chance > to pin down the original memory cgroup. > > An example from the real life: some service runs periodically and > updates rpm packages. Each time in a new memory cgroup. Installed > .so files are heavily used by other cgroups, so corresponding inodes > tend to stay alive for a long. So do pinned memory cgroups. > In production I've seen many hosts with 1-2 thousands of dying > cgroups. > > This is not the first problem with the dying memory cgroups. As > always, the problem is with their relative size: memory cgroups > are large objects, easily 100x-1000x larger that inodes. So keeping > a couple of thousands of dying cgroups in memory without a good reason > (what we easily do with inodes) is quite costly (and is measured > in tens and hundreds of Mb). > > One possible approach to this problem is to switch inodes associated > with dying wbs to the root wb. Switching is a best effort operation > which can fail silently, so unfortunately we can't run once over a > list of associated inodes (even if we'd have such a list). So we > really have to scan all inodes. > > In the proposed patch I schedule a work on each memory cgroup > deletion, which is probably too often. Alternatively, we can do it > periodically under some conditions (e.g. the number of dying memory > cgroups is larger than X). So it's basically a gc run. > > I wonder if there are any better ideas? > > Signed-off-by: Roman Gushchin <guro@fb.com> > --- > fs/fs-writeback.c | 29 +++++++++++++++++++++++++++++ > mm/memcontrol.c | 5 +++++ > 2 files changed, 34 insertions(+) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index 542b02d170f8..4bbc9a200b2c 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -545,6 +545,35 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id) > up_read(&bdi->wb_switch_rwsem); > } > > +static void reparent_dirty_inodes_one_sb(struct super_block *sb, void *arg) > +{ > + struct inode *inode, *next; > + > + spin_lock(&sb->s_inode_list_lock); > + list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) { > + spin_lock(&inode->i_lock); > + if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) { > + spin_unlock(&inode->i_lock); > + continue; > + } > + > + if (inode->i_wb && wb_dying(inode->i_wb)) { > + spin_unlock(&inode->i_lock); > + inode_switch_wbs(inode, root_mem_cgroup->css.id); > + continue; > + } > + > + spin_unlock(&inode->i_lock); > + } > + spin_unlock(&sb->s_inode_list_lock); No idea what the best solution is, but I think this is fundamentally unworkable. It's not uncommon to have a hundred million cached inodes these days, often on a single filesystem. Anything that requires a brute-force system wide inode scan, especially without conditional reschedule points, is largely a non-starter. Also, inode_switch_wbs() is not guaranteed to move the inode to the destination wb. There can only be WB_FRN_MAX_IN_FLIGHT (1024) switches in flight at once and switches are run via RCU callbacks, so I suspect that using inode_switch_wbs() for bulk re-assignment is going to be a lot more complex than just finding inodes to call inode_switch_wbs() on.... Cheers, Dave.
On Tue 08-10-19 05:38:59, Roman Gushchin wrote: > On Tue, Oct 08, 2019 at 03:06:31PM +1100, Dave Chinner wrote: > > On Fri, Oct 04, 2019 at 03:11:04PM -0700, Roman Gushchin wrote: > > > This is a RFC patch, which is not intended to be merged as is, > > > but hopefully will start a discussion which can result in a good > > > solution for the described problem. > > > > > > -- > > > > > > We've noticed that the number of dying cgroups on our production hosts > > > tends to grow with the uptime. This time it's caused by the writeback > > > code. > > > > > > An inode which is getting dirty for the first time is associated > > > with the wb structure (look at __inode_attach_wb()). It can later > > > be switched to another wb under some conditions (e.g. some other > > > cgroup is writing a lot of data to the same inode), but generally > > > stays associated up to the end of life of the inode structure. > > > > > > The problem is that the wb structure holds a reference to the original > > > memory cgroup. So if the inode was dirty once, it has a good chance > > > to pin down the original memory cgroup. > > > > > > An example from the real life: some service runs periodically and > > > updates rpm packages. Each time in a new memory cgroup. Installed > > > .so files are heavily used by other cgroups, so corresponding inodes > > > tend to stay alive for a long. So do pinned memory cgroups. > > > In production I've seen many hosts with 1-2 thousands of dying > > > cgroups. > > > > > > This is not the first problem with the dying memory cgroups. As > > > always, the problem is with their relative size: memory cgroups > > > are large objects, easily 100x-1000x larger that inodes. So keeping > > > a couple of thousands of dying cgroups in memory without a good reason > > > (what we easily do with inodes) is quite costly (and is measured > > > in tens and hundreds of Mb). > > > > > > One possible approach to this problem is to switch inodes associated > > > with dying wbs to the root wb. Switching is a best effort operation > > > which can fail silently, so unfortunately we can't run once over a > > > list of associated inodes (even if we'd have such a list). So we > > > really have to scan all inodes. > > > > > > In the proposed patch I schedule a work on each memory cgroup > > > deletion, which is probably too often. Alternatively, we can do it > > > periodically under some conditions (e.g. the number of dying memory > > > cgroups is larger than X). So it's basically a gc run. > > > > > > I wonder if there are any better ideas? > > > > > > Signed-off-by: Roman Gushchin <guro@fb.com> > > > --- > > > fs/fs-writeback.c | 29 +++++++++++++++++++++++++++++ > > > mm/memcontrol.c | 5 +++++ > > > 2 files changed, 34 insertions(+) > > > > > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > > > index 542b02d170f8..4bbc9a200b2c 100644 > > > --- a/fs/fs-writeback.c > > > +++ b/fs/fs-writeback.c > > > @@ -545,6 +545,35 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id) > > > up_read(&bdi->wb_switch_rwsem); > > > } > > > > > > +static void reparent_dirty_inodes_one_sb(struct super_block *sb, void *arg) > > > +{ > > > + struct inode *inode, *next; > > > + > > > + spin_lock(&sb->s_inode_list_lock); > > > + list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) { > > > + spin_lock(&inode->i_lock); > > > + if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) { > > > + spin_unlock(&inode->i_lock); > > > + continue; > > > + } > > > + > > > + if (inode->i_wb && wb_dying(inode->i_wb)) { > > > + spin_unlock(&inode->i_lock); > > > + inode_switch_wbs(inode, root_mem_cgroup->css.id); > > > + continue; > > > + } > > > + > > > + spin_unlock(&inode->i_lock); > > > + } > > > + spin_unlock(&sb->s_inode_list_lock); > > > > No idea what the best solution is, but I think this is fundamentally > > unworkable. It's not uncommon to have a hundred million cached > > inodes these days, often on a single filesystem. Anything that > > requires a brute-force system wide inode scan, especially without > > conditional reschedule points, is largely a non-starter. > > > > Also, inode_switch_wbs() is not guaranteed to move the inode to the > > destination wb. There can only be WB_FRN_MAX_IN_FLIGHT (1024) > > switches in flight at once and switches are run via RCU callbacks, > > so I suspect that using inode_switch_wbs() for bulk re-assignment is > > going to be a lot more complex than just finding inodes to call > > inode_switch_wbs() on.... > > We can schedule it only if the number of dying cgroups exceeds a certain > number (like 100), which will make it relatively rare event. Maybe we can > add some other conditions, e.g. count the number of inodes associated with > a wb and skip scanning if it's zero. > > Alternatively the wb structure can keep the list of associated inodes, > and scan only them, but then it's not trivial to implement without > additional complication of already quite complex locking scheme. > And because inode_switch_wbs() can fail, we can't guarantee that a single > pass over such a list will be enough. That means the we need to schedule > scans periodically until all inodes will be switched. > > So I really don't know which option is better, but at the same time > doing nothing isn't the option too. Somehow the problem should be solved. I agree with Dave that scanning all inodes in the system can get really expensive quickly. So what I rather think we could do is create another 'IO list' (linked by inode->i_io_list) where we would put inodes that reference the wb but are not in any other IO list of the wb. And then we would switch inodes on this list when the wb is dying... One would have to be somewhat careful with properly draining this list since new inodes can be added to it while we work on it but otherwise I don't see any complication with this. Honza
On Tue, Oct 08, 2019 at 10:20:39AM +0200, Jan Kara wrote: > On Tue 08-10-19 05:38:59, Roman Gushchin wrote: > > On Tue, Oct 08, 2019 at 03:06:31PM +1100, Dave Chinner wrote: > > > On Fri, Oct 04, 2019 at 03:11:04PM -0700, Roman Gushchin wrote: > > > > This is a RFC patch, which is not intended to be merged as is, > > > > but hopefully will start a discussion which can result in a good > > > > solution for the described problem. > > > > > > > > -- > > > > > > > > We've noticed that the number of dying cgroups on our production hosts > > > > tends to grow with the uptime. This time it's caused by the writeback > > > > code. > > > > > > > > An inode which is getting dirty for the first time is associated > > > > with the wb structure (look at __inode_attach_wb()). It can later > > > > be switched to another wb under some conditions (e.g. some other > > > > cgroup is writing a lot of data to the same inode), but generally > > > > stays associated up to the end of life of the inode structure. > > > > > > > > The problem is that the wb structure holds a reference to the original > > > > memory cgroup. So if the inode was dirty once, it has a good chance > > > > to pin down the original memory cgroup. > > > > > > > > An example from the real life: some service runs periodically and > > > > updates rpm packages. Each time in a new memory cgroup. Installed > > > > .so files are heavily used by other cgroups, so corresponding inodes > > > > tend to stay alive for a long. So do pinned memory cgroups. > > > > In production I've seen many hosts with 1-2 thousands of dying > > > > cgroups. > > > > > > > > This is not the first problem with the dying memory cgroups. As > > > > always, the problem is with their relative size: memory cgroups > > > > are large objects, easily 100x-1000x larger that inodes. So keeping > > > > a couple of thousands of dying cgroups in memory without a good reason > > > > (what we easily do with inodes) is quite costly (and is measured > > > > in tens and hundreds of Mb). > > > > > > > > One possible approach to this problem is to switch inodes associated > > > > with dying wbs to the root wb. Switching is a best effort operation > > > > which can fail silently, so unfortunately we can't run once over a > > > > list of associated inodes (even if we'd have such a list). So we > > > > really have to scan all inodes. > > > > > > > > In the proposed patch I schedule a work on each memory cgroup > > > > deletion, which is probably too often. Alternatively, we can do it > > > > periodically under some conditions (e.g. the number of dying memory > > > > cgroups is larger than X). So it's basically a gc run. > > > > > > > > I wonder if there are any better ideas? > > > > > > > > Signed-off-by: Roman Gushchin <guro@fb.com> > > > > --- > > > > fs/fs-writeback.c | 29 +++++++++++++++++++++++++++++ > > > > mm/memcontrol.c | 5 +++++ > > > > 2 files changed, 34 insertions(+) > > > > > > > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > > > > index 542b02d170f8..4bbc9a200b2c 100644 > > > > --- a/fs/fs-writeback.c > > > > +++ b/fs/fs-writeback.c > > > > @@ -545,6 +545,35 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id) > > > > up_read(&bdi->wb_switch_rwsem); > > > > } > > > > > > > > +static void reparent_dirty_inodes_one_sb(struct super_block *sb, void *arg) > > > > +{ > > > > + struct inode *inode, *next; > > > > + > > > > + spin_lock(&sb->s_inode_list_lock); > > > > + list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) { > > > > + spin_lock(&inode->i_lock); > > > > + if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) { > > > > + spin_unlock(&inode->i_lock); > > > > + continue; > > > > + } > > > > + > > > > + if (inode->i_wb && wb_dying(inode->i_wb)) { > > > > + spin_unlock(&inode->i_lock); > > > > + inode_switch_wbs(inode, root_mem_cgroup->css.id); > > > > + continue; > > > > + } > > > > + > > > > + spin_unlock(&inode->i_lock); > > > > + } > > > > + spin_unlock(&sb->s_inode_list_lock); > > > > > > No idea what the best solution is, but I think this is fundamentally > > > unworkable. It's not uncommon to have a hundred million cached > > > inodes these days, often on a single filesystem. Anything that > > > requires a brute-force system wide inode scan, especially without > > > conditional reschedule points, is largely a non-starter. > > > > > > Also, inode_switch_wbs() is not guaranteed to move the inode to the > > > destination wb. There can only be WB_FRN_MAX_IN_FLIGHT (1024) > > > switches in flight at once and switches are run via RCU callbacks, > > > so I suspect that using inode_switch_wbs() for bulk re-assignment is > > > going to be a lot more complex than just finding inodes to call > > > inode_switch_wbs() on.... > > > > We can schedule it only if the number of dying cgroups exceeds a certain > > number (like 100), which will make it relatively rare event. Maybe we can > > add some other conditions, e.g. count the number of inodes associated with > > a wb and skip scanning if it's zero. > > > > Alternatively the wb structure can keep the list of associated inodes, > > and scan only them, but then it's not trivial to implement without > > additional complication of already quite complex locking scheme. > > And because inode_switch_wbs() can fail, we can't guarantee that a single > > pass over such a list will be enough. That means the we need to schedule > > scans periodically until all inodes will be switched. > > > > So I really don't know which option is better, but at the same time > > doing nothing isn't the option too. Somehow the problem should be solved. > > I agree with Dave that scanning all inodes in the system can get really > expensive quickly. So what I rather think we could do is create another 'IO > list' (linked by inode->i_io_list) where we would put inodes that reference > the wb but are not in any other IO list of the wb. And then we would > switch inodes on this list when the wb is dying... One would have to be > somewhat careful with properly draining this list since new inodes can be > added to it while we work on it but otherwise I don't see any complication > with this. Good idea! I've mastered something like this, will test it for a day or two and post here. Thank you!
On Tue, Oct 08, 2019 at 10:20:39AM +0200, Jan Kara wrote: > On Tue 08-10-19 05:38:59, Roman Gushchin wrote: > > On Tue, Oct 08, 2019 at 03:06:31PM +1100, Dave Chinner wrote: > > > On Fri, Oct 04, 2019 at 03:11:04PM -0700, Roman Gushchin wrote: > > > > This is a RFC patch, which is not intended to be merged as is, > > > > but hopefully will start a discussion which can result in a good > > > > solution for the described problem. > > > > > > > > -- > > > > > > > > We've noticed that the number of dying cgroups on our production hosts > > > > tends to grow with the uptime. This time it's caused by the writeback > > > > code. > > > > > > > > An inode which is getting dirty for the first time is associated > > > > with the wb structure (look at __inode_attach_wb()). It can later > > > > be switched to another wb under some conditions (e.g. some other > > > > cgroup is writing a lot of data to the same inode), but generally > > > > stays associated up to the end of life of the inode structure. > > > > > > > > The problem is that the wb structure holds a reference to the original > > > > memory cgroup. So if the inode was dirty once, it has a good chance > > > > to pin down the original memory cgroup. > > > > > > > > An example from the real life: some service runs periodically and > > > > updates rpm packages. Each time in a new memory cgroup. Installed > > > > .so files are heavily used by other cgroups, so corresponding inodes > > > > tend to stay alive for a long. So do pinned memory cgroups. > > > > In production I've seen many hosts with 1-2 thousands of dying > > > > cgroups. > > > > > > > > This is not the first problem with the dying memory cgroups. As > > > > always, the problem is with their relative size: memory cgroups > > > > are large objects, easily 100x-1000x larger that inodes. So keeping > > > > a couple of thousands of dying cgroups in memory without a good reason > > > > (what we easily do with inodes) is quite costly (and is measured > > > > in tens and hundreds of Mb). > > > > > > > > One possible approach to this problem is to switch inodes associated > > > > with dying wbs to the root wb. Switching is a best effort operation > > > > which can fail silently, so unfortunately we can't run once over a > > > > list of associated inodes (even if we'd have such a list). So we > > > > really have to scan all inodes. > > > > > > > > In the proposed patch I schedule a work on each memory cgroup > > > > deletion, which is probably too often. Alternatively, we can do it > > > > periodically under some conditions (e.g. the number of dying memory > > > > cgroups is larger than X). So it's basically a gc run. > > > > > > > > I wonder if there are any better ideas? > > > > > > > > Signed-off-by: Roman Gushchin <guro@fb.com> > > > > --- > > > > fs/fs-writeback.c | 29 +++++++++++++++++++++++++++++ > > > > mm/memcontrol.c | 5 +++++ > > > > 2 files changed, 34 insertions(+) > > > > > > > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > > > > index 542b02d170f8..4bbc9a200b2c 100644 > > > > --- a/fs/fs-writeback.c > > > > +++ b/fs/fs-writeback.c > > > > @@ -545,6 +545,35 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id) > > > > up_read(&bdi->wb_switch_rwsem); > > > > } > > > > > > > > +static void reparent_dirty_inodes_one_sb(struct super_block *sb, void *arg) > > > > +{ > > > > + struct inode *inode, *next; > > > > + > > > > + spin_lock(&sb->s_inode_list_lock); > > > > + list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) { > > > > + spin_lock(&inode->i_lock); > > > > + if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) { > > > > + spin_unlock(&inode->i_lock); > > > > + continue; > > > > + } > > > > + > > > > + if (inode->i_wb && wb_dying(inode->i_wb)) { > > > > + spin_unlock(&inode->i_lock); > > > > + inode_switch_wbs(inode, root_mem_cgroup->css.id); > > > > + continue; > > > > + } > > > > + > > > > + spin_unlock(&inode->i_lock); > > > > + } > > > > + spin_unlock(&sb->s_inode_list_lock); > > > > > > No idea what the best solution is, but I think this is fundamentally > > > unworkable. It's not uncommon to have a hundred million cached > > > inodes these days, often on a single filesystem. Anything that > > > requires a brute-force system wide inode scan, especially without > > > conditional reschedule points, is largely a non-starter. > > > > > > Also, inode_switch_wbs() is not guaranteed to move the inode to the > > > destination wb. There can only be WB_FRN_MAX_IN_FLIGHT (1024) > > > switches in flight at once and switches are run via RCU callbacks, > > > so I suspect that using inode_switch_wbs() for bulk re-assignment is > > > going to be a lot more complex than just finding inodes to call > > > inode_switch_wbs() on.... > > > > We can schedule it only if the number of dying cgroups exceeds a certain > > number (like 100), which will make it relatively rare event. Maybe we can > > add some other conditions, e.g. count the number of inodes associated with > > a wb and skip scanning if it's zero. > > > > Alternatively the wb structure can keep the list of associated inodes, > > and scan only them, but then it's not trivial to implement without > > additional complication of already quite complex locking scheme. > > And because inode_switch_wbs() can fail, we can't guarantee that a single > > pass over such a list will be enough. That means the we need to schedule > > scans periodically until all inodes will be switched. > > > > So I really don't know which option is better, but at the same time > > doing nothing isn't the option too. Somehow the problem should be solved. > > I agree with Dave that scanning all inodes in the system can get really > expensive quickly. So what I rather think we could do is create another 'IO > list' (linked by inode->i_io_list) where we would put inodes that reference > the wb but are not in any other IO list of the wb. And then we would > switch inodes on this list when the wb is dying... One would have to be > somewhat careful with properly draining this list since new inodes can be > added to it while we work on it but otherwise I don't see any complication > with this. > > Honza How about this one? -- From e74bd7f3cf79e07e8d6e776ee2558a729664cbb8 Mon Sep 17 00:00:00 2001 From: Roman Gushchin <guro@fb.com> Date: Wed, 9 Oct 2019 13:14:04 -0700 Subject: [PATCH] cgroup, blkcg: prevent dirty inodes to pin dying memory cgroups We've noticed that the number of dying cgroups on our production hosts tends to grow with the uptime. This time it's caused by the writeback code. An inode which is getting dirty for the first time is associated with the wb structure (look at __inode_attach_wb()). It can later be switched to another wb under some conditions (e.g. some other cgroup is writing a lot of data to the same inode), but generally stays associated up to the end of life of the inode structure. The problem is that the wb structure holds a reference to the original memory cgroup. So if an inode has been dirty once, it has a good chance to pin down the original memory cgroup. An example from the real life: some service runs periodically and updates rpm packages. Each time in a new memory cgroup. Installed .so files are heavily used by other cgroups, so corresponding inodes tend to stay alive for a long. So do pinned memory cgroups. In production I've seen many hosts with 1-2 thousands of dying cgroups. This is not the first problem with the dying memory cgroups. As always, the problem is with their relative size: memory cgroups are large objects, easily 100x-1000x larger that inodes. So keeping a couple of thousands of dying cgroups in memory without a good reason (what we easily do with inodes) is quite costly (and is measured in tens and hundreds of Mb). To solve this problem let's perform a periodic scan of inodes attached to dying wbs, which don't have active io operations, and switched them to the root memory cgroup's wb. That will eventually release the wb structure and corresponding memory cgroup. To make this scanning effective, let's keep a list of attached inodes. inode->i_io_list can be reused for this purpose. This idea was suggested by Jan Kara. The scan is performed from the cgroup offlining path. Dying wbs are placed on the global list. On each cgroup removal we traverse the whole list ignoring wbs with active io operations. That will allow the majority of io operations to be finished after the removal of the cgroup. To avoid scheduling too many switch operations, let's stop on a first failure. To make it's possible, inode_switch_wbs() can return a boolean value: false if it's failed to schedule a switching operation because there are already too many in flight, or if there is not enough memory; true otherwise. Signed-off-by: Roman Gushchin <guro@fb.com> --- fs/fs-writeback.c | 64 +++++++++++++++++++++++++------ include/linux/backing-dev-defs.h | 2 + include/linux/writeback.h | 2 + mm/backing-dev.c | 66 ++++++++++++++++++++++++++++++-- 4 files changed, 119 insertions(+), 15 deletions(-) diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index e88421d9a48d..af608276fbf6 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -136,16 +136,21 @@ static bool inode_io_list_move_locked(struct inode *inode, * inode_io_list_del_locked - remove an inode from its bdi_writeback IO list * @inode: inode to be removed * @wb: bdi_writeback @inode is being removed from + * @keep_attached: keep the inode on the list of inodes attached to wb * * Remove @inode which may be on one of @wb->b_{dirty|io|more_io} lists and * clear %WB_has_dirty_io if all are empty afterwards. */ static void inode_io_list_del_locked(struct inode *inode, - struct bdi_writeback *wb) + struct bdi_writeback *wb, + bool keep_attached) { assert_spin_locked(&wb->list_lock); - list_del_init(&inode->i_io_list); + if (keep_attached) + list_move(&inode->i_io_list, &wb->b_attached); + else + list_del_init(&inode->i_io_list); wb_io_lists_depopulated(wb); } @@ -426,7 +431,7 @@ static void inode_switch_wbs_work_fn(struct work_struct *work) if (!list_empty(&inode->i_io_list)) { struct inode *pos; - inode_io_list_del_locked(inode, old_wb); + inode_io_list_del_locked(inode, old_wb, false); inode->i_wb = new_wb; list_for_each_entry(pos, &new_wb->b_dirty, i_io_list) if (time_after_eq(inode->dirtied_when, @@ -485,24 +490,29 @@ static void inode_switch_wbs_rcu_fn(struct rcu_head *rcu_head) * * Switch @inode's wb association to the wb identified by @new_wb_id. The * switching is performed asynchronously and may fail silently. + * + * Returns %true is the operation has been scheduled successfully or + * if the inode cannot be switched because of its own state + * (e.g. inode is already switching). Returns %false otherwise. */ -static void inode_switch_wbs(struct inode *inode, int new_wb_id) +static bool inode_switch_wbs(struct inode *inode, int new_wb_id) { struct backing_dev_info *bdi = inode_to_bdi(inode); struct cgroup_subsys_state *memcg_css; struct inode_switch_wbs_context *isw; + bool ret = false; /* noop if seems to be already in progress */ if (inode->i_state & I_WB_SWITCH) - return; + return true; /* avoid queueing a new switch if too many are already in flight */ if (atomic_read(&isw_nr_in_flight) > WB_FRN_MAX_IN_FLIGHT) - return; + return false; isw = kzalloc(sizeof(*isw), GFP_ATOMIC); if (!isw) - return; + return true; /* find and pin the new wb */ rcu_read_lock(); @@ -519,6 +529,7 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id) inode->i_state & (I_WB_SWITCH | I_FREEING) || inode_to_wb(inode) == isw->new_wb) { spin_unlock(&inode->i_lock); + ret = true; goto out_free; } inode->i_state |= I_WB_SWITCH; @@ -536,12 +547,43 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id) call_rcu(&isw->rcu_head, inode_switch_wbs_rcu_fn); atomic_inc(&isw_nr_in_flight); - return; + return true; out_free: if (isw->new_wb) wb_put(isw->new_wb); kfree(isw); + return ret; +} + +/** + * cleanup_offline_wb - switch attached inodes to the root wb + * @wb: target wb + * + * Switch inodes attached to @wb to the root memory cgroup's wb. + * Switching is performed asynchronously and may fail silently. + * + * Returns %false if at least one switching attempt has been failed, + * %true otherwise. + */ +bool cleanup_offline_wb(struct bdi_writeback *wb) +{ + struct inode *inode; + bool ret = true; + + spin_lock(&wb->list_lock); + if (list_empty(&wb->b_attached)) + goto unlock; + + list_for_each_entry(inode, &wb->b_attached, i_io_list) { + ret = inode_switch_wbs(inode, root_mem_cgroup->css.id); + if (!ret) + break; + } +unlock: + spin_unlock(&wb->list_lock); + + return ret; } /** @@ -1120,7 +1162,7 @@ void inode_io_list_del(struct inode *inode) struct bdi_writeback *wb; wb = inode_to_wb_and_lock_list(inode); - inode_io_list_del_locked(inode, wb); + inode_io_list_del_locked(inode, wb, false); spin_unlock(&wb->list_lock); } @@ -1425,7 +1467,7 @@ static void requeue_inode(struct inode *inode, struct bdi_writeback *wb, inode_io_list_move_locked(inode, wb, &wb->b_dirty_time); } else { /* The inode is clean. Remove from writeback lists. */ - inode_io_list_del_locked(inode, wb); + inode_io_list_del_locked(inode, wb, true); } } @@ -1570,7 +1612,7 @@ static int writeback_single_inode(struct inode *inode, * touch it. See comment above for explanation. */ if (!(inode->i_state & I_DIRTY_ALL)) - inode_io_list_del_locked(inode, wb); + inode_io_list_del_locked(inode, wb, true); spin_unlock(&wb->list_lock); inode_sync_complete(inode); out: diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h index 4fc87dee005a..68b167fda259 100644 --- a/include/linux/backing-dev-defs.h +++ b/include/linux/backing-dev-defs.h @@ -137,6 +137,7 @@ struct bdi_writeback { struct list_head b_io; /* parked for writeback */ struct list_head b_more_io; /* parked for more writeback */ struct list_head b_dirty_time; /* time stamps are dirty */ + struct list_head b_attached; /* attached inodes */ spinlock_t list_lock; /* protects the b_* lists */ struct percpu_counter stat[NR_WB_STAT_ITEMS]; @@ -177,6 +178,7 @@ struct bdi_writeback { struct cgroup_subsys_state *blkcg_css; /* and blkcg */ struct list_head memcg_node; /* anchored at memcg->cgwb_list */ struct list_head blkcg_node; /* anchored at blkcg->cgwb_list */ + struct list_head offline_node; union { struct work_struct release_work; diff --git a/include/linux/writeback.h b/include/linux/writeback.h index a19d845dd7eb..7f430644a629 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -220,6 +220,7 @@ void wbc_account_cgroup_owner(struct writeback_control *wbc, struct page *page, int cgroup_writeback_by_id(u64 bdi_id, int memcg_id, unsigned long nr_pages, enum wb_reason reason, struct wb_completion *done); void cgroup_writeback_umount(void); +bool cleanup_offline_wb(struct bdi_writeback *wb); /** * inode_attach_wb - associate an inode with its wb @@ -247,6 +248,7 @@ static inline void inode_detach_wb(struct inode *inode) if (inode->i_wb) { WARN_ON_ONCE(!(inode->i_state & I_CLEAR)); wb_put(inode->i_wb); + WARN_ON_ONCE(!list_empty(&inode->i_io_list)); inode->i_wb = NULL; } } diff --git a/mm/backing-dev.c b/mm/backing-dev.c index d9daa3e422d0..774c05672a27 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -52,10 +52,10 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v) unsigned long background_thresh; unsigned long dirty_thresh; unsigned long wb_thresh; - unsigned long nr_dirty, nr_io, nr_more_io, nr_dirty_time; + unsigned long nr_dirty, nr_io, nr_more_io, nr_dirty_time, nr_attached; struct inode *inode; - nr_dirty = nr_io = nr_more_io = nr_dirty_time = 0; + nr_dirty = nr_io = nr_more_io = nr_dirty_time = nr_attached = 0; spin_lock(&wb->list_lock); list_for_each_entry(inode, &wb->b_dirty, i_io_list) nr_dirty++; @@ -66,6 +66,8 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v) list_for_each_entry(inode, &wb->b_dirty_time, i_io_list) if (inode->i_state & I_DIRTY_TIME) nr_dirty_time++; + list_for_each_entry(inode, &wb->b_attached, i_io_list) + nr_attached++; spin_unlock(&wb->list_lock); global_dirty_limits(&background_thresh, &dirty_thresh); @@ -85,6 +87,7 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v) "b_io: %10lu\n" "b_more_io: %10lu\n" "b_dirty_time: %10lu\n" + "b_attached: %10lu\n" "bdi_list: %10u\n" "state: %10lx\n", (unsigned long) K(wb_stat(wb, WB_WRITEBACK)), @@ -99,6 +102,7 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v) nr_io, nr_more_io, nr_dirty_time, + nr_attached, !list_empty(&bdi->bdi_list), bdi->wb.state); #undef K @@ -295,6 +299,7 @@ static int wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi, INIT_LIST_HEAD(&wb->b_io); INIT_LIST_HEAD(&wb->b_more_io); INIT_LIST_HEAD(&wb->b_dirty_time); + INIT_LIST_HEAD(&wb->b_attached); spin_lock_init(&wb->list_lock); wb->bw_time_stamp = jiffies; @@ -385,11 +390,12 @@ static void wb_exit(struct bdi_writeback *wb) /* * cgwb_lock protects bdi->cgwb_tree, bdi->cgwb_congested_tree, - * blkcg->cgwb_list, and memcg->cgwb_list. bdi->cgwb_tree is also RCU - * protected. + * blkcg->cgwb_list, offline_cgwbs and memcg->cgwb_list. + * bdi->cgwb_tree is also RCU protected. */ static DEFINE_SPINLOCK(cgwb_lock); static struct workqueue_struct *cgwb_release_wq; +static LIST_HEAD(offline_cgwbs); /** * wb_congested_get_create - get or create a wb_congested @@ -486,6 +492,10 @@ static void cgwb_release_workfn(struct work_struct *work) mutex_lock(&wb->bdi->cgwb_release_mutex); wb_shutdown(wb); + spin_lock_irq(&cgwb_lock); + list_del(&wb->offline_node); + spin_unlock_irq(&cgwb_lock); + css_put(wb->memcg_css); css_put(wb->blkcg_css); mutex_unlock(&wb->bdi->cgwb_release_mutex); @@ -513,6 +523,7 @@ static void cgwb_kill(struct bdi_writeback *wb) WARN_ON(!radix_tree_delete(&wb->bdi->cgwb_tree, wb->memcg_css->id)); list_del(&wb->memcg_node); list_del(&wb->blkcg_node); + list_add(&wb->offline_node, &offline_cgwbs); percpu_ref_kill(&wb->refcnt); } @@ -734,6 +745,50 @@ static void cgwb_bdi_unregister(struct backing_dev_info *bdi) mutex_unlock(&bdi->cgwb_release_mutex); } +/** + * cleanup_offline_cgwbs - try to release dying cgwbs + * + * Try to release dying cgwbs by switching attached inodes to the wb + * belonging to the root memory cgroup. Processed wbs are placed at the + * end of the list to guarantee the forward progress. + * + * Should be called with the acquired cgwb_lock lock, which might + * be released and re-acquired in the process. + */ +static void cleanup_offline_cgwbs(void) +{ + struct bdi_writeback *wb; + LIST_HEAD(processed); + bool cont = true; + + lockdep_assert_held(&cgwb_lock); + + do { + wb = list_first_entry_or_null(&offline_cgwbs, + struct bdi_writeback, + offline_node); + if (!wb) + break; + + list_move_tail(&wb->offline_node, &processed); + + if (wb_has_dirty_io(wb)) + continue; + + if (!percpu_ref_tryget(&wb->refcnt)) + continue; + + spin_unlock_irq(&cgwb_lock); + cont = cleanup_offline_wb(wb); + spin_lock_irq(&cgwb_lock); + + wb_put(wb); + } while (cont); + + if (!list_empty(&processed)) + list_splice_tail(&processed, &offline_cgwbs); +} + /** * wb_memcg_offline - kill all wb's associated with a memcg being offlined * @memcg: memcg being offlined @@ -749,6 +804,9 @@ void wb_memcg_offline(struct mem_cgroup *memcg) list_for_each_entry_safe(wb, next, memcg_cgwb_list, memcg_node) cgwb_kill(wb); memcg_cgwb_list->next = NULL; /* prevent new wb's */ + + cleanup_offline_cgwbs(); + spin_unlock_irq(&cgwb_lock); }
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index 542b02d170f8..4bbc9a200b2c 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -545,6 +545,35 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id) up_read(&bdi->wb_switch_rwsem); } +static void reparent_dirty_inodes_one_sb(struct super_block *sb, void *arg) +{ + struct inode *inode, *next; + + spin_lock(&sb->s_inode_list_lock); + list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) { + spin_lock(&inode->i_lock); + if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) { + spin_unlock(&inode->i_lock); + continue; + } + + if (inode->i_wb && wb_dying(inode->i_wb)) { + spin_unlock(&inode->i_lock); + inode_switch_wbs(inode, root_mem_cgroup->css.id); + continue; + } + + spin_unlock(&inode->i_lock); + } + spin_unlock(&sb->s_inode_list_lock); + +} + +void reparent_dirty_inodes(struct work_struct *work) +{ + iterate_supers(reparent_dirty_inodes_one_sb, NULL); +} + /** * wbc_attach_and_unlock_inode - associate wbc with target inode and unlock it * @wbc: writeback_control of interest diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 9ec5e12486a7..ea8bc8d1403b 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -4911,6 +4911,9 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css) return 0; } +extern void reparent_dirty_inodes(struct work_struct *w); +static DECLARE_WORK(dirty_work, reparent_dirty_inodes); + static void mem_cgroup_css_offline(struct cgroup_subsys_state *css) { struct mem_cgroup *memcg = mem_cgroup_from_css(css); @@ -4934,6 +4937,8 @@ static void mem_cgroup_css_offline(struct cgroup_subsys_state *css) memcg_offline_kmem(memcg); wb_memcg_offline(memcg); + schedule_work(&dirty_work); + drain_all_stock(memcg); mem_cgroup_id_put(memcg);
This is a RFC patch, which is not intended to be merged as is, but hopefully will start a discussion which can result in a good solution for the described problem. -- We've noticed that the number of dying cgroups on our production hosts tends to grow with the uptime. This time it's caused by the writeback code. An inode which is getting dirty for the first time is associated with the wb structure (look at __inode_attach_wb()). It can later be switched to another wb under some conditions (e.g. some other cgroup is writing a lot of data to the same inode), but generally stays associated up to the end of life of the inode structure. The problem is that the wb structure holds a reference to the original memory cgroup. So if the inode was dirty once, it has a good chance to pin down the original memory cgroup. An example from the real life: some service runs periodically and updates rpm packages. Each time in a new memory cgroup. Installed .so files are heavily used by other cgroups, so corresponding inodes tend to stay alive for a long. So do pinned memory cgroups. In production I've seen many hosts with 1-2 thousands of dying cgroups. This is not the first problem with the dying memory cgroups. As always, the problem is with their relative size: memory cgroups are large objects, easily 100x-1000x larger that inodes. So keeping a couple of thousands of dying cgroups in memory without a good reason (what we easily do with inodes) is quite costly (and is measured in tens and hundreds of Mb). One possible approach to this problem is to switch inodes associated with dying wbs to the root wb. Switching is a best effort operation which can fail silently, so unfortunately we can't run once over a list of associated inodes (even if we'd have such a list). So we really have to scan all inodes. In the proposed patch I schedule a work on each memory cgroup deletion, which is probably too often. Alternatively, we can do it periodically under some conditions (e.g. the number of dying memory cgroups is larger than X). So it's basically a gc run. I wonder if there are any better ideas? Signed-off-by: Roman Gushchin <guro@fb.com> --- fs/fs-writeback.c | 29 +++++++++++++++++++++++++++++ mm/memcontrol.c | 5 +++++ 2 files changed, 34 insertions(+)