Received-SPF: pass (google.com: domain of linux-kernel+bounces-52668-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <7330574a-edc5-4585-8f1a-367871271786@oracle.com>
Date: Mon, 5 Feb 2024 12:51:07 +0000
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 3/6] fs: xfs: Support FS_XFLAG_ATOMICWRITES for rtvol
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: hch@lst.de, viro@zeniv.linux.org.uk, brauner@kernel.org,
        dchinner@redhat.com, jack@suse.cz, chandan.babu@oracle.com,
        martin.petersen@oracle.com, linux-kernel@vger.kernel.org,
        linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        tytso@mit.edu, jbongio@google.com, ojaswin@linux.ibm.com
References: <20240124142645.9334-1-john.g.garry@oracle.com>
 <20240124142645.9334-4-john.g.garry@oracle.com>
 <20240202175225.GH6184@frogsfrogsfrogs>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <20240202175225.GH6184@frogsfrogsfrogs>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?SHZUVUZiTGJDVlp0SmRSOFV3ZzE5RjR2amoyMFR1RFRiMmYvQ3dvWFk5L2RE?=
 =?utf-8?B?ZXJybVJaZ2VJZXlqMnNteFA5SWxhcG1VMHNuM05KSHhaMFdUYWdzL2toYkVF?=
 =?utf-8?B?NldxYS9yeVJCL1dwWFRISEE0OGJJdWdhc0JpQm1TM3dCTHFPT2MvMjM0cUFT?=
 =?utf-8?B?YUxPNm5Dc3QxVnR4alNGbmJWRFRPWkdnbmlXZkt4Ymd1UFExTGtGc2RjYy9G?=
 =?utf-8?B?Z1RJRVR6amVTR2hPLzdpQTU0QnJyWG5qTWQzeURmMkdTZmIzZWc4Mnk4MGhm?=
 =?utf-8?B?akJIL21GU2VzR3BoMHFPTnArWjUyS09ud1FrS3lYOXQ4K2dyR1hEY2JqNkwy?=
 =?utf-8?B?eCtGTkJtSCtCM3VNUWZTNnZzY1ZXdjJkMDZzd2NQQ3gwaFFKQmdpQmRPTFFo?=
 =?utf-8?B?QytoNE05OG5iYnFTdTMvemhqVjYrQm5qb2t5TnZyaDE0ZjBsUHN1L2N2UkdY?=
 =?utf-8?B?U2VrdC9tdFBZTjg2RzZaNlFzQlJFcDc5eGpwclJsaFhrbWx1Y2JHK2YzTTF3?=
 =?utf-8?B?OWUydWcxTlJIYWVHU3gwYWUwNW5ib3hCTjZORVRVRjdXOFhVNlloSTBaTGVy?=
 =?utf-8?B?NWZRSjhEQ21rY2F3VE1DNDJvbG15Y3VlUU5pU3llKzhyRTdncFlIbTkvNVJG?=
 =?utf-8?B?QzRtYm9QdEovd2tEcnB0UVlCWFBKV2V3K2VsaFlZQk4wR29SSGpVYXNuK0lZ?=
 =?utf-8?B?L25ZZGVKaVd3Zm9lczZQVVBNMEpZL0ZNQVB5dnZMdnNzWTNrWXhyUW9ERVVJ?=
 =?utf-8?B?VnAra2hsOHVyTnJjYUFSeitoSlFaRWh2ZkE3Qm5YZGZjV0F2UXJGTGowdkZi?=
 =?utf-8?B?eFNydTFMR05iOGg1eVl2clk0Tk9ObDRrK0xLTnFKNS81M1VNSkFFU3Ftdkd0?=
 =?utf-8?B?UVZHdVFLREpkYTNKZGNSMUNYMWhWYWpnVk9wN3U4VlZLNDRjSmNOTHBPQU1q?=
 =?utf-8?B?bWdrS3Nocjh6Z2dLbmgwMjJzSEZMZDVyamJQbkgxK3RSMGg0eUd4ejkyTU9h?=
 =?utf-8?B?amNFaFl0MC9jNDJYc0c5NzR6YTYyb1JYS2ttV2VVT1dBTFBIN2M0dzVTUVNn?=
 =?utf-8?B?eFhKV0FDTU9XeTJkZkxlc212RnU5SStkTW9lZVcycVpBbDNwdmV2RkxqcEpn?=
 =?utf-8?B?NmwxLysrQkxha2VINWptQlZNbkhMMVgxVElYUEJwVzlxT1Nya0hCcVpiNVRW?=
 =?utf-8?B?UnB5NXVZeXZvU1Y4YmMrS2xJT1Y1dGxYaXJtdUVidmh4V0pCZWhHdUFqRU1t?=
 =?utf-8?B?RlR6WTRWK3BlNkFNZlloWFc3eG5jTzhVU0gyR1hVY05uUHFWUXp1SUpaUnZp?=
 =?utf-8?B?RCtmRGt6NyswUi9rNHdnSDMxSndIUjdJNlRMZVRSTm9WZjhSWnRSeGE2aHJq?=
 =?utf-8?B?Z0szQTM2MVhQVEg4NktqTTRhLzYraGFQVWJaMkJ3REtYa3FaQWhjUWdxS0l3?=
 =?utf-8?B?Z3JzcDVER01CK3RiVjkxdjMwYTBGTEVUTjBYdGZwaDAzZ2pJTkNqWmdaZG9h?=
 =?utf-8?B?dm9wOENJM29JaDl5SlprWUR2eW8vMVBvdmVNeDNJQTBEZEljN1RQK0JPQWhO?=
 =?utf-8?B?aXBnTVhIWEVJU0pjd0VZTXRGbk95T05pT1RuVlAyaVJvakU5ZVo0djUzTzNY?=
 =?utf-8?B?RHpKWXVFbWxrNUl6WnVuUENXeXhzME4yaXFiN3p6Q0plNkY0RGUwUVhOY2JH?=
 =?utf-8?B?M3RXb1U3UGgvRzRxUjBsM1ZIQ3IyYVk3MGgxcnZmUGNXQzZYVjZZaHU5cHVK?=
 =?utf-8?B?MkUzV1J4UXdjK0YwQ01xa3hBUDhNU1pYZHFKeVVkWnY5U3JYRlR3YzJKcVFj?=
 =?utf-8?B?bkpuWXNSWlRmSE9MZEJsbStmbXoyUlJCek9iOUFtMWgvNnExd0FBME1xWGlM?=
 =?utf-8?B?bXFOdFUyTGVpUW04OUJyQVBFWVU2bkxjODNoMVh2enNzOVlxVlRwekN2Q0dM?=
 =?utf-8?B?dUpwZkJpNWZTTGQ2YVNZS0xOa0p1SVd4SUtGNXJsbW1sSmx4Z3V1cEVTSGxX?=
 =?utf-8?B?a0h1MnJGZWQxcEx4ZXZXNUpYNTZQdDQwS1BnVUZaMy9oL2Q1U05vMHJkZ2sz?=
 =?utf-8?B?WXZBZk5ZY0E1Z2M0cXp6aVhRS3dGSXdHRk0yWWRpWEdtNE5BVVhNSTF6clZv?=
 =?utf-8?Q?kqZpgScCHptbuth5M5c9IanrX?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	mZeFUW5jwhmdzClW3agruHIS7Zq0m+TcjdaTOJ7Zrb9Bp1pbyPa3zZoTpbfXQWZRfPpWEPgiOgquuMnKX7HZK3hzuT46F7ve5w9IchR6jl171gKfiThd2qYbCO3jEkq2BERcs3fs50TqhEyKjAgkM9YZYMT2NW2c4zuMeKNMH0EeTjMugF0JIozt8D81J/haisyXXmT4+E78JEDoEwuJt7o1dwdr+b1CYuUewuaOr2/pPBTEgKGuy2D3rJJGoByRegdPuVs+cYmX5nZQt32ZZMso8nSWcJWlWrdS/8vRVL41po6Dr7ENazbsWH2SgBEJWV5V4mNbGqsyN8+j9Q9tAQ24jd1EoHRUFuitvGamWgI0/B5hkdHoFFzMR06NEWvvH4FdOO6ybFiXihgik6ZsPVuesxq3zcXxEfhGVu/0OLex9dGk/lXPw0iEBaZoh7zyHT2n3yYonuIjLAUEeEBzYpEr5Gz/1hp8AtaT64JZ0Iox9iDelg7XnSkM080vk+2/Iq9nuCHHInUDS879d46CUGpYurJzYoHA3pryxgktB9jnSK70SuMbJfIQMEf7iHz3QdWkilCNo7flDdAIcrSdUaonW6UUkn/Auv2Ez0w8gm0=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 92a6fc4b-b1bd-4b70-34f4-08dc26492128
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Feb 2024 12:51:11.2845
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: EnQgsX7eofTiBVRtlM45msov2fgZNl4RGAdlDv8mCCKvSIYfJ06jZVJmhF4GhXnGSegFG+Jf0fLLdGZtJoflPg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR10MB6453
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-05_07,2024-01-31_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0
 mlxlogscore=999 suspectscore=0 adultscore=0 spamscore=0 malwarescore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402050097
