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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Jun 16, 2020 at 03:57:08PM +0200, Andreas Gruenbacher wrote:
> On Tue, Jun 16, 2020 at 3:23 PM Matthew Wilcox <willy@infradead.org> wrote:
> > On Tue, Jun 16, 2020 at 08:17:28AM -0400, Bob Peterson wrote:
> > > ----- Original Message -----
> > > > > 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....
> > >
> > > Yes, it's gfs2, which already has iomap. I found the bug while just browsing
> > > the code: gfs2 takes a lock in the begin code. If there's an error,
> > > however unlikely, the end code is never called, so we would never unlock.
> > > It doesn't matter to me whether the error is -EIO because it's very unlikely
> > > in the first place. I haven't looked back to see where the problem was
> > > introduced, but I suspect it should be ported back to stable releases.
> >
> > It shouldn't just be "unlikely", it should be impossible.  This is the
> > iomap code checking whether you've returned an extent which doesn't cover
> > the range asked for.  I don't think it needs to be backported, and I'm
> > pretty neutral on whether it needs to be applied.
> 
> Right, when these warnings trigger, the filesystem has already screwed
> up; this fix only makes things less bad. Those kinds of issues are
> very likely to be fixed long before the code hits users, so it
> shouldn't be backported.
> 
> This bug was in iomap_apply right from the start, so:
> 
> Fixes: ae259a9c8593 ("fs: introduce iomap infrastructure")

So... you found this through code inspection, and not because you
actually hit this on gfs2, or any of the other iomap users?

I generally think this looks ok, but I want to know if I should be
looking deeper. :)

--D

> Thanks,
> Andreas
>