On Thu, Jun 01, 2023 at 05:25:00PM -0700, Darrick J. Wong wrote: > On Tue, May 30, 2023 at 10:19:28AM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@xxxxxxxxxx> > > > > Unlinked list recovery requires errors removing the inode the from > > the unlinked list get fed back to the main recovery loop. Now that > > we offload the unlinking to the inodegc work, we don't get errors > > being fed back when we trip over a corruption that prevents the > > inode from being removed from the unlinked list. > > > > This means we never clear the corrupt unlinked list bucket, > > resulting in runtime operations eventually tripping over it and > > shutting down. > > > > Fix this by collecting inodegc worker errors and feed them > > back to the flush caller. This is largely best effort - the only > > context that really cares is log recovery, and it only flushes a > > single inode at a time so we don't need complex synchronised > > handling. Essentially the inodegc workers will capture the first > > error that occurs and the next flush will gather them and clear > > them. The flush itself will only report the first gathered error. > > > > In the cases where callers can return errors, propagate the > > collected inodegc flush error up the error handling chain. > > > > In the case of inode unlinked list recovery, there are several > > superfluous calls to flush queued unlinked inodes - > > xlog_recover_iunlink_bucket() guarantees that it has flushed the > > inodegc and collected errors before it returns. Hence nothing in the > > calling path needs to run a flush, even when an error is returned. > > Hmm. So I guess what you're saying is that xfs_inactive now returns > negative errnos, and everything that calls down to that function will > pass the error upwards through the stack? Yes. Effectively inodegc workers capture the errors from xfs_inactive(), and the next xfs_inode_flush() call will gather the errors, clear them and report them to the caller. It's then up to the caller to decide what to do with an error from xfs_inode_flush()... > Any of those call paths that already could handle a negative errno will > now fail on a corrupt inactive inode; and the only place that will > silently "drop" the negative errno is unmount? Yes. That is the largely the what I've tried to do. The main thing was to make the unlinked inode error reporting work, and having other paths (like remount,ro) also be able to fail and propagate errors is a bonus. > If 'yes' and 'yes' and the kbuild robot warnings get fixed, > Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx> Thanks! -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
