Re: [PATCH v3 3/3] bcache: Move journal work to new background wq

[Kai Krakow <kai@kaishome.de>]

Am Fr., 29. Jan. 2021 um 17:06 Uhr schrieb Coly Li <colyli@suse.de>:
>
> On 1/29/21 7:28 AM, Kai Krakow wrote:
> > This is potentially long running and not latency sensitive, let's get
> > it out of the way of other latency sensitive events.
> >
> > As observed in the previous commit, the `system_wq` comes easily
> > congested by bcache, and this fixes a few more stalls I was observing
> > every once in a while.
> >
> > Let's not make this `WQ_MEM_RECLAIM` as it showed to reduce performance
> > of boot and file system operations in my tests. Also, without
> > `WQ_MEM_RECLAIM`, I no longer see desktop stalls. This matches the
> > previous behavior as `system_wq` also does no memory reclaim:
> >
> >> // workqueue.c:
> >> system_wq = alloc_workqueue("events", 0, 0);
> >
>
> Your text is convinced. I am OK with this patch. One more request, could
> you please add a comment at alloc_workqueue() to explain why setting
> flags as 0 and no WQ_MEM_RECLAIM. It should be helpful to others who
> don't read our discussion.

I'll post a v4.

> > Cc: Coly Li <colyli@suse.de>
> > Signed-off-by: Kai Krakow <kai@kaishome.de>
> > ---
> >  drivers/md/bcache/bcache.h  | 1 +
> >  drivers/md/bcache/journal.c | 4 ++--
> >  drivers/md/bcache/super.c   | 7 +++++++
> >  3 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> > index b1ed16c7a5341..e8bf4f752e8be 100644
> > --- a/drivers/md/bcache/bcache.h
> > +++ b/drivers/md/bcache/bcache.h
> > @@ -1001,6 +1001,7 @@ void bch_write_bdev_super(struct cached_dev *dc, struct closure *parent);
> >
> >  extern struct workqueue_struct *bcache_wq;
> >  extern struct workqueue_struct *bch_journal_wq;
> > +extern struct workqueue_struct *bch_flush_wq;
> >  extern struct mutex bch_register_lock;
> >  extern struct list_head bch_cache_sets;
> >
> > diff --git a/drivers/md/bcache/journal.c b/drivers/md/bcache/journal.c
> > index aefbdb7e003bc..c6613e8173337 100644
> > --- a/drivers/md/bcache/journal.c
> > +++ b/drivers/md/bcache/journal.c
> > @@ -932,8 +932,8 @@ atomic_t *bch_journal(struct cache_set *c,
> >               journal_try_write(c);
> >       } else if (!w->dirty) {
> >               w->dirty = true;
> > -             schedule_delayed_work(&c->journal.work,
> > -                                   msecs_to_jiffies(c->journal_delay_ms));
> > +             queue_delayed_work(bch_flush_wq, &c->journal.work,
> > +                                msecs_to_jiffies(c->journal_delay_ms));
> >               spin_unlock(&c->journal.lock);
> >       } else {
> >               spin_unlock(&c->journal.lock);
> > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> > index 77c5d8b6d4316..817b36c39b4fc 100644
> > --- a/drivers/md/bcache/super.c
> > +++ b/drivers/md/bcache/super.c
> > @@ -49,6 +49,7 @@ static int bcache_major;
> >  static DEFINE_IDA(bcache_device_idx);
> >  static wait_queue_head_t unregister_wait;
> >  struct workqueue_struct *bcache_wq;
> > +struct workqueue_struct *bch_flush_wq;
> >  struct workqueue_struct *bch_journal_wq;
> >
> >
> > @@ -2821,6 +2822,8 @@ static void bcache_exit(void)
> >               destroy_workqueue(bcache_wq);
> >       if (bch_journal_wq)
> >               destroy_workqueue(bch_journal_wq);
> > +     if (bch_flush_wq)
> > +             destroy_workqueue(bch_flush_wq);
> >       bch_btree_exit();
> >
> >       if (bcache_major)
> > @@ -2884,6 +2887,10 @@ static int __init bcache_init(void)
> >       if (!bcache_wq)
> >               goto err;
> >
> > +     bch_flush_wq = alloc_workqueue("bch_flush", 0, 0);
> > +     if (!bch_flush_wq)
> > +             goto err;
> > +
> >       bch_journal_wq = alloc_workqueue("bch_journal", WQ_MEM_RECLAIM, 0);
> >       if (!bch_journal_wq)
> >               goto err;
> >
>