X-Proofpoint-GUID: PBKn916vGCuhmsoiG80ZZ8X-fxfpUL8g
X-Proofpoint-ORIG-GUID: PBKn916vGCuhmsoiG80ZZ8X-fxfpUL8g

On 02/02/2024 17:52, Darrick J. Wong wrote:
> On Wed, Jan 24, 2024 at 02:26:42PM +0000, John Garry wrote:
>> Add initial support for FS_XFLAG_ATOMICWRITES in rtvol.
>>
>> Current kernel support for atomic writes is based on HW support (for atomic
>> writes). As such, it is required to ensure extent alignment with
>> atomic_write_unit_max so that an atomic write can result in a single
>> HW-compliant IO operation.
>>
>> rtvol already guarantees extent alignment, so initially add support there.
>>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_format.h |  8 ++++++--
>>   fs/xfs/libxfs/xfs_sb.c     |  2 ++
>>   fs/xfs/xfs_inode.c         | 22 ++++++++++++++++++++++
>>   fs/xfs/xfs_inode.h         |  7 +++++++
>>   fs/xfs/xfs_ioctl.c         | 19 +++++++++++++++++--
>>   fs/xfs/xfs_mount.h         |  2 ++
>>   fs/xfs/xfs_super.c         |  4 ++++
>>   7 files changed, 60 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
>> index 382ab1e71c0b..79fb0d4adeda 100644
>> --- a/fs/xfs/libxfs/xfs_format.h
>> +++ b/fs/xfs/libxfs/xfs_format.h
>> @@ -353,11 +353,13 @@ xfs_sb_has_compat_feature(
>>   #define XFS_SB_FEAT_RO_COMPAT_RMAPBT   (1 << 1)		/* reverse map btree */
>>   #define XFS_SB_FEAT_RO_COMPAT_REFLINK  (1 << 2)		/* reflinked files */
>>   #define XFS_SB_FEAT_RO_COMPAT_INOBTCNT (1 << 3)		/* inobt block counts */
>> +#define XFS_SB_FEAT_RO_COMPAT_ATOMICWRITES (1 << 29)	/* aligned file data extents */
> 
> I thought FORCEALIGN was going to signal aligned file data extent
> allocations being mandatory?

Right, I'll fix that comment

> 
> This flag (AFAICT) simply marks the inode as something that gets
> FMODE_CAN_ATOMIC_WRITES, right?

Correct

> 
>>   #define XFS_SB_FEAT_RO_COMPAT_ALL \
>>   		(XFS_SB_FEAT_RO_COMPAT_FINOBT | \
>>   		 XFS_SB_FEAT_RO_COMPAT_RMAPBT | \
>>   		 XFS_SB_FEAT_RO_COMPAT_REFLINK| \
>> -		 XFS_SB_FEAT_RO_COMPAT_INOBTCNT)
>> +		 XFS_SB_FEAT_RO_COMPAT_INOBTCNT | \
>> +		 XFS_SB_FEAT_RO_COMPAT_ATOMICWRITES)
>>   #define XFS_SB_FEAT_RO_COMPAT_UNKNOWN	~XFS_SB_FEAT_RO_COMPAT_ALL
>>   static inline bool
>>   xfs_sb_has_ro_compat_feature(
>> @@ -1085,16 +1087,18 @@ static inline void xfs_dinode_put_rdev(struct xfs_dinode *dip, xfs_dev_t rdev)
>>   #define XFS_DIFLAG2_COWEXTSIZE_BIT   2  /* copy on write extent size hint */
>>   #define XFS_DIFLAG2_BIGTIME_BIT	3	/* big timestamps */
>>   #define XFS_DIFLAG2_NREXT64_BIT 4	/* large extent counters */
>> +#define XFS_DIFLAG2_ATOMICWRITES_BIT 6
> 
> Needs a comment here ("files flagged for atomic writes"). 

ok

> Also not sure
> why you skipped bit 5, though I'm guessing it's because the forcealign
> series is/was using it?

Right, I'll fix that

> 
>>   #define XFS_DIFLAG2_DAX		(1 << XFS_DIFLAG2_DAX_BIT)
>>   #define XFS_DIFLAG2_REFLINK     (1 << XFS_DIFLAG2_REFLINK_BIT)
>>   #define XFS_DIFLAG2_COWEXTSIZE  (1 << XFS_DIFLAG2_COWEXTSIZE_BIT)
>>   #define XFS_DIFLAG2_BIGTIME	(1 << XFS_DIFLAG2_BIGTIME_BIT)
>>   #define XFS_DIFLAG2_NREXT64	(1 << XFS_DIFLAG2_NREXT64_BIT)
>> +#define XFS_DIFLAG2_ATOMICWRITES	(1 << XFS_DIFLAG2_ATOMICWRITES_BIT)
>>   
>>   #define XFS_DIFLAG2_ANY \
>>   	(XFS_DIFLAG2_DAX | XFS_DIFLAG2_REFLINK | XFS_DIFLAG2_COWEXTSIZE | \
>> -	 XFS_DIFLAG2_BIGTIME | XFS_DIFLAG2_NREXT64)
>> +	 XFS_DIFLAG2_BIGTIME | XFS_DIFLAG2_NREXT64 | XFS_DIFLAG2_ATOMICWRITES)
>>   
>>   static inline bool xfs_dinode_has_bigtime(const struct xfs_dinode *dip)
>>   {
>> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
>> index 4a9e8588f4c9..28a98130a56d 100644
>> --- a/fs/xfs/libxfs/xfs_sb.c
>> +++ b/fs/xfs/libxfs/xfs_sb.c
>> @@ -163,6 +163,8 @@ xfs_sb_version_to_features(
>>   		features |= XFS_FEAT_REFLINK;
>>   	if (sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_INOBTCNT)
>>   		features |= XFS_FEAT_INOBTCNT;
>> +	if (sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_ATOMICWRITES)
>> +		features |= XFS_FEAT_ATOMICWRITES;
>>   	if (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_FTYPE)
>>   		features |= XFS_FEAT_FTYPE;
>>   	if (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_SPINODES)
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index 1fd94958aa97..0b0f525fd043 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -65,6 +65,26 @@ xfs_get_extsz_hint(
>>   	return 0;
>>   }
>>   
>> +/*
>> + * helper function to extract extent size
> 
> How does that differ from xfs_get_extsz_hint?

The idea of this function is to return the guaranteed extent alignment, 
and not just the hint

> 
>> + */
>> +xfs_extlen_t
>> +xfs_get_extsz(
>> +	struct xfs_inode	*ip)
>> +{
>> +	/*
>> +	 * No point in aligning allocations if we need to COW to actually
>> +	 * write to them.
> 
> What does alwayscow have to do with untorn writes?

Nothing at the moment, so I'll remove this.

> 
>> +	 */
>> +	if (xfs_is_always_cow_inode(ip))
>> +		return 0;
>> +
>> +	if (XFS_IS_REALTIME_INODE(ip))
>> +		return ip->i_mount->m_sb.sb_rextsize;
>> +
>> +	return 1;
>> +}
> 
> Does this function exist to return the allocation unit for a given file?
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/commit/?h=djwong-wtf&id=b8ddcef3df8da02ed2c4aacbed1d811e60372006
> 

Yes, something like xfs_inode_alloc_unitsize() there.

What's the upstream status for that change? I see it mentioned in 
linux-xfs lore and seems to be part of a mega patchset.

>> +
>>   /*
>>    * Helper function to extract CoW extent size hint from inode.
>>    * Between the extent size hint and the CoW extent size hint, we
>> @@ -629,6 +649,8 @@ xfs_ip2xflags(
>>   			flags |= FS_XFLAG_DAX;
>>   		if (ip->i_diflags2 & XFS_DIFLAG2_COWEXTSIZE)
>>   			flags |= FS_XFLAG_COWEXTSIZE;
>> +		if (ip->i_diflags2 & XFS_DIFLAG2_ATOMICWRITES)
>> +			flags |= FS_XFLAG_ATOMICWRITES;
>>   	}
>>   
>>   	if (xfs_inode_has_attr_fork(ip))
>> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
>> index 97f63bacd4c2..0e0a21d9d30f 100644
>> --- a/fs/xfs/xfs_inode.h
>> +++ b/fs/xfs/xfs_inode.h
>> @@ -305,6 +305,11 @@ static inline bool xfs_inode_has_large_extent_counts(struct xfs_inode *ip)
>>   	return ip->i_diflags2 & XFS_DIFLAG2_NREXT64;
>>   }
>>   
>> +static inline bool xfs_inode_atomicwrites(struct xfs_inode *ip)
> 
> I think this predicate wants a verb in its name, the rest of them have
> "is" or "has" somewhere:
> 
> "xfs_inode_has_atomicwrites"

ok, fine.

Note that I was copying xfs_inode_forcealign() in terms of naming.

> 
>> +{
>> +	return ip->i_diflags2 & XFS_DIFLAG2_ATOMICWRITES;
>> +}
>> +
>>   /*
>>    * Return the buftarg used for data allocations on a given inode.
>>    */
>> @@ -542,7 +547,9 @@ void		xfs_lock_two_inodes(struct xfs_inode *ip0, uint ip0_mode,
>>   				struct xfs_inode *ip1, uint ip1_mode);
>>   
>>   xfs_extlen_t	xfs_get_extsz_hint(struct xfs_inode *ip);
>> +xfs_extlen_t	xfs_get_extsz(struct xfs_inode *ip);
>>   xfs_extlen_t	xfs_get_cowextsz_hint(struct xfs_inode *ip);
>> +xfs_extlen_t	xfs_get_atomicwrites_size(struct xfs_inode *ip);
>>   
>>   int xfs_init_new_inode(struct mnt_idmap *idmap, struct xfs_trans *tp,
>>   		struct xfs_inode *pip, xfs_ino_t ino, umode_t mode,
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index f02b6e558af5..c380a3055be7 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -1110,6 +1110,8 @@ xfs_flags2diflags2(
>>   		di_flags2 |= XFS_DIFLAG2_DAX;
>>   	if (xflags & FS_XFLAG_COWEXTSIZE)
>>   		di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
>> +	if (xflags & FS_XFLAG_ATOMICWRITES)
>> +		di_flags2 |= XFS_DIFLAG2_ATOMICWRITES;
>>   
>>   	return di_flags2;
>>   }
>> @@ -1122,10 +1124,12 @@ xfs_ioctl_setattr_xflags(
>>   {
>>   	struct xfs_mount	*mp = ip->i_mount;
>>   	bool			rtflag = (fa->fsx_xflags & FS_XFLAG_REALTIME);
>> +	bool			atomic_writes = fa->fsx_xflags & FS_XFLAG_ATOMICWRITES;
>>   	uint64_t		i_flags2;
>>   
>> -	if (rtflag != XFS_IS_REALTIME_INODE(ip)) {
>> -		/* Can't change realtime flag if any extents are allocated. */
> 
> Please augment this comment ("Can't change realtime or atomicwrites
> flags if any extents are allocated") instead of deleting it.

I wasn't supposed to delete that - will remedy.

>  This is
> validation code, the requirements should be spelled out in English.
> 
>> +
>> +	if (rtflag != XFS_IS_REALTIME_INODE(ip) ||
>> +	    atomic_writes != xfs_inode_atomicwrites(ip)) {
>>   		if (ip->i_df.if_nextents || ip->i_delayed_blks)
>>   			return -EINVAL;
>>   	}
>> @@ -1146,6 +1150,17 @@ xfs_ioctl_setattr_xflags(
>>   	if (i_flags2 && !xfs_has_v3inodes(mp))
>>   		return -EINVAL;
>>   
>> +	if (atomic_writes) {
>> +		if (!xfs_has_atomicwrites(mp))
>> +			return -EINVAL;
>> +
>> +		if (!rtflag)
>> +			return -EINVAL;
>> +
>> +		if (!is_power_of_2(mp->m_sb.sb_rextsize))
>> +			return -EINVAL;
> 
> Shouldn't we check sb_rextsize w.r.t. the actual block device queue
> limits here?  I keep seeing similar validation logic open-coded
> throughout both atomic write patchsets:
> 
> 	if (l < queue_atomic_write_unit_min_bytes())
> 		/* fail */
> 	if (l > queue_atomic_write_unit_max_bytes())
> 		/* fail */
> 	if (!is_power_of_2(l))
> 		/* fail */
> 	/* ok */
> 
> which really should be a common helper somewhere.

I think that it is a reasonable comment about duplication the atomic 
writes checks for the bdev and iomap write paths - I can try to improve 
that.

But the is_power_of_2(mp->m_sb.sb_rextsize) check is to ensure that the 
extent size is suitable for enabling atomic writes. I don't see a point 
in checking the bdev queue limits here.

> 
> 		/*
> 		 * Don't set atomic write if the allocation unit doesn't
> 		 * align with the device requirements.
> 		 */
> 		if (!bdev_validate_atomic_write(<target blockdev>,
> 				XFS_FSB_TO_B(mp, mp->m_sb.sb_rextsize))
> 			return -EINVAL;
> 
> Too bad we have to figure out the target blockdev and file allocation
> unit based on the ioctl in-params and can't use the xfs_inode helpers
> here.

I am not sure what bdev_validate_atomic_write() would even do. If 
sb_rextsize exceeded the bdev atomic write unit max, then we just cap 
reported atomic write unit max in statx to that which the bdev reports 
and vice-versa.

And didn't we previously have a concern that it is possible to change 
the geometry of the device? If so, not much point in this check.

Thanks,
John