Received-SPF: pass (google.com: domain of linux-kernel+bounces-95319-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <07d53a91-5451-43e2-ae8c-0c863cf0ec9f@oracle.com>
Date: Thu, 7 Mar 2024 10:19:24 +0000
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 13/14] fs: xfs: Validate atomic writes
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-14-john.g.garry@oracle.com>
 <ZejersjddMNdkDqz@dread.disaster.area>
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ZejersjddMNdkDqz@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?dUh4a3UzY1h3TkRReGdVdlFiTWdOOHg3Y2lEUUpFdGd3RU03WnQ3RGNwS3Y5?=
 =?utf-8?B?QUlENkVOYVJPQ2l2c0Nmd2Z0UmRCZC9NTWhzdHlQOGR0WVBPOXl3RDFJbHZE?=
 =?utf-8?B?KzU1NnZaMzRwWU9GRlFIb0MxK3ZldHloNzZXVEphbU9aRnFmbWljMnNHQSt6?=
 =?utf-8?B?cTJHeUxreW84RVdyTkJzZExkVG9lUlNEQ2svN29CYzhWWHNPVkthRjV1NVcz?=
 =?utf-8?B?ZnB6cGJ3TlAvR24ydTJMTFczZW5zaVdLS2hUYTBlc2R0VlRmUWpVTFQ4d1k0?=
 =?utf-8?B?VitVRUxxcVVYRkRLQ2l2R01OdlpxaFhUT1NDZjFvZGEwenRpK20vSlNSRnpv?=
 =?utf-8?B?aVdBTmd4WXNmaStEbTh4cGxsN2lMSnZMcWorZU01TGZSZTNFNFpGYkkvYXBS?=
 =?utf-8?B?ckpnL1NDbE82SHEvSkNLVEVMeGllMnIvK2hsb3lmWkE3NjdYM2Z5cWdvTVdj?=
 =?utf-8?B?cTdoR1Nsak5zN0h3TmpRZjdzNmgzNEplSjBxc2ZTc0pvTWVWVkN1cFNTT1Fa?=
 =?utf-8?B?NHMvczVCUDFRelg5R0dmTXJvYzZUZDZ1Qm1FajMvZDhvTCs0M21jUzZncFJp?=
 =?utf-8?B?RmZPS2VLdUpZMUsyYnlER2xkczFueVpzc2psUDd4Z082Y1JZMzRWbTBjN21V?=
 =?utf-8?B?dys5cW1RVFlGWEJHREFaRExSYlRvb2lpNzQ2aVg3L0NEY3RkcFRkT0J6U2dF?=
 =?utf-8?B?S2lyVGhxdHNzSXJZUURlSWRvRGhSSCtaNVRmZ1l2TWVjejlnOVowekpoVDRa?=
 =?utf-8?B?YTdyUVNCeWxpck1QRlBWNTdGSTM4NlQrdlNBV0dhNElSSE9mNEVoaXcyMVQz?=
 =?utf-8?B?dkFNamFmUjluVUNwYVd3ZmhtdmdNd3BTQ2ZleXltREc5ZGpyZGhGU1VDaTRa?=
 =?utf-8?B?bmZKSVJ1U0N5STZsbExCTHhCNTZwYVBWZjgrcWc0MGdYQjNyTFZmQjBYUlZ6?=
 =?utf-8?B?czFkRE41a3VPM2JyMUlCblFrZXNWN0hBTnlBNndKNWRIQ1dHNlRrVDZZOGd2?=
 =?utf-8?B?UXFhVHQ4b3lJMUpxeUJzMEpJNEkzSkoxaURjYjArUENicnBBUHBFaEN5aEdB?=
 =?utf-8?B?YzJ4eWtLb2RLWFVEOGE3RUVoVHp3WWxKVjBWL3F5b2d5b2Nmd2E1SU9SYnhJ?=
 =?utf-8?B?TWl0cGUxTXhNcGNyOEFYUjlMZzAzeXlTZWxpQkhRWWpjWm9vd2IyZ3ozdzNl?=
 =?utf-8?B?RUxYTW9UUWVxdTVrKzduM2NrK055NjlhZDErWmMrYzl4L05TMTEva2FDNURC?=
 =?utf-8?B?ZnZQdGVVMFZHOENRYXlvak9Yd0RQLzRBdDh1OXlzRTc1RVdGODVraTM1VGpz?=
 =?utf-8?B?dnFlUkNrTmRUM0p6UWF5VTdWWGYzWnQrdENvY3RmVGUwWGtEOTlicDBKMTcv?=
 =?utf-8?B?ZFBieUZxYWhubVZOMU03blM2ZVpscXZHaEtKZGJrcGZoRlZBYUorcU5JWUdH?=
 =?utf-8?B?RWZXVGRRN3JRb3BWT2xvbE5wcnU2Rzg5ejU3ajBEc0g1TWM0TUpiaEl3TGJI?=
 =?utf-8?B?L0xvQlhTSDkvbmovaU9Rb0JiaE5XaDJjbHFTR1BOOWU2L3c2VHMzNFBJaFNW?=
 =?utf-8?B?bXFkTUlkZWpUbXRRcDRLSDl0dy8vTmFpaHh0YzZKYlhYd0o0VVhWSUkyN21X?=
 =?utf-8?B?WTZheEx3WE13cm9lUXlZQ2RtVVRYSWlrY09kZXBacTZ6c3MwWkgxRyt0ZVps?=
 =?utf-8?B?UjlLdnZBbW5obkEyNG5CbFZkTFRTT1E0M3NjUGFSUGdVZ1d4NWI4WVBLNzJU?=
 =?utf-8?B?YVdkZVA4SzRMNmVZNEk1OE40T0t0QUlvV29GeEZTTDJKdUlubG5NdXpoY3NW?=
 =?utf-8?B?Y24yTFU1UEo2SDlDM2ZVUGN1eEYvVVdmbStmUlYyMjFxV2hoV2pBdjdmVUht?=
 =?utf-8?B?MmRjRUF2dDU3bDhGbXhKZ3lpMlhBeXBnNmttU2kybTBLOXk0WmsyWlFiZnQ1?=
 =?utf-8?B?SzIyMmtOVkxUSmhpaGRzZWVZbTFFNld0Z08xMUU2RWtGM3dZZ3Fua3ZDM1NY?=
 =?utf-8?B?cHBtR2ZJNUZsTlEza1RnOTZ6Z3NmSjJ3Y0E1UG92Qkk4YTdzeGRjV0UxaFJs?=
 =?utf-8?B?K2dUdkptY1cyZG45VEVyUjRNcC8wL09STTVVSDUrQ3NLNmxkeUN5UndVL2Vo?=
 =?utf-8?B?ZlRnNTd3MWsvLzk4VVEvdkQ2MHZQZ1QwcVRDQ25wRkQ0QjdzMG01SHRvcGQv?=
 =?utf-8?B?YVE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	PCJvkbiBKNOO1c9Wp/I0vVyrU9PkBeIU3QibwCgZN7MZ0rqtp5XzbHEV3EA4EyrQ1yVB7RoeiDiVohOzymo4EDn+6GjSofD6RvacuALS5UwKBkl1Z9tgXKhi4S7Geyq0GhtQut1ilC5MqF1Hk6dbX/rdSrDqzryBdOaWant4nUq/FA4YlXrlOtnPbhmTtPt4yUNdO9jdoy42lRDJQNZv2++V6sSTKKLzVO/d+Y06D4j9RmiSQOmj+H2Csmg4VzstRFMKhTPg7ycyHF/q+Ts93dbkKZWrpm3uFI8/MLjTDJ6fnuoqu1WMoRON5pMJegC3HDfXXYhunwjTfGndaaHdP0EBUgeZNpBe1FrI/151BZtnxY4F7+3FIcsTtW1O3i+zXS/eSpj4yAzzMv8pM/qYhuUCOMoYt/hfsDbW1xO33qUz69rOPfnne21BBa/E++/utbaAifA2voZl7F0Wx6DsHZuY6ghpSF4CHLDUehn3aGknddjLmDwDdXn7iVnWDAZg7aOs3OJtzi2OYfiDDY0YJMmd/moxWihHwScflHJSHBVRAjbdeBpG8w5RYL20vt7nJZ8Qz6UDWEdhJg27jeGI7NnyLKb9bfKnGKBpCbKMd+Q=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 68b37d82-1ea8-49b4-3157-08dc3e9012ca
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Mar 2024 10:19:29.2913
 (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: DPe5RDBPAjeYNXoBmSBfQMo5fGrMh50FLdjBEyY/hKtMN3zukbklKCMtnyoxycWXWcZ02hTAk73X1Z+shyc50g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR10MB5900
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_06,2024-03-06_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 adultscore=0
 mlxlogscore=999 suspectscore=0 spamscore=0 malwarescore=0 mlxscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2403070075
X-Proofpoint-GUID: 6nQ78oHlel_xXKa7-dN0f2Qho4ueciz9
X-Proofpoint-ORIG-GUID: 6nQ78oHlel_xXKa7-dN0f2Qho4ueciz9

On 06/03/2024 21:22, Dave Chinner wrote:
> On Mon, Mar 04, 2024 at 01:04:27PM +0000, John Garry wrote:
>> Validate that an atomic write adheres to length/offset rules. Since we
>> require extent alignment for atomic writes, this effectively also enforces
>> that the BIO which iomap produces is aligned.
>>
>> Signed-off-by: John Garry<john.g.garry@oracle.com>
>> ---
>>   fs/xfs/xfs_file.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
>> index d0bd9d5f596c..cecc5428fd7c 100644
>> --- a/fs/xfs/xfs_file.c
>> +++ b/fs/xfs/xfs_file.c
>> @@ -709,11 +709,20 @@ xfs_file_dio_write(
>>   	struct kiocb		*iocb,
>>   	struct iov_iter		*from)
>>   {
>> -	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
>> +	struct inode		*inode = file_inode(iocb->ki_filp);
>> +	struct xfs_inode	*ip = XFS_I(inode);
>>   	struct xfs_mount	*mp = ip->i_mount;
>>   	struct xfs_buftarg      *target = xfs_inode_buftarg(ip);
>>   	size_t			count = iov_iter_count(from);
>>   
>> +	if (iocb->ki_flags & IOCB_ATOMIC) {
>> +		if (!generic_atomic_write_valid(iocb->ki_pos, from,
>> +			i_blocksize(inode),
> a.k.a. mp->m_bsize. If you use that here, then the need for the VFS
> inode goes away, too.

ok

> 
>> +			XFS_FSB_TO_B(mp, xfs_get_extsz(ip)))) {
>> +			return -EINVAL;
>> +		}
>> +	}
> Also, I think the checks are the wrong way around here. We can only
> do IOCB_ATOMIC IO on force aligned/atomic write inodes, so shouldn't
> we be checking that first,

We are checking that, but not here.

In 14/14, we only set FMODE_CAN_ATOMIC_WRITE for when 
xfs_inode_has_atomicwrites() is true, and only when 
FMODE_CAN_ATOMIC_WRITE is set can we get this far.

I don't see a point in duplicating this xfs_inode_has_atomicwrites() 
check, so I will make the commit message clearer on this - ok? Or add a 
comment.

> then basing the rest of the checks on the
> assumption that atomic writes are allowed and have been set up
> correctly on the inode? i.e.
> 
> 	if (iocb->ki_flags & IOCB_ATOMIC) {
> 		if (!xfs_inode_has_atomicwrites(ip))
> 			return -EINVAL;
> 		if (!generic_atomic_write_valid(iocb->ki_pos, from,
> 				mp->m_bsize, ip->i_extsize))
> 			return -EINVAL;
> 	}
> 
> because xfs_inode_has_atomicwrites() implies ip->i_extsize has been
> set to the required atomic IO size?

I was not too comfortable using ip->i_extsize, as this can be set 
without forcealign being set. I know that we would not get this far 
without forcealign (being set).

Having said that, I don't like all the xfs_get_extsz() calls, so 
something better is required. Let me know you you think.

Thanks,
John