From: Roman Gushchin <hidden> Date: 2021-06-04 01:32:07
When an inode is getting dirty for the first time it's associated
with a wb structure (see __inode_attach_wb()). It can later be
switched to another wb (if e.g. some other cgroup is writing a lot of
data to the same inode), but otherwise stays attached to the original
wb until being reclaimed.
The problem is that the wb structure holds a reference to the original
memory and blkcg cgroups. So if an inode has been dirty once and later
is actively used in read-only mode, it has a good chance to pin down
the original memory and blkcg cgroups forewer. This is often the case with
services bringing data for other services, e.g. updating some rpm
packages.
In the real life it becomes a problem due to a large size of the memcg
structure, which can easily be 1000x larger than an inode. Also a
really large number of dying cgroups can raise different scalability
issues, e.g. making the memory reclaim costly and less effective.
To solve the problem inodes should be eventually detached from the
corresponding writeback structure. It's inefficient to do it after
every writeback completion. Instead it can be done whenever the
original memory cgroup is offlined and writeback structure is getting
killed. Scanning over a (potentially long) list of inodes and detach
them from the writeback structure can take quite some time. To avoid
scanning all inodes, attached inodes are kept on a new list (b_attached).
To make it less noticeable to a user, the scanning and switching is performed
from a work context.
Big thanks to Jan Kara, Dennis Zhou and Hillf Danton for their ideas and
contribution to this patchset.
v7:
- shared locking for multiple inode switching
- introduced inode_prepare_wbs_switch() helper
- extended the pre-switch inode check for I_WILL_FREE
- added comments here and there
v6:
- extended and reused wbs switching functionality to switch inodes
on cgwb cleanup
- fixed offline_list handling
- switched to the unbound_wq
- other minor fixes
v5:
- switch inodes to bdi->wb instead of zeroing inode->i_wb
- split the single patch into two
- only cgwbs maintain lists of attached inodes
- added cond_resched()
- fixed !CONFIG_CGROUP_WRITEBACK handling
- extended list of prohibited inodes flag
- other small fixes
Roman Gushchin (6):
writeback, cgroup: do not switch inodes with I_WILL_FREE flag
writeback, cgroup: switch to rcu_work API in inode_switch_wbs()
writeback, cgroup: keep list of inodes attached to bdi_writeback
writeback, cgroup: split out the functional part of
inode_switch_wbs_work_fn()
writeback, cgroup: support switching multiple inodes at once
writeback, cgroup: release dying cgwbs by switching attached inodes
fs/fs-writeback.c | 302 +++++++++++++++++++++----------
include/linux/backing-dev-defs.h | 20 +-
include/linux/writeback.h | 1 +
mm/backing-dev.c | 69 ++++++-
4 files changed, 293 insertions(+), 99 deletions(-)
--
2.31.1
From: Roman Gushchin <hidden> Date: 2021-06-04 01:32:09
If an inode's state has I_WILL_FREE flag set, the inode will be
freed soon, so there is no point in trying to switch the inode
to a different cgwb.
I_WILL_FREE was ignored since the introduction of the inode switching,
so it looks like it doesn't lead to any noticeable issues for a user.
This is why the patch is not intended for a stable backport.
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Roman Gushchin <redacted>
---
fs/fs-writeback.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -517,7 +517,7 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)/* while holding I_WB_SWITCH, no one else can update the association */spin_lock(&inode->i_lock);if(!(inode->i_sb->s_flags&SB_ACTIVE)||-inode->i_state&(I_WB_SWITCH|I_FREEING)||+inode->i_state&(I_WB_SWITCH|I_FREEING|I_WILL_FREE)||inode_to_wb(inode)==isw->new_wb){spin_unlock(&inode->i_lock);gotoout_free;
From: Roman Gushchin <hidden> Date: 2021-06-04 01:32:12
Split out the functional part of the inode_switch_wbs_work_fn()
function as inode_do switch_wbs() to reuse it later for switching
inodes attached to dying cgwbs.
This commit doesn't bring any functional changes.
Signed-off-by: Roman Gushchin <redacted>
Reviewed-by: Jan Kara <jack@suse.cz>
---
fs/fs-writeback.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
From: Roman Gushchin <hidden> Date: 2021-06-04 01:32:14
Currently only a single inode can be switched to another writeback
structure at once. That means to switch an inode a separate
inode_switch_wbs_context structure must be allocated, and a separate
rcu callback and work must be scheduled.
It's fine for the existing ad-hoc switching, which is not happening
that often, but sub-optimal for massive switching required in order to
release a writeback structure. To prepare for it, let's add a support
for switching multiple inodes at once.
Instead of containing a single inode pointer, inode_switch_wbs_context
will contain a NULL-terminated array of inode pointers.
inode_do_switch_wbs() will be called for each inode.
To optimize the locking bdi->wb_switch_rwsem, old_wb's and new_wb's
list_locks will be acquired and released only once altogether for all
inodes. wb_wakeup() will be also be called only once. Instead of
calling wb_put(old_wb) after each successful switch, wb_put_many()
is introduced and used.
Signed-off-by: Roman Gushchin <redacted>
---
fs/fs-writeback.c | 105 ++++++++++++++++++-------------
include/linux/backing-dev-defs.h | 18 +++++-
2 files changed, 79 insertions(+), 44 deletions(-)
From: Roman Gushchin <hidden> Date: 2021-06-04 01:32:16
Asynchronously try to release dying cgwbs by switching attached inodes
to the bdi's wb. It helps to get rid of per-cgroup writeback
structures themselves and of pinned memory and block cgroups, which
are significantly larger structures (mostly due to large per-cpu
statistics data). This prevents memory waste and helps to avoid
different scalability problems caused by large piles of dying cgroups.
Reuse the existing mechanism of inode switching used for foreign inode
detection. To speed things up batch up to 115 inode switching in a
single operation (the maximum number is selected so that the resulting
struct inode_switch_wbs_context can fit into 1024 bytes). Because
every switching consists of two steps divided by an RCU grace period,
it would be too slow without batching. Please note that the whole
batch counts as a single operation (when increasing/decreasing
isw_nr_in_flight). This allows to keep umounting working (flush the
switching queue), however prevents cleanups from consuming the whole
switching quota and effectively blocking the frn switching.
A cgwb cleanup operation can fail due to different reasons (e.g. not
enough memory, the cgwb has an in-flight/pending io, an attached inode
in a wrong state, etc). In this case the next scheduled cleanup will
make a new attempt. An attempt is made each time a new cgwb is offlined
(in other words a memcg and/or a blkcg is deleted by a user). In the
future an additional attempt scheduled by a timer can be implemented.
Signed-off-by: Roman Gushchin <redacted>
---
fs/fs-writeback.c | 93 ++++++++++++++++++++++++++++----
include/linux/backing-dev-defs.h | 1 +
include/linux/writeback.h | 1 +
mm/backing-dev.c | 67 ++++++++++++++++++++++-
4 files changed, 150 insertions(+), 12 deletions(-)
@@ -225,6 +225,12 @@ void wb_wait_for_completion(struct wb_completion *done)/* one round can affect upto 5 slots */#define WB_FRN_MAX_IN_FLIGHT 1024 /* don't queue too many concurrently */+/*+*Maximuminodesperisw.Aspecificvaluehasbeenchosentomake+*structinode_switch_wbs_contextfitinto1024byteskmalloc.+*/+#define WB_MAX_INODES_PER_ISW 115+staticatomic_tisw_nr_in_flight=ATOMIC_INIT(0);staticstructworkqueue_struct*isw_wq;
@@ -502,6 +508,24 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)atomic_dec(&isw_nr_in_flight);}+staticboolinode_prepare_wbs_switch(structinode*inode,+structbdi_writeback*new_wb)+{+/* while holding I_WB_SWITCH, no one else can update the association */+spin_lock(&inode->i_lock);+if(!(inode->i_sb->s_flags&SB_ACTIVE)||+inode->i_state&(I_WB_SWITCH|I_FREEING|I_WILL_FREE)||+inode_to_wb(inode)==new_wb){+spin_unlock(&inode->i_lock);+returnfalse;+}+inode->i_state|=I_WB_SWITCH;+__iget(inode);+spin_unlock(&inode->i_lock);++returntrue;+}+/***inode_switch_wbs-changethewbassociationofaninode*@inode:targetinode
@@ -537,17 +561,8 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)if(!isw->new_wb)gotoout_free;-/* while holding I_WB_SWITCH, no one else can update the association */-spin_lock(&inode->i_lock);-if(!(inode->i_sb->s_flags&SB_ACTIVE)||-inode->i_state&(I_WB_SWITCH|I_FREEING|I_WILL_FREE)||-inode_to_wb(inode)==isw->new_wb){-spin_unlock(&inode->i_lock);+if(!inode_prepare_wbs_switch(inode,isw->new_wb))gotoout_free;-}-inode->i_state|=I_WB_SWITCH;-__iget(inode);-spin_unlock(&inode->i_lock);isw->inodes[0]=inode;
@@ -569,6 +584,64 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)kfree(isw);}+/**+*cleanup_offline_cgwb-detachassociatedinodes+*@wb:targetwb+*+*Switchallinodesattachedto@wbtothebdi'srootwbinordertoeventually+*releasethedying@wb.Returns%trueifnotallinodeswereswitchedand+*thefunctionhastoberestarted.+*/+boolcleanup_offline_cgwb(structbdi_writeback*wb)+{+structinode_switch_wbs_context*isw;+structinode*inode;+intnr;+boolrestart=false;++isw=kzalloc(sizeof(*isw)+WB_MAX_INODES_PER_ISW*+sizeof(structinode*),GFP_KERNEL);+if(!isw)+returnrestart;++/* no need to call wb_get() here: bdi's root wb is not refcounted */+isw->new_wb=&wb->bdi->wb;++nr=0;+spin_lock(&wb->list_lock);+list_for_each_entry(inode,&wb->b_attached,i_io_list){+if(!inode_prepare_wbs_switch(inode,isw->new_wb))+continue;++isw->inodes[nr++]=inode;++if(nr>=WB_MAX_INODES_PER_ISW-1){+restart=true;+break;+}+}+spin_unlock(&wb->list_lock);++/* no attached inodes? bail out */+if(nr==0){+kfree(isw);+returnrestart;+}++/*+*Inadditiontosynchronizingamongswitchers,I_WB_SWITCHtells+*theRCUprotectedstatupdatepathstograbthei_page+*locksothatstattransfercansynchronizeagainstthem.+*Let'scontinueafterI_WB_SWITCHisguaranteedtobevisible.+*/+INIT_RCU_WORK(&isw->work,inode_switch_wbs_work_fn);+queue_rcu_work(isw_wq,&isw->work);++atomic_inc(&isw_nr_in_flight);++returnrestart;+}+/***wbc_attach_and_unlock_inode-associatewbcwithtargetinodeandunlockit*@wbc:writeback_controlofinterest
From: Roman Gushchin <hidden> Date: 2021-06-04 01:32:22
Currently there is no way to iterate over inodes attached to a
specific cgwb structure. It limits the ability to efficiently
reclaim the writeback structure itself and associated memory and
block cgroup structures without scanning all inodes belonging to a sb,
which can be prohibitively expensive.
While dirty/in-active-writeback an inode belongs to one of the
bdi_writeback's io lists: b_dirty, b_io, b_more_io and b_dirty_time.
Once cleaned up, it's removed from all io lists. So the
inode->i_io_list can be reused to maintain the list of inodes,
attached to a bdi_writeback structure.
This patch introduces a new wb->b_attached list, which contains all
inodes which were dirty at least once and are attached to the given
cgwb. Inodes attached to the root bdi_writeback structures are never
placed on such list. The following patch will use this list to try to
release cgwbs structures more efficiently.
Suggested-by: Jan Kara <redacted>
Signed-off-by: Roman Gushchin <redacted>
Reviewed-by: Jan Kara <redacted>
---
fs/fs-writeback.c | 93 ++++++++++++++++++++------------
include/linux/backing-dev-defs.h | 1 +
mm/backing-dev.c | 2 +
3 files changed, 62 insertions(+), 34 deletions(-)
From: Roman Gushchin <hidden> Date: 2021-06-04 01:32:27
Inode's wb switching requires two steps divided by an RCU grace
period. It's currently implemented as an RCU callback
inode_switch_wbs_rcu_fn(), which schedules inode_switch_wbs_work_fn()
as a work.
Switching to the rcu_work API allows to do the same in a cleaner and
slightly shorter form.
Signed-off-by: Roman Gushchin <redacted>
Reviewed-by: Jan Kara <redacted>
---
fs/fs-writeback.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
Hello,
On Thu, Jun 03, 2021 at 06:31:59PM -0700, Roman Gushchin wrote:
+bool cleanup_offline_cgwb(struct bdi_writeback *wb)
+{
+ struct inode_switch_wbs_context *isw;
+ struct inode *inode;
+ int nr;
+ bool restart = false;
+
+ isw = kzalloc(sizeof(*isw) + WB_MAX_INODES_PER_ISW *
+ sizeof(struct inode *), GFP_KERNEL);
+ if (!isw)
+ return restart;
+
+ /* no need to call wb_get() here: bdi's root wb is not refcounted */
+ isw->new_wb = &wb->bdi->wb;
Not a deal breaker but I wonder whether it'd be safer to migrate it to the
nearest live ancestor rather than directly to the root. As adaptive
migration isn't something guaranteed, there's some chance that this can
behave as escape-to-root path in pathological cases especially for inodes
which may be written to by multiple cgroups.
Thanks.
--
tejun
On Thu, Jun 03, 2021 at 06:31:53PM -0700, Roman Gushchin wrote:
To solve the problem inodes should be eventually detached from the
corresponding writeback structure. It's inefficient to do it after
every writeback completion. Instead it can be done whenever the
original memory cgroup is offlined and writeback structure is getting
killed. Scanning over a (potentially long) list of inodes and detach
them from the writeback structure can take quite some time. To avoid
scanning all inodes, attached inodes are kept on a new list (b_attached).
To make it less noticeable to a user, the scanning and switching is performed
from a work context.
Sorry for chiming in late but the series looks great to me and the only
comment I have is the migration target on the last patch, which isn't a
critical issue. Please feel free to add
Acked-by: Tejun Heo [off-list ref]
Thanks.
--
tejun
From: Roman Gushchin <hidden> Date: 2021-06-04 22:25:00
On Fri, Jun 04, 2021 at 11:53:02AM -0400, Tejun Heo wrote:
On Thu, Jun 03, 2021 at 06:31:53PM -0700, Roman Gushchin wrote:
quoted
To solve the problem inodes should be eventually detached from the
corresponding writeback structure. It's inefficient to do it after
every writeback completion. Instead it can be done whenever the
original memory cgroup is offlined and writeback structure is getting
killed. Scanning over a (potentially long) list of inodes and detach
them from the writeback structure can take quite some time. To avoid
scanning all inodes, attached inodes are kept on a new list (b_attached).
To make it less noticeable to a user, the scanning and switching is performed
from a work context.
Sorry for chiming in late but the series looks great to me and the only
comment I have is the migration target on the last patch, which isn't a
critical issue. Please feel free to add
Acked-by: Tejun Heo [off-list ref]
Thank you for taking a look and for acking the series!
I agree that switching to the nearest ancestor makes sense. If I remember
correctly, I was doing this in v1 (or at least planned to do), but then
switched to zeroing the pointer and then to bdi's wb.
I fixed it in v8 and pushed it here: https://github.com/rgushchin/linux/tree/cgwb.8 .
I'll wait a bit for Jan's and others feedback and will post v8 on Monday.
Hopefully, it will be the final version.
Btw, how are such patches usually routed? Through Jens's tree?
Thanks!
Hello,
On Fri, Jun 04, 2021 at 03:24:38PM -0700, Roman Gushchin wrote:
I agree that switching to the nearest ancestor makes sense. If I remember
correctly, I was doing this in v1 (or at least planned to do), but then
switched to zeroing the pointer and then to bdi's wb.
I fixed it in v8 and pushed it here: https://github.com/rgushchin/linux/tree/cgwb.8 .
I'll wait a bit for Jan's and others feedback and will post v8 on Monday.
Hopefully, it will be the final version.
Sounds great.
Btw, how are such patches usually routed? Through Jens's tree?
I think the past writeback patches went through -mm.
Thanks.
--
tejun
From: Dennis Zhou <dennis@kernel.org> Date: 2021-06-05 21:35:03
Hello,
On Thu, Jun 03, 2021 at 06:31:59PM -0700, Roman Gushchin wrote:
Asynchronously try to release dying cgwbs by switching attached inodes
to the bdi's wb. It helps to get rid of per-cgroup writeback
structures themselves and of pinned memory and block cgroups, which
are significantly larger structures (mostly due to large per-cpu
statistics data). This prevents memory waste and helps to avoid
different scalability problems caused by large piles of dying cgroups.
Reuse the existing mechanism of inode switching used for foreign inode
detection. To speed things up batch up to 115 inode switching in a
single operation (the maximum number is selected so that the resulting
struct inode_switch_wbs_context can fit into 1024 bytes). Because
every switching consists of two steps divided by an RCU grace period,
it would be too slow without batching. Please note that the whole
batch counts as a single operation (when increasing/decreasing
isw_nr_in_flight). This allows to keep umounting working (flush the
switching queue), however prevents cleanups from consuming the whole
switching quota and effectively blocking the frn switching.
A cgwb cleanup operation can fail due to different reasons (e.g. not
enough memory, the cgwb has an in-flight/pending io, an attached inode
in a wrong state, etc). In this case the next scheduled cleanup will
make a new attempt. An attempt is made each time a new cgwb is offlined
(in other words a memcg and/or a blkcg is deleted by a user). In the
future an additional attempt scheduled by a timer can be implemented.
I've been thinking about this for a little while and the only thing I'm
not super thrilled by is that the subsequent cleanup work trigger isn't
due to forward progress.
As future work, we could tag the inodes to switch when writeback
completes instead of using a timer. This would be nice because then we
only have to make a single (successful) pass switching the inodes we can
and then mark the others to switch. Once a cgwb is killed no one else
can attach to it so we should be good there.
I don't think this is a blocker or even necessary, I just wanted to put
it out there as possible future direction instead of a timer.
Thanks,
Dennis
@@ -225,6 +225,12 @@ void wb_wait_for_completion(struct wb_completion *done)/* one round can affect upto 5 slots */#define WB_FRN_MAX_IN_FLIGHT 1024 /* don't queue too many concurrently */+/*+*Maximuminodesperisw.Aspecificvaluehasbeenchosentomake+*structinode_switch_wbs_contextfitinto1024byteskmalloc.+*/+#define WB_MAX_INODES_PER_ISW 115+staticatomic_tisw_nr_in_flight=ATOMIC_INIT(0);staticstructworkqueue_struct*isw_wq;
@@ -502,6 +508,24 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)atomic_dec(&isw_nr_in_flight);}+staticboolinode_prepare_wbs_switch(structinode*inode,+structbdi_writeback*new_wb)+{+/* while holding I_WB_SWITCH, no one else can update the association */+spin_lock(&inode->i_lock);+if(!(inode->i_sb->s_flags&SB_ACTIVE)||+inode->i_state&(I_WB_SWITCH|I_FREEING|I_WILL_FREE)||+inode_to_wb(inode)==new_wb){+spin_unlock(&inode->i_lock);+returnfalse;+}+inode->i_state|=I_WB_SWITCH;+__iget(inode);+spin_unlock(&inode->i_lock);++returntrue;+}+/***inode_switch_wbs-changethewbassociationofaninode*@inode:targetinode
@@ -537,17 +561,8 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)if(!isw->new_wb)gotoout_free;-/* while holding I_WB_SWITCH, no one else can update the association */-spin_lock(&inode->i_lock);-if(!(inode->i_sb->s_flags&SB_ACTIVE)||-inode->i_state&(I_WB_SWITCH|I_FREEING|I_WILL_FREE)||-inode_to_wb(inode)==isw->new_wb){-spin_unlock(&inode->i_lock);+if(!inode_prepare_wbs_switch(inode,isw->new_wb))gotoout_free;-}-inode->i_state|=I_WB_SWITCH;-__iget(inode);-spin_unlock(&inode->i_lock);isw->inodes[0]=inode;
@@ -569,6 +584,64 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)kfree(isw);}+/**+*cleanup_offline_cgwb-detachassociatedinodes+*@wb:targetwb+*+*Switchallinodesattachedto@wbtothebdi'srootwbinordertoeventually+*releasethedying@wb.Returns%trueifnotallinodeswereswitchedand+*thefunctionhastoberestarted.+*/+boolcleanup_offline_cgwb(structbdi_writeback*wb)+{+structinode_switch_wbs_context*isw;+structinode*inode;+intnr;+boolrestart=false;++isw=kzalloc(sizeof(*isw)+WB_MAX_INODES_PER_ISW*+sizeof(structinode*),GFP_KERNEL);+if(!isw)+returnrestart;++/* no need to call wb_get() here: bdi's root wb is not refcounted */+isw->new_wb=&wb->bdi->wb;++nr=0;+spin_lock(&wb->list_lock);+list_for_each_entry(inode,&wb->b_attached,i_io_list){+if(!inode_prepare_wbs_switch(inode,isw->new_wb))+continue;++isw->inodes[nr++]=inode;++if(nr>=WB_MAX_INODES_PER_ISW-1){+restart=true;+break;+}+}+spin_unlock(&wb->list_lock);++/* no attached inodes? bail out */+if(nr==0){+kfree(isw);+returnrestart;+}++/*+*Inadditiontosynchronizingamongswitchers,I_WB_SWITCHtells+*theRCUprotectedstatupdatepathstograbthei_page+*locksothatstattransfercansynchronizeagainstthem.+*Let'scontinueafterI_WB_SWITCHisguaranteedtobevisible.+*/+INIT_RCU_WORK(&isw->work,inode_switch_wbs_work_fn);+queue_rcu_work(isw_wq,&isw->work);++atomic_inc(&isw_nr_in_flight);++returnrestart;+}+/***wbc_attach_and_unlock_inode-associatewbcwithtargetinodeandunlockit*@wbc:writeback_controlofinterest
From: Dennis Zhou <dennis@kernel.org> Date: 2021-06-05 21:37:58
Hello,
On Thu, Jun 03, 2021 at 06:31:53PM -0700, Roman Gushchin wrote:
When an inode is getting dirty for the first time it's associated
with a wb structure (see __inode_attach_wb()). It can later be
switched to another wb (if e.g. some other cgroup is writing a lot of
data to the same inode), but otherwise stays attached to the original
wb until being reclaimed.
The problem is that the wb structure holds a reference to the original
memory and blkcg cgroups. So if an inode has been dirty once and later
is actively used in read-only mode, it has a good chance to pin down
the original memory and blkcg cgroups forewer. This is often the case with
services bringing data for other services, e.g. updating some rpm
packages.
In the real life it becomes a problem due to a large size of the memcg
structure, which can easily be 1000x larger than an inode. Also a
really large number of dying cgroups can raise different scalability
issues, e.g. making the memory reclaim costly and less effective.
To solve the problem inodes should be eventually detached from the
corresponding writeback structure. It's inefficient to do it after
every writeback completion. Instead it can be done whenever the
original memory cgroup is offlined and writeback structure is getting
killed. Scanning over a (potentially long) list of inodes and detach
them from the writeback structure can take quite some time. To avoid
scanning all inodes, attached inodes are kept on a new list (b_attached).
To make it less noticeable to a user, the scanning and switching is performed
from a work context.
Big thanks to Jan Kara, Dennis Zhou and Hillf Danton for their ideas and
contribution to this patchset.
v7:
- shared locking for multiple inode switching
- introduced inode_prepare_wbs_switch() helper
- extended the pre-switch inode check for I_WILL_FREE
- added comments here and there
v6:
- extended and reused wbs switching functionality to switch inodes
on cgwb cleanup
- fixed offline_list handling
- switched to the unbound_wq
- other minor fixes
v5:
- switch inodes to bdi->wb instead of zeroing inode->i_wb
- split the single patch into two
- only cgwbs maintain lists of attached inodes
- added cond_resched()
- fixed !CONFIG_CGROUP_WRITEBACK handling
- extended list of prohibited inodes flag
- other small fixes
Roman Gushchin (6):
writeback, cgroup: do not switch inodes with I_WILL_FREE flag
writeback, cgroup: switch to rcu_work API in inode_switch_wbs()
writeback, cgroup: keep list of inodes attached to bdi_writeback
writeback, cgroup: split out the functional part of
inode_switch_wbs_work_fn()
writeback, cgroup: support switching multiple inodes at once
writeback, cgroup: release dying cgwbs by switching attached inodes
fs/fs-writeback.c | 302 +++++++++++++++++++++----------
include/linux/backing-dev-defs.h | 20 +-
include/linux/writeback.h | 1 +
mm/backing-dev.c | 69 ++++++-
4 files changed, 293 insertions(+), 99 deletions(-)
--
2.31.1
I too am a bit late to the party. Feel free to add mine as well to the
series.
Acked-by: Dennis Zhou <redacted>
I left my one comment on the last patch regarding a possible future
extension.
Thanks,
Dennis
From: Jan Kara <jack@suse.cz> Date: 2021-06-07 08:48:35
On Thu 03-06-21 18:31:54, Roman Gushchin wrote:
If an inode's state has I_WILL_FREE flag set, the inode will be
freed soon, so there is no point in trying to switch the inode
to a different cgwb.
I_WILL_FREE was ignored since the introduction of the inode switching,
so it looks like it doesn't lead to any noticeable issues for a user.
This is why the patch is not intended for a stable backport.
Suggested-by: Jan Kara <redacted>
Signed-off-by: Roman Gushchin <redacted>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <redacted>
Honza
@@ -517,7 +517,7 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)/* while holding I_WB_SWITCH, no one else can update the association */spin_lock(&inode->i_lock);if(!(inode->i_sb->s_flags&SB_ACTIVE)||-inode->i_state&(I_WB_SWITCH|I_FREEING)||+inode->i_state&(I_WB_SWITCH|I_FREEING|I_WILL_FREE)||inode_to_wb(inode)==isw->new_wb){spin_unlock(&inode->i_lock);gotoout_free;
From: Jan Kara <jack@suse.cz> Date: 2021-06-07 09:00:53
On Thu 03-06-21 18:31:58, Roman Gushchin wrote:
Currently only a single inode can be switched to another writeback
structure at once. That means to switch an inode a separate
inode_switch_wbs_context structure must be allocated, and a separate
rcu callback and work must be scheduled.
It's fine for the existing ad-hoc switching, which is not happening
that often, but sub-optimal for massive switching required in order to
release a writeback structure. To prepare for it, let's add a support
for switching multiple inodes at once.
Instead of containing a single inode pointer, inode_switch_wbs_context
will contain a NULL-terminated array of inode pointers.
inode_do_switch_wbs() will be called for each inode.
To optimize the locking bdi->wb_switch_rwsem, old_wb's and new_wb's
list_locks will be acquired and released only once altogether for all
inodes. wb_wakeup() will be also be called only once. Instead of
calling wb_put(old_wb) after each successful switch, wb_put_many()
is introduced and used.
Signed-off-by: Roman Gushchin <redacted>
You have to be careful here as iput() can be dropping last inode reference
and in that case it can sleep and do a lot of heavylifting (which cannot
happen under the locks you hold). So you need another loop after dropping
all the locks to do iput() on all inodes. After fixing this feel free to
add:
Reviewed-by: Jan Kara <redacted>
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-06-07 09:24:30
On Thu 03-06-21 18:31:59, Roman Gushchin wrote:
Asynchronously try to release dying cgwbs by switching attached inodes
to the bdi's wb. It helps to get rid of per-cgroup writeback
structures themselves and of pinned memory and block cgroups, which
are significantly larger structures (mostly due to large per-cpu
statistics data). This prevents memory waste and helps to avoid
different scalability problems caused by large piles of dying cgroups.
Reuse the existing mechanism of inode switching used for foreign inode
detection. To speed things up batch up to 115 inode switching in a
single operation (the maximum number is selected so that the resulting
struct inode_switch_wbs_context can fit into 1024 bytes). Because
every switching consists of two steps divided by an RCU grace period,
it would be too slow without batching. Please note that the whole
batch counts as a single operation (when increasing/decreasing
isw_nr_in_flight). This allows to keep umounting working (flush the
switching queue), however prevents cleanups from consuming the whole
switching quota and effectively blocking the frn switching.
Hum, your comment about unmount made me think... Isn't all that stuff racy?
generic_shutdown_super() has:
sync_filesystem(sb);
sb->s_flags &= ~SB_ACTIVE;
cgroup_writeback_umount();
and cgroup_writeback_umount() is:
if (atomic_read(&isw_nr_in_flight)) {
/*
* Use rcu_barrier() to wait for all pending callbacks to
* ensure that all in-flight wb switches are in the workqueue.
*/
rcu_barrier();
flush_workqueue(isw_wq);
}
So we are clearly missing a smp_mb() here (likely in
cgroup_writeback_umount()) as clearing of SB_ACTIVE needs to be reliably
happing before atomic_read(&isw_nr_in_flight).
Also ...
+bool cleanup_offline_cgwb(struct bdi_writeback *wb)
+{
+ struct inode_switch_wbs_context *isw;
+ struct inode *inode;
+ int nr;
+ bool restart = false;
+
+ isw = kzalloc(sizeof(*isw) + WB_MAX_INODES_PER_ISW *
+ sizeof(struct inode *), GFP_KERNEL);
+ if (!isw)
+ return restart;
+
+ /* no need to call wb_get() here: bdi's root wb is not refcounted */
+ isw->new_wb = &wb->bdi->wb;
+
+ nr = 0;
+ spin_lock(&wb->list_lock);
+ list_for_each_entry(inode, &wb->b_attached, i_io_list) {
+ if (!inode_prepare_wbs_switch(inode, isw->new_wb))
+ continue;
+
+ isw->inodes[nr++] = inode;
+
+ if (nr >= WB_MAX_INODES_PER_ISW - 1) {
+ restart = true;
+ break;
+ }
+ }
+ spin_unlock(&wb->list_lock);
+
+ /* no attached inodes? bail out */
+ if (nr == 0) {
+ kfree(isw);
+ return restart;
+ }
+
+ /*
+ * In addition to synchronizing among switchers, I_WB_SWITCH tells
+ * the RCU protected stat update paths to grab the i_page
+ * lock so that stat transfer can synchronize against them.
+ * Let's continue after I_WB_SWITCH is guaranteed to be visible.
+ */
+ INIT_RCU_WORK(&isw->work, inode_switch_wbs_work_fn);
+ queue_rcu_work(isw_wq, &isw->work);
+
+ atomic_inc(&isw_nr_in_flight);
... the increment of isw_nr_in_flight needs to happen before we start to
grab any inodes. Otherwise unmount can pass past cgroup_writeback_umount()
while we are still holding inode references in cleanup_offline_cgwb() the
result will be "Busy inodes after unmount." message and use-after-free
issues (with inode->i_sb which gets freed).
Frankly, I think much safer option would be to wait in evict() for
I_WB_SWITCH similarly as we wait for I_SYNC (through
inode_wait_for_writeback()). And with that we can do away with
cgroup_writeback_umount() altogether. But I guess that's out of scope of
this series.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Roman Gushchin <hidden> Date: 2021-06-08 00:20:40
On Sat, Jun 05, 2021 at 09:34:41PM +0000, Dennis Zhou wrote:
Hello,
On Thu, Jun 03, 2021 at 06:31:59PM -0700, Roman Gushchin wrote:
quoted
Asynchronously try to release dying cgwbs by switching attached inodes
to the bdi's wb. It helps to get rid of per-cgroup writeback
structures themselves and of pinned memory and block cgroups, which
are significantly larger structures (mostly due to large per-cpu
statistics data). This prevents memory waste and helps to avoid
different scalability problems caused by large piles of dying cgroups.
Reuse the existing mechanism of inode switching used for foreign inode
detection. To speed things up batch up to 115 inode switching in a
single operation (the maximum number is selected so that the resulting
struct inode_switch_wbs_context can fit into 1024 bytes). Because
every switching consists of two steps divided by an RCU grace period,
it would be too slow without batching. Please note that the whole
batch counts as a single operation (when increasing/decreasing
isw_nr_in_flight). This allows to keep umounting working (flush the
switching queue), however prevents cleanups from consuming the whole
switching quota and effectively blocking the frn switching.
A cgwb cleanup operation can fail due to different reasons (e.g. not
enough memory, the cgwb has an in-flight/pending io, an attached inode
in a wrong state, etc). In this case the next scheduled cleanup will
make a new attempt. An attempt is made each time a new cgwb is offlined
(in other words a memcg and/or a blkcg is deleted by a user). In the
future an additional attempt scheduled by a timer can be implemented.
I've been thinking about this for a little while and the only thing I'm
not super thrilled by is that the subsequent cleanup work trigger isn't
due to forward progress.
As future work, we could tag the inodes to switch when writeback
completes instead of using a timer. This would be nice because then we
only have to make a single (successful) pass switching the inodes we can
and then mark the others to switch. Once a cgwb is killed no one else
can attach to it so we should be good there.
I don't think this is a blocker or even necessary, I just wanted to put
it out there as possible future direction instead of a timer.
Yeah, I agree that it's a good direction to explore. It will be likely
more intrusive and will require new inode flag. So I'd leave it for further
improvements.
Thank you for reviewing the series!