linux-kernel - Re: [PATCH v5 05/10] xfs: Iomap SW-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: <Z9E5nDg3_cred1bH@infradead.org>
Date: Wed, 12 Mar 2025 00:37:00 -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 05/10] xfs: Iomap SW-based atomic write support

On Mon, Mar 10, 2025 at 06:39:41PM +0000, John Garry wrote:
> In cases of an atomic write occurs for misaligned or discontiguous disk
> blocks, we will use a CoW-based method to issue the atomic write.
> 
> So, for that case, return -EAGAIN to request that the write be issued in
> CoW atomic write mode. The dio write path should detect this, similar to
> how misaligned regular DIO writes are handled.

How is -EAGAIN going to work here given that it is also used to defer
non-blocking requests to the caller blocking context?

What is the probem with only setting the flag that causes REQ_ATOMIC
to be set from the file system instead of forcing it when calling
iomap_dio_rw?

Also how you ensure this -EAGAIN only happens on the first extent
mapped and you doesn't cause double writes?

> +	bool			atomic_hw = flags & IOMAP_ATOMIC_HW;

Again, atomic_hw is not a very useful variable name.  But the
whole idea of using a non-descriptive bool variable for a flags
field feels like an antipattern to me.

> -		if (shared)
> +		if (shared) {
> +			if (atomic_hw &&
> +			    !xfs_bmap_valid_for_atomic_write(&cmap,
> +					offset_fsb, end_fsb)) {
> +				error = -EAGAIN;
> +				goto out_unlock;
> +			}
>  			goto out_found_cow;

This needs a big fat comment explaining why bailing out here is
fine and how it works.

> +		/*
> +		 * Use CoW method for when we need to alloc > 1 block,
> +		 * otherwise we might allocate less than what we need here and
> +		 * have multiple mappings.
> +		*/

Describe why this is done, not just what is done.