Re: [PATCH] iomap: iomap_write_end cleanup

[Matthew Wilcox <willy@infradead.org>]

On Tue, May 03, 2022 at 04:02:26PM -0700, Darrick J. Wong wrote:
> On Wed, May 04, 2022 at 12:15:45AM +0200, Andreas Gruenbacher wrote:
> > On Tue, May 3, 2022 at 11:53 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > On Tue, May 03, 2022 at 11:37:27PM +0200, Andreas Gruenbacher wrote:
> > > > In iomap_write_end(), only call iomap_write_failed() on the byte range
> > > > that has failed.  This should improve code readability, but doesn't fix
> > > > an actual bug because iomap_write_failed() is called after updating the
> > > > file size here and it only affects the memory beyond the end of the
> > > > file.
> > >
> > > I can't find a way to set 'ret' to anything other than 0 or len.  I know
> > > the code is written to make it look like we can return a short write,
> > > but I can't see a way to do it.
> > 
> > Good point, but that doesn't make the code any less confusing in my eyes.
> 
> Not to mention it leaves a logic bomb if we ever /do/ start returning
> 0 < ret < len.

This is one of the things I noticed when folioising iomap and didn't
get round to cleaning up, but I feel like we should change the calling
convention here to bool (true = success, false = fail).  Changing
block_write_end() might not be on the cards, unless someone's really
motivated, but we can at least change iomap_write_end() to not have this
stupid calling convention.

I mean, I won't NAK this patch, it is somewhat better with it than without
it, but it perpetuates the myth that this is in some way ever going to
happen, and the code could be a lot simpler if we stopped pretending.