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

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Aug 01, 2022 at 10:24:27AM -0700, Christoph Hellwig wrote:
> 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.

/me guesses that once we hit the first error, the rest of the calls will
probably crash and burn, and if they don't, the next fs call will, so it
likely doesn't matter if we keep going or not.

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

Ok, will do.

--D