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 >
