Re: [PATCH 1/2] xfs: remove xfs_release

[Dave Chinner <david@fromorbit.com>]

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@fromorbit.com