linux-kernel - Re: [PATCH v2 06/11] 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: <306b49d9-f2a9-410d-b6df-ae0ba17eee7f@oracle.com>
Date: Tue, 25 Feb 2025 18:02:06 +0000
From: John Garry <john.g.garry@...cle.com>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: brauner@...nel.org, cem@...nel.org, dchinner@...hat.com, hch@....de,
        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, tytso@....edu,
        linux-ext4@...r.kernel.org
Subject: Re: [PATCH v2 06/11] xfs: Reflink CoW-based atomic write support

On 25/02/2025 17:37, Darrick J. Wong wrote:
> On Tue, Feb 25, 2025 at 10:58:56AM +0000, John Garry wrote:
>> On 24/02/2025 20:32, Darrick J. Wong wrote:
>>> On Thu, Feb 13, 2025 at 01:56:14PM +0000, John Garry wrote:
>>>> For CoW-based atomic write support, always allocate a cow hole in
>>>> xfs_reflink_allocate_cow() to write the new data.
>>>>
>>>> The semantics is that if @atomic is set, we will be passed a CoW fork
>>>> extent mapping for no error returned.
>>>>
>>>> Signed-off-by: John Garry <john.g.garry@...cle.com>
>>>> ---
>>>>    fs/xfs/xfs_iomap.c   |  2 +-
>>>>    fs/xfs/xfs_reflink.c | 12 +++++++-----
>>>>    fs/xfs/xfs_reflink.h |  2 +-
>>>>    3 files changed, 9 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
>>>> index d61460309a78..ab79f0080288 100644
>>>> --- a/fs/xfs/xfs_iomap.c
>>>> +++ b/fs/xfs/xfs_iomap.c
>>>> @@ -865,7 +865,7 @@ xfs_direct_write_iomap_begin(
>>>>    		/* may drop and re-acquire the ilock */
>>>>    		error = xfs_reflink_allocate_cow(ip, &imap, &cmap, &shared,
>>>>    				&lockmode,
>>>> -				(flags & IOMAP_DIRECT) || IS_DAX(inode));
>>>> +				(flags & IOMAP_DIRECT) || IS_DAX(inode), false);
>>>
>>> Now I'm /really/ think it's time for some reflink allocation flags,
>>> because the function signature now involves two booleans...
>>
>> ok, but the @convert_now arg is passed to other functions from
>> xfs_reflink_allocate_cow() - so would you prefer to create a bool
>> @convert_now inside xfs_reflink_allocate_cow() and pass that bool as before?
>> Or pass the flags all the way down to end users of @convert_now?
>>
>>>
>>>>    		if (error)
>>>>    			goto out_unlock;
>>>>    		if (shared)
>>>> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
>>>> index 8428f7b26ee6..3dab3ba900a3 100644
>>>> --- a/fs/xfs/xfs_reflink.c
>>>> +++ b/fs/xfs/xfs_reflink.c
>>>> @@ -435,7 +435,8 @@ xfs_reflink_fill_cow_hole(
>>>>    	struct xfs_bmbt_irec	*cmap,
>>>>    	bool			*shared,
>>>>    	uint			*lockmode,
>>>> -	bool			convert_now)
>>>> +	bool			convert_now,
>>>> +	bool			atomic)
>>>
>>> ...but this can come later.
>>
>> Do you mean that this would just be a new flag to set?
> 
> Sorry, I meant that the double booleans -> flags conversion could be a
> cleanup patch at the end of the series.  But first we'd have to figure
> out where we want the flags boundaries to be -- do we just pass the
> IOMAP_{DIRECT,DAX,ATOMIC_*} flags directly to the reflink code and let
> it figure out what to do? 

We have the odd case of @convert_now being set from IS_DAX(inode) in 
xfs_direct_write_iomap_begin() -> xfs_reflink_allocate_cow(), so that 
thwarts the idea of passing the IOMAP flags directly. BTW, it may be 
possible to clear up that IS_DAX() usage - I'm not sure, so I'll check 
again.

> Or do we make the xfs_iomap.c code translate
> that into XFS_REFLINK_ALLOC_* flags?

That is what I was thinking of doing. But, as mentioned, it needs to be 
decided if we pass XFS_REFLINK_ALLOC_* to callees of 
xfs_reflink_allocate_cow(). I'm thinking 'no', as it will only create churn.

> 
> Either way, that is not something that needs to be done in this patch.

Sure


Thanks,
John