Received-SPF: pass (google.com: domain of linux-kernel+bounces-95551-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <6dbb5eac-1e3a-4d2d-8e74-38f6aac1b06b@oracle.com>
Date: Thu, 7 Mar 2024 12:42:58 +0000
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 11/14] fs: xfs: Support FS_XFLAG_ATOMICWRITES for
 forcealign
Content-Language: en-US
To: Dave Chinner <david@fromorbit.com>
Cc: djwong@kernel.org, hch@lst.de, viro@zeniv.linux.org.uk, brauner@kernel.org,
        jack@suse.cz, chandan.babu@oracle.com, axboe@kernel.dk,
        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,
        ritesh.list@gmail.com, linux-block@vger.kernel.org
References: <20240304130428.13026-1-john.g.garry@oracle.com>
 <20240304130428.13026-12-john.g.garry@oracle.com>
 <ZejjjaTFPi+qEDgS@dread.disaster.area>
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ZejjjaTFPi+qEDgS@dread.disaster.area>
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?dkNHT2t4eWdNdXJSMFdkVnc0bTNPRXJnVGNoM255eGsvSmVBNkUwdkVTdHA2?=
 =?utf-8?B?cVkyc0d0QzdPWGpCbEQzeDBkR2U5b2txVFZITFZxTTcyVHJldlVnR1kyNFNl?=
 =?utf-8?B?UzY4bFg3SWZESXdFUjVVZC82czFMUE50M2tRNjZESlJBY1dEWGN3Z2RNVHk0?=
 =?utf-8?B?OWJTcG5zS1YwelcwZWVzQmt2Y2VpMkRrRVBrbk5Tem92ajZ5Z3Yweks4U2Vq?=
 =?utf-8?B?czFQS3RIeWd6ZTAwZVJ0d2R1Q3ZEN1pIbWlDNkZzcDBSQlNsNFJBamFGT24w?=
 =?utf-8?B?dEFnWjlYUWxXR0R3Qi9CT2kzSXhnNUVQb05Bc3JlUFp5d25JVERTUjZSZU13?=
 =?utf-8?B?WHZwa01kZ2hKSWpVWTVEQWFraFBBcEtZbDdITFF1QVVYbUgxMXQyaEVBOWVa?=
 =?utf-8?B?MnIzdm9lUUo5R01yQ1I0aUg3OXVuQlJZZG1JWS8rdEFZOTBnaHJCRWYvZ3Jy?=
 =?utf-8?B?ZGh4N2JmblIrbGFJUVZKb05lb2lvQm1WN0JUZlpFb3h6UlR4bXhNaU9BdnEv?=
 =?utf-8?B?eWgzL2NqMFFaYVk1Z3RsODBPeDg4SFpacmM0cnIzbTdkdDJnbXhGTjRlZSsy?=
 =?utf-8?B?bjllZFJweXU2NTZxNmROcFo4TlNrM3V0ZGFsYUJBTlpFbVZmZWtqNU1DS3VW?=
 =?utf-8?B?UDVMZ1F0UlA1bUtNUmRzM0M5T1ZZT3BBaUNFTXlra2hUdnBiQkppdDhrRTRs?=
 =?utf-8?B?cG9ibGg2V2xlMy9rUXdmR0R0QTBOeHpOczB6NXIzMVBzTDlSS0tnRHBWaG1R?=
 =?utf-8?B?ZXJ1YzB2dndJemlESkNjbHVpSndTK3FDSFdneHlqTkhzb1MwcDZwL1kxZTds?=
 =?utf-8?B?TWtSaThRU2ZOWHM2bTg3bDJRZ0VaaTFkcXJvNW1ySEpqTlUrUHV4WE1ENmdC?=
 =?utf-8?B?UW1HQXQ0RGVHZi9zVjBJUU1YOTRENlJSSUF1Y3hmelo5TGsxbDBwV09RSHhj?=
 =?utf-8?B?blVVTC9uWklwNGVxZ1VDRVFpd2FLV3A2d1RTVmZ0elBYTXJEbmNpYXlqQ0th?=
 =?utf-8?B?dkpJaFVCL0F4YUNESDFJS0VmSDJWeEhSVU9rR1NFbHBiYXZtRTVGRUFKT3Ji?=
 =?utf-8?B?ZU1DeGpVRmxXTDg3bUJla2J3Z1BvODVCWnhPL2VyKytuVHg4NjBLUFN4Rm1m?=
 =?utf-8?B?VlhESDE0UzUyVDZUeHdEeDdnRHc3T0dwZjNRWUhVdURLTVBSUjZ0aGNVaTFl?=
 =?utf-8?B?ekZIVEN3MVViem54dXR2b3RkRmJna1R1Wm9hazA1ZldyY0VwZks3VjVYRmF2?=
 =?utf-8?B?VWsyczJua0NycjRPNGFxa0VBTS9PUWswWWZiTmxTelp1d2NCcVU3aVI3ZWwr?=
 =?utf-8?B?N0F2RG9IQXl3VGFFRi9IL2hiTmoxMUxqMEEwUWNrMndpVS9tUlRuZXFvQUtM?=
 =?utf-8?B?aExMR0NSTlBwVEUrUGtqRDhYeXFBMGZtZmNSQU91WHNvRGR5R1FVbDlLZEJS?=
 =?utf-8?B?MWVuaVFmZ0Z5YkFxay9iTEtUTEc0QlprSVJVWkpGajdsZEgxbHh6RXl0d2xE?=
 =?utf-8?B?U3dsekxVTDczcUFwL1l0TDRlNnFMdUZZN3JXUFpvNnQ3d3BDWEdieEd1ZndU?=
 =?utf-8?B?ZDlpY0t6bDJ0c1NoK25GZHlIajRWcFhDb0krQm1TMDc0dlIxcmkzMmUwL3VY?=
 =?utf-8?B?Yld2clNKS0svNHpBYUJia0NmemVEYmRVdm82akxZREx3RFNlTlpBZVZQNHRo?=
 =?utf-8?B?Y2ZLMnBoSUNnOWh4c2NweVNUU1A4NmdDcEZtNUtGaFlyaWVMQzk2M0FseWhG?=
 =?utf-8?B?cVFoclM0elY0MFJmcDhxUGpEM2MwdHhOYUZmSHY5SjVOQkIxV3dTbmtwNDg0?=
 =?utf-8?B?MTNubDNLcFhuVUplVnZsTWVwNFJqQ01BSnVJUlFDMGdoV0t4NmY1bFBLbUVM?=
 =?utf-8?B?YjQ1TmNOSktNMXlXaVlabEcvcGp2VTVPRHVzLzZpcTRtSXpkdmFqWTZTWEo3?=
 =?utf-8?B?bWlBeDZ6ZmRUZnJ3ZzRJUm1LY3N4ZTl2K25iQnk2T0x1NCtvSFVxTkpSOXMx?=
 =?utf-8?B?Mm1YMzF2aEtxSUZuV2xsRGJBWXpwSC9qZ3kzbUptdlVuQkVxUHdYS0FBQjVC?=
 =?utf-8?B?akNlaWFIUWpiUnRTd2NPQ0xySHFQS0l4eHVZWlZaS2xJSE9Kb3g4aHpuQVBv?=
 =?utf-8?Q?hTL43mkn5MBm8D7dAWQln1APn?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	IlUaL2AAH733bt3Avcn0h6SB7nxRnEVyIsDUmt7r/U6Lhbpt3AS762OYxw4ipdquqaJP4c1RMBpZU6mKL3zc1rPncvQVand8sXvjnLHNioR/rPX9ZoppZAiyZyNk61CvX73uqXj9zqX63qP3MZUYhjIMY/ZRPeqP9Cxshshxbd8BizfngTpaJW/cP9kuXlx335CqgEw461rk+q0/8Sc8BzJLOECcafyZHZxMVSeR6LNAs4+53/V6XVQfJPgM+t1wa1Lkg+BadhxVrkK6LKFZZ717wWyfYTw0D2oTvxYZvNC/1pJS0dqAOsvompeXxT7TL79Roe1EfEtLV8eWLucR+PgShXBHUSJ9ae0lSR5YbkXAW1HS8CdUrZI6Jrf1mg8qLrMtvaMjOa7RhvmGWsL/Lc5hPe2fz6WTpWI2EgQ3BQPw6/mcaym/S+akP5DhkHJTg1q3LRe5mJfRqMX197KS1eGWZg2Oopbho9rCWG0BCk3EHp9DfKxUWExlfFgzLNQC9l/oovRJaYsWQvVr+zEImCi5dY6EbrZg3o1M6UoByCONdXpwUECgMQ4nB1VmByHr5uX/1nwhR8YCtYuZrpmNip8m2yoiJ7zoHaOVt3Cz4IM=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d09bde67-c90f-4b83-6d73-08dc3ea42153
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Mar 2024 12:43:03.6302
 (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: 79H7r1t+qajILkBgWTLmH3CqElF/QRi+/xeU32Dupy/0yePS8BRBHH/BZEIaKlYOtEkNEcS7YKePQ44vBZKhyQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR10MB5995
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-03-07_08,2024-03-06_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxscore=0
 mlxlogscore=999 spamscore=0 adultscore=0 malwarescore=0 phishscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2403070087
X-Proofpoint-GUID: F7K6I1vSzNd7SLnXbgCZ0MqhSJcboqxz
X-Proofpoint-ORIG-GUID: F7K6I1vSzNd7SLnXbgCZ0MqhSJcboqxz


>>   #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_FORCEALIGN)
>> +		 XFS_SB_FEAT_RO_COMPAT_INOBTCNT| \
>> +		 XFS_SB_FEAT_RO_COMPAT_FORCEALIGN| \
> 
> Please leave a spave between the feature name and the '| \'.
> 

ok

>> +		 XFS_SB_FEAT_RO_COMPAT_ATOMICWRITES)
>> +

