Re: [PATCH 02/10] iomap: improve shared block detection in iomap_unshare_iter

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Aug 27, 2024 at 07:09:49AM +0200, Christoph Hellwig wrote:
> Currently iomap_unshare_iter relies on the IOMAP_F_SHARED flag to detect
> blocks to unshare.  This is reasonable, but IOMAP_F_SHARED is also useful
> for the file system to do internal book keeping for out of place writes.
> XFS used to that, until it got removed in commit 72a048c1056a
> ("xfs: only set IOMAP_F_SHARED when providing a srcmap to a write")
> because unshare for incorrectly unshare such blocks.
> 
> Add an extra safeguard by checking the explicitly provided srcmap instead
> of the fallback to the iomap for valid data, as that catches the case
> where we'd just copy from the same place we'd write to easily, allowing
> to reinstate setting IOMAP_F_SHARED for all XFS writes that go to the
> COW fork.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/iomap/buffered-io.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 69a931de1979b9..737a005082e035 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -1337,16 +1337,25 @@ EXPORT_SYMBOL_GPL(iomap_file_buffered_write_punch_delalloc);
>  static loff_t iomap_unshare_iter(struct iomap_iter *iter)
>  {
>  	struct iomap *iomap = &iter->iomap;
> -	const struct iomap *srcmap = iomap_iter_srcmap(iter);
>  	loff_t pos = iter->pos;
>  	loff_t length = iomap_length(iter);
>  	loff_t written = 0;
>  
> -	/* don't bother with blocks that are not shared to start with */
> +	/* Don't bother with blocks that are not shared to start with. */
>  	if (!(iomap->flags & IOMAP_F_SHARED))
>  		return length;
> -	/* don't bother with holes or unwritten extents */
> -	if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN)
> +
> +	/*
> +	 * Don't bother with holes or unwritten extents.
> +	 *
> +	 * Note that we use srcmap directly instead of iomap_iter_srcmap as
> +	 * unsharing requires providing a separate source map, and the presence
> +	 * of one is a good indicator that unsharing is needed, unlike
> +	 * IOMAP_F_SHARED which can be set for any data that goes into the COW

Maybe we should rename it then?

IOMAP_F_OUT_OF_PLACE_WRITE.

Yuck.

IOMAP_F_ELSEWHERE

Not much better.  Maybe just add a comment that "IOMAP_F_SHARED" really
just means an out of place write (aka srcmap is not just a hole).

--D

> +	 * fork for XFS.
> +	 */
> +	if (iter->srcmap.type == IOMAP_HOLE ||
> +	    iter->srcmap.type == IOMAP_UNWRITTEN)
>  		return length;
>  
>  	do {
> -- 
> 2.43.0
> 
>