Thanks for the explanation and the link. That addresses my question about JBD2.
Reviewed-by: Dongsheng Yang <[email protected]> 2026年10月15日 14:02, "郭玲兴" <[email protected] mailto:[email protected]?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/[email protected]/ > > 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 > > 发件人：[email protected] > 发送日期：2026-10-15 13:51:23 > 收件人：guolingxing <[email protected]>,Zheng Gu > <[email protected]>,[email protected] > 抄送人：Mikulas Patocka > <[email protected]>,[email protected],guolingxing > <[email protected]>,[email protected] > 主题：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" <[email protected] > > mailto:[email protected]?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: [email protected] > > > Signed-off-by: guolingxing <[email protected]> > > > --- > > > 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 > > > > > >
