On 01/14/2018 03:42 PM, Coly Li wrote:
> In patch "bcache: fix cached_dev->count usage for bch_cache_set_error()",
> cached_dev_get() is called when creating dc->writeback_thread, and
> cached_dev_put() is called when exiting dc->writeback_thread. This
> modification works well unless people detach the bcache device manually by
> 'echo 1 > /sys/block/bcache<N>/bcache/detach'
> Because this sysfs interface only calls bch_cached_dev_detach() which wakes
> up dc->writeback_thread but does not stop it. The reason is, before patch
> "bcache: fix cached_dev->count usage for bch_cache_set_error()", inside
> bch_writeback_thread(), if cache is not dirty after writeback,
> cached_dev_put() will be called here. And in cached_dev_make_request() when
> a new write request makes cache from clean to dirty, cached_dev_get() will
> be called there. Since we don't operate dc->count in these locations,
> refcount d->count cannot be dropped after cache becomes clean, and
> cached_dev_detach_finish() won't be called to detach bcache device.
>
> This patch fixes the issue by checking whether BCACHE_DEV_DETACHING is
> set inside bch_writeback_thread(). If this bit is set and cache is clean
> (no existing writeback_keys), break the while-loop, call cached_dev_put()
> and quit the writeback thread.
>
> Please note if cache is still dirty, even BCACHE_DEV_DETACHING is set the
> writeback thread should continue to perform writeback, this is the original
> design of manually detach.
>
> I compose a separte patch because that patch "bcache: fix cached_dev->count
> usage for bch_cache_set_error()" already gets a "Reviewed-by:" from Hannes
> Reinecke. Also this fix is not trivial and good for a separate patch.
>
> Signed-off-by: Coly Li <colyli@xxxxxxx>
> Cc: Michael Lyle <mlyle@xxxxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxxx>
> Cc: Huijun Tang <tang.junhui@xxxxxxxxxx>
> ---
> drivers/md/bcache/writeback.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
> index b280c134dd4d..4dbeaaa575bf 100644
> --- a/drivers/md/bcache/writeback.c
> +++ b/drivers/md/bcache/writeback.c
> @@ -565,9 +565,15 @@ static int bch_writeback_thread(void *arg)
> while (!kthread_should_stop()) {
> down_write(&dc->writeback_lock);
> set_current_state(TASK_INTERRUPTIBLE);
> - if (!atomic_read(&dc->has_dirty) ||
> - (!test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) &&
> - !dc->writeback_running)) {
> + /*
> + * If the bache device is detaching, skip here and continue
> + * to perform writeback. Otherwise, if no dirty data on cache,
> + * or there is dirty data on cache but writeback is disabled,
> + * the writeback thread should sleep here and wait for others
> + * to wake up it.
> + */
> + if (!test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) &&
> + (!atomic_read(&dc->has_dirty) || !dc->writeback_running)) {
> up_write(&dc->writeback_lock);
>
> if (kthread_should_stop()) {
> @@ -587,6 +593,14 @@ static int bch_writeback_thread(void *arg)
> atomic_set(&dc->has_dirty, 0);
> SET_BDEV_STATE(&dc->sb, BDEV_STATE_CLEAN);
> bch_write_bdev_super(dc, NULL);
> + /*
> + * If bcache device is detaching via sysfs interface,
> + * writeback thread should stop after there is no dirty
> + * data on cache. BCACHE_DEV_DETACHING flag is set in
> + * bch_cached_dev_detach().
> + */
> + if (test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags))
> + break;
> }
>
> up_write(&dc->writeback_lock);
>
Checking several atomic flags in one statement renders the atomic pretty
much pointless; you need to protect the 'if' clause with some lock or
just check _one_ atomic statement.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
