On Sat 03-08-19 07:01:52, Tejun Heo wrote:
> wb_completion is used to track writeback completions. We want to use
> it from memcg side for foreign inode flushes. This patch updates it
> to remember the target waitq instead of assuming bdi->wb_waitq and
> expose it outside of fs-writeback.c.
>
> Signed-off-by: Tejun Heo <tj@xxxxxxxxxx>
The patch looks good to me. You can add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Honza
> ---
> fs/fs-writeback.c | 47 ++++++++++----------------------
> include/linux/backing-dev-defs.h | 20 ++++++++++++++
> include/linux/backing-dev.h | 2 ++
> 3 files changed, 36 insertions(+), 33 deletions(-)
>
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 542b02d170f8..6129debdc938 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -36,10 +36,6 @@
> */
> #define MIN_WRITEBACK_PAGES (4096UL >> (PAGE_SHIFT - 10))
>
> -struct wb_completion {
> - atomic_t cnt;
> -};
> -
> /*
> * Passed into wb_writeback(), essentially a subset of writeback_control
> */
> @@ -60,19 +56,6 @@ struct wb_writeback_work {
> struct wb_completion *done; /* set if the caller waits */
> };
>
> -/*
> - * If one wants to wait for one or more wb_writeback_works, each work's
> - * ->done should be set to a wb_completion defined using the following
> - * macro. Once all work items are issued with wb_queue_work(), the caller
> - * can wait for the completion of all using wb_wait_for_completion(). Work
> - * items which are waited upon aren't freed automatically on completion.
> - */
> -#define DEFINE_WB_COMPLETION_ONSTACK(cmpl) \
> - struct wb_completion cmpl = { \
> - .cnt = ATOMIC_INIT(1), \
> - }
> -
> -
> /*
> * If an inode is constantly having its pages dirtied, but then the
> * updates stop dirtytime_expire_interval seconds in the past, it's
> @@ -182,7 +165,7 @@ static void finish_writeback_work(struct bdi_writeback *wb,
> if (work->auto_free)
> kfree(work);
> if (done && atomic_dec_and_test(&done->cnt))
> - wake_up_all(&wb->bdi->wb_waitq);
> + wake_up_all(done->waitq);
> }
>
> static void wb_queue_work(struct bdi_writeback *wb,
> @@ -206,20 +189,18 @@ static void wb_queue_work(struct bdi_writeback *wb,
>
> /**
> * wb_wait_for_completion - wait for completion of bdi_writeback_works
> - * @bdi: bdi work items were issued to
> * @done: target wb_completion
> *
> * Wait for one or more work items issued to @bdi with their ->done field
> - * set to @done, which should have been defined with
> - * DEFINE_WB_COMPLETION_ONSTACK(). This function returns after all such
> - * work items are completed. Work items which are waited upon aren't freed
> + * set to @done, which should have been initialized with
> + * DEFINE_WB_COMPLETION(). This function returns after all such work items
> + * are completed. Work items which are waited upon aren't freed
> * automatically on completion.
> */
> -static void wb_wait_for_completion(struct backing_dev_info *bdi,
> - struct wb_completion *done)
> +void wb_wait_for_completion(struct wb_completion *done)
> {
> atomic_dec(&done->cnt); /* put down the initial count */
> - wait_event(bdi->wb_waitq, !atomic_read(&done->cnt));
> + wait_event(*done->waitq, !atomic_read(&done->cnt));
> }
>
> #ifdef CONFIG_CGROUP_WRITEBACK
> @@ -843,7 +824,7 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
> restart:
> rcu_read_lock();
> list_for_each_entry_continue_rcu(wb, &bdi->wb_list, bdi_node) {
> - DEFINE_WB_COMPLETION_ONSTACK(fallback_work_done);
> + DEFINE_WB_COMPLETION(fallback_work_done, bdi);
> struct wb_writeback_work fallback_work;
> struct wb_writeback_work *work;
> long nr_pages;
> @@ -890,7 +871,7 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
> last_wb = wb;
>
> rcu_read_unlock();
> - wb_wait_for_completion(bdi, &fallback_work_done);
> + wb_wait_for_completion(&fallback_work_done);
> goto restart;
> }
> rcu_read_unlock();
> @@ -2362,7 +2343,8 @@ static void wait_sb_inodes(struct super_block *sb)
> static void __writeback_inodes_sb_nr(struct super_block *sb, unsigned long nr,
> enum wb_reason reason, bool skip_if_busy)
> {
> - DEFINE_WB_COMPLETION_ONSTACK(done);
> + struct backing_dev_info *bdi = sb->s_bdi;
> + DEFINE_WB_COMPLETION(done, bdi);
> struct wb_writeback_work work = {
> .sb = sb,
> .sync_mode = WB_SYNC_NONE,
> @@ -2371,14 +2353,13 @@ static void __writeback_inodes_sb_nr(struct super_block *sb, unsigned long nr,
> .nr_pages = nr,
> .reason = reason,
> };
> - struct backing_dev_info *bdi = sb->s_bdi;
>
> if (!bdi_has_dirty_io(bdi) || bdi == &noop_backing_dev_info)
> return;
> WARN_ON(!rwsem_is_locked(&sb->s_umount));
>
> bdi_split_work_to_wbs(sb->s_bdi, &work, skip_if_busy);
> - wb_wait_for_completion(bdi, &done);
> + wb_wait_for_completion(&done);
> }
>
> /**
> @@ -2440,7 +2421,8 @@ EXPORT_SYMBOL(try_to_writeback_inodes_sb);
> */
> void sync_inodes_sb(struct super_block *sb)
> {
> - DEFINE_WB_COMPLETION_ONSTACK(done);
> + struct backing_dev_info *bdi = sb->s_bdi;
> + DEFINE_WB_COMPLETION(done, bdi);
> struct wb_writeback_work work = {
> .sb = sb,
> .sync_mode = WB_SYNC_ALL,
> @@ -2450,7 +2432,6 @@ void sync_inodes_sb(struct super_block *sb)
> .reason = WB_REASON_SYNC,
> .for_sync = 1,
> };
> - struct backing_dev_info *bdi = sb->s_bdi;
>
> /*
> * Can't skip on !bdi_has_dirty() because we should wait for !dirty
> @@ -2464,7 +2445,7 @@ void sync_inodes_sb(struct super_block *sb)
> /* protect against inode wb switch, see inode_switch_wbs_work_fn() */
> bdi_down_write_wb_switch_rwsem(bdi);
> bdi_split_work_to_wbs(bdi, &work, false);
> - wb_wait_for_completion(bdi, &done);
> + wb_wait_for_completion(&done);
> bdi_up_write_wb_switch_rwsem(bdi);
>
> wait_sb_inodes(sb);
> diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
> index 6a1a8a314d85..8fb740178d5d 100644
> --- a/include/linux/backing-dev-defs.h
> +++ b/include/linux/backing-dev-defs.h
> @@ -67,6 +67,26 @@ enum wb_reason {
> WB_REASON_MAX,
> };
>
> +struct wb_completion {
> + atomic_t cnt;
> + wait_queue_head_t *waitq;
> +};
> +
> +#define __WB_COMPLETION_INIT(_waitq) \
> + (struct wb_completion){ .cnt = ATOMIC_INIT(1), .waitq = (_waitq) }
> +
> +/*
> + * If one wants to wait for one or more wb_writeback_works, each work's
> + * ->done should be set to a wb_completion defined using the following
> + * macro. Once all work items are issued with wb_queue_work(), the caller
> + * can wait for the completion of all using wb_wait_for_completion(). Work
> + * items which are waited upon aren't freed automatically on completion.
> + */
> +#define WB_COMPLETION_INIT(bdi) __WB_COMPLETION_INIT(&(bdi)->wb_waitq)
> +
> +#define DEFINE_WB_COMPLETION(cmpl, bdi) \
> + struct wb_completion cmpl = WB_COMPLETION_INIT(bdi)
> +
> /*
> * For cgroup writeback, multiple wb's may map to the same blkcg. Those
> * wb's can operate mostly independently but should share the congested
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index 35b31d176f74..02650b1253a2 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -44,6 +44,8 @@ void wb_start_background_writeback(struct bdi_writeback *wb);
> void wb_workfn(struct work_struct *work);
> void wb_wakeup_delayed(struct bdi_writeback *wb);
>
> +void wb_wait_for_completion(struct wb_completion *done);
> +
> extern spinlock_t bdi_lock;
> extern struct list_head bdi_list;
>
> --
> 2.17.1
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
