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

[dongsheng.yang@linux.dev]

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
> > >
> >
>