Re: [PATCH v2] bcache: fix deadlock in bcache_allocator

[Andrea Righi]

On Tue, Aug 06, 2019 at 07:36:48PM +0200, Andrea Righi wrote:
> On Tue, Aug 06, 2019 at 11:18:01AM +0200, Andrea Righi wrote:
> > bcache_allocator() can call the following:
> > 
> >  bch_allocator_thread()
> >   -> bch_prio_write()
> >      -> bch_bucket_alloc()
> >         -> wait on &ca->set->bucket_wait
> > 
> > But the wake up event on bucket_wait is supposed to come from
> > bch_allocator_thread() itself => deadlock:
> > 
> > [ 1158.490744] INFO: task bcache_allocato:15861 blocked for more than 10 seconds.
> > [ 1158.495929]       Not tainted 5.3.0-050300rc3-generic #201908042232
> > [ 1158.500653] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> > [ 1158.504413] bcache_allocato D    0 15861      2 0x80004000
> > [ 1158.504419] Call Trace:
> > [ 1158.504429]  __schedule+0x2a8/0x670
> > [ 1158.504432]  schedule+0x2d/0x90
> > [ 1158.504448]  bch_bucket_alloc+0xe5/0x370 [bcache]
> > [ 1158.504453]  ? wait_woken+0x80/0x80
> > [ 1158.504466]  bch_prio_write+0x1dc/0x390 [bcache]
> > [ 1158.504476]  bch_allocator_thread+0x233/0x490 [bcache]
> > [ 1158.504491]  kthread+0x121/0x140
> > [ 1158.504503]  ? invalidate_buckets+0x890/0x890 [bcache]
> > [ 1158.504506]  ? kthread_park+0xb0/0xb0
> > [ 1158.504510]  ret_from_fork+0x35/0x40
> > 
> > Fix by making the call to bch_prio_write() non-blocking, so that
> > bch_allocator_thread() never waits on itself.
> > 
> > Moreover, make sure to wake up the garbage collector thread when
> > bch_prio_write() is failing to allocate buckets.
> > 
> > BugLink: https://bugs.launchpad.net/bugs/1784665
> > BugLink: https://bugs.launchpad.net/bugs/1796292
> > Signed-off-by: Andrea Righi <andrea.righi@xxxxxxxxxxxxx>
> > ---
> > Changes in v2:
> >  - prevent retry_invalidate busy loop in bch_allocator_thread()
> > 
> >  drivers/md/bcache/alloc.c  |  5 ++++-
> >  drivers/md/bcache/bcache.h |  2 +-
> >  drivers/md/bcache/super.c  | 13 +++++++++----
> >  3 files changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
> > index 6f776823b9ba..a1df0d95151c 100644
> > --- a/drivers/md/bcache/alloc.c
> > +++ b/drivers/md/bcache/alloc.c
> > @@ -377,7 +377,10 @@ static int bch_allocator_thread(void *arg)
> >  			if (!fifo_full(&ca->free_inc))
> >  				goto retry_invalidate;
> >  
> > -			bch_prio_write(ca);
> > +			if (bch_prio_write(ca, false) < 0) {
> > +				ca->invalidate_needs_gc = 1;
> > +				wake_up_gc(ca->set);
> > +			}
> >  		}
> >  	}
> >  out:
> > diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> > index 013e35a9e317..deb924e1d790 100644
> > --- a/drivers/md/bcache/bcache.h
> > +++ b/drivers/md/bcache/bcache.h
> > @@ -977,7 +977,7 @@ bool bch_cached_dev_error(struct cached_dev *dc);
> >  __printf(2, 3)
> >  bool bch_cache_set_error(struct cache_set *c, const char *fmt, ...);
> >  
> > -void bch_prio_write(struct cache *ca);
> > +int bch_prio_write(struct cache *ca, bool wait);
> >  void bch_write_bdev_super(struct cached_dev *dc, struct closure *parent);
> >  
> >  extern struct workqueue_struct *bcache_wq;
> > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> > index 20ed838e9413..716ea272fb55 100644
> > --- a/drivers/md/bcache/super.c
> > +++ b/drivers/md/bcache/super.c
> > @@ -529,7 +529,7 @@ static void prio_io(struct cache *ca, uint64_t bucket, int op,
> >  	closure_sync(cl);
> >  }
> >  
> > -void bch_prio_write(struct cache *ca)
> > +int bch_prio_write(struct cache *ca, bool wait)
> >  {
> >  	int i;
> >  	struct bucket *b;
> > @@ -564,8 +564,12 @@ void bch_prio_write(struct cache *ca)
> >  		p->magic	= pset_magic(&ca->sb);
> >  		p->csum		= bch_crc64(&p->magic, bucket_bytes(ca) - 8);
> >  
> > -		bucket = bch_bucket_alloc(ca, RESERVE_PRIO, true);
> > -		BUG_ON(bucket == -1);
> > +		bucket = bch_bucket_alloc(ca, RESERVE_PRIO, wait);
> > +		if (bucket == -1) {
> > +			if (!wait)
> > +				return -ENOMEM;
> > +			BUG_ON(1);
> > +		}
> 
> Coly,
> 
> looking more at this change, I think we should handle the failure path
> properly or we may leak buckets, am I right? (sorry for not realizing
> this before). Maybe we need something like the following on top of my
> previous patch.
> 
> I'm going to run more stress tests with this patch applied and will try
> to figure out if we're actually leaking buckets without it.
> 
> ---
> Subject: bcache: prevent leaking buckets in bch_prio_write()
> 
> Handle the allocation failure path properly in bch_prio_write() to avoid
> leaking buckets from the previous successful iterations.
> 
> Signed-off-by: Andrea Righi <andrea.righi@xxxxxxxxxxxxx>

Coly, ignore this one please. A v3 of the previous patch with a better
fix for this potential buckets leak is on the way.

Thanks,
-Andrea