[PATCH v2] xfs: check the disk address for hardware atomic writes

[PATCH v2] xfs: check the disk address for hardware atomic writes

[Daejun Park via B4 Relay]

From: Daejun Park <daejun7.park@samsung.com>

xfs_bmap_hw_atomic_write_possible() checks the alignment of the encoded
br_startblock, not of the disk address. This was harmless while atomic
writes were capped at the greatest power-of-two factor of the group
size.

Commit ec8f62cc109a ("xfs: don't limit software atomic writes by the
group alignment") lifted that cap. The block layer can now fail a
misaligned write with -EINVAL, and XFS does not fall back to COW.
Unaligned AGs, such as those made by mkfs.xfs before 6.16, and rtgroups
made by any version can hit this.

Check the disk address instead.

Fixes: ec8f62cc109a ("xfs: don't limit software atomic writes by the group alignment")
Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
The bug needs a group size that is not a multiple of the device's
atomic write unit max. mkfs.xfs 6.16+ rounds agsize to a multiple
of it if the device reports one at mkfs time, but never rounds
rgsize. Before 6.16, a single device of 4 TB or more got an odd
agsize.

Tested on emulated NVMe with a 64 KiB atomic unit and boundary.
64 KiB atomic overwrites in AG 1-3 (mkfs.xfs 6.6.0 -d agsize=262142b)
and rtgroups 1-7 (xfsprogs 7ebfc20eb88e -r rgsize=65534b) failed with
EINVAL before and used COW after. fstests -g atomicwrites and xfs/838
(rt) give the same results before and after (generic/773 fails with
ENOSPC on both).

Changes in v2:
- Explain why the check was harmless before ec8f62cc109a and which
  filesystems are affected (Pankaj).
- Reword the comment: an fsbno is not always a linear disk address
  (Pankaj).
---
 fs/xfs/xfs_iomap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index c3465204b8d..119ff8e4c95 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -848,8 +848,10 @@ xfs_bmap_hw_atomic_write_possible(
 	 * atomic writes are required to be naturally aligned for disk blocks,
 	 * which ensures that we adhere to block layer rules that we won't
 	 * straddle any boundary or violate write alignment requirement.
+	 * Check the disk address: an fsbno is not always a linear disk address.
 	 */
-	if (!IS_ALIGNED(imap->br_startblock, imap->br_blockcount))
+	if (!IS_ALIGNED(xfs_fsb_to_db(ip, imap->br_startblock),
+			XFS_FSB_TO_BB(mp, imap->br_blockcount)))
 		return false;
 
 	/*

---
base-commit: 48326a2382d80715475d10ce98734ac65d2d2150
change-id: 20261110-xfs-hw-atomic-align-0eab6833447a

Best regards,
-- 
Daejun Park <daejun7.park@samsung.com>

Re: [PATCH v2] xfs: check the disk address for hardware atomic writes

[John Garry]

On 11/12/26 00:41, Daejun Park via B4 Relay wrote:
> From: Daejun Park <daejun7.park@samsung.com>
> 
> xfs_bmap_hw_atomic_write_possible() checks the alignment of the encoded
> br_startblock, not of the disk address. This was harmless while atomic
> writes were capped at the greatest power-of-two factor of the group
> size.
> 
> Commit ec8f62cc109a ("xfs: don't limit software atomic writes by the
> group alignment") lifted that cap. The block layer can now fail a
> misaligned write with -EINVAL, 

It's a good thing that we have those block layer checks...

> and XFS does not fall back to COW.
> Unaligned AGs, such as those made by mkfs.xfs before 6.16, and rtgroups
> made by any version can hit this.
> 
> Check the disk address instead.
> 
> Fixes: ec8f62cc109a ("xfs: don't limit software atomic writes by the group alignment")
> Signed-off-by: Daejun Park <daejun7.park@samsung.com>
> ---
> The bug needs a group size that is not a multiple of the device's
> atomic write unit max. mkfs.xfs 6.16+ rounds agsize to a multiple
> of it if the device reports one at mkfs time, but never rounds
> rgsize. Before 6.16, a single device of 4 TB or more got an odd
> agsize.
> 
> Tested on emulated NVMe with a 64 KiB atomic unit and boundary.
> 64 KiB atomic overwrites in AG 1-3 (mkfs.xfs 6.6.0 -d agsize=262142b)
> and rtgroups 1-7 (xfsprogs 7ebfc20eb88e -r rgsize=65534b) failed with
> EINVAL before and used COW after. fstests -g atomicwrites and xfs/838
> (rt) give the same results before and after (generic/773 fails with
> ENOSPC on both).
> 
> Changes in v2:
> - Explain why the check was harmless before ec8f62cc109a and which
>    filesystems are affected (Pankaj).
> - Reword the comment: an fsbno is not always a linear disk address
>    (Pankaj).
> ---
>   fs/xfs/xfs_iomap.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index c3465204b8d..119ff8e4c95 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -848,8 +848,10 @@ xfs_bmap_hw_atomic_write_possible(
>   	 * atomic writes are required to be naturally aligned for disk blocks,
>   	 * which ensures that we adhere to block layer rules that we won't
>   	 * straddle any boundary or violate write alignment requirement.
> +	 * Check the disk address: an fsbno is not always a linear disk address.
>   	 */
> -	if (!IS_ALIGNED(imap->br_startblock, imap->br_blockcount))

Is there anywhere else in which we do an alignment check on 
br_startblock that relies on the group size to be a multiple of the HW 
awu max?

> +	if (!IS_ALIGNED(xfs_fsb_to_db(ip, imap->br_startblock),
> +			XFS_FSB_TO_BB(mp, imap->br_blockcount)))
>   		return false;
>   
>   	/*
> 
> ---
> base-commit: 48326a2382d80715475d10ce98734ac65d2d2150
> change-id: 20261110-xfs-hw-atomic-align-0eab6833447a
> 
> Best regards,

Re: [PATCH v2] xfs: check the disk address for hardware atomic writes

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>