On 2019/8/7 5:25 下午, Andrea Righi wrote: > 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. Sure, waiting for next version :-) -- Coly Li
