Re: [PATCH] dm-pcache: do not advance dirty_tail after backing flush failure

[dongsheng . yang]

Hi lingxing,
Thanks for the fix. Keeping dirty_tail unchanged when the backing-device
flush fails looks correct to me.

One related question: jbd2_cleanup_journal_tail() also ignores the return
value of blkdev_issue_flush() before calling __jbd2_update_log_tail().
Do you know whether JBD2 has another mechanism that makes this safe, or
whether it has a similar error-handling gap?

This does not invalidate the dm-pcache fix, but understanding the
difference would be useful.

2026年10月14日 11:11, "guolingxing" <guolingxing@xxxxxxxxxx mailto:guolingxing@xxxxxxxxxx?to=%22guolingxing%22%20%3Cguolingxing%40tju.edu.cn%3E > 写到:


> 
> After all writeback bios for a kset complete, writeback_ctx_end()
> flushes the backing device and then advances the persistent dirty_tail.
> However, backing_dev_flush() discards the error returned by
> blkdev_issue_flush(), so dirty_tail is advanced even when the written
> data is not known to be durable.
> 
> dirty_tail points to the first kset that still requires writeback.
> Advancing it causes recovery to treat the preceding kset as clean and
> allows its cache copy to eventually be reclaimed by garbage collection.
> If the backing device flush failed, a subsequent power failure may
> therefore result in data loss.
> 
> Make backing_dev_flush() return the flush status and handle a flush
> failure like a writeback I/O failure. Leave dirty_tail unchanged so
> that the same kset will be retried.
> 
> Fixes: 1d57628ff95b ("dm-pcache: add persistent cache target in device-mapper")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: guolingxing <guolingxing@xxxxxxxxxx>
> ---
>  drivers/md/dm-pcache/backing_dev.c | 4 ++--
>  drivers/md/dm-pcache/backing_dev.h | 2 +-
>  drivers/md/dm-pcache/cache_writeback.c | 28 +++++++++++++++++++-------
>  3 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/md/dm-pcache/backing_dev.c b/drivers/md/dm-pcache/backing_dev.c
> index 7165fc0364bb..0c15bb01068e 100644
> --- a/drivers/md/dm-pcache/backing_dev.c
> +++ b/drivers/md/dm-pcache/backing_dev.c
> @@ -336,9 +336,9 @@ struct pcache_backing_dev_req *backing_dev_req_create(struct pcache_backing_dev
>  return backing_req;
>  }
>  
> -void backing_dev_flush(struct pcache_backing_dev *backing_dev)
> +int backing_dev_flush(struct pcache_backing_dev *backing_dev)
>  {
> - blkdev_issue_flush(backing_dev->dm_dev->bdev);
> + return blkdev_issue_flush(backing_dev->dm_dev->bdev);
>  }
>  
>  int pcache_backing_init(void)
> diff --git a/drivers/md/dm-pcache/backing_dev.h b/drivers/md/dm-pcache/backing_dev.h
> index b371cba483b9..8422e5e76e29 100644
> --- a/drivers/md/dm-pcache/backing_dev.h
> +++ b/drivers/md/dm-pcache/backing_dev.h
> @@ -120,7 +120,7 @@ struct pcache_backing_dev_req *backing_dev_req_alloc(struct pcache_backing_dev *
>  struct pcache_backing_dev_req_opts *opts);
>  void backing_dev_req_init(struct pcache_backing_dev_req *backing_req,
>  struct pcache_backing_dev_req_opts *opts);
> -void backing_dev_flush(struct pcache_backing_dev *backing_dev);
> +int backing_dev_flush(struct pcache_backing_dev *backing_dev);
>  
>  int pcache_backing_init(void);
>  void pcache_backing_exit(void);
> diff --git a/drivers/md/dm-pcache/cache_writeback.c b/drivers/md/dm-pcache/cache_writeback.c
> index c8a4c8110a58..e09560f172dd 100644
> --- a/drivers/md/dm-pcache/cache_writeback.c
> +++ b/drivers/md/dm-pcache/cache_writeback.c
> @@ -18,12 +18,18 @@ static void writeback_ctx_end(struct pcache_cache *cache, int ret)
>  return;
>  
>  if (!cache->writeback_ctx.ret) {
> - backing_dev_flush(cache->backing_dev);
> -
> - mutex_lock(&cache->dirty_tail_lock);
> - cache_pos_advance(&cache->dirty_tail, cache->writeback_ctx.advance);
> - cache_encode_dirty_tail(cache);
> - mutex_unlock(&cache->dirty_tail_lock);
> + ret = backing_dev_flush(cache->backing_dev);
> + if (ret) {
> + pcache_dev_err(CACHE_TO_PCACHE(cache),
> + "failed to flush backing device: %d", ret);
> + cache->writeback_ctx.ret = ret;
> + } else {
> + mutex_lock(&cache->dirty_tail_lock);
> + cache_pos_advance(&cache->dirty_tail,
> + cache->writeback_ctx.advance);
> + cache_encode_dirty_tail(cache);
> + mutex_unlock(&cache->dirty_tail_lock);
> + }
>  }
>  queue_delayed_work(cache_get_wq(cache), &cache->writeback_work, 0);
>  }
> @@ -76,8 +82,16 @@ static inline bool is_cache_clean(struct pcache_cache *cache, struct pcache_cach
>  
>  void cache_writeback_exit(struct pcache_cache *cache)
>  {
> + int ret;
> +
>  cancel_delayed_work_sync(&cache->writeback_work);
> - backing_dev_flush(cache->backing_dev);
> +
> + ret = backing_dev_flush(cache->backing_dev);
> + if (ret)
> + pcache_dev_err(CACHE_TO_PCACHE(cache),
> + "failed to flush backing device during exit: %d",
> + ret);
> +
>  cache_tree_exit(&cache->writeback_key_tree);
>  }
>  
> -- 
> 2.34.1
>