[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z9fOoE3LxcLNcddh@infradead.org>
Date: Mon, 17 Mar 2025 08:26:24 +0100
From: Christoph Hellwig <hch@...radead.org>
To: John Garry <john.g.garry@...cle.com>
Cc: brauner@...nel.org, djwong@...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 v6 10/13] xfs: iomap COW-based atomic write support
> +static bool
> +xfs_bmap_valid_for_atomic_write(
This is misnamed. It checks if the hardware offload an be used.
> + /* Misaligned start block wrt size */
> + if (!IS_ALIGNED(imap->br_startblock, imap->br_blockcount))
> + return false;
It is very obvious that this checks for a natural alignment of the
block number. But it fails to explain why it checks for that.
> +
> + /* Discontiguous extents */
> + if (!imap_spans_range(imap, offset_fsb, end_fsb))
Same here.
> + if (shared) {
> + /*
> + * Since we got a CoW fork extent mapping, ensure that
> + * the mapping is actually suitable for an
> + * REQ_ATOMIC-based atomic write, i.e. properly aligned
> + * and covers the full range of the write. Otherwise,
> + * we need to use the COW-based atomic write mode.
> + */
> + if ((flags & IOMAP_ATOMIC) &&
> + !xfs_bmap_valid_for_atomic_write(&cmap,
The "Since.." implies there is something special about COW fork mappings.
But I don't think there is, or am I missing something?
If xfs_bmap_valid_for_atomic_write was properly named and documented
this comment should probably just go away.
> +static int
> +xfs_atomic_write_cow_iomap_begin(
Should the atomic and cow be together for coherent naming?
But even if the naming is coherent it isn't really
self-explanatory, so please add a little top of the function
comment introducing it.
> + error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, &imap,
> + &nimaps, 0);
> + if (error)
> + goto out_unlock;
Why does this need to read the existing data for mapping? You'll
overwrite everything through the COW fork anyway.
Powered by blists - more mailing lists