..

>>   }
>>   
>> +static inline bool xfs_inode_atomicwrites(struct xfs_inode *ip)
>> +{
>> +	return ip->i_diflags2 & XFS_DIFLAG2_ATOMICWRITES;
>> +}
> 
> I'd really like this to be more readable:
> xfs_inode_has_atomic_writes().
> 
> Same for the force align check, now that I notice it:
> xfs_inode_has_force_align().

ok, will change

> 
>> +
>>   /*
>>    * Return the buftarg used for data allocations on a given inode.
>>    */
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index 867d8d51a3d0..f118a1ae39b5 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -1112,6 +1112,8 @@ xfs_flags2diflags2(
>>   		di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
>>   	if (xflags & FS_XFLAG_FORCEALIGN)
>>   		di_flags2 |= XFS_DIFLAG2_FORCEALIGN;
>> +	if (xflags & FS_XFLAG_ATOMICWRITES)
>> +		di_flags2 |= XFS_DIFLAG2_ATOMICWRITES;
>>   
>>   	return di_flags2;
>>   }
>> @@ -1124,10 +1126,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. */
>> +	/* Can't change RT or atomic flags if any extents are allocated. */
>> +	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;
>>   	}
>> @@ -1164,6 +1168,13 @@ xfs_ioctl_setattr_xflags(
>>   			return -EINVAL;
>>   	}
>>   
>> +	if (atomic_writes) {
>> +		if (!xfs_has_atomicwrites(mp))
>> +			return -EINVAL;
> 
> That looks wrong - if we are trying to turn on atomic writes, then
> shouldn't this be returning an error if atomic writes are already
> configured?

I think that you are talking about a xfs_inode_atomicwrites() check.


> 
>> +		if (!(fa->fsx_xflags & FS_XFLAG_FORCEALIGN))
>> +			return -EINVAL;

> 
> Where's the check for xfs_has_atomicwrites(mp) here? 

please see above

> We can't allow
> this inode flag to be set if the superblock does not have the
> feature bit that says it's a known feature bit set.
> 
> Which reminds me: both the forcealign and the atomicwrite inode flag
> need explicit checking in the inode verifier. i.e. checking that if
> the inode flag bit is set, the relevant superblock feature bit is
> set.

We do have that in the xfs_has_atomicwrites() and xfs_has_forcealign() 
checks - is that ok?

> 
> ....
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index 74dcafddf6a9..efe4b4234b2e 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -1712,6 +1712,10 @@ xfs_fs_fill_super(
>>   		xfs_warn(mp,
>>   "EXPERIMENTAL forced data extent alignment feature in use. Use at your own risk!");
>>   
>> +	if (xfs_has_atomicwrites(mp))
>> +		xfs_warn(mp,
>> +"EXPERIMENTAL atomicwrites feature in use. Use at your own risk!");
> 
> "EXPERIMENTAL atomic write IO feature is in use. Use at your own risk!");
> 

ok

Thanks,
John