linux-kernel - Re: [PATCH v5 04/10] xfs: Reflink CoW-based atomic write support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z9E3Sbh4AWm1C1IQ@infradead.org>
Date: Wed, 12 Mar 2025 00:27:05 -0700
From: Christoph Hellwig <hch@...radead.org>
To: John Garry <john.g.garry@...cle.com>
Cc: brauner@...nel.org, djwong@...nel.org, cem@...nel.org,
	linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, ojaswin@...ux.ibm.com,
	ritesh.list@...il.com, martin.petersen@...cle.com
Subject: Re: [PATCH v5 04/10] xfs: Reflink CoW-based atomic write support

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

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

> +	bool			atomic_sw = flags & XFS_REFLINK_ATOMIC_SW;

atomic_sw is not a very descriptive variable name.

>  
>  	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?
Maybe use that fact as it describes the semantics at this level
instead of the very high level intent?

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