| Message ID | 20231126124720.1249310-11-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/13] iomap: clear the per-folio dirty bits on all writeback failures | expand |
On Sun, Nov 26, 2023 at 01:47:17PM +0100, Christoph Hellwig wrote: > Instead of clling mapping_set_error once per folio, only do that once > per bio, and consolidate all the writeback error handling code in > iomap_finish_ioend. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/iomap/buffered-io.c | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 17760f3e67c61e..e1d5076251702d 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -1464,15 +1464,10 @@ vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf, const struct iomap_ops *ops) > EXPORT_SYMBOL_GPL(iomap_page_mkwrite); > > static void iomap_finish_folio_write(struct inode *inode, struct folio *folio, > - size_t len, int error) > + size_t len) > { > struct iomap_folio_state *ifs = folio->private; > > - if (error) { > - folio_set_error(folio); > - mapping_set_error(inode->i_mapping, error); > - } > - > WARN_ON_ONCE(i_blocks_per_folio(inode, folio) > 1 && !ifs); > WARN_ON_ONCE(ifs && atomic_read(&ifs->write_bytes_pending) <= 0); > > @@ -1493,18 +1488,24 @@ iomap_finish_ioend(struct iomap_ioend *ioend, int error) > struct folio_iter fi; > u32 folio_count = 0; > > + if (error) { > + mapping_set_error(inode->i_mapping, error); > + if (!bio_flagged(bio, BIO_QUIET)) { > + pr_err_ratelimited( > +"%s: writeback error on inode %lu, offset %lld, sector %llu", > + inode->i_sb->s_id, inode->i_ino, > + ioend->io_offset, ioend->io_sector); Something that's always bothered me: Why don't we log the *amount* of data that just failed writeback? "XFS: writeback error on inode 63, offset 4096, length 8192, sector 27" Now we'd actually have some way to work out that we've lost 8k worth of data. OFC maybe the better solution is to wire up that inotify error reporting interface that ext4 added a while back...? This patch is pretty straightforward tho Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > + } > + } > + > /* walk all folios in bio, ending page IO on them */ > bio_for_each_folio_all(fi, bio) { > - iomap_finish_folio_write(inode, fi.folio, fi.length, error); > + if (error) > + folio_set_error(fi.folio); > + iomap_finish_folio_write(inode, fi.folio, fi.length); > folio_count++; > } > > - if (unlikely(error && !bio_flagged(bio, BIO_QUIET))) { > - printk_ratelimited(KERN_ERR > -"%s: writeback error on inode %lu, offset %lld, sector %llu", > - inode->i_sb->s_id, inode->i_ino, > - ioend->io_offset, ioend->io_sector); > - } > bio_put(bio); /* frees the ioend */ > return folio_count; > } > -- > 2.39.2 > >
On Tue, Nov 28, 2023 at 09:03:22PM -0800, Darrick J. Wong wrote: > > + if (error) { > > + mapping_set_error(inode->i_mapping, error); > > + if (!bio_flagged(bio, BIO_QUIET)) { > > + pr_err_ratelimited( > > +"%s: writeback error on inode %lu, offset %lld, sector %llu", > > + inode->i_sb->s_id, inode->i_ino, > > + ioend->io_offset, ioend->io_sector); > > Something that's always bothered me: Why don't we log the *amount* of > data that just failed writeback? > > "XFS: writeback error on inode 63, offset 4096, length 8192, sector 27" > > Now we'd actually have some way to work out that we've lost 8k worth of > data. OFC maybe the better solution is to wire up that inotify error > reporting interface that ext4 added a while back...? Just adding the amount sounds sane to me and I can add a patch to do that. I think this message originated in buffer.c, where it was printed once for each block before rate limiting got added..
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 17760f3e67c61e..e1d5076251702d 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -1464,15 +1464,10 @@ vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf, const struct iomap_ops *ops) EXPORT_SYMBOL_GPL(iomap_page_mkwrite); static void iomap_finish_folio_write(struct inode *inode, struct folio *folio, - size_t len, int error) + size_t len) { struct iomap_folio_state *ifs = folio->private; - if (error) { - folio_set_error(folio); - mapping_set_error(inode->i_mapping, error); - } - WARN_ON_ONCE(i_blocks_per_folio(inode, folio) > 1 && !ifs); WARN_ON_ONCE(ifs && atomic_read(&ifs->write_bytes_pending) <= 0); @@ -1493,18 +1488,24 @@ iomap_finish_ioend(struct iomap_ioend *ioend, int error) struct folio_iter fi; u32 folio_count = 0; + if (error) { + mapping_set_error(inode->i_mapping, error); + if (!bio_flagged(bio, BIO_QUIET)) { + pr_err_ratelimited( +"%s: writeback error on inode %lu, offset %lld, sector %llu", + inode->i_sb->s_id, inode->i_ino, + ioend->io_offset, ioend->io_sector); + } + } + /* walk all folios in bio, ending page IO on them */ bio_for_each_folio_all(fi, bio) { - iomap_finish_folio_write(inode, fi.folio, fi.length, error); + if (error) + folio_set_error(fi.folio); + iomap_finish_folio_write(inode, fi.folio, fi.length); folio_count++; } - if (unlikely(error && !bio_flagged(bio, BIO_QUIET))) { - printk_ratelimited(KERN_ERR -"%s: writeback error on inode %lu, offset %lld, sector %llu", - inode->i_sb->s_id, inode->i_ino, - ioend->io_offset, ioend->io_sector); - } bio_put(bio); /* frees the ioend */ return folio_count; }
Instead of clling mapping_set_error once per folio, only do that once per bio, and consolidate all the writeback error handling code in iomap_finish_ioend. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/iomap/buffered-io.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)