Re: [PATCH] Bcache: don't return BLK_STS_IOERR during cache detach

[Coly Li <colyli@suse.de>]

On 3/10/22 10:50 AM, Zhang Zhen wrote:
> Before this patch, if cache device missing, cached_dev_submit_bio 
> return io err
> to fs during cache detach, randomly lead to xfs do force shutdown.
>
> This patch delay the cache io submit in cached_dev_submit_bio
> and wait for cache set detach finish.
> So if the cache device become missing, bcache detach cache set 
> automatically,
> and the io will sumbit normally.
>
> Feb  2 20:59:23  kernel: bcache: bch_count_io_errors() nvme0n1p56: IO 
> error on writing btree.
> Feb  2 20:59:23  kernel: bcache: bch_count_io_errors() nvme0n1p57: IO 
> error on writing btree.
> Feb  2 20:59:23  kernel: bcache: bch_count_io_errors() nvme0n1p56: IO 
> error on writing btree.
> Feb  2 20:59:23  kernel: bcache: bch_btree_insert() error -5
> Feb  2 20:59:23  kernel: XFS (bcache43): metadata I/O error in 
> "xfs_buf_iodone_callback_error" at daddr 0x80034658 len 32 error 12
> Feb  2 20:59:23  kernel: bcache: bch_btree_insert() error -5
> Feb  2 20:59:23  kernel: bcache: bch_btree_insert() error -5
> Feb  2 20:59:23  kernel: bcache: bch_btree_insert() error -5
> Feb  2 20:59:23  kernel: bcache: bch_btree_insert() error -5
> Feb  2 20:59:23  kernel: bcache: bch_cache_set_error() bcache: error 
> on 004f8aa7-561a-4ba7-bf7b-292e461d3f18:
> Feb  2 20:59:23  kernel: journal io error
> Feb  2 20:59:23  kernel: bcache: bch_cache_set_error() , disabling 
> caching
> Feb  2 20:59:23  kernel: bcache: bch_btree_insert() error -5
> Feb  2 20:59:23  kernel: bcache: conditional_stop_bcache_device() 
> stop_when_cache_set_failed of bcache43 is "auto" and cache is clean, 
> keep it alive.
> Feb  2 20:59:23  kernel: XFS (bcache43): metadata I/O error in 
> "xlog_iodone" at daddr 0x400123e60 len 64 error 12
> Feb  2 20:59:23  kernel: XFS (bcache43): xfs_do_force_shutdown(0x2) 
> called from line 1298 of file fs/xfs/xfs_log.c. Return address = 
> 00000000c1c8077f
> Feb  2 20:59:23  kernel: XFS (bcache43): Log I/O Error Detected. 
> Shutting down filesystem
> Feb  2 20:59:23  kernel: XFS (bcache43): Please unmount the filesystem 
> and rectify the problem(s)
>
> Signed-off-by: Zhen Zhang <zhangzhen.email@gmail.com>
> ---
>  drivers/md/bcache/bcache.h  | 5 -----
>  drivers/md/bcache/request.c | 8 ++++----
>  drivers/md/bcache/super.c   | 3 ++-
>  3 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> index 9ed9c955add7..e5227dd08e3a 100644
> --- a/drivers/md/bcache/bcache.h
> +++ b/drivers/md/bcache/bcache.h
> @@ -928,11 +928,6 @@ static inline void closure_bio_submit(struct 
> cache_set *c,
>                        struct closure *cl)
>  {
>      closure_get(cl);
> -    if (unlikely(test_bit(CACHE_SET_IO_DISABLE, &c->flags))) {
> -        bio->bi_status = BLK_STS_IOERR;
> -        bio_endio(bio);
> -        return;
> -    }
>      submit_bio_noacct(bio);
>  }


Comparing to make bcache device living as it looks like, avoiding data 
corruption or stale is much more critical. Therefore once there is 
critical device failure detected, the following I/O requests must be 
stopped (especially write request) to avoid further data corruption. 
Without the above checking for CACHE_SET_IO_DISABLE, the cache has to be 
attached until there is no I/O. For a busy system it should be quite 
long time. Then users may encounter silent data corruption or 
inconsistency after a long time since hardware failed.


Again, with the above change, you may introduce other issue which more 
hard to detect.


>  diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> index d15aae6c51c1..36f0ee95b51f 100644
> --- a/drivers/md/bcache/request.c
> +++ b/drivers/md/bcache/request.c
> @@ -13,6 +13,7 @@
>  #include "request.h"
>  #include "writeback.h"
>  +#include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/hash.h>
>  #include <linux/random.h>
> @@ -1172,11 +1173,10 @@ void cached_dev_submit_bio(struct bio *bio)
>      unsigned long start_time;
>      int rw = bio_data_dir(bio);
>  -    if (unlikely((d->c && test_bit(CACHE_SET_IO_DISABLE, 
> &d->c->flags)) ||
> +    while (unlikely((d->c && test_bit(CACHE_SET_IO_DISABLE, 
> &d->c->flags)) ||
>               dc->io_disable)) {
> -        bio->bi_status = BLK_STS_IOERR;
> -        bio_endio(bio);
> -        return;
> +        /* wait for detach finish and d->c == NULL. */
> +        msleep(2);
>      }

This is unacceptible, neither the infinite loop nor the msleep. You 
cannot solve the target issue by an infinite retry, such method will 
introduce more issue from other place.


>       if (likely(d->c)) {
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 140f35dc0c45..8d9a5e937bc8 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -661,7 +661,8 @@ int bch_prio_write(struct cache *ca, bool wait)
>          p->csum        = bch_crc64(&p->magic, 
> meta_bucket_bytes(&ca->sb) - 8);
>           bucket = bch_bucket_alloc(ca, RESERVE_PRIO, wait);
> -        BUG_ON(bucket == -1);
> +        if (bucket == -1)
> +            return -1;

This change is wrong. bucket == -1 indicates the bucket allocator 
doesn't work properly, if it happens something really critical 
happening. This is why BUG_ON is used here.

With the above change, you will encounter other strange issue sooner or 
later and hard to tell the root cause.


> mutex_unlock(&ca->set->bucket_lock);
>          prio_io(ca, bucket, REQ_OP_WRITE, 0);


Currently I don't have clear idea on how to avoid the IO error return 
value during cache set detaching for a failed cache device. But it 
cannot be such simple change by the this patch.


Coly Li