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

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

[guolingxing]

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@vger.kernel.org
Signed-off-by: guolingxing <guolingxing@tju.edu.cn>
---
 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

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@tju.edu.cn mailto:guolingxing@tju.edu.cn?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@vger.kernel.org
> Signed-off-by: guolingxing <guolingxing@tju.edu.cn>
> ---
>  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
>

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

[dongsheng.yang]

Thanks for the explanation and the link. That addresses my question
about JBD2.

Reviewed-by: Dongsheng Yang <dongsheng.yang@linux.dev>



2026年10月15日 14:02, "郭玲兴" <guolingxing@tju.edu.cn mailto:guolingxing@tju.edu.cn?to=%22%E9%83%AD%E7%8E%B2%E5%85%B4%22%20%3Cguolingxing%40tju.edu.cn%3E > 写到:


> 
> Hi,
> 
> Thanks for reviewing the fix.
> 
> Yes, JBD2 has a similar error-handling gap. I submitted a patch for that exact issue earlier, and it has already received a Reviewed-by from the maintainer:
> 
> https://lore.kernel.org/all/20261012114713.4049391-1-guolingxing@tju.edu.cn/
> 
> The subsequent journal superblock write uses REQ_FUA, but that does not guarantee that the preceding checkpoint writes are durable, particularly when the journal is on a separate device.
> 
> It was while working on the JBD2 fix that I checked other blkdev_issue_flush() call sites and found similar omissions in dm-pcache and ext4. This dm-pcache patch came out of that investigation.
> 
> Thanks,
> lingxing
> 
> 发件人：dongsheng.yang@linux.dev
> 发送日期：2026-10-15 13:51:23
> 收件人：guolingxing <guolingxing@tju.edu.cn>,Zheng Gu <cengku@gmail.com>,dm-devel@lists.linux.dev
> 抄送人：Mikulas Patocka <mpatocka@redhat.com>,linux-kernel@vger.kernel.org,guolingxing <guolingxing@tju.edu.cn>,stable@vger.kernel.org
> 主题：Re: [PATCH] dm-pcache: do not advance dirty_tail after backing flush failure
> 
> > 
> > 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@tju.edu.cn mailto:guolingxing@tju.edu.cn?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@vger.kernel.org
> > > Signed-off-by: guolingxing <guolingxing@tju.edu.cn>
> > > ---
> > >  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
> > >
> >
>