| Message ID | 20210401093343.51299-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] writeback: fix obtain a reference to a freeing memcg css | expand |
On Thu 01-04-21 17:33:43, Muchun Song wrote: > 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. > > And it is unnecessary to use GFP_ATOMIC, so replace it with > GFP_NOIO. This should go into it's own patch. With more explanation why NOIO is required. > Fixes: 682aa8e1a6a1 ("writeback: implement unlocked_inode_to_wb transaction and use it for stat updates") > Signed-off-by: Muchun Song <songmuchun@bytedance.com> For the css part feel free to add Acked-by: Michal Hocko <mhocko@suse.com> Even if the css ref count is not really necessary it shouldn't cause any harm and it makes the code easier to understand. At least a comment explaining why that is not necessary would be required without it. Thanks! > --- > Changelog in v2: > 1. Replace GFP_ATOMIC with GFP_NOIO suggested by Matthew. > > fs/fs-writeback.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index e91980f49388..df7f89f8f771 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -501,16 +501,21 @@ 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), GFP_NOIO); > if (!isw) > return; > > /* find and pin the new wb */ > rcu_read_lock(); > memcg_css = css_from_id(new_wb_id, &memory_cgrp_subsys); > - if (memcg_css) > - isw->new_wb = wb_get_create(bdi, memcg_css, GFP_ATOMIC); > + 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; > > -- > 2.11.0
On Thu, Apr 1, 2021 at 6:26 PM Michal Hocko <mhocko@suse.com> wrote: > > On Thu 01-04-21 17:33:43, Muchun Song wrote: > > 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. > > > > And it is unnecessary to use GFP_ATOMIC, so replace it with > > GFP_NOIO. > > This should go into it's own patch. With more explanation why NOIO is > required. OK. Replacing GFP_ATOMIC with NOIO should be a separate patch. I will remove this in the next version. > > > Fixes: 682aa8e1a6a1 ("writeback: implement unlocked_inode_to_wb transaction and use it for stat updates") > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > For the css part feel free to add > Acked-by: Michal Hocko <mhocko@suse.com> Thanks. > > Even if the css ref count is not really necessary it shouldn't cause any > harm and it makes the code easier to understand. At least a comment > explaining why that is not necessary would be required without it OK. I will add a comment here to explain why we need to hold a ref. > > Thanks! > > > --- > > Changelog in v2: > > 1. Replace GFP_ATOMIC with GFP_NOIO suggested by Matthew. > > > > fs/fs-writeback.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > > index e91980f49388..df7f89f8f771 100644 > > --- a/fs/fs-writeback.c > > +++ b/fs/fs-writeback.c > > @@ -501,16 +501,21 @@ 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), GFP_NOIO); > > if (!isw) > > return; > > > > /* find and pin the new wb */ > > rcu_read_lock(); > > memcg_css = css_from_id(new_wb_id, &memory_cgrp_subsys); > > - if (memcg_css) > > - isw->new_wb = wb_get_create(bdi, memcg_css, GFP_ATOMIC); > > + 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; > > > > -- > > 2.11.0 > > -- > Michal Hocko > SUSE Labs
On Thu 01-04-21 21:59:13, Muchun Song wrote: > On Thu, Apr 1, 2021 at 6:26 PM Michal Hocko <mhocko@suse.com> wrote: [...] > > Even if the css ref count is not really necessary it shouldn't cause any > > harm and it makes the code easier to understand. At least a comment > > explaining why that is not necessary would be required without it > > OK. I will add a comment here to explain why we need to hold a > ref. I do not think this is necessary. Taking the reference is a standard way and I am not sure it requires a comment. I meant to say that not having a reference should really have a comment explaining why. Thanks!
On Thu, Apr 1, 2021 at 10:37 PM Michal Hocko <mhocko@suse.com> wrote: > > On Thu 01-04-21 21:59:13, Muchun Song wrote: > > On Thu, Apr 1, 2021 at 6:26 PM Michal Hocko <mhocko@suse.com> wrote: > [...] > > > Even if the css ref count is not really necessary it shouldn't cause any > > > harm and it makes the code easier to understand. At least a comment > > > explaining why that is not necessary would be required without it > > > > OK. I will add a comment here to explain why we need to hold a > > ref. > > I do not think this is necessary. Taking the reference is a standard > way and I am not sure it requires a comment. I meant to say that not > having a reference should really have a comment explaining why. Very Sorry. I got it wrong, thank you for further explanation. > > Thanks! > -- > Michal Hocko > SUSE Labs
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index e91980f49388..df7f89f8f771 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -501,16 +501,21 @@ 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), GFP_NOIO); if (!isw) return; /* find and pin the new wb */ rcu_read_lock(); memcg_css = css_from_id(new_wb_id, &memory_cgrp_subsys); - if (memcg_css) - isw->new_wb = wb_get_create(bdi, memcg_css, GFP_ATOMIC); + 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;
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. And it is unnecessary to use GFP_ATOMIC, so replace it with GFP_NOIO. Fixes: 682aa8e1a6a1 ("writeback: implement unlocked_inode_to_wb transaction and use it for stat updates") Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- Changelog in v2: 1. Replace GFP_ATOMIC with GFP_NOIO suggested by Matthew. fs/fs-writeback.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)