From patchwork Mon Mar 19 00:36:14 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michael Lyle X-Patchwork-Id: 10291455 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 4BBCE60291 for ; Mon, 19 Mar 2018 00:36:46 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3D8AC28EDF for ; Mon, 19 Mar 2018 00:36:46 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 31BA628F56; Mon, 19 Mar 2018 00:36:46 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 43D7B28EDF for ; Mon, 19 Mar 2018 00:36:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754673AbeCSAgn (ORCPT ); Sun, 18 Mar 2018 20:36:43 -0400 Received: from mail-pl0-f66.google.com ([209.85.160.66]:45389 "EHLO mail-pl0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754301AbeCSAgl (ORCPT ); Sun, 18 Mar 2018 20:36:41 -0400 Received: by mail-pl0-f66.google.com with SMTP id n15-v6so1481489plp.12 for ; Sun, 18 Mar 2018 17:36:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=lyle-org.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=y2wiscpVEQDEhvDNo74VCq0l0BSLgzzLq+v2nJWVjVg=; b=vk/YuAeCDvFlLfFIgApQ+GhcZAOrgQ0xssbHxdbOmHwqGEgTwo1s4s57VMX0S6Lxbh mcV5yPC1IXG4f6EuGN8zpU3sOtbtQBlic4S0L2k/4+pYcgezuPC0q1K9KBjVCDW39eqD cSYS8FpWFHibtmqWCqE3XZ8eSgtYTRM1OE3HWhuQKQRQrrwACWEAEcnROg6dCe5HfT5V 4LhogRuA5JOjL8oy5otH9UvBIY934sTJ3ML/P38tdWqFjB0+2YusDLl7Nb6Q0HBmAx0d NEj43hz/kXVBRXs8QNwy82GE1VwJmvXSyUE6+eWYp8LeRW7uZEChrkZSdijBu1wsmmQZ fcNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=y2wiscpVEQDEhvDNo74VCq0l0BSLgzzLq+v2nJWVjVg=; b=dt/cQ92YwTwuCpQjsGer2HHP5JxiIVIGnvhajpPOlnEDEARQ3+t1IKXCfbrGU+BXgY leA/RYY8sxED8ZUNhKHAGuSQ5FHZg7M0vZgoUKaNxTKoaLPNjU9WI4C3lgMR91RSR8Pk cfeIVHBqfjE6YLzOY+m1P3sjMc2q6WTDP1xqWeNmKSSV4QxoWOvPSvEwBWEiLXGIIZ+0 gr5EEdoXYjCmX/uqCdF4w5AO1VIJsfogZy3VweUKf/Gg2+CQFWgaBCwGt7UDBHvomEw9 KMR6VUOec18Gh61I796c/V+u/OtD2vbToPcN+l1wj0MpBzLUvu/2HARhkx6CnE/9ZpOI ITmA== X-Gm-Message-State: AElRT7H/Xqe/HVM6k89lyDgS+V4rJR44ljugQbhstgDhKBGy0oksMHhp HpI4SvtNn4tO3rzI+fMYuBt+K2Yj X-Google-Smtp-Source: AG47ELvAxuY+GwC4eCUw+BbgE04ZLgeAZFtlBw9QEGHpbpueC/n0e0Zz/qxYSH3QIOwAmCZHXN5Yjw== X-Received: by 2002:a17:902:2862:: with SMTP id e89-v6mr10373889plb.348.1521419800543; Sun, 18 Mar 2018 17:36:40 -0700 (PDT) Received: from midnight.lan (2600-6c52-6200-09b7-0000-0000-0000-0d66.dhcp6.chtrptr.net. [2600:6c52:6200:9b7::d66]) by smtp.gmail.com with ESMTPSA id a17sm27674857pfc.122.2018.03.18.17.36.39 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 18 Mar 2018 17:36:39 -0700 (PDT) From: Michael Lyle To: linux-bcache@vger.kernel.org, linux-block@vger.kernel.org Cc: axboe@fb.com, Coly Li , Michael Lyle , Junhui Tang Subject: [for-4.17 01/20] bcache: fix cached_dev->count usage for bch_cache_set_error() Date: Sun, 18 Mar 2018 17:36:14 -0700 Message-Id: <20180319003633.27225-2-mlyle@lyle.org> X-Mailer: git-send-email 2.14.1 In-Reply-To: <20180319003633.27225-1-mlyle@lyle.org> References: <20180319003633.27225-1-mlyle@lyle.org> Sender: linux-block-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP From: Coly Li When bcache metadata I/O fails, bcache will call bch_cache_set_error() to retire the whole cache set. The expected behavior to retire a cache set is to unregister the cache set, and unregister all backing device attached to this cache set, then remove sysfs entries of the cache set and all attached backing devices, finally release memory of structs cache_set, cache, cached_dev and bcache_device. In my testing when journal I/O failure triggered by disconnected cache device, sometimes the cache set cannot be retired, and its sysfs entry /sys/fs/bcache/ still exits and the backing device also references it. This is not expected behavior. When metadata I/O failes, the call senquence to retire whole cache set is, bch_cache_set_error() bch_cache_set_unregister() bch_cache_set_stop() __cache_set_unregister() <- called as callback by calling clousre_queue(&c->caching) cache_set_flush() <- called as a callback when refcount of cache_set->caching is 0 cache_set_free() <- called as a callback when refcount of catch_set->cl is 0 bch_cache_set_release() <- called as a callback when refcount of catch_set->kobj is 0 I find if kernel thread bch_writeback_thread() quits while-loop when kthread_should_stop() is true and searched_full_index is false, clousre callback cache_set_flush() set by continue_at() will never be called. The result is, bcache fails to retire whole cache set. cache_set_flush() will be called when refcount of closure c->caching is 0, and in function bcache_device_detach() refcount of closure c->caching is released to 0 by clousre_put(). In metadata error code path, function bcache_device_detach() is called by cached_dev_detach_finish(). This is a callback routine being called when cached_dev->count is 0. This refcount is decreased by cached_dev_put(). The above dependence indicates, cache_set_flush() will be called when refcount of cache_set->cl is 0, and refcount of cache_set->cl to be 0 when refcount of cache_dev->count is 0. The reason why sometimes cache_dev->count is not 0 (when metadata I/O fails and bch_cache_set_error() called) is, in bch_writeback_thread(), refcount of cache_dev is not decreased properly. In bch_writeback_thread(), cached_dev_put() is called only when searched_full_index is true and cached_dev->writeback_keys is empty, a.k.a there is no dirty data on cache. In most of run time it is correct, but when bch_writeback_thread() quits the while-loop while cache is still dirty, current code forget to call cached_dev_put() before this kernel thread exits. This is why sometimes cache_set_flush() is not executed and cache set fails to be retired. The reason to call cached_dev_put() in bch_writeback_rate() is, when the cache device changes from clean to dirty, cached_dev_get() is called, to make sure during writeback operatiions both backing and cache devices won't be released. Adding following code in bch_writeback_thread() does not work, static int bch_writeback_thread(void *arg) } + if (atomic_read(&dc->has_dirty)) + cached_dev_put() + return 0; } because writeback kernel thread can be waken up and start via sysfs entry: echo 1 > /sys/block/bcache/bcache/writeback_running It is difficult to check whether backing device is dirty without race and extra lock. So the above modification will introduce potential refcount underflow in some conditions. The correct fix is, to take cached dev refcount when creating the kernel thread, and put it before the kernel thread exits. Then bcache does not need to take a cached dev refcount when cache turns from clean to dirty, or to put a cached dev refcount when cache turns from ditry to clean. The writeback kernel thread is alwasy safe to reference data structure from cache set, cache and cached device (because a refcount of cache device is taken for it already), and no matter the kernel thread is stopped by I/O errors or system reboot, cached_dev->count can always be used correctly. The patch is simple, but understanding how it works is quite complicated. Changelog: v2: set dc->writeback_thread to NULL in this patch, as suggested by Hannes. v1: initial version for review. Signed-off-by: Coly Li Reviewed-by: Hannes Reinecke Reviewed-by: Michael Lyle Cc: Michael Lyle Cc: Junhui Tang --- drivers/md/bcache/super.c | 1 - drivers/md/bcache/writeback.c | 11 ++++++++--- drivers/md/bcache/writeback.h | 2 -- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c index f1729df9fb74..f4dea0209fce 100644 --- a/drivers/md/bcache/super.c +++ b/drivers/md/bcache/super.c @@ -1065,7 +1065,6 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c, if (BDEV_STATE(&dc->sb) == BDEV_STATE_DIRTY) { bch_sectors_dirty_init(&dc->disk); atomic_set(&dc->has_dirty, 1); - refcount_inc(&dc->count); bch_writeback_queue(dc); } diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c index f1d2fc15abcc..b280c134dd4d 100644 --- a/drivers/md/bcache/writeback.c +++ b/drivers/md/bcache/writeback.c @@ -572,7 +572,7 @@ static int bch_writeback_thread(void *arg) if (kthread_should_stop()) { set_current_state(TASK_RUNNING); - return 0; + break; } schedule(); @@ -585,7 +585,6 @@ static int bch_writeback_thread(void *arg) if (searched_full_index && RB_EMPTY_ROOT(&dc->writeback_keys.keys)) { atomic_set(&dc->has_dirty, 0); - cached_dev_put(dc); SET_BDEV_STATE(&dc->sb, BDEV_STATE_CLEAN); bch_write_bdev_super(dc, NULL); } @@ -606,6 +605,9 @@ static int bch_writeback_thread(void *arg) } } + dc->writeback_thread = NULL; + cached_dev_put(dc); + return 0; } @@ -669,10 +671,13 @@ int bch_cached_dev_writeback_start(struct cached_dev *dc) if (!dc->writeback_write_wq) return -ENOMEM; + cached_dev_get(dc); dc->writeback_thread = kthread_create(bch_writeback_thread, dc, "bcache_writeback"); - if (IS_ERR(dc->writeback_thread)) + if (IS_ERR(dc->writeback_thread)) { + cached_dev_put(dc); return PTR_ERR(dc->writeback_thread); + } schedule_delayed_work(&dc->writeback_rate_update, dc->writeback_rate_update_seconds * HZ); diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h index 587b25599856..0bba8f1c6cdf 100644 --- a/drivers/md/bcache/writeback.h +++ b/drivers/md/bcache/writeback.h @@ -105,8 +105,6 @@ static inline void bch_writeback_add(struct cached_dev *dc) { if (!atomic_read(&dc->has_dirty) && !atomic_xchg(&dc->has_dirty, 1)) { - refcount_inc(&dc->count); - if (BDEV_STATE(&dc->sb) != BDEV_STATE_DIRTY) { SET_BDEV_STATE(&dc->sb, BDEV_STATE_DIRTY); /* XXX: should do this synchronously */