* [PATCH] iomap: iomap_write_end cleanup @ 2022-05-03 21:37 Andreas Gruenbacher 2022-05-03 21:52 ` Matthew Wilcox 2022-05-04 14:13 ` Christoph Hellwig 0 siblings, 2 replies; 9+ messages in thread From: Andreas Gruenbacher @ 2022-05-03 21:37 UTC (permalink / raw) To: Christoph Hellwig, Darrick J . Wong Cc: Linus Torvalds, linux-xfs, linux-fsdevel, Andreas Gruenbacher 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. Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> --- fs/iomap/buffered-io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 358ee1fb6f0d..8fb9b2797fc5 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -734,7 +734,7 @@ static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len, folio_put(folio); if (ret < len) - iomap_write_failed(iter->inode, pos, len); + iomap_write_failed(iter->inode, pos + ret, len - ret); return ret; } -- 2.35.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] iomap: iomap_write_end cleanup 2022-05-03 21:37 [PATCH] iomap: iomap_write_end cleanup Andreas Gruenbacher @ 2022-05-03 21:52 ` Matthew Wilcox 2022-05-03 22:15 ` Andreas Gruenbacher 2022-05-04 14:13 ` Christoph Hellwig 1 sibling, 1 reply; 9+ messages in thread From: Matthew Wilcox @ 2022-05-03 21:52 UTC (permalink / raw) To: Andreas Gruenbacher Cc: Christoph Hellwig, Darrick J . Wong, Linus Torvalds, linux-xfs, linux-fsdevel 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. > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> > --- > fs/iomap/buffered-io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 358ee1fb6f0d..8fb9b2797fc5 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -734,7 +734,7 @@ static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len, > folio_put(folio); > > if (ret < len) > - iomap_write_failed(iter->inode, pos, len); > + iomap_write_failed(iter->inode, pos + ret, len - ret); > return ret; > } > > -- > 2.35.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iomap: iomap_write_end cleanup 2022-05-03 21:52 ` Matthew Wilcox @ 2022-05-03 22:15 ` Andreas Gruenbacher 2022-05-03 23:02 ` Darrick J. Wong 0 siblings, 1 reply; 9+ messages in thread From: Andreas Gruenbacher @ 2022-05-03 22:15 UTC (permalink / raw) To: Matthew Wilcox Cc: Christoph Hellwig, Darrick J . Wong, Linus Torvalds, linux-xfs, linux-fsdevel 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. Thanks, Andreas > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> > > --- > > fs/iomap/buffered-io.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > > index 358ee1fb6f0d..8fb9b2797fc5 100644 > > --- a/fs/iomap/buffered-io.c > > +++ b/fs/iomap/buffered-io.c > > @@ -734,7 +734,7 @@ static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len, > > folio_put(folio); > > > > if (ret < len) > > - iomap_write_failed(iter->inode, pos, len); > > + iomap_write_failed(iter->inode, pos + ret, len - ret); > > return ret; > > } > > > > -- > > 2.35.1 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iomap: iomap_write_end cleanup 2022-05-03 22:15 ` Andreas Gruenbacher @ 2022-05-03 23:02 ` Darrick J. Wong 2022-05-04 0:27 ` Matthew Wilcox 2022-05-04 8:02 ` Andreas Gruenbacher 0 siblings, 2 replies; 9+ messages in thread From: Darrick J. Wong @ 2022-05-03 23:02 UTC (permalink / raw) To: Andreas Gruenbacher Cc: Matthew Wilcox, Christoph Hellwig, Linus Torvalds, linux-xfs, linux-fsdevel 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. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > Thanks, > Andreas > > > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> > > > --- > > > fs/iomap/buffered-io.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > > > index 358ee1fb6f0d..8fb9b2797fc5 100644 > > > --- a/fs/iomap/buffered-io.c > > > +++ b/fs/iomap/buffered-io.c > > > @@ -734,7 +734,7 @@ static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len, > > > folio_put(folio); > > > > > > if (ret < len) > > > - iomap_write_failed(iter->inode, pos, len); > > > + iomap_write_failed(iter->inode, pos + ret, len - ret); > > > return ret; > > > } > > > > > > -- > > > 2.35.1 > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iomap: iomap_write_end cleanup 2022-05-03 23:02 ` Darrick J. Wong @ 2022-05-04 0:27 ` Matthew Wilcox 2022-05-04 14:22 ` Christoph Hellwig 2022-05-04 8:02 ` Andreas Gruenbacher 1 sibling, 1 reply; 9+ messages in thread From: Matthew Wilcox @ 2022-05-04 0:27 UTC (permalink / raw) To: Darrick J. Wong Cc: Andreas Gruenbacher, Christoph Hellwig, Linus Torvalds, linux-xfs, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iomap: iomap_write_end cleanup 2022-05-04 0:27 ` Matthew Wilcox @ 2022-05-04 14:22 ` Christoph Hellwig 2022-05-04 17:21 ` Andreas Gruenbacher 0 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2022-05-04 14:22 UTC (permalink / raw) To: Matthew Wilcox Cc: Darrick J. Wong, Andreas Gruenbacher, Christoph Hellwig, Linus Torvalds, linux-xfs, linux-fsdevel 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; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] iomap: iomap_write_end cleanup 2022-05-04 14:22 ` Christoph Hellwig @ 2022-05-04 17:21 ` Andreas Gruenbacher 0 siblings, 0 replies; 9+ messages in thread From: Andreas Gruenbacher @ 2022-05-04 17:21 UTC (permalink / raw) To: Christoph Hellwig Cc: Matthew Wilcox, Darrick J. Wong, Linus Torvalds, linux-xfs, linux-fsdevel On Wed, May 4, 2022 at 4:22 PM Christoph Hellwig <hch@infradead.org> wrote: > 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: This doesn't really strike me as better, just different. > 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; Ought to turn 'status' into 'copied' here, not '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; > Thanks, Andreas ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iomap: iomap_write_end cleanup 2022-05-03 23:02 ` Darrick J. Wong 2022-05-04 0:27 ` Matthew Wilcox @ 2022-05-04 8:02 ` Andreas Gruenbacher 1 sibling, 0 replies; 9+ messages in thread From: Andreas Gruenbacher @ 2022-05-04 8:02 UTC (permalink / raw) To: Matthew Wilcox Cc: Andreas Gruenbacher, Darrick J. Wong, Christoph Hellwig, Linus Torvalds, linux-xfs, linux-fsdevel 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); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] iomap: iomap_write_end cleanup 2022-05-03 21:37 [PATCH] iomap: iomap_write_end cleanup Andreas Gruenbacher 2022-05-03 21:52 ` Matthew Wilcox @ 2022-05-04 14:13 ` Christoph Hellwig 1 sibling, 0 replies; 9+ messages in thread From: Christoph Hellwig @ 2022-05-04 14:13 UTC (permalink / raw) To: Andreas Gruenbacher Cc: Christoph Hellwig, Darrick J . Wong, Linus Torvalds, linux-xfs, linux-fsdevel Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-05-04 18:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-03 21:37 [PATCH] iomap: iomap_write_end cleanup Andreas Gruenbacher 2022-05-03 21:52 ` Matthew Wilcox 2022-05-03 22:15 ` Andreas Gruenbacher 2022-05-03 23:02 ` Darrick J. Wong 2022-05-04 0:27 ` Matthew Wilcox 2022-05-04 14:22 ` Christoph Hellwig 2022-05-04 17:21 ` Andreas Gruenbacher 2022-05-04 8:02 ` Andreas Gruenbacher 2022-05-04 14:13 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.