Re: [PATCH] xfs: check return codes when flushing block devices

[Christoph Hellwig <hch@infradead.org>]

On Mon, Aug 01, 2022 at 10:06:41AM +1000, Dave Chinner wrote:
> > @@ -173,8 +175,11 @@ xfs_file_fsync(
> >  	 * that happen concurrently to the fsync call, but fsync semantics
> >  	 * only require to sync previously completed I/O.
> >  	 */
> > -	if (xfs_ipincount(ip))
> > +	if (xfs_ipincount(ip)) {
> >  		error = xfs_fsync_flush_log(ip, datasync, &log_flushed);
> > +		if (error)
> > +			return error;
> > +	}
> 
> Shouldn't we still try to flush the data device if necessary, even
> if the log flush failed?

xfs_fsync_flush_log ails only if the log it shut down.  Does it really
make sense to flush the data cache for a pure overwrite of a data
block when the fs is toast?  I can't really see any benefit in that.

> > +		if (log->l_targ != log->l_mp->m_ddev_targp &&
> > +		    blkdev_issue_flush(log->l_mp->m_ddev_targp->bt_bdev)) {
> > +			xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
> > +			return;
> > +		}
> 
> That seems pretty drastic, though I'm not sure what else apart from
> ignoring the data device flush error can be done here. Also, it's
> not actually a log IO error - it's a data device IO error so it's a
> really a metadata writeback problem. Hence the use of
> SHUTDOWN_LOG_IO_ERROR probably needs a comment to explain why it
> needs to be used here...

Yes, the comment would be useful.  But if a cache flush fails data
integrity of the device must at this point be considered as fucked
up beyond belief, so shutting down the log and thus the file system
is the right thing to do.

So modulo a comment here the patch looks good to me.