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;
