diff mbox series

[3/4] fsdax: remove zeroing code from dax_unshare_iter

Message ID 172796813311.1131942.16033376284752798632.stgit@frogsfrogsfrogs (mailing list archive)
State New
Headers show
Series [1/4] xfs: don't allocate COW extents when unsharing a hole | expand

Commit Message

Darrick J. Wong Oct. 3, 2024, 3:09 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Remove the code in dax_unshare_iter that zeroes the destination memory
because it's not necessary.

If srcmap is unwritten, we don't have to do anything because that
unwritten extent came from the regular file mapping, and unwritten
extents cannot be shared.  The same applies to holes.

Furthermore, zeroing to unshare a mapping is just plain wrong because
unsharing means copy on write, and we should be copying data.

This is effectively a revert of commit 13dd4e04625f ("fsdax: unshare:
zero destination if srcmap is HOLE or UNWRITTEN")

Cc: ruansy.fnst@fujitsu.com
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/dax.c |    8 --------
 1 file changed, 8 deletions(-)

Comments

Christoph Hellwig Oct. 4, 2024, 12:09 p.m. UTC | #1 
On Thu, Oct 03, 2024 at 08:09:32AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Remove the code in dax_unshare_iter that zeroes the destination memory
> because it's not necessary.
> 
> If srcmap is unwritten, we don't have to do anything because that
> unwritten extent came from the regular file mapping, and unwritten
> extents cannot be shared.  The same applies to holes.
> 
> Furthermore, zeroing to unshare a mapping is just plain wrong because
> unsharing means copy on write, and we should be copying data.
> 
> This is effectively a revert of commit 13dd4e04625f ("fsdax: unshare:
> zero destination if srcmap is HOLE or UNWRITTEN")

The original commit claims it fixed a bug, so I'm curious how
that happend and got fixed differently now.  But manually zeroing
data on an unshare does seem very wrong to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/fs/dax.c b/fs/dax.c
index 5064eefb1c1e..9fbbdaa784b4 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -1276,14 +1276,6 @@  static s64 dax_unshare_iter(struct iomap_iter *iter)
 	if (ret < 0)
 		goto out_unlock;
 
-	/* zero the distance if srcmap is HOLE or UNWRITTEN */
-	if (srcmap->flags & IOMAP_F_SHARED || srcmap->type == IOMAP_UNWRITTEN) {
-		memset(daddr, 0, length);
-		dax_flush(iomap->dax_dev, daddr, length);
-		ret = length;
-		goto out_unlock;
-	}
-
 	ret = dax_iomap_direct_access(srcmap, pos, length, &saddr, NULL);
 	if (ret < 0)
 		goto out_unlock;