Re: [xfstests PATCH v3 1/5] generic: add a writeback error handling test

[Darrick J. Wong]

On Tue, Jun 06, 2017 at 04:12:58PM -0400, Jeff Layton wrote:
> On Tue, 2017-06-06 at 10:17 -0700, Darrick J. Wong wrote:
> > On Tue, Jun 06, 2017 at 08:23:25PM +0800, Eryu Guan wrote:
> > > On Tue, Jun 06, 2017 at 06:15:57AM -0400, Jeff Layton wrote:
> > > > On Tue, 2017-06-06 at 16:58 +0800, Eryu Guan wrote:
> > > > > On Wed, May 31, 2017 at 09:08:16AM -0400, Jeff Layton wrote:
> > > > > > I'm working on a set of kernel patches to change how writeback errors
> > > > > > are handled and reported in the kernel. Instead of reporting a
> > > > > > writeback error to only the first fsync caller on the file, I aim
> > > > > > to make the kernel report them once on every file description.
> > > > > > 
> > > > > > This patch adds a test for the new behavior. Basically, open many fds
> > > > > > to the same file, turn on dm_error, write to each of the fds, and then
> > > > > > fsync them all to ensure that they all get an error back.
> > > > > > 
> > > > > > To do that, I'm adding a new tools/dmerror script that the C program
> > > > > > can use to load the error table. For now, that's all it can do, but
> > > > > > we can fill it out with other commands as necessary.
> > > > > > 
> > > > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > > > 
> > > > > Thanks for the new tests! And sorry for the late review..
> > > > > 
> > > > > It's testing a new behavior on error reporting on writeback, I'm not
> > > > > sure if we can call it a new feature or it fixed a bug? But it's more
> > > > > like a behavior change, I'm not sure how to categorize it.
> > > > > 
> > > > > Because if it's testing a new feature, we usually let test do proper
> > > > > detection of current test environment (based on actual behavior not
> > > > > kernel version) and _notrun on filesystems that don't have this feature
> > > > > yet, instead of failing the test; if it's testing a bug fix, we could
> > > > > leave the test fail on unfixed filesystems, this also serves as a
> > > > > reminder that there's bug to fix.
> > > > > 
> > > > 
> > > > Thanks for the review! I'm not sure how to categorize this either. Since
> > > > the plan is to convert all the filesystems piecemeal, maybe we should
> > > > just consider it a new feature.
> > > 
> > > Then we need a new _require rule to properly detect for the 'feature'
> > > support. I'm not sure if this is doable, but something like
> > > _require_statx, _require_seek_data_hole would be good.
> > > 
> > > > 
> > > > > I pulled your test kernel tree, and test passed on EXT4 but failed on
> > > > > other local filesystems (XFS, btrfs). I assume that's expected.
> > > > > 
> > > > > Besides this kinda high-level question, some minor comments inline.
> > > > > 
> > > > 
> > > > Yes, ext4 should pass on my latest kernel tree, but everything else
> > > > should fail. 
> 
> Oh, and I should mention that ext2/3 also pass when mounted using ext4
> driver. Legacy ext2 driver sort of works, but it reports a few too many
> errors because of the way the ext2_error macro works. That shouldn't be
> too hard to fix, I just need some guidance on that one.
> 
> I had xfs and btrfs working with an earlier iteration of the patches,
> but now that we're converting a fs at a time, it's a little more work to
> get there. It shouldn't be too hard to do though. I'll probably re-post
> in a few days, and will try to take a stab at XFS and btrfs conversion
> too.
> 
> > > 
> > > With the new _require rule, test should _notrun on XFS and btrfs then.
> > 
> > Frankly I personally prefer that upstream XFS fails until someone fixes it. :)
> > (But that's just my opinion.)
> > 
> > That said, I'm not 100% sure what's required of XFS to play nicely with
> > this new mechanism -- glancing at the ext* patches it looks like we'd
> > need to set a fs flag and possibly change some or all of the "write
> > cached dirty buffers out to disk" calls to their _since variants?
> 
> Yeah, that's pretty much the size of it.
> 
> In fact, the latter part (changing to the _since variants) is somewhat
> optional. We can have the errseq_t based tracking coexist with the
> AS_EIO/AS_ENOSPC flags. It's weird but I don't see a real downside to
> preserving them until we've got more of this converted over.
> 
> In the latest branch I'm working on, I'm breaking up those changes into
> different patches so it should be a little clearer for other fs
> maintainers to see what I'm doing and why. Stay tuned...

Ok.

> > Metadata writeback errors are handled by retrying writes and/or shutting
> > down the fs, so I think the f_md_wb_error case is already covered.
> 
> Thanks. I think we do need f_md_wb_err for ext2/4 though, IIUC?

Yes.  Sorry, the previous statement applies only to XFS.

> > That said, I agree that it's useful to detect that the kernel simply
> > lacks any of the new wb error reporting at all, so therefore we can skip
> > the tests.
> > 
> 
> Suggestions on ways to implement such a check would be welcome. Maybe a
> file in /sys or in debugfs?

Assuming that this patchset applies the same wb error reporting behavior
to block devices, you could open a bunch of file descriptors to a linear
dm target sitting atop $SCRATCH_DEV, switch out the table to dm_error,
then write something, fsync, and see if we get more than one EIO.  Then
you'd know if the kernel supports it, at least... I think?

--D

> 
> -- 
> Jeff Layton <jlayton@xxxxxxxxxx>