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
