* Re: [PATCH v2] xfs: disallow atomic writes on DAX
2025-07-22 17:58 ` [PATCH v2] xfs: disallow atomic writes on DAX Darrick J. Wong
@ 2025-07-23 7:36 ` John Garry
2025-08-05 4:32 ` Ritesh Harjani
1 sibling, 0 replies; 3+ messages in thread
From: John Garry @ 2025-07-23 7:36 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: hch, cem, linux-xfs, linux-ext4
On 22/07/2025 18:58, Darrick J. Wong wrote:
> [cc linux-ext4 because ext4_file_write_iter has the same problem]
>
> On Tue, Jul 22, 2025 at 03:00:16PM +0000, John Garry wrote:
>> Atomic writes are not currently supported for DAX, but two problems exist:
>> - we may go down DAX write path for IOCB_ATOMIC, which does not handle
>> IOCB_ATOMIC properly
>> - we report non-zero atomic write limits in statx (for DAX inodes)
>>
>> We may want atomic writes support on DAX in future, but just disallow for
>> now.
>>
>> For this, ensure when IOCB_ATOMIC is set that we check the write size
>> versus the atomic write min and max before branching off to the DAX write
>> path. This is not strictly required for DAX, as we should not get this far
>> in the write path as FMODE_CAN_ATOMIC_WRITE should not be set.
>>
>> In addition, due to reflink being supported for DAX, we automatically get
>> CoW-based atomic writes support being advertised. Remedy this by
>> disallowing atomic writes for a DAX inode for both sw and hw modes.
> You might want to add a separate patch to insert:
>
> if (WARN_ON_ONCE(iocb->ki_flags & IOCB_ATOMIC))
> return -EIO;
>
> into dax_iomap_rw to make it clear that DAX doesn't support ATOMIC
> writes.
ok, I can do that.
>
>> Reported-by: Darrick J. Wong<djwong@kernel.org>
>> Fixes: 9dffc58f2384 ("xfs: update atomic write limits")
>> Signed-off-by: John Garry<john.g.garry@oracle.com>
> Otherwise seems reasonable to me...
> Reviewed-by: "Darrick J. Wong"<djwong@kernel.org>
cheers
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] xfs: disallow atomic writes on DAX
2025-07-22 17:58 ` [PATCH v2] xfs: disallow atomic writes on DAX Darrick J. Wong
2025-07-23 7:36 ` John Garry
@ 2025-08-05 4:32 ` Ritesh Harjani
1 sibling, 0 replies; 3+ messages in thread
From: Ritesh Harjani @ 2025-08-05 4:32 UTC (permalink / raw)
To: Darrick J. Wong, John Garry; +Cc: hch, cem, linux-xfs, linux-ext4
"Darrick J. Wong" <djwong@kernel.org> writes:
> [cc linux-ext4 because ext4_file_write_iter has the same problem]
>
Thanks Darrick for the cc.
So ext4 currently will not issue atomic writes requests on DAX device,
unless the DAX device advertizes atomic write support (which IMO, it
doesn't). That is because, sbi->s_awu_min should be 0. I guess the
problem in case of XFS was the software fallback, where we only check if
the xfs_mount has reflink enabled, if yes, then we set
FMODE_CAN_ATOMIC_WRITE on file open. Since ext4 does not have such a
fallback, then the atomic write requests on EXT4 DAX should fail with
-EOPNOTSUPP.
static inline bool ext4_inode_can_atomic_write(struct inode *inode)
{
return S_ISREG(inode->i_mode) &&
ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
EXT4_SB(inode->i_sb)->s_awu_min > 0;
}
But having said that - I guess we could still add an explicit check in
above to disable atomic write if inode is IS_DAX(inode) and make the
same changes in ext4_file_write_iter() as XFS. Logically it make sense to
disable atomic writes explicitly if inode is of type DAX and also to do
any generic checks on the iocb before calling their respective file I/O
operations in ext4_file_write_iter().
-ritesh
> On Tue, Jul 22, 2025 at 03:00:16PM +0000, John Garry wrote:
>> Atomic writes are not currently supported for DAX, but two problems exist:
>> - we may go down DAX write path for IOCB_ATOMIC, which does not handle
>> IOCB_ATOMIC properly
>> - we report non-zero atomic write limits in statx (for DAX inodes)
>>
>> We may want atomic writes support on DAX in future, but just disallow for
>> now.
>>
>> For this, ensure when IOCB_ATOMIC is set that we check the write size
>> versus the atomic write min and max before branching off to the DAX write
>> path. This is not strictly required for DAX, as we should not get this far
>> in the write path as FMODE_CAN_ATOMIC_WRITE should not be set.
>>
>> In addition, due to reflink being supported for DAX, we automatically get
>> CoW-based atomic writes support being advertised. Remedy this by
>> disallowing atomic writes for a DAX inode for both sw and hw modes.
>
> You might want to add a separate patch to insert:
>
> if (WARN_ON_ONCE(iocb->ki_flags & IOCB_ATOMIC))
> return -EIO;
>
> into dax_iomap_rw to make it clear that DAX doesn't support ATOMIC
> writes.
>
>> Reported-by: Darrick J. Wong <djwong@kernel.org>
>> Fixes: 9dffc58f2384 ("xfs: update atomic write limits")
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>
> Otherwise seems reasonable to me...
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
>
> --D
>
>> ---
>> Difference to v1:
>> - allow use max atomic mount option and always dax together (Darrick)
>>
>> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
>> index ed69a65f56d7..979abcb25bc7 100644
>> --- a/fs/xfs/xfs_file.c
>> +++ b/fs/xfs/xfs_file.c
>> @@ -1099,9 +1099,6 @@ xfs_file_write_iter(
>> if (xfs_is_shutdown(ip->i_mount))
>> return -EIO;
>>
>> - if (IS_DAX(inode))
>> - return xfs_file_dax_write(iocb, from);
>> -
>> if (iocb->ki_flags & IOCB_ATOMIC) {
>> if (ocount < xfs_get_atomic_write_min(ip))
>> return -EINVAL;
>> @@ -1114,6 +1111,9 @@ xfs_file_write_iter(
>> return ret;
>> }
>>
>> + if (IS_DAX(inode))
>> + return xfs_file_dax_write(iocb, from);
>> +
>> if (iocb->ki_flags & IOCB_DIRECT) {
>> /*
>> * Allow a directio write to fall back to a buffered
>> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
>> index 07fbdcc4cbf5..bd6d33557194 100644
>> --- a/fs/xfs/xfs_inode.h
>> +++ b/fs/xfs/xfs_inode.h
>> @@ -358,9 +358,20 @@ static inline bool xfs_inode_has_bigrtalloc(const struct xfs_inode *ip)
>>
>> static inline bool xfs_inode_can_hw_atomic_write(const struct xfs_inode *ip)
>> {
>> + if (IS_DAX(VFS_IC(ip)))
>> + return false;
>> +
>> return xfs_inode_buftarg(ip)->bt_awu_max > 0;
>> }
>>
>> +static inline bool xfs_inode_can_sw_atomic_write(const struct xfs_inode *ip)
>> +{
>> + if (IS_DAX(VFS_IC(ip)))
>> + return false;
>> +
>> + return xfs_can_sw_atomic_write(ip->i_mount);
>> +}
>> +
>> /*
>> * In-core inode flags.
>> */
>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>> index 149b5460fbfd..603effabe1ee 100644
>> --- a/fs/xfs/xfs_iops.c
>> +++ b/fs/xfs/xfs_iops.c
>> @@ -616,7 +616,8 @@ xfs_get_atomic_write_min(
>> * write of exactly one single fsblock if the bdev will make that
>> * guarantee for us.
>> */
>> - if (xfs_inode_can_hw_atomic_write(ip) || xfs_can_sw_atomic_write(mp))
>> + if (xfs_inode_can_hw_atomic_write(ip) ||
>> + xfs_inode_can_sw_atomic_write(ip))
>> return mp->m_sb.sb_blocksize;
>>
>> return 0;
>> @@ -633,7 +634,7 @@ xfs_get_atomic_write_max(
>> * write of exactly one single fsblock if the bdev will make that
>> * guarantee for us.
>> */
>> - if (!xfs_can_sw_atomic_write(mp)) {
>> + if (!xfs_inode_can_sw_atomic_write(ip)) {
>> if (xfs_inode_can_hw_atomic_write(ip))
>> return mp->m_sb.sb_blocksize;
>> return 0;
>> --
>> 2.43.5
>>
^ permalink raw reply [flat|nested] 3+ messages in thread