On 12/03/2025 07:27, Christoph Hellwig wrote:
On Mon, Mar 10, 2025 at 06:39:40PM +0000, John Garry wrote:
Base SW-based atomic writes on CoW.
For SW-based atomic write support, always allocate a cow hole in
xfs_reflink_allocate_cow() to write the new data.
What is a "COW hole"?
I really mean a cow mapping. I can reword that.
The semantics is that if @atomic_sw is set, we will be passed a CoW fork
extent mapping for no error returned.
This commit log feels extremely sparse for a brand new feature with
data integrity impact. Can you expand on it a little?
Sure, will do
+ bool atomic_sw = flags & XFS_REFLINK_ATOMIC_SW;
atomic_sw is not a very descriptive variable name.
ack
resaligned = xfs_aligned_fsb_count(imap->br_startoff,
imap->br_blockcount, xfs_get_cowextsz_hint(ip));
@@ -466,7 +467,7 @@ xfs_reflink_fill_cow_hole(
*lockmode = XFS_ILOCK_EXCL;
error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found);
- if (error || !*shared)
+ if (error || (!*shared && !atomic_sw))
And it's pnly used once. Basically is is used to force COW, right?
Yes, we force it. Indeed, I think that is term you used a long time ago
in your RFC for atomic file updates.
But that flag is being used to set XFS_BMAPI_EXTSZALIGN, so feels like a
bit of a disconnect as why we would set XFS_BMAPI_EXTSZALIGN for "forced
cow". I would need to spell that out.
Maybe use that fact as it describes the semantics at this level
instead of the very high level intent?
ok, fine
@@ -10,6 +10,7 @@
* Flags for xfs_reflink_allocate_cow()
*/
#define XFS_REFLINK_CONVERT (1u << 0) /* convert unwritten extents now */
+#define XFS_REFLINK_ATOMIC_SW (1u << 1) /* alloc for SW-based atomic write */
Please expand what this actually means at the xfs_reflink_allocate_cow.
Of if it is just a force flag as I suspect speel that out. And
move the comment up to avoid the overly long line as well as giving
you space to actually spell the semantics out.
OK, I can do that, especially since XFS_REFLINK_CONVERT is going to be
renamed.
Thanks,
John
