Re: [PATCH] iomap: iomap_write_end cleanup

[Christoph Hellwig <hch@infradead.org>]

On Wed, May 04, 2022 at 01:27:36AM +0100, Matthew Wilcox wrote:
> 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 kinda hate the bools for something that is not a simple

	if (foo()))
		return;

as propagating them is a bit of a mess.  I do however thing that
switching to 0 / -errno might work nicely here, completely untested
patch below:

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 8bc0989cf447fa..764174e2f1a183 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -685,13 +685,13 @@ static size_t __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
 	 * redo the whole thing.
 	 */
 	if (unlikely(copied < len && !folio_test_uptodate(folio)))
-		return 0;
+		return -EIO;
 	iomap_set_range_uptodate(folio, iop, offset_in_folio(folio, pos), len);
 	filemap_dirty_folio(inode->i_mapping, folio);
-	return copied;
+	return 0;
 }
 
-static size_t iomap_write_end_inline(const struct iomap_iter *iter,
+static void iomap_write_end_inline(const struct iomap_iter *iter,
 		struct folio *folio, loff_t pos, size_t copied)
 {
 	const struct iomap *iomap = &iter->iomap;
@@ -706,23 +706,22 @@ static size_t iomap_write_end_inline(const struct iomap_iter *iter,
 	kunmap_local(addr);
 
 	mark_inode_dirty(iter->inode);
-	return copied;
 }
 
-/* Returns the number of bytes copied.  May be 0.  Cannot be an errno. */
-static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len,
+static int iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len,
 		size_t copied, struct folio *folio)
 {
 	const struct iomap_page_ops *page_ops = iter->iomap.page_ops;
 	const struct iomap *srcmap = iomap_iter_srcmap(iter);
 	loff_t old_size = iter->inode->i_size;
-	size_t ret;
+	int ret = 0;
 
 	if (srcmap->type == IOMAP_INLINE) {
-		ret = iomap_write_end_inline(iter, folio, pos, copied);
+		iomap_write_end_inline(iter, folio, pos, copied);
 	} else if (srcmap->flags & IOMAP_F_BUFFER_HEAD) {
-		ret = block_write_end(NULL, iter->inode->i_mapping, pos, len,
-				copied, &folio->page, NULL);
+		if (block_write_end(NULL, iter->inode->i_mapping, pos, len,
+				    copied, &folio->page, NULL) < len)
+			ret = -EIO;
 	} else {
 		ret = __iomap_write_end(iter->inode, pos, len, copied, folio);
 	}
@@ -732,8 +731,8 @@ 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) {
-		i_size_write(iter->inode, pos + ret);
+	if (!ret && pos + len > old_size) {
+		i_size_write(iter->inode, pos + len);
 		iter->iomap.flags |= IOMAP_F_SIZE_CHANGED;
 	}
 	folio_unlock(folio);
@@ -741,10 +740,11 @@ static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len,
 	if (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);
+		page_ops->page_done(iter->inode, pos, ret ? ret : len,
+				    &folio->page);
 	folio_put(folio);
 
-	if (ret < len)
+	if (ret)
 		iomap_write_failed(iter->inode, pos, len);
 	return ret;
 }
@@ -754,7 +754,7 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 	loff_t length = iomap_length(iter);
 	loff_t pos = iter->pos;
 	ssize_t written = 0;
-	long status = 0;
+	int status = 0;
 
 	do {
 		struct folio *folio;
@@ -792,26 +792,24 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		copied = copy_page_from_iter_atomic(page, offset, bytes, i);
 
 		status = iomap_write_end(iter, pos, bytes, copied, folio);
-
-		if (unlikely(copied != status))
-			iov_iter_revert(i, copied - status);
-
-		cond_resched();
-		if (unlikely(status == 0)) {
+		if (unlikely(status)) {
 			/*
 			 * A short copy made iomap_write_end() reject the
 			 * thing entirely.  Might be memory poisoning
 			 * halfway through, might be a race with munmap,
 			 * might be severe memory pressure.
 			 */
-			if (copied)
+			if (copied) {
+				iov_iter_revert(i, copied);
 				bytes = copied;
+			}
 			goto again;
 		}
-		pos += status;
-		written += status;
-		length -= status;
+		pos += bytes;
+		written += bytes;
+		length -= bytes;
 
+		cond_resched();
 		balance_dirty_pages_ratelimited(iter->inode->i_mapping);
 	} while (iov_iter_count(i) && length);
 
@@ -844,7 +842,6 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter)
 	const struct iomap *srcmap = iomap_iter_srcmap(iter);
 	loff_t pos = iter->pos;
 	loff_t length = iomap_length(iter);
-	long status = 0;
 	loff_t written = 0;
 
 	/* don't bother with blocks that are not shared to start with */
@@ -858,20 +855,21 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter)
 		unsigned long offset = offset_in_page(pos);
 		unsigned long bytes = min_t(loff_t, PAGE_SIZE - offset, length);
 		struct folio *folio;
+		int status;
 
 		status = iomap_write_begin(iter, pos, bytes, &folio);
 		if (unlikely(status))
 			return status;
 
 		status = iomap_write_end(iter, pos, bytes, bytes, folio);
-		if (WARN_ON_ONCE(status == 0))
-			return -EIO;
+		if (WARN_ON_ONCE(status))
+			return status;
 
 		cond_resched();
 
-		pos += status;
-		written += status;
-		length -= status;
+		pos += bytes;
+		written += bytes;
+		length -= bytes;
 
 		balance_dirty_pages_ratelimited(iter->inode->i_mapping);
 	} while (length);
@@ -925,9 +923,9 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 		folio_zero_range(folio, offset, bytes);
 		folio_mark_accessed(folio);
 
-		bytes = iomap_write_end(iter, pos, bytes, bytes, folio);
-		if (WARN_ON_ONCE(bytes == 0))
-			return -EIO;
+		status = iomap_write_end(iter, pos, bytes, bytes, folio);
+		if (WARN_ON_ONCE(status))
+			return status;
 
 		pos += bytes;
 		length -= bytes;