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