On Wed, Sep 18, 2019 at 11:21:35AM -0700, Darrick J. Wong wrote: > On Wed, Sep 18, 2019 at 02:12:04PM -0400, Brian Foster wrote: > > On Wed, Sep 18, 2019 at 06:49:38PM +0200, Christoph Hellwig wrote: > > > On Mon, Sep 16, 2019 at 08:53:11AM -0400, Brian Foster wrote: > > > > The caller might not care if this call generates errors, but shouldn't > > > > we care if something fails? IOW, perhaps we should have an exit path > > > > with a WARN_ON_ONCE() or some such to indicate that an unhandled error > > > > has occurred..? > > > > > > Not sure there is much of a point. Basically all errors are either > > > due to a forced shutdown or cause a forced shutdown anyway, so we'll > > > already get warnings. > > > > Well, what's the point of this change in the first place? I see various > > error paths that aren't directly related to shutdown. A writeback > > submission error for instance looks like it will warn, but not > > necessarily shut down (and the filemap_flush() call is already within a > > !XFS_FORCED_SHUTDOWN() check). So not all errors are associated with or > > cause shutdown. I suppose you could audit the various error paths that > > lead back into this function and document that further if you really > > wanted to go that route... > > I agree with Brian, there ought to be some kind of warning that some > error happened with inode XXX even if we do end up shutting down > immediately afterwards. FWIW, we have precedence for that - see xfs_inactive_ifree(), which logs errors noisily because we can't return errors to the VFS inode teardown path (i.e. evict -> destroy_inode -> xfs_fs_destroy_inode -> xfs_inactive path). These were originally added because a static error return checker flagged xfs_inactive() as a place where errors were silently ignored and users had no indication that somethign bad had happened to their file. -> release -> xfs_file_release -> xfs_release is no different in this respect.... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
