On 2020/10/14 15:46, Dongsheng Yang wrote:
> There is a race condition in detaching as below:
> A. detaching B. Write request
> (1) writing back
> (2) write back done, set bdev state to clean.
> (3) cached_dev_put() and schedule_work(&dc->detach);
> (4) write data [0 - 4K] directly into backing and ack to user.
> (5) power-failure...
>
> When we restart this bcache device, this bdev is clean but not detached, and read [0 - 4K],
> we will get unexpected old data from cache device.
>
> To fix this problem, set the bdev state to none when we writeback done in detaching,
> and then if power-failure happend as above, the data in cache will not be used in next
> bcache device starting, it's detached, we will read the correct data from backing derectly.
>
> Signed-off-by: Dongsheng Yang <dongsheng.yang@xxxxxxxxxxxx>
Hi Dongsheng,
It takes me for a while to understand and make sure the whole code flow
works correctly. Thank you for catching such a very rare problem. IMHO
this patch is cool, I will add it into the for-test queue.
Thanks.
Coly Li
> ---
> drivers/md/bcache/super.c | 9 ---------
> drivers/md/bcache/writeback.c | 9 +++++++++
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 1bbdc41..9298fc7 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1128,9 +1128,6 @@ static void cancel_writeback_rate_update_dwork(struct cached_dev *dc)
> static void cached_dev_detach_finish(struct work_struct *w)
> {
> struct cached_dev *dc = container_of(w, struct cached_dev, detach);
> - struct closure cl;
> -
> - closure_init_stack(&cl);
>
> BUG_ON(!test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags));
> BUG_ON(refcount_read(&dc->count));
> @@ -1144,12 +1141,6 @@ static void cached_dev_detach_finish(struct work_struct *w)
> dc->writeback_thread = NULL;
> }
>
> - memset(&dc->sb.set_uuid, 0, 16);
> - SET_BDEV_STATE(&dc->sb, BDEV_STATE_NONE);
> -
> - bch_write_bdev_super(dc, &cl);
> - closure_sync(&cl);
> -
> mutex_lock(&bch_register_lock);
>
> calc_cached_dev_sectors(dc->disk.c);
> diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
> index 4f4ad6b..2cd0340 100644
> --- a/drivers/md/bcache/writeback.c
> +++ b/drivers/md/bcache/writeback.c
> @@ -705,6 +705,15 @@ static int bch_writeback_thread(void *arg)
> * bch_cached_dev_detach().
> */
> if (test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags)) {
> + struct closure cl;
> +
> + closure_init_stack(&cl);
> + memset(&dc->sb.set_uuid, 0, 16);
> + SET_BDEV_STATE(&dc->sb, BDEV_STATE_NONE);
> +
> + bch_write_bdev_super(dc, &cl);
> + closure_sync(&cl);
> +
> up_write(&dc->writeback_lock);
> break;
> }
>
