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.
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 (5):
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 | 201 ++++++++++++++++++++++---------
include/linux/backing-dev-defs.h | 2 +
include/linux/writeback.h | 1 +
mm/backing-dev.c | 60 ++++++++-
4 files changed, 204 insertions(+), 60 deletions(-)
--
2.31.1
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 <[email protected]>
---
fs/fs-writeback.c | 68 ++++++++++++++++++++++++++++++++
include/linux/backing-dev-defs.h | 1 +
include/linux/writeback.h | 1 +
mm/backing-dev.c | 58 ++++++++++++++++++++++++++-
4 files changed, 126 insertions(+), 2 deletions(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 49d7b23a7cfe..e8517ad677eb 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -225,6 +225,8 @@ 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 */
+#define WB_MAX_INODES_PER_ISW 116 /* maximum inodes per isw */
+
static atomic_t isw_nr_in_flight = ATOMIC_INIT(0);
static struct workqueue_struct *isw_wq;
@@ -552,6 +554,72 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
kfree(isw);
}
+/**
+ * cleanup_offline_cgwb - detach associated inodes
+ * @wb: target wb
+ *
+ * Switch all inodes attached to @wb to the bdi's root wb in order to eventually
+ * release the dying @wb. Returns %true if not all inodes were switched and
+ * the function has to be restarted.
+ */
+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) {
+ spin_lock(&inode->i_lock);
+ if (!(inode->i_sb->s_flags & SB_ACTIVE) ||
+ inode->i_state & (I_WB_SWITCH | I_FREEING) ||
+ inode_to_wb(inode) == isw->new_wb) {
+ spin_unlock(&inode->i_lock);
+ continue;
+ }
+ inode->i_state |= I_WB_SWITCH;
+ __iget(inode);
+ spin_unlock(&inode->i_lock);
+
+ 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);
+
+ return restart;
+}
+
/**
* wbc_attach_and_unlock_inode - associate wbc with target inode and unlock it
* @wbc: writeback_control of interest
diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
index e5dc238ebe4f..07d6b6d6dbdf 100644
--- a/include/linux/backing-dev-defs.h
+++ b/include/linux/backing-dev-defs.h
@@ -155,6 +155,7 @@ struct bdi_writeback {
struct list_head memcg_node; /* anchored at memcg->cgwb_list */
struct list_head blkcg_node; /* anchored at blkcg->cgwb_list */
struct list_head b_attached; /* attached inodes, protected by list_lock */
+ struct list_head offline_node; /* anchored at offline_cgwbs */
union {
struct work_struct release_work;
diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index 8e5c5bb16e2d..95de51c10248 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -221,6 +221,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_cgwb(struct bdi_writeback *wb);
/**
* inode_attach_wb - associate an inode with its wb
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 54c5dc4b8c24..f1fc04412bd7 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -371,12 +371,16 @@ static void wb_exit(struct bdi_writeback *wb)
#include <linux/memcontrol.h>
/*
- * cgwb_lock protects bdi->cgwb_tree, blkcg->cgwb_list, and memcg->cgwb_list.
- * bdi->cgwb_tree is also RCU protected.
+ * cgwb_lock protects bdi->cgwb_tree, 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);
+static void cleanup_offline_cgwbs_workfn(struct work_struct *work);
+static DECLARE_WORK(cleanup_offline_cgwbs_work, cleanup_offline_cgwbs_workfn);
+
static void cgwb_release_workfn(struct work_struct *work)
{
struct bdi_writeback *wb = container_of(work, struct bdi_writeback,
@@ -395,6 +399,11 @@ static void cgwb_release_workfn(struct work_struct *work)
fprop_local_destroy_percpu(&wb->memcg_completions);
percpu_ref_exit(&wb->refcnt);
+
+ spin_lock_irq(&cgwb_lock);
+ list_del(&wb->offline_node);
+ spin_unlock_irq(&cgwb_lock);
+
wb_exit(wb);
WARN_ON_ONCE(!list_empty(&wb->b_attached));
kfree_rcu(wb, rcu);
@@ -414,6 +423,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);
}
@@ -635,6 +645,48 @@ 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_workfn(struct work_struct *work)
+{
+ struct bdi_writeback *wb;
+ LIST_HEAD(processed);
+
+ spin_lock_irq(&cgwb_lock);
+
+ while (!list_empty(&offline_cgwbs)) {
+ wb = list_first_entry(&offline_cgwbs, struct bdi_writeback,
+ offline_node);
+ list_move(&wb->offline_node, &processed);
+
+ if (wb_has_dirty_io(wb))
+ continue;
+
+ if (!wb_tryget(wb))
+ continue;
+
+ spin_unlock_irq(&cgwb_lock);
+ while ((cleanup_offline_cgwb(wb)))
+ cond_resched();
+ spin_lock_irq(&cgwb_lock);
+
+ wb_put(wb);
+ }
+
+ if (!list_empty(&processed))
+ list_splice_tail(&processed, &offline_cgwbs);
+
+ spin_unlock_irq(&cgwb_lock);
+}
+
/**
* wb_memcg_offline - kill all wb's associated with a memcg being offlined
* @memcg: memcg being offlined
@@ -651,6 +703,8 @@ void wb_memcg_offline(struct mem_cgroup *memcg)
cgwb_kill(wb);
memcg_cgwb_list->next = NULL; /* prevent new wb's */
spin_unlock_irq(&cgwb_lock);
+
+ queue_work(system_unbound_wq, &cleanup_offline_cgwbs_work);
}
/**
--
2.31.1
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 <[email protected]>
---
fs/fs-writeback.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 09d2770449ef..212494d89cc2 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -351,15 +351,12 @@ static void bdi_up_write_wb_switch_rwsem(struct backing_dev_info *bdi)
up_write(&bdi->wb_switch_rwsem);
}
-static void inode_switch_wbs_work_fn(struct work_struct *work)
+static void inode_do_switch_wbs(struct inode *inode,
+ struct bdi_writeback *new_wb)
{
- struct inode_switch_wbs_context *isw =
- container_of(to_rcu_work(work), struct inode_switch_wbs_context, work);
- struct inode *inode = isw->inode;
struct backing_dev_info *bdi = inode_to_bdi(inode);
struct address_space *mapping = inode->i_mapping;
struct bdi_writeback *old_wb = inode->i_wb;
- struct bdi_writeback *new_wb = isw->new_wb;
XA_STATE(xas, &mapping->i_pages, 0);
struct page *page;
bool switched = false;
@@ -470,11 +467,17 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
wb_wakeup(new_wb);
wb_put(old_wb);
}
- wb_put(new_wb);
+}
- iput(inode);
- kfree(isw);
+static void inode_switch_wbs_work_fn(struct work_struct *work)
+{
+ struct inode_switch_wbs_context *isw =
+ container_of(to_rcu_work(work), struct inode_switch_wbs_context, work);
+ inode_do_switch_wbs(isw->inode, isw->new_wb);
+ wb_put(isw->new_wb);
+ iput(isw->inode);
+ kfree(isw);
atomic_dec(&isw_nr_in_flight);
}
--
2.31.1
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.
Signed-off-by: Roman Gushchin <[email protected]>
---
fs/fs-writeback.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 212494d89cc2..49d7b23a7cfe 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -335,10 +335,10 @@ static struct bdi_writeback *inode_to_wb_and_lock_list(struct inode *inode)
}
struct inode_switch_wbs_context {
- struct inode *inode;
- struct bdi_writeback *new_wb;
-
struct rcu_work work;
+
+ struct bdi_writeback *new_wb;
+ struct inode *inodes[];
};
static void bdi_down_write_wb_switch_rwsem(struct backing_dev_info *bdi)
@@ -473,10 +473,14 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
{
struct inode_switch_wbs_context *isw =
container_of(to_rcu_work(work), struct inode_switch_wbs_context, work);
+ struct inode **inodep;
+
+ for (inodep = &isw->inodes[0]; *inodep; inodep++) {
+ inode_do_switch_wbs(*inodep, isw->new_wb);
+ iput(*inodep);
+ }
- inode_do_switch_wbs(isw->inode, isw->new_wb);
wb_put(isw->new_wb);
- iput(isw->inode);
kfree(isw);
atomic_dec(&isw_nr_in_flight);
}
@@ -503,7 +507,7 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
if (atomic_read(&isw_nr_in_flight) > WB_FRN_MAX_IN_FLIGHT)
return;
- isw = kzalloc(sizeof(*isw), GFP_ATOMIC);
+ isw = kzalloc(sizeof(*isw) + 2 * sizeof(struct inode *), GFP_ATOMIC);
if (!isw)
return;
@@ -528,7 +532,7 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
__iget(inode);
spin_unlock(&inode->i_lock);
- isw->inode = inode;
+ isw->inodes[0] = inode;
/*
* In addition to synchronizing among switchers, I_WB_SWITCH tells
--
2.31.1
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 <[email protected]>
---
fs/fs-writeback.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index e91980f49388..1f51857e41d1 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -335,8 +335,7 @@ struct inode_switch_wbs_context {
struct inode *inode;
struct bdi_writeback *new_wb;
- struct rcu_head rcu_head;
- struct work_struct work;
+ struct rcu_work work;
};
static void bdi_down_write_wb_switch_rwsem(struct backing_dev_info *bdi)
@@ -352,7 +351,7 @@ static void bdi_up_write_wb_switch_rwsem(struct backing_dev_info *bdi)
static void inode_switch_wbs_work_fn(struct work_struct *work)
{
struct inode_switch_wbs_context *isw =
- container_of(work, struct inode_switch_wbs_context, work);
+ container_of(to_rcu_work(work), struct inode_switch_wbs_context, work);
struct inode *inode = isw->inode;
struct backing_dev_info *bdi = inode_to_bdi(inode);
struct address_space *mapping = inode->i_mapping;
@@ -469,16 +468,6 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
atomic_dec(&isw_nr_in_flight);
}
-static void inode_switch_wbs_rcu_fn(struct rcu_head *rcu_head)
-{
- struct inode_switch_wbs_context *isw = container_of(rcu_head,
- struct inode_switch_wbs_context, rcu_head);
-
- /* needs to grab bh-unsafe locks, bounce to work item */
- INIT_WORK(&isw->work, inode_switch_wbs_work_fn);
- queue_work(isw_wq, &isw->work);
-}
-
/**
* inode_switch_wbs - change the wb association of an inode
* @inode: target inode
@@ -534,7 +523,8 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
* lock so that stat transfer can synchronize against them.
* Let's continue after I_WB_SWITCH is guaranteed to be visible.
*/
- call_rcu(&isw->rcu_head, inode_switch_wbs_rcu_fn);
+ INIT_RCU_WORK(&isw->work, inode_switch_wbs_work_fn);
+ queue_rcu_work(isw_wq, &isw->work);
atomic_inc(&isw_nr_in_flight);
return;
--
2.31.1
On Wed 02-06-21 17:55:13, Roman Gushchin wrote:
> 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 <[email protected]>
Looks good to me. You can add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/fs-writeback.c | 18 ++++--------------
> 1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index e91980f49388..1f51857e41d1 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -335,8 +335,7 @@ struct inode_switch_wbs_context {
> struct inode *inode;
> struct bdi_writeback *new_wb;
>
> - struct rcu_head rcu_head;
> - struct work_struct work;
> + struct rcu_work work;
> };
>
> static void bdi_down_write_wb_switch_rwsem(struct backing_dev_info *bdi)
> @@ -352,7 +351,7 @@ static void bdi_up_write_wb_switch_rwsem(struct backing_dev_info *bdi)
> static void inode_switch_wbs_work_fn(struct work_struct *work)
> {
> struct inode_switch_wbs_context *isw =
> - container_of(work, struct inode_switch_wbs_context, work);
> + container_of(to_rcu_work(work), struct inode_switch_wbs_context, work);
> struct inode *inode = isw->inode;
> struct backing_dev_info *bdi = inode_to_bdi(inode);
> struct address_space *mapping = inode->i_mapping;
> @@ -469,16 +468,6 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
> atomic_dec(&isw_nr_in_flight);
> }
>
> -static void inode_switch_wbs_rcu_fn(struct rcu_head *rcu_head)
> -{
> - struct inode_switch_wbs_context *isw = container_of(rcu_head,
> - struct inode_switch_wbs_context, rcu_head);
> -
> - /* needs to grab bh-unsafe locks, bounce to work item */
> - INIT_WORK(&isw->work, inode_switch_wbs_work_fn);
> - queue_work(isw_wq, &isw->work);
> -}
> -
> /**
> * inode_switch_wbs - change the wb association of an inode
> * @inode: target inode
> @@ -534,7 +523,8 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
> * lock so that stat transfer can synchronize against them.
> * Let's continue after I_WB_SWITCH is guaranteed to be visible.
> */
> - call_rcu(&isw->rcu_head, inode_switch_wbs_rcu_fn);
> + INIT_RCU_WORK(&isw->work, inode_switch_wbs_work_fn);
> + queue_rcu_work(isw_wq, &isw->work);
>
> atomic_inc(&isw_nr_in_flight);
> return;
> --
> 2.31.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed 02-06-21 17:55:15, Roman Gushchin wrote:
> Split out the functional part of the inode_switch_wbs_work_fn()
> function as inode_do switch_wbs() to reuse it later for switching
^ underscore here
> inodes attached to dying cgwbs.
>
> This commit doesn't bring any functional changes.
>
> Signed-off-by: Roman Gushchin <[email protected]>
The patch looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/fs-writeback.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 09d2770449ef..212494d89cc2 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -351,15 +351,12 @@ static void bdi_up_write_wb_switch_rwsem(struct backing_dev_info *bdi)
> up_write(&bdi->wb_switch_rwsem);
> }
>
> -static void inode_switch_wbs_work_fn(struct work_struct *work)
> +static void inode_do_switch_wbs(struct inode *inode,
> + struct bdi_writeback *new_wb)
> {
> - struct inode_switch_wbs_context *isw =
> - container_of(to_rcu_work(work), struct inode_switch_wbs_context, work);
> - struct inode *inode = isw->inode;
> struct backing_dev_info *bdi = inode_to_bdi(inode);
> struct address_space *mapping = inode->i_mapping;
> struct bdi_writeback *old_wb = inode->i_wb;
> - struct bdi_writeback *new_wb = isw->new_wb;
> XA_STATE(xas, &mapping->i_pages, 0);
> struct page *page;
> bool switched = false;
> @@ -470,11 +467,17 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
> wb_wakeup(new_wb);
> wb_put(old_wb);
> }
> - wb_put(new_wb);
> +}
>
> - iput(inode);
> - kfree(isw);
> +static void inode_switch_wbs_work_fn(struct work_struct *work)
> +{
> + struct inode_switch_wbs_context *isw =
> + container_of(to_rcu_work(work), struct inode_switch_wbs_context, work);
>
> + inode_do_switch_wbs(isw->inode, isw->new_wb);
> + wb_put(isw->new_wb);
> + iput(isw->inode);
> + kfree(isw);
> atomic_dec(&isw_nr_in_flight);
> }
>
> --
> 2.31.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed 02-06-21 17:55:17, 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.
>
> Signed-off-by: Roman Gushchin <[email protected]>
I think we are getting close :). Some comments are below.
> ---
> fs/fs-writeback.c | 68 ++++++++++++++++++++++++++++++++
> include/linux/backing-dev-defs.h | 1 +
> include/linux/writeback.h | 1 +
> mm/backing-dev.c | 58 ++++++++++++++++++++++++++-
> 4 files changed, 126 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 49d7b23a7cfe..e8517ad677eb 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -225,6 +225,8 @@ 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 */
>
> +#define WB_MAX_INODES_PER_ISW 116 /* maximum inodes per isw */
> +
Why this number? Please add an explanation here...
> static atomic_t isw_nr_in_flight = ATOMIC_INIT(0);
> static struct workqueue_struct *isw_wq;
>
> @@ -552,6 +554,72 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
> kfree(isw);
> }
>
> +/**
> + * cleanup_offline_cgwb - detach associated inodes
> + * @wb: target wb
> + *
> + * Switch all inodes attached to @wb to the bdi's root wb in order to eventually
> + * release the dying @wb. Returns %true if not all inodes were switched and
> + * the function has to be restarted.
> + */
> +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) {
> + spin_lock(&inode->i_lock);
> + if (!(inode->i_sb->s_flags & SB_ACTIVE) ||
> + inode->i_state & (I_WB_SWITCH | I_FREEING) ||
> + inode_to_wb(inode) == isw->new_wb) {
> + spin_unlock(&inode->i_lock);
> + continue;
> + }
> + inode->i_state |= I_WB_SWITCH;
> + __iget(inode);
> + spin_unlock(&inode->i_lock);
This hunk is identical with the one in inode_switch_wbs(). Maybe create a
helper for it like inode_prepare_wb_switch() or something like that. Also
we need to check for I_WILL_FREE flag as well as I_FREEING (see the code in
iput_final()) - that's actually a bug in inode_switch_wbs() as well so
probably a separate fix for that should come earlier in the series.
> +
> + isw->inodes[nr++] = inode;
At first it seemed a bit silly to allocate an array of inode pointers when
we have them in the list. But after some thought I agree that dealing with
other switching being triggered from other sources in parallel would be
really difficult so your decision makes sense. Just maybe add an
explanation in a comment somewhere about this design decision.
> +
> + if (nr >= WB_MAX_INODES_PER_ISW - 1) {
> + restart = true;
> + break;
> + }
> + }
> + spin_unlock(&wb->list_lock);
...
> +static void cleanup_offline_cgwbs_workfn(struct work_struct *work)
> +{
> + struct bdi_writeback *wb;
> + LIST_HEAD(processed);
> +
> + spin_lock_irq(&cgwb_lock);
> +
> + while (!list_empty(&offline_cgwbs)) {
> + wb = list_first_entry(&offline_cgwbs, struct bdi_writeback,
> + offline_node);
> + list_move(&wb->offline_node, &processed);
> +
> + if (wb_has_dirty_io(wb))
> + continue;
Maybe explain in a comment why skipping wbs with dirty inodes is fine?
Because honestly, I'm not sure... I guess the rationale is that inodes
should get cleaned eventually and if they are getting redirtied, they will
be switched to another wb anyway?
> +
> + if (!wb_tryget(wb))
> + continue;
> +
> + spin_unlock_irq(&cgwb_lock);
> + while ((cleanup_offline_cgwb(wb)))
> + cond_resched();
> + spin_lock_irq(&cgwb_lock);
> +
> + wb_put(wb);
> + }
> +
> + if (!list_empty(&processed))
> + list_splice_tail(&processed, &offline_cgwbs);
> +
> + spin_unlock_irq(&cgwb_lock);
> +}
> +
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed 02-06-21 17:55:16, 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.
>
> Signed-off-by: Roman Gushchin <[email protected]>
Two small comments below:
> @@ -473,10 +473,14 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
> {
> struct inode_switch_wbs_context *isw =
> container_of(to_rcu_work(work), struct inode_switch_wbs_context, work);
> + struct inode **inodep;
> +
> + for (inodep = &isw->inodes[0]; *inodep; inodep++) {
^^^^ why not just isw->inodes?
> + inode_do_switch_wbs(*inodep, isw->new_wb);
> + iput(*inodep);
> + }
I was kind of hoping that we would save the repeated locking of
bdi->wb_switch_rwsem, old_wb->list_lock, and new_wb->list_lock for multiple
inodes. Maybe we can have 'old_wb' as part of isw as well and assert that
all inodes are still attached to the old_wb at this point to make this a
bit simpler. Or we can fetch old_wb from the first inode and then just
lock & assert using that one.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Thu, Jun 03, 2021 at 12:02:33PM +0200, Jan Kara wrote:
> On Wed 02-06-21 17:55:17, 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.
> >
> > Signed-off-by: Roman Gushchin <[email protected]>
>
> I think we are getting close :). Some comments are below.
Great! Thank for reviewing the code!
>
> > ---
> > fs/fs-writeback.c | 68 ++++++++++++++++++++++++++++++++
> > include/linux/backing-dev-defs.h | 1 +
> > include/linux/writeback.h | 1 +
> > mm/backing-dev.c | 58 ++++++++++++++++++++++++++-
> > 4 files changed, 126 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index 49d7b23a7cfe..e8517ad677eb 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -225,6 +225,8 @@ 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 */
> >
> > +#define WB_MAX_INODES_PER_ISW 116 /* maximum inodes per isw */
> > +
>
> Why this number? Please add an explanation here...
Added.
>
> > static atomic_t isw_nr_in_flight = ATOMIC_INIT(0);
> > static struct workqueue_struct *isw_wq;
> >
> > @@ -552,6 +554,72 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
> > kfree(isw);
> > }
> >
> > +/**
> > + * cleanup_offline_cgwb - detach associated inodes
> > + * @wb: target wb
> > + *
> > + * Switch all inodes attached to @wb to the bdi's root wb in order to eventually
> > + * release the dying @wb. Returns %true if not all inodes were switched and
> > + * the function has to be restarted.
> > + */
> > +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) {
> > + spin_lock(&inode->i_lock);
> > + if (!(inode->i_sb->s_flags & SB_ACTIVE) ||
> > + inode->i_state & (I_WB_SWITCH | I_FREEING) ||
> > + inode_to_wb(inode) == isw->new_wb) {
> > + spin_unlock(&inode->i_lock);
> > + continue;
> > + }
> > + inode->i_state |= I_WB_SWITCH;
> > + __iget(inode);
> > + spin_unlock(&inode->i_lock);
>
> This hunk is identical with the one in inode_switch_wbs(). Maybe create a
> helper for it like inode_prepare_wb_switch() or something like that. Also
> we need to check for I_WILL_FREE flag as well as I_FREEING (see the code in
> iput_final()) - that's actually a bug in inode_switch_wbs() as well so
> probably a separate fix for that should come earlier in the series.
Good point, added in v7.
>
> > +
> > + isw->inodes[nr++] = inode;
>
> At first it seemed a bit silly to allocate an array of inode pointers when
> we have them in the list. But after some thought I agree that dealing with
> other switching being triggered from other sources in parallel would be
> really difficult so your decision makes sense. Just maybe add an
> explanation in a comment somewhere about this design decision.
Added in v7.
>
> > +
> > + if (nr >= WB_MAX_INODES_PER_ISW - 1) {
> > + restart = true;
> > + break;
> > + }
> > + }
> > + spin_unlock(&wb->list_lock);
>
> ...
>
> > +static void cleanup_offline_cgwbs_workfn(struct work_struct *work)
> > +{
> > + struct bdi_writeback *wb;
> > + LIST_HEAD(processed);
> > +
> > + spin_lock_irq(&cgwb_lock);
> > +
> > + while (!list_empty(&offline_cgwbs)) {
> > + wb = list_first_entry(&offline_cgwbs, struct bdi_writeback,
> > + offline_node);
> > + list_move(&wb->offline_node, &processed);
> > +
> > + if (wb_has_dirty_io(wb))
> > + continue;
>
> Maybe explain in a comment why skipping wbs with dirty inodes is fine?
> Because honestly, I'm not sure... I guess the rationale is that inodes
> should get cleaned eventually and if they are getting redirtied, they will
> be switched to another wb anyway?
The main rationale here is that the deletion of a memory/blkcg cgroup by a user
shouldn't affect the io distribution. In other words, the remaining io shouldn't
be performed faster than it could be finished had the cgroup remain existing.