From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ipmail07.adl2.internode.on.net ([150.101.137.131]:29934 "EHLO ipmail07.adl2.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753718AbeDSXrr (ORCPT ); Thu, 19 Apr 2018 19:47:47 -0400 Date: Fri, 20 Apr 2018 09:47:45 +1000 From: Dave Chinner To: Jeff Layton Cc: linux-fsdevel , lsf-pc@lists.linux-foundation.org, Andres Freund , Matthew Wilcox Subject: Re: [Lsf-pc] [LSF/MM TOPIC] improving writeback error handling Message-ID: <20180419234745.GN27893@dastard> References: <1523963281.4779.21.camel@kernel.org> <20180417225309.GA27893@dastard> <1524067210.27056.28.camel@kernel.org> <20180419004411.GG27893@dastard> <1524158044.2943.17.camel@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1524158044.2943.17.camel@kernel.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Apr 19, 2018 at 01:14:04PM -0400, Jeff Layton wrote: > On Thu, 2018-04-19 at 10:44 +1000, Dave Chinner wrote: > > On Wed, Apr 18, 2018 at 12:00:10PM -0400, Jeff Layton wrote: > > > > > 4) syncfs doesn't currently report an error when a single inode fails > > > > > writeback, only when syncing out the block device. Should it report > > > > > errors in that case as well? > > > > > > > > Yes. > > > > > > I have a small patch that implements this that I posted a few days ago. > > > I meant to mark it as an RFC, but didn't, fwiw. I'm not convinced that > > > it's the right approach. > > > > > > Instead of growing struct file to accommodate a second errseq cursor, > > > this only implements that behavior when the file is opened with O_PATH > > > (as we know that that will have the fsync op set to NULL). Maybe we can > > > do this better somehow though. > > > > No idea whether this is possible, or even a good idea, but could we > > just have syncfs() create a temporary struct file for the duration > > of the syncfs call, so it works on any fd passed in? (sorta like an > > internal dupfd() call?) > > > > No, we need something that will persist the errseq_t cursor between > syncfs calls. > > If we did what you're suggesting, once your temporary file goes away, > you'd lose your place in the error stream and you'd end up reporting > the same errors more than once on subsequent calls to syncfs. Which, IMO, is correct behaviour. If there is a persistent data writeback errors, then syncfs() should report it *every time it is called* because that syncfs() call did not result in the filesystem being fully committed to stable storage. I don't care whether the error has been reported before - the context of syncfs() is "commit the entire filesystem to stable storage". If any IO failed then we have not acheived what the application asked us to do and so we should be returning an error on every call that sees a data writeback error. Cheers, Dave. -- Dave Chinner david@fromorbit.com