Re: [PATCH 10/13] iomap: only call mapping_set_error once for each failed bio

From: "Darrick J. Wong" <djwong@kernel.org>
To: Christoph Hellwig <hch@lst.de>
Cc: Christian Brauner <brauner@kernel.org>,
	Chandan Babu R <chandan.babu@oracle.com>,
	Zhang Yi <yi.zhang@huaweicloud.com>,
	Ritesh Harjani <ritesh.list@gmail.com>,
	linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 10/13] iomap: only call mapping_set_error once for each failed bio
Date: Tue, 28 Nov 2023 21:03:22 -0800	[thread overview]
Message-ID: <20231129050322.GP4167244@frogsfrogsfrogs> (raw)
In-Reply-To: <20231126124720.1249310-11-hch@lst.de>

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
> 
> 