Re: [PATCH] iomap: Make sure iomap_end is called after iomap_begin

From: Dave Chinner <david@fromorbit.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: Andreas Gruenbacher <agruenba@redhat.com>,
	Christoph Hellwig <hch@infradead.org>,
	"Darrick J . Wong" <darrick.wong@oracle.com>,
	linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	Bob Peterson <rpeterso@redhat.com>
Subject: Re: [PATCH] iomap: Make sure iomap_end is called after iomap_begin
Date: Tue, 16 Jun 2020 10:39:03 +1000	[thread overview]
Message-ID: <20200616003903.GC2005@dread.disaster.area> (raw)
In-Reply-To: <20200615234437.GX8681@bombadil.infradead.org>

On Mon, Jun 15, 2020 at 04:44:37PM -0700, Matthew Wilcox wrote:
> On Tue, Jun 16, 2020 at 09:32:39AM +1000, Dave Chinner wrote:
> > On Mon, Jun 15, 2020 at 06:02:44PM +0200, Andreas Gruenbacher wrote:
> > > Make sure iomap_end is always called when iomap_begin succeeds: the
> > > filesystem may take locks in iomap_begin and release them in iomap_end,
> > > for example.
> > 
> > Ok, i get that from the patch, but I don't know anything else about
> > this problem, and nor will anyone else trying to determine if this
> > is a fix they need to backport to other kernels. Can you add some
> > more information to the commit message, such as how was this found
> > and what filesystems it affects? It would also be good to know what
> > commit introduced this issue and whether it need stable back ports
> > (i.e. a Fixes tag).
> 
> I'd assume Andreas is looking at converting a filesystem to use iomap,
> since this problem only occurs for filesystems which have returned an
> invalid extent.

Well, I can assume it's gfs2, but you know what happens when you
assume something....

> I almost wonder if this should return -EFSCORRUPTED rather than -EIO.

We've typically used -EFSCORRUPTED for metadata corruptions and -EIO
for data IO errors that result from metadata corruption. -EIO
implies that the IO failed and the state of the data is
indeterminate (i.e. may be corrupted), but the filesystem is still
ok, whereas EFSCORRUPTED implies the filesystem needs to have fsck
run on it to diagnose and fix the metadata corruption.

This code sort of spans the grey area between them. The error could
come from in in-memory bug and not actually a filesystem corruption,
so it's not clear that corruption is the right thing to report
here...

> Um, except that's currently a per-fs define.  Is it time to move that
> up to errno.h?

Has the "EFSCORRUPTED = EUCLEAN is stupid - let's break a
longstanding user API by defining it to something completely new"
yelling died down enough in the 6 months since this was last
proposed?

https://lore.kernel.org/linux-xfs/20191031010736.113783-1-Valdis.Kletnieks@vt.edu/
https://lore.kernel.org/linux-xfs/20191104014510.102356-11-Valdis.Kletnieks@vt.edu/

We've only been trying to get a generic -EFSCORRUPTED definition
into the kernel errno headers for ~15 years... :(

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com