[PATCH] bcache: block shutdown if cached_dev detaching still ongoing

[Jianjian Huo]

Backing device detaching will write back dirty data and update super block when it's finished, but won't set any flag to superblock after detaching starts. If system shuts down before detaching finishs, later cache set would never know detaching ever started before; what's worse, if users think detaching is done after shutdown and remove SSD which still contains dirty data, user would have a difficult time to bring up backing device again.

This patch will block bcache_reboot, if any cached_dev is still detaching, to guarantee detaching is done before shutdown. Another way will be adding a flag to superblock, then next bootup will continue unfinished detaching, but this won't solve the above worse case scenario.

This patch has been verified ay my setup for several test cases. And below is the new screen printout during shutdown, in case writeback is still ongoing.

bcache: bcache_reboot() Stopping all devices:
bcache: bcache_reboot() Backing device detaching hasn't been finished yet, please wait.
bcache: cached_dev_detach_finish() Caching disabled for loop1
bcache: cache_set_free() Cache set 69f2e475-3afc-42bf-b94d-8beb40030f59 unregistered
bcache: bcache_reboot() Timeout waiting for devices to be closed
reboot: System halted

Signed-off-by: Jianjian Huo <***@gmail.com>
---
drivers/md/bcache/super.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 5a78df8..92dcbfd 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1981,6 +1981,22 @@ err:
goto out;
}

+static bool should_block_reboot(void)
+{
+ struct cache_set *c, *tc;
+ struct cached_dev *dc, *t;
+
+ lockdep_assert_held(&bch_register_lock);
+
+ /*block reboot if any cached_dev is detaching*/
+ list_for_each_entry_safe(c, tc, &bch_cache_sets, list)
+ list_for_each_entry_safe(dc, t, &c->cached_devs, list)
+ if (test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags))
+ return true;
+
+ return false;
+}
+
static int bcache_reboot(struct notifier_block *n, unsigned long code, void *x)
{
if (code == SYS_DOWN ||
@@ -2010,18 +2026,27 @@ static int bcache_reboot(struct notifier_block *n, unsigned long code, void *x)
/* What's a condition variable? */
while (1) {
long timeout = start + 2 * HZ - jiffies;
+ bool blocking = false;

stopped = list_empty(&bch_cache_sets) &&
list_empty(&uncached_devices);

- if (timeout < 0 || stopped)
+ blocking = should_block_reboot();
+
+ if ((timeout < 0 && !blocking) || stopped)
break;

prepare_to_wait(&unregister_wait, &wait,
TASK_UNINTERRUPTIBLE);

mutex_unlock(&bch_register_lock);
- schedule_timeout(timeout);
+ if (blocking) {
+ pr_notice("Backing device detaching hasn't "
+ "been finished yet, please wait.");
+ schedule();
+ } else {
+ schedule_timeout(timeout);
+ }
mutex_lock(&bch_register_lock);
}

[Jianjian Huo]

Since bch_is_open will iterate linked list bch_cache_sets and
uncached_devices, it needs bch_register_lock.

Signed-off-by: Jianjian Huo <***@gmail.com>
---
drivers/md/bcache/super.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 92dcbfd..3391c53 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1933,10 +1933,12 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
if (IS_ERR(bdev)) {
if (bdev == ERR_PTR(-EBUSY)) {
bdev = lookup_bdev(strim(path));
+ mutex_lock(&bch_register_lock);
if (!IS_ERR(bdev) && bch_is_open(bdev))
err = "device already registered";
else
err = "device busy";
+ mutex_unlock(&bch_register_lock);
}
goto err;
}

[Slava Pestov]

Nice catch, applied.

Post by Jianjian Huo
Since bch_is_open will iterate linked list bch_cache_sets and
uncached_devices, it needs bch_register_lock.
---
drivers/md/bcache/super.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 92dcbfd..3391c53 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1933,10 +1933,12 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
if (IS_ERR(bdev)) {
if (bdev == ERR_PTR(-EBUSY)) {
bdev = lookup_bdev(strim(path));
+ mutex_lock(&bch_register_lock);
if (!IS_ERR(bdev) && bch_is_open(bdev))
err = "device already registered";
else
err = "device busy";
+ mutex_unlock(&bch_register_lock);
}
goto err;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Slava Pestov]

It seems like it would be simpler to eliminate the timeout altogether
and fix any cases where the shutdown path hangs. I'd like Kent to
chime in on this one.

Post by Jianjian Huo
Backing device detaching will write back dirty data and update super block when it's finished, but won't set any flag to superblock after detaching starts. If system shuts down before detaching finishs, later cache set would never know detaching ever started before; what's worse, if users think detaching is done after shutdown and remove SSD which still contains dirty data, user would have a difficult time to bring up backing device again.
This patch will block bcache_reboot, if any cached_dev is still detaching, to guarantee detaching is done before shutdown. Another way will be adding a flag to superblock, then next bootup will continue unfinished detaching, but this won't solve the above worse case scenario.
This patch has been verified ay my setup for several test cases. And below is the new screen printout during shutdown, in case writeback is still ongoing.
bcache: bcache_reboot() Backing device detaching hasn't been finished yet, please wait.
bcache: cached_dev_detach_finish() Caching disabled for loop1
bcache: cache_set_free() Cache set 69f2e475-3afc-42bf-b94d-8beb40030f59 unregistered
bcache: bcache_reboot() Timeout waiting for devices to be closed
reboot: System halted
---
drivers/md/bcache/super.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 5a78df8..92dcbfd 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
goto out;
}
+static bool should_block_reboot(void)
+{
+ struct cache_set *c, *tc;
+ struct cached_dev *dc, *t;
+
+ lockdep_assert_held(&bch_register_lock);
+
+ /*block reboot if any cached_dev is detaching*/
+ list_for_each_entry_safe(c, tc, &bch_cache_sets, list)
+ list_for_each_entry_safe(dc, t, &c->cached_devs, list)
+ if (test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags))
+ return true;
+
+ return false;
+}
+
static int bcache_reboot(struct notifier_block *n, unsigned long code, void *x)
{
if (code == SYS_DOWN ||
@@ -2010,18 +2026,27 @@ static int bcache_reboot(struct notifier_block *n, unsigned long code, void *x)
/* What's a condition variable? */
while (1) {
long timeout = start + 2 * HZ - jiffies;
+ bool blocking = false;
stopped = list_empty(&bch_cache_sets) &&
list_empty(&uncached_devices);
- if (timeout < 0 || stopped)
+ blocking = should_block_reboot();
+
+ if ((timeout < 0 && !blocking) || stopped)
break;
prepare_to_wait(&unregister_wait, &wait,
TASK_UNINTERRUPTIBLE);
mutex_unlock(&bch_register_lock);
- schedule_timeout(timeout);
+ if (blocking) {
+ pr_notice("Backing device detaching hasn't "
+ "been finished yet, please wait.");
+ schedule();
+ } else {
+ schedule_timeout(timeout);
+ }
mutex_lock(&bch_register_lock);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
More majordomo info at http://vger.kernel.org/majordomo-info.html