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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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.

> Also, you snipped the rest of my feedback... how does the fact that the
> caller doesn't care about errors have anything to do with the fact that
> the existing logic within this function does? I'm not convinced the
> changes here are quite correct, but at the very least the commit log
> description is lacking/misleading.

I was wondering that too -- if filemap_flush, we'd stop immediately, but
now we keep going.  That certainly seems like a behavior change that
ought to be a separate patch from combining the two release functions?

--D

> Brian