Re: [PATCH] iomap: iomap_write_end cleanup

[Andreas Gruenbacher <agruenba@redhat.com>]

On Wed, May 4, 2022 at 2:27 AM Matthew Wilcox <willy@infradead.org> wrote:
> 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.

Hmm, I don't really see how this would make things significantly
simpler.  Trying it out made me notice the following problem, though.
Any thoughts?

Of course this was copied from generic_write_end(), and so we have the same
issue there as well as in nobh_write_end().

Thanks,
Andreas

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 8fb9b2797fc5..1938dbbda1c0 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -721,13 +721,13 @@ static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len,
 	 * cache.  It's up to the file system to write the updated size to disk,
 	 * preferably after I/O completion so that no stale data is exposed.
 	 */
-	if (pos + ret > old_size) {
+	if (ret && pos + ret > old_size) {
 		i_size_write(iter->inode, pos + ret);
 		iter->iomap.flags |= IOMAP_F_SIZE_CHANGED;
 	}
 	folio_unlock(folio);

-	if (old_size < pos)
+	if (ret && old_size < pos)
 		pagecache_isize_extended(iter->inode, old_size, pos);
 	if (page_ops && page_ops->page_done)
 		page_ops->page_done(iter->inode, pos, ret, &folio->page);