| Message ID | 20210330092933.81311-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | writeback: fix obtain a reference to a freeing memcg css | expand |
On Tue, Mar 30, 2021 at 05:29:33PM +0800, Muchun Song wrote: > +++ b/fs/fs-writeback.c > @@ -506,8 +506,10 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id) > /* find and pin the new wb */ > rcu_read_lock(); > memcg_css = css_from_id(new_wb_id, &memory_cgrp_subsys); > - if (memcg_css) > + if (memcg_css && css_tryget(memcg_css)) { > isw->new_wb = wb_get_create(bdi, memcg_css, GFP_ATOMIC); > + css_put(memcg_css); > + } > rcu_read_unlock(); > if (!isw->new_wb) > goto out_free; This seems like an unnecessary use of GFP_ATOMIC. Why not: rcu_read_lock(); memcg_css = css_from_id(new_wb_id, &memory_cgrp_subsys); if (memcg_css && !css_tryget(memcg_css)) memcg_css = NULL; rcu_read_unlock(); if (!memcg_css) goto out_free; isw->new_wb = wb_get_create(bdi, memcg_css, GFP_NOIO); css_put(memcg_css); if (!isw->new_wb) goto out_free; (inode_switch_wbs can't be called in interrupt context because it takes inode->i_lock, which is not interrupt-safe. it's not clear to me whether it is allowed to start IO or do FS reclaim, given where it is in the I/O path, so i went with GFP_NOIO rather than GFP_KERNEL) (also there's another use of GFP_ATOMIC in that function, which is probably wrong)
On Tue, Mar 30, 2021 at 7:34 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Tue, Mar 30, 2021 at 05:29:33PM +0800, Muchun Song wrote: > > +++ b/fs/fs-writeback.c > > @@ -506,8 +506,10 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id) > > /* find and pin the new wb */ > > rcu_read_lock(); > > memcg_css = css_from_id(new_wb_id, &memory_cgrp_subsys); > > - if (memcg_css) > > + if (memcg_css && css_tryget(memcg_css)) { > > isw->new_wb = wb_get_create(bdi, memcg_css, GFP_ATOMIC); > > + css_put(memcg_css); > > + } > > rcu_read_unlock(); > > if (!isw->new_wb) > > goto out_free; > > This seems like an unnecessary use of GFP_ATOMIC. Why not: > > rcu_read_lock(); > memcg_css = css_from_id(new_wb_id, &memory_cgrp_subsys); > if (memcg_css && !css_tryget(memcg_css)) > memcg_css = NULL; > rcu_read_unlock(); > if (!memcg_css) > goto out_free; > isw->new_wb = wb_get_create(bdi, memcg_css, GFP_NOIO); > css_put(memcg_css); > if (!isw->new_wb) > goto out_free; Thanks. I will reuse this. > > (inode_switch_wbs can't be called in interrupt context because it takes > inode->i_lock, which is not interrupt-safe. it's not clear to me whether > it is allowed to start IO or do FS reclaim, given where it is in the > I/O path, so i went with GFP_NOIO rather than GFP_KERNEL) > > (also there's another use of GFP_ATOMIC in that function, which is > probably wrong) Do you mean the allocation of struct inode_switch_wbs_context in inode_switch_wbs?
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index 3ac002561327..afa658ffc09f 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -506,8 +506,10 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id) /* find and pin the new wb */ rcu_read_lock(); memcg_css = css_from_id(new_wb_id, &memory_cgrp_subsys); - if (memcg_css) + if (memcg_css && css_tryget(memcg_css)) { isw->new_wb = wb_get_create(bdi, memcg_css, GFP_ATOMIC); + css_put(memcg_css); + } rcu_read_unlock(); if (!isw->new_wb) goto out_free;
The caller of wb_get_create() should pin the memcg, because wb_get_create() relies on this guarantee. The rcu read lock only can guarantee that the memcg css returned by css_from_id() cannot be released, but the reference of the memcg can be zero. Fix it by holding a reference to the css before calling wb_get_create(). This is not a problem I encountered in the real world. Just the result of a code review. Fixes: 682aa8e1a6a1 ("writeback: implement unlocked_inode_to_wb transaction and use it for stat updates") Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- fs/fs-writeback.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)