linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: "Darrick J. Wong" <darrick.wong@oracle.com>,
	Matthew Wilcox <willy@infradead.org>,
	"Theodore Y. Ts'o" <tytso@mit.edu>,
	linux-fsdevel@vger.kernel.org, fstests@vger.kernel.org,
	xfs <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH] fs: clear writeback errors in inode_init_always
Date: Wed, 23 May 2018 08:09:26 +1000	[thread overview]
Message-ID: <20180522220926.GW10363@dastard> (raw)
In-Reply-To: <bb659d80a9ed7c50199ffd4976cdcc48d3cd0174.camel@kernel.org>

On Tue, May 22, 2018 at 06:30:40AM -0400, Jeff Layton wrote:
> On Mon, 2018-05-21 at 10:54 -0700, Darrick J. Wong wrote:
> > On Sat, May 19, 2018 at 08:36:19AM -0700, Matthew Wilcox wrote:
> > > On Sat, May 19, 2018 at 08:27:00AM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > In inode_init_always(), we clear the inode mapping flags, which clears
> > > > any retained error (AS_EIO, AS_ENOSC) bits.  Unfortunately, we do not
> > > 
> > > typo of ENOSPC in case you do a new version
> > 
> > Fixed, thanks.
> > 
> > > > also clear wb_err, which means that old mapping errors can leak through
> > > > to new inodes.
> > > > 
> > > > This is crucial for the XFS inode allocation path because we recycle old
> > > > in-core inodes and we do not want error state from an old file to leak
> > > > into the new file.  This bug was discovered by running generic/036 and
> > > > generic/047 in a loop and noticing that the EIOs generated by the
> > > > collision of direct and buffered writes in generic/036 would survive the
> > > > remount between 036 and 047, and get reported to the fsyncs (on
> > > > different files on a reformatted fs!) in generic/047.
> > > > 
> > > > Since we're changing the semantics of inode_init_always, we must also
> > > > change xfs_reinit_inode to retain the writeback error state when we go
> > > > to recover an inode that has been torn down in the vfs but not yet
> > > > disposed of by XFS.
> > > 
> > > Don't you also need to preserve inode->i_mapping->flags across the
> > > reinitialisation for the users which aren't yet using ->wb_err?
> > 
> > At first I thought (mistakenly) that xfs had been completely converted,
> > but digging further we still use the old filemap_check_errors.  It seems
> > reasonable to me that if we're going to resurrect an incore inode then
> > we should try to hang on to AS_EIO/AS_ENOSPC for as long as we can.
> > 
> > 
> 
> Yes, I think the patch you sent makes sense.
> 
> Most of the XFS callers are using filemap_write_and_wait{_range}, and
> most of those seem to be called from ioctls (->setattr op being the
> notable exception).
> 
> We could (in principle) pass a pointer to file->f_wb_err to most of
> those callers, and use that to check for errors instead of looking for
> AS_EIO/AS_ENOSPC. We wouldn't want to advance the error cursor for those
> (as that should only be done in the context of an fsync), but it might
> be more reliable than using the flags here.

Why wouldn't we advance the error pointer? The data writeback error
caused an operation to fail and the error has been reported to
userspace, so how is that different to fsync() failing and reporting
the error to userspace?

This seems like a slippery slope of inconsistency to me, where data
writeback errors are only reported once on fsync(), but can be
reported multiple times through different filesystem operations...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply	other threads:[~2018-05-22 22:09 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-18 22:50 commit b4678df184b causing xfstests regressions Theodore Y. Ts'o
2018-05-19  2:17 ` Matthew Wilcox
2018-05-19 13:09 ` Jeff Layton
2018-05-19 15:25   ` Darrick J. Wong
2018-05-19 15:27   ` [PATCH] fs: clear writeback errors in inode_init_always Darrick J. Wong
2018-05-19 15:36     ` Matthew Wilcox
2018-05-21 17:54       ` Darrick J. Wong
2018-05-22 10:30         ` Jeff Layton
2018-05-22 22:09           ` Dave Chinner [this message]
2018-05-23 10:56             ` Jeff Layton
2018-05-24  3:59               ` Dave Chinner
2018-05-19 23:19     ` Theodore Y. Ts'o
2018-05-20 11:45       ` Jeff Layton
2018-05-20 12:58         ` Matthew Wilcox
2018-05-20 13:18           ` Jeff Layton
2018-05-20 16:29           ` Theodore Y. Ts'o
2018-05-20 19:20             ` Matthew Wilcox
2018-05-20 19:41               ` Theodore Y. Ts'o
2018-05-21 11:20                 ` Jeff Layton
2018-05-21 14:43                   ` Theodore Y. Ts'o
2018-05-20 17:57         ` Theodore Y. Ts'o
2018-05-22  4:06     ` [PATCH v2] " Darrick J. Wong
2018-05-22 10:14       ` Jeff Layton
2018-05-22 12:14       ` Brian Foster
2018-05-22 14:37         ` Darrick J. Wong
2018-05-22 16:43     ` [PATCH v3] " Darrick J. Wong
2018-05-22 18:40       ` Brian Foster
2018-05-22 18:47         ` Darrick J. Wong
2018-05-22 22:05       ` Dave Chinner
2018-05-23  3:00         ` Darrick J. Wong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180522220926.GW10363@dastard \
    --to=david@fromorbit.com \
    --cc=darrick.wong@oracle.com \
    --cc=fstests@vger.kernel.org \
    --cc=jlayton@kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=tytso@mit.edu \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH] fs: clear writeback errors in inode_init_always' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).