| Message ID | 20190803140155.181190-4-tj@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] writeback: Generalize and expose wb_completion | expand |
On Sat 03-08-19 07:01:54, Tejun Heo wrote: > Implement cgroup_writeback_by_id() which initiates cgroup writeback > from bdi and memcg IDs. This will be used by memcg foreign inode > flushing. > > Signed-off-by: Tejun Heo <tj@kernel.org> > --- > fs/fs-writeback.c | 64 +++++++++++++++++++++++++++++++++++++++ > include/linux/writeback.h | 4 +++ > 2 files changed, 68 insertions(+) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index 6129debdc938..5c79d7acefdb 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -880,6 +880,70 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi, > wb_put(last_wb); > } > > +/** > + * cgroup_writeback_by_id - initiate cgroup writeback from bdi and memcg IDs > + * @bdi_id: target bdi id > + * @memcg_id: target memcg css id > + * @nr_pages: number of pages to write > + * @reason: reason why some writeback work initiated > + * @done: target wb_completion > + * > + * Initiate flush of the bdi_writeback identified by @bdi_id and @memcg_id > + * with the specified parameters. > + */ > +int cgroup_writeback_by_id(u64 bdi_id, int memcg_id, unsigned long nr, > + enum wb_reason reason, struct wb_completion *done) > +{ > + struct backing_dev_info *bdi; > + struct cgroup_subsys_state *memcg_css; > + struct bdi_writeback *wb; > + struct wb_writeback_work *work; > + int ret; > + > + /* lookup bdi and memcg */ > + bdi = bdi_get_by_id(bdi_id); > + if (!bdi) > + return -ENOENT; > + > + rcu_read_lock(); > + memcg_css = css_from_id(memcg_id, &memory_cgrp_subsys); > + if (memcg_css && !css_tryget(memcg_css)) > + memcg_css = NULL; > + rcu_read_unlock(); > + if (!memcg_css) { > + ret = -ENOENT; > + goto out_bdi_put; > + } > + > + /* and find the associated wb */ > + wb = wb_get_create(bdi, memcg_css, GFP_NOWAIT | __GFP_NOWARN); > + if (!wb) { > + ret = -ENOMEM; > + goto out_css_put; > + } > + > + /* issue the writeback work */ > + work = kzalloc(sizeof(*work), GFP_NOWAIT | __GFP_NOWARN); > + if (work) { > + work->nr_pages = nr; > + work->sync_mode = WB_SYNC_NONE; > + work->reason = reason; > + work->done = done; > + work->auto_free = 1; > + wb_queue_work(wb, work); > + ret = 0; > + } else { > + ret = -ENOMEM; > + } > + > + wb_put(wb); > +out_css_put: > + css_put(memcg_css); > +out_bdi_put: > + bdi_put(bdi); > + return ret; > +} > + > /** > * cgroup_writeback_umount - flush inode wb switches for umount > * > diff --git a/include/linux/writeback.h b/include/linux/writeback.h > index 8945aac31392..ad794f2a7d42 100644 > --- a/include/linux/writeback.h > +++ b/include/linux/writeback.h > @@ -217,6 +217,10 @@ void wbc_attach_and_unlock_inode(struct writeback_control *wbc, > void wbc_detach_inode(struct writeback_control *wbc); > void wbc_account_cgroup_owner(struct writeback_control *wbc, struct page *page, > size_t bytes); > +int cgroup_writeback_by_id(u64 bdi_id, int memcg_id, unsigned long nr_pages, > + enum wb_reason reason, struct wb_completion *done); > +int writeback_by_id(int id, unsigned long nr, enum wb_reason reason, > + struct wb_completion *done); I guess this writeback_by_id() is stale? I didn't find it anywhere else... Honza
On Sat 03-08-19 07:01:54, Tejun Heo wrote: > Implement cgroup_writeback_by_id() which initiates cgroup writeback > from bdi and memcg IDs. This will be used by memcg foreign inode > flushing. > > Signed-off-by: Tejun Heo <tj@kernel.org> > --- > fs/fs-writeback.c | 64 +++++++++++++++++++++++++++++++++++++++ > include/linux/writeback.h | 4 +++ > 2 files changed, 68 insertions(+) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index 6129debdc938..5c79d7acefdb 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -880,6 +880,70 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi, > wb_put(last_wb); > } > > +/** > + * cgroup_writeback_by_id - initiate cgroup writeback from bdi and memcg IDs > + * @bdi_id: target bdi id > + * @memcg_id: target memcg css id > + * @nr_pages: number of pages to write > + * @reason: reason why some writeback work initiated > + * @done: target wb_completion > + * > + * Initiate flush of the bdi_writeback identified by @bdi_id and @memcg_id > + * with the specified parameters. > + */ > +int cgroup_writeback_by_id(u64 bdi_id, int memcg_id, unsigned long nr, > + enum wb_reason reason, struct wb_completion *done) > +{ > + struct backing_dev_info *bdi; > + struct cgroup_subsys_state *memcg_css; > + struct bdi_writeback *wb; > + struct wb_writeback_work *work; > + int ret; > + > + /* lookup bdi and memcg */ > + bdi = bdi_get_by_id(bdi_id); > + if (!bdi) > + return -ENOENT; > + > + rcu_read_lock(); > + memcg_css = css_from_id(memcg_id, &memory_cgrp_subsys); > + if (memcg_css && !css_tryget(memcg_css)) > + memcg_css = NULL; > + rcu_read_unlock(); > + if (!memcg_css) { > + ret = -ENOENT; > + goto out_bdi_put; > + } > + > + /* and find the associated wb */ > + wb = wb_get_create(bdi, memcg_css, GFP_NOWAIT | __GFP_NOWARN); > + if (!wb) { > + ret = -ENOMEM; > + goto out_css_put; > + } One more thought: You don't want the "_create" part here, do you? If there's any point in writing back using this wb, it must be attached to some inode and thus it must exist. In the normal case wb_get_create() will just fetch the reference and be done with it but when you feed garbage into this function due to id going stale or frn structures getting corrupted due to concurrent access, you can be creating bogus wb structures in bdi... Honza
On Thu, Aug 15, 2019 at 04:05:35PM +0200, Jan Kara wrote: > > +int cgroup_writeback_by_id(u64 bdi_id, int memcg_id, unsigned long nr_pages, > > + enum wb_reason reason, struct wb_completion *done); > > +int writeback_by_id(int id, unsigned long nr, enum wb_reason reason, > > + struct wb_completion *done); > > I guess this writeback_by_id() is stale? I didn't find it anywhere else... Yes, removed. Thanks.
On Thu, Aug 15, 2019 at 04:54:21PM +0200, Jan Kara wrote: > > + /* and find the associated wb */ > > + wb = wb_get_create(bdi, memcg_css, GFP_NOWAIT | __GFP_NOWARN); > > + if (!wb) { > > + ret = -ENOMEM; > > + goto out_css_put; > > + } > > One more thought: You don't want the "_create" part here, do you? If > there's any point in writing back using this wb, it must be attached to > some inode and thus it must exist. In the normal case wb_get_create() will > just fetch the reference and be done with it but when you feed garbage into > this function due to id going stale or frn structures getting corrupted due > to concurrent access, you can be creating bogus wb structures in bdi... Yeah, it can create wbs unnecessarily which isn't critical but also is easy to fix. Will update. Thanks.
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index 6129debdc938..5c79d7acefdb 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -880,6 +880,70 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi, wb_put(last_wb); } +/** + * cgroup_writeback_by_id - initiate cgroup writeback from bdi and memcg IDs + * @bdi_id: target bdi id + * @memcg_id: target memcg css id + * @nr_pages: number of pages to write + * @reason: reason why some writeback work initiated + * @done: target wb_completion + * + * Initiate flush of the bdi_writeback identified by @bdi_id and @memcg_id + * with the specified parameters. + */ +int cgroup_writeback_by_id(u64 bdi_id, int memcg_id, unsigned long nr, + enum wb_reason reason, struct wb_completion *done) +{ + struct backing_dev_info *bdi; + struct cgroup_subsys_state *memcg_css; + struct bdi_writeback *wb; + struct wb_writeback_work *work; + int ret; + + /* lookup bdi and memcg */ + bdi = bdi_get_by_id(bdi_id); + if (!bdi) + return -ENOENT; + + rcu_read_lock(); + memcg_css = css_from_id(memcg_id, &memory_cgrp_subsys); + if (memcg_css && !css_tryget(memcg_css)) + memcg_css = NULL; + rcu_read_unlock(); + if (!memcg_css) { + ret = -ENOENT; + goto out_bdi_put; + } + + /* and find the associated wb */ + wb = wb_get_create(bdi, memcg_css, GFP_NOWAIT | __GFP_NOWARN); + if (!wb) { + ret = -ENOMEM; + goto out_css_put; + } + + /* issue the writeback work */ + work = kzalloc(sizeof(*work), GFP_NOWAIT | __GFP_NOWARN); + if (work) { + work->nr_pages = nr; + work->sync_mode = WB_SYNC_NONE; + work->reason = reason; + work->done = done; + work->auto_free = 1; + wb_queue_work(wb, work); + ret = 0; + } else { + ret = -ENOMEM; + } + + wb_put(wb); +out_css_put: + css_put(memcg_css); +out_bdi_put: + bdi_put(bdi); + return ret; +} + /** * cgroup_writeback_umount - flush inode wb switches for umount * diff --git a/include/linux/writeback.h b/include/linux/writeback.h index 8945aac31392..ad794f2a7d42 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -217,6 +217,10 @@ void wbc_attach_and_unlock_inode(struct writeback_control *wbc, void wbc_detach_inode(struct writeback_control *wbc); void wbc_account_cgroup_owner(struct writeback_control *wbc, struct page *page, size_t bytes); +int cgroup_writeback_by_id(u64 bdi_id, int memcg_id, unsigned long nr_pages, + enum wb_reason reason, struct wb_completion *done); +int writeback_by_id(int id, unsigned long nr, enum wb_reason reason, + struct wb_completion *done); void cgroup_writeback_umount(void); /**
Implement cgroup_writeback_by_id() which initiates cgroup writeback from bdi and memcg IDs. This will be used by memcg foreign inode flushing. Signed-off-by: Tejun Heo <tj@kernel.org> --- fs/fs-writeback.c | 64 +++++++++++++++++++++++++++++++++++++++ include/linux/writeback.h | 4 +++ 2 files changed, 68 insertions(+)