Received-SPF: pass (google.com: domain of linux-kernel+bounces-165120-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: <cc54060a-2dc3-45e4-b47c-a9926553e59b@oracle.com>
Date: Wed, 1 May 2024 11:03:06 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 08/21] xfs: Introduce FORCEALIGN inode flag
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, willy@infradead.org,
        axboe@kernel.dk, martin.petersen@oracle.com,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        tytso@mit.edu, jbongio@google.com, ojaswin@linux.ibm.com,
        ritesh.list@gmail.com, mcgrof@kernel.org, p.raghav@samsung.com,
        linux-xfs@vger.kernel.org, catherine.hoang@oracle.com
References: <20240429174746.2132161-1-john.g.garry@oracle.com>
 <20240429174746.2132161-9-john.g.garry@oracle.com>
 <ZjF9RVetf+Xt70BX@dread.disaster.area>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ZjF9RVetf+Xt70BX@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?ZTA1YjNiTlJwbGZicUE0MHVxZGhIN2JRbXM1WXUxVERJUVpMaUpocmlRbnBj?=
 =?utf-8?B?TnQreU11NVdLZ2pPaUtsNEtpQTFUdFVKYmQwZXhRVWZDTUdtRnllb0JRTC9x?=
 =?utf-8?B?b01jYVZGTVJIdkpOR2tCNjBLbDlhRHF0R3VvamhNZEhCVTNhcUNxOGdVWE03?=
 =?utf-8?B?YWFjd09aWnRlQTFkZy95K1J2Mkpxd0Z2L1pBbjNoTVR0alUwdG82Sm52UE5k?=
 =?utf-8?B?dzUyM3g2dEhVYTdDemYzK0NOMDZjUEFOUlJCcUd4cjNMQjh6MVdRYW5qUGNq?=
 =?utf-8?B?S0RlYVZ4bEFmb0VBMEhpZGgwbFM0T09jdXlWZy8ycHAzV0RadEJ1VGpNMjUx?=
 =?utf-8?B?UkZTeFo0d1NYZzZQUWJMeDF6WmVxaEJmNmRMZkorQ3ZQWUxpcUM3ek0zakFZ?=
 =?utf-8?B?emZlMUplL2VFRjNqVjFhR2dRdkQ1dnpuVnZ0bnBhc2FDRUlkSlI2WWdURUZY?=
 =?utf-8?B?VkltVnpyR0lBak42dVAwTWs0bC95cG82NTh5U1h6TEZqQUt1MVFtamNldTds?=
 =?utf-8?B?UDBia3FVVUpiMDVORjJ4TElFNGtOcW83WEFjdkZwZjAyOXJ3N1ArV1hEQmpP?=
 =?utf-8?B?QnJEbDlJOG1OK1dqYkV1SWJ5MXVmaGhmVkVqYWRyRlBTMXkxYXR4ZkMzc3ky?=
 =?utf-8?B?bVIxWTVONUZMcjdDQjRFeXo5TW92TWtEZ3hsOENuS3JhTGVzT1gwdll6d2lM?=
 =?utf-8?B?QjNDajRuT1hQcDMvdm1OWWhDd2ErbUpybldNK3pWeGtsSVgyY2RUS2pCbUNj?=
 =?utf-8?B?ZmtyMUxKdUhEcklGV1BSMStDRHhESjlqaEpZWEw3Mm9FRExqbU5wMzFnMlVk?=
 =?utf-8?B?dUpwdFRoc1ZHSHFHakVKRVFvOVpBaDBsSnRVY3FCL2ZkT1VXem5PYkNhMTk5?=
 =?utf-8?B?YTZzOWIvUGZHbHVBNHlqL2hMMFRaditNUE42WmFwVytBcVhYdHlTbTNqcVlp?=
 =?utf-8?B?RjZoMVdBWjgzQlY3RjR6Wk5VRjljNkxodS9aaWZVMUN4ZVMvRVYwenByWW9N?=
 =?utf-8?B?QzJRek5wWjRnWktUMnBJVHIremFrWWtvWEpJczREWEZTWTd3aVBIcVhERHRN?=
 =?utf-8?B?K3Y3bURWSHFyTWRhWHlTeGxQeXMydk93OUZabDluR3NqN0hKYUo0cXFBcE1D?=
 =?utf-8?B?WGl5OXVqelRUZG5BbHBIL3Q0a3hGUjFnTXVFT05yNkFZOTN6VmJRUDJWTTJp?=
 =?utf-8?B?Vmo0ZFVYRStYZXBVaGZaZk1tdEhOSGVmd1RlNEozQ1R5OUtyUGZmZEdxTW91?=
 =?utf-8?B?RGhrMkNLdTA5aERzV0JwdXcyMUFKMWlGTGI4WG1waFRUNXVQQTJybVRtN0E3?=
 =?utf-8?B?ZHFielZwVmdtc2RXVEF1dUZtWkY0Yko0cE51MThQUlVoWFZGSktiUXNSM1M3?=
 =?utf-8?B?UXl2VlY3M2JVTTZGV0hHODZlZU5OQTRoeGdHTVQ0WmM3QjZpUjRpcmpsM2RP?=
 =?utf-8?B?dWx4RVI5TmVsZCtIaGdkL3BralN5bkh2anNsY2RCZTd3Q3ozMXR4ajdheVNa?=
 =?utf-8?B?UmVjOHg5bzZDVVNjdzVSZGtLdHpqcHN1VmpBTHJaellyTDBaelVtY2FyTjNB?=
 =?utf-8?B?blNnTU5SYWNxeWVSZFRHWkhuVVAvdHNNcWVIbkxsR2FCWWZOV0NYWkVQN2Zy?=
 =?utf-8?B?VDg1ZHU3SjMxSUN6UlhhejlUWmtwMWFpY2JGN0xqS0puUUY3MnRXcFhDSnhi?=
 =?utf-8?B?aHV0K1czRDJIc3M0NGhDWXVrajdLOW1GK0ZuaDA0djJrQTJ5Q1lXbi91TDhK?=
 =?utf-8?B?ZmVSSUI3UUtoRkVuZ25SckMyVldMdDBaRWVidll4SFozaUEraDRDQzZNUzNW?=
 =?utf-8?B?clpvZGtUTUQ1d3VWdERLWkcyd0JwUXF5Z05RaWUzQ0FjY0NMcjI5bUlveDVR?=
 =?utf-8?B?OVRJd3hLUE5tK28wWTlQV2YzTG5IMGJwVlJrc093V0ovNHhjMlNXeTJqcGx5?=
 =?utf-8?B?QXdSc0owWUt3TURmcURBL1VONTczSTNua1ZiMTduZXJkYThVSVBpcm9YeHIx?=
 =?utf-8?B?MUdWVjE5N2Y4bW1SYWtZL2lkemlEZEpBYTAvcklqYmRtakk4cnl2Z1ZHcVQr?=
 =?utf-8?B?NGR0QnRWcE5iZ1ZEVWY1cTVIL1dhdnlhVXV0QnBRSHRxV3ZGUmswQ2pTUkhP?=
 =?utf-8?B?OGhIc0IzMU5jMEFReDZQS25GNU5PNmlsbzFhR3BqUE5PRHBYWWZhYmZBeExW?=
 =?utf-8?B?bWc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	ypXDGhfsDyQ8fHTcczdW3sA29flHhgmbfiqVTizVE5m/StFKDgvqwwhwg5R2zKpwCDvN6qo+KqM/SX3oi2RuaBKObKRR1L0AdfofpYmgOqbCuKI8Q14moWlAjf/OyltmMQPbuKKefogVc6Tuh7yAUXqJ1EULuDC0e6MXB9aQb4+uKbT8tGsKDJV1gimRx95kuwMOda1FqUusme5zDytRSBTl1pBKAaOcg9MFMD+8nrb4r89xgEzfOVELIrFlBmdOCK8/BDB0yDqWFxfNmyqYUHGhYbvwpmDa/yaR6AmpxwpaN67Nxs20OBp+GYmxtJtZDpONd33xXv1572WHaPmzDNkOaXubMfrNmPxJF4uIYewyUQ74aVMm+LHrLLkKZXUu7S8+CM8gP2M6dr+hMm0pvVEVhC3yZ3+8KvDbT6lniQSQIj3XC/HkrMIfWUfZ1Zz7qjDFySbfjyWNBzqR1G3ZQiyzuMXr890SopNkBtVNwznAAJt+8hANZSWbSTdj8VIYk51cg1i7G7H+e0lVvwFRXrOYfMjxxXRhihKCY8CnlNgtoFp3eWjdpb4wyuODt4EEdnfzOFLbF4LQQxV5ym7m6Ku0zhWjU09nzaods9TlMUo=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bda5cf96-6f01-488e-eda4-08dc69c5e8fd
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 May 2024 10:03:12.0261
 (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: va9ij/r+hAutPG5c2CmPo9eD6voK1eGpOZuvb86uyoX3jhlXxD6mHWcUVZkoWvpZnLCXbP+NAF8755B3ThF5bQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR10MB7051
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1011,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-05-01_09,2024-04-30_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 malwarescore=0
 mlxlogscore=999 bulkscore=0 suspectscore=0 adultscore=0 phishscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2405010070
X-Proofpoint-GUID: mQPRSZRA0YpluBNHp6TENZAf7OxyCgws
X-Proofpoint-ORIG-GUID: mQPRSZRA0YpluBNHp6TENZAf7OxyCgws


>> +/* Validate the forcealign inode flag */
>> +xfs_failaddr_t
>> +xfs_inode_validate_forcealign(
>> +	struct xfs_mount	*mp,
>> +	uint16_t		mode,
> 
> 	umode_t			mode,

ok. BTW, other functions like xfs_inode_validate_extsize() use uint16_t

> 
>> +	uint16_t		flags,
>> +	uint32_t		extsize,
>> +	uint32_t		cowextsize)
> 
> extent sizes are xfs_extlen_t types.

ok

> 
>> +{
>> +	/* superblock rocompat feature flag */
>> +	if (!xfs_has_forcealign(mp))
>> +		return __this_address;
>> +
>> +	/* Only regular files and directories */
>> +	if (!S_ISDIR(mode) && !S_ISREG(mode))
>> +		return __this_address;
>> +
>> +	/* Doesn't apply to realtime files */
>> +	if (flags & XFS_DIFLAG_REALTIME)
>> +		return __this_address;
> 
> Why not? A rt device with an extsize of 1 fsb could make use of
> forced alignment just like the data device to allow larger atomic
> writes to be done. I mean, just because we haven't written the code
> to do this yet doesn't mean it is an illegal on-disk format state.

ok, so where is a better place to disallow forcealign for RT now (since 
we have not written the code to support it nor verified it)?

> 
>> +	/* Requires a non-zero power-of-2 extent size hint */
>> +	if (extsize == 0 || !is_power_of_2(extsize) ||
>> +	    (mp->m_sb.sb_agblocks % extsize))
>> +		return __this_address;
> 
> Please do these as indiviual checks with their own fail address.

ok

> That way we can tell which check failed from the console output.
> Also, the agblocks check is already split out below, so it's being
> checked twice...
> 
> Also, why does force-align require a power-of-2 extent size? Why
> does it require the extent size to be an exact divisor of the AG
> size? Aren't these atomic write alignment restrictions? i.e.
> shouldn't these only be enforced when the atomic writes inode flag
> is set?

With regards the power-of-2 restriction, I think that the code changes 
are going to become a lot more complex if we don't enforce this for 
forcealign.

For example, consider xfs_file_dio_write(), where we check for an 
unaligned write based on forcealign extent mask. It's much simpler to 
rely on a power-of-2 size. And same for iomap extent zeroing.

So then it can be asked, for what reason do we want to support 
unorthodox, non-power-of-2 sizes? Who would want this?

As for AG size, again I think that it is required to be aligned to the 
forcealign extsize. As I remember, when converting from an FSB to a DB, 
if the AG itself is not aligned to the forcealign extsize, then the DB 
will not be aligned to the forcealign extsize. More below...

> 
>> +	/* Requires agsize be a multiple of extsize */
>> +	if (mp->m_sb.sb_agblocks % extsize)
>> +		return __this_address;
>> +
>> +	/* Requires stripe unit+width (if set) be a multiple of extsize */
>> +	if ((mp->m_dalign && (mp->m_dalign % extsize)) ||
>> +	    (mp->m_swidth && (mp->m_swidth % extsize)))
>> +		return __this_address;
> 
> Again, this is an atomic write constraint, isn't it?

So why do we want forcealign? It is to only align extent FSBs? Or to 
align extents to DBs? I would have thought the latter. If so, it seems 
sensible to do this check also.

> 
>> +	/* Requires no cow extent size hint */
>> +	if (cowextsize != 0)
>> +		return __this_address;
> 
> What if it's a reflinked file?

Yeah, I think that we want to disallow that.

> 
> .....
> 
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index d0e2cec6210d..d1126509ceb9 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_FORCEALIGN)
>> +		di_flags2 |= XFS_DIFLAG2_FORCEALIGN;
>>   
>>   	return di_flags2;
>>   }
>> @@ -1146,6 +1148,22 @@ xfs_ioctl_setattr_xflags(
>>   	if (i_flags2 && !xfs_has_v3inodes(mp))
>>   		return -EINVAL;
>>   
>> +	/*
>> +	 * Force-align requires a nonzero extent size hint and a zero cow
>> +	 * extent size hint.  It doesn't apply to realtime files.
>> +	 */
>> +	if (fa->fsx_xflags & FS_XFLAG_FORCEALIGN) {
>> +		if (!xfs_has_forcealign(mp))
>> +			return -EINVAL;
>> +		if (fa->fsx_xflags & FS_XFLAG_COWEXTSIZE)
>> +			return -EINVAL;
>> +		if (!(fa->fsx_xflags & (FS_XFLAG_EXTSIZE |
>> +					FS_XFLAG_EXTSZINHERIT)))
>> +			return -EINVAL;
>> +		if (fa->fsx_xflags & FS_XFLAG_REALTIME)
>> +			return -EINVAL;
>> +	}
> 
> What about if the file already has shared extents on it (i.e.
> reflinked or deduped?)

At the top of the function we have this check for RT:

	if (rtflag != XFS_IS_REALTIME_INODE(ip)) {
		/* Can't change realtime flag if any extents are allocated. */
		if (ip->i_df.if_nextents || ip->i_delayed_blks)
			return -EINVAL;
	}

Would expanding that check for forcealign also suffice? Indeed, later in 
this series I expanded this check to cover atomicwrites (when I really 
intended it for forcealign).

> 
> Also, why is this getting checked here instead of in
> xfs_ioctl_setattr_check_extsize()?
> 
> 
>> @@ -1263,7 +1283,19 @@ xfs_ioctl_setattr_check_extsize(
>>   	failaddr = xfs_inode_validate_extsize(ip->i_mount,
>>   			XFS_B_TO_FSB(mp, fa->fsx_extsize),
>>   			VFS_I(ip)->i_mode, new_diflags);
>> -	return failaddr != NULL ? -EINVAL : 0;
>> +	if (failaddr)
>> +		return -EINVAL;
>> +
>> +	if (new_diflags2 & XFS_DIFLAG2_FORCEALIGN) {
>> +		failaddr = xfs_inode_validate_forcealign(ip->i_mount,
>> +				VFS_I(ip)->i_mode, new_diflags,
>> +				XFS_B_TO_FSB(mp, fa->fsx_extsize),
>> +				XFS_B_TO_FSB(mp, fa->fsx_cowextsize));
>> +		if (failaddr)
>> +			return -EINVAL;
>> +	}
> 
> Oh, it's because you're trying to use on-disk format validation
> routines for user API validation. That, IMO, is a bad idea because
> the on-disk format and kernel/user APIs should not be tied
> together as they have different constraints and error conditions.
> 
> That also explains why xfs_inode_validate_forcealign() doesn't just
> get passed the inode to validate - it's because you want to pass
> information from the user API to it. This results in sub-optimal
> code for both on-disk format validation and user API validation.
> 
> Can you please separate these and put all the force align user API
> validation checks in the one function?
> 

ok, fine. But it would be good to have clarification on function of 
forcealign, above, i.e. does it always align extents to disk blocks?

Thanks,
John