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: <81247acc-f0fe-4d10-a0cd-bbd5b792267f@oracle.com>
Date: Wed, 12 Mar 2025 09:13:45 +0000
From: John Garry <john.g.garry@...cle.com>
To: Christoph Hellwig <hch@...radead.org>
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 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