Received-SPF: pass (google.com: domain of linux-kernel+bounces-165170-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <d39c46b7-185c-4175-b909-2ba307c177c9@oracle.com>
Date: Wed, 1 May 2024 12:08:34 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 17/21] iomap: Atomic write support
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-18-john.g.garry@oracle.com>
 <ZjGfOyJl5y3D49fC@dread.disaster.area>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ZjGfOyJl5y3D49fC@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?ajFLd29OZ1ZydTB0UW5ZakU0MmhsdzNrd0N5Yi8xK3Fma015VTZjWDZxYUl5?=
 =?utf-8?B?bTdXQXJickVMcVVvWCtVM0ZxcG9icW5PTCsxM3F4TCtSRjRnbDRDaG14cEtv?=
 =?utf-8?B?UUNCSERrMFhWVkNSbW9kZ2UvaUp1MFJiMUpvb1pocjcyVzVRbWNoYlVEbnAz?=
 =?utf-8?B?R3VuU0FuYTBsNlVlS0JTQjFZaFBWYlVwMWJEYzNhZFFMcFRUQ1NPSm1WaU1j?=
 =?utf-8?B?NGFLVGluRzVRbDlBLzhqMXMwUXpaNk1weUFhazl4Qi9heThLaVlxM0YwdUMv?=
 =?utf-8?B?WE9Qa3p5d1RrWTIydllQUXJJMUgzbHliRGNZL3BiWE9WdFgrdmhPL0NxUndR?=
 =?utf-8?B?MHdoeVdoOEdHL21vQlY5VTdGL1d6YlVCZERQckt2YjFKTTl6UEpnK2ZqMTFv?=
 =?utf-8?B?eGl0ODQ2czRsN3ZHM0JzZDdaa0x4bTVodXdldXo1NVVKUDl6WTdVUHI2VXY0?=
 =?utf-8?B?VE4rT01jaG9sVEhJaVlGcWQ0TFp5VzFwcnlJeG4zL2ordDRMVy9zOE15WXhT?=
 =?utf-8?B?WEFpaXh2aFFPSVEzTUdOZ1FCMlltZlM4c0p0aDlHQWtXbytlMGNqK1lRbEMx?=
 =?utf-8?B?eXZ4MGQxS0xnNnd5eWlzSmczSE1YMVl2MG9pQkk3QTlMY0FrL2V6VGwyQVlR?=
 =?utf-8?B?Vm1GVW1BdWozQ0xZUHdWWEtBY01xSkluTWZ6bm1yUTAwTEF2VG5xTXYvbjk1?=
 =?utf-8?B?MFVmK3NxYmZSTHdUTjh2NW1ORUJWanJicFJWVFVSTXF0cEI1a015SVJaYjZK?=
 =?utf-8?B?anZYdmsxOFFRRGdSNjdCcHRMbzVYTCtpbVZHVTIrdVBDdmFIYXk0RXVIUkdv?=
 =?utf-8?B?TThsMlRGQi9wNXNsanpFc2JTcFYybGF6QldHSnM4RUN4VFJVeDdvUXdnMTgy?=
 =?utf-8?B?ZXc3V21ITG1TRzlidVVLN3poRm15QnZ1THdZSktNR2lYUVNxRGo1cjdKNVhk?=
 =?utf-8?B?VFV4ckpZQmRtdklEa3AwV3ZVLzl3NU1ZSDNVVUNPbVVTendpMXBLTTdFelIz?=
 =?utf-8?B?RjFhU0lPSU5TSzNINmpYL0tobTAwdzlDWmJMSW5OZjBDS3d0SS9qMDBTM2ZC?=
 =?utf-8?B?QzBhREtWaGR4OXUwU2hTWmdHQ2lsNko0THIwdlBiRkdpamhsWDFoWGxtbmY4?=
 =?utf-8?B?bWpyLzZPMjZCTWJiRXN0K0ltcm16SGtjKzV4aFQzU3VRRWZuKzJ3WVl5ZmJl?=
 =?utf-8?B?ZS9TMjlXZmhySG5wVE8rOG1xUS9wTExHZ2s4elhPWTBvMytVcHpWQTBNYWZH?=
 =?utf-8?B?dk9GTlp2bnNQUUFrellGR0pyTHIyQnYwUUNkQWZFSDdQZ3lkVGhpWW5Ta2dk?=
 =?utf-8?B?WCt0WStOaHpjOHk1cDhVZjAxMGFFTTVkVWhjSG9oVjArbTdtUmo1V0ExWGdw?=
 =?utf-8?B?ZzdyaGpicUYrY2RVTUpvRm9xcFRXYXdISURQVU1MSEtjSWl3eVRCNUdyWE5C?=
 =?utf-8?B?aGI2WGlQZ3lmWVlhazRwMXNPblQzMmZnQ1JpbEpwVFdRdGF0WjhuTnJ5S3U4?=
 =?utf-8?B?ZlhsYWZVcEtiL2crMXp0NGJONGNSMUg0cTM0aVc1U0pOUGQvNHd1WmcvdFZJ?=
 =?utf-8?B?TmUzSlpicUNzV0QrSGpkREpxbldETlQwK1FTR2loQWU2Q1BsOGFaNDdkclFv?=
 =?utf-8?B?TFNJWUNBSlkwazVGL1NsZy9lRzdNVnFaQTEwcUFNYkMrL0RvVTdqcGUvUkNT?=
 =?utf-8?B?Z3pZZ2p6cE9LQ2NWc3JrWkZYcjMvOWxjWFJ6L0hMaUUyV2RLTTMzb2JBM09W?=
 =?utf-8?B?UCtSQ1N5YWZ2UlF0M2FnNEdVMnRpQ0V2N0pjT3V4ODJ4TjgzQ3pMUW1YOUJG?=
 =?utf-8?B?bERNcld0NXAwWlpBejYzNVJTUDVtcFlVMnpLMENjcjZFbFB5OUdKY2EvYXNM?=
 =?utf-8?B?VFN5em9YK1hXSGVsTmlhRDBhWmVVNjRWczJvQU5Wb0wwanIxV00vVEhIemE5?=
 =?utf-8?B?ZGxacVZuM28wRFAwYmtNTkwxNVI3LzNLSG1VN3FJZEpzVHQyVk1ramFydWdV?=
 =?utf-8?B?dEM5bk9FOXpXQlIrVTRiWUptZktpRk5BanJGM3JKbDFGbnpIT29QSlhTVHF3?=
 =?utf-8?B?V3g0K3dHQmtDblJKeXNrQlVaTlBQWExzcC9laVEzS1FqZjlkdFBRMFpNWkdW?=
 =?utf-8?B?TGNlcTArNDVjaExtaXlGRjZnRk4zOUtQb1cwM2VwV2FzOWhlRkRxSjFhYURT?=
 =?utf-8?B?dmc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	Sk36+BuIu/K5KM3OfWw71EwkF7jF/8GxhJIXgadFJ74PaOpVsQQHIuXkLculq+QJPzA+UJiyh7lB1W0AVXSSGCUzDSw9H5aLNaOaGZH+Y7+G5a1OEBvMDg0QJgP+ChNmJZAg4I0xkpClKQ42V0O4mE5sfF1Ksbk7C7+mn+8oYEy+NsIMk64ZeatscTNY683k4jEvIbDImeicoWw3ZyE0FbsRtHdDjnd89C1sWWEHEQNyJYmsLd+4T/DyOJkysEc2JV9qgIwPVCip2XQ2c4KB7DDOXY0KMZtsYTyClA3fTAhcCgk9VJbBlbJ8zm8MBR3qNLtEep2DL5Fzmxrw03dJETJZzq21K+anqy2CECRmO9ixOIOJ6FIcU4AFsq+Gc5hKgJa/j+CEdc7DAvijw9Mr9IV4bApQaKOnIqY9O4vxI3mfGNLaUX6ZqUuw2mVHxcjsIm9QJEEahmEuxaHrxAmXCTcq+nAauoG9w9P8u+07s87AHj6MdaW4mo9aVOy35dFUryIjQJWNUXtciEkQsy1/tk/IeHkia2unYNEjADbb6pDUh3Z9SNtzb3fm8i2zW1/P4MrH9k6LS9O78ExrgqxqVifHnTuw6UQyBoOEHvXeFgI=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 562a97f3-e712-493c-dbaf-08dc69cf0ef7
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 May 2024 11:08:41.3076
 (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: FFcVSlDCIGUz6II4LtdoiFNG/xPvAjuunLLo0HXHsS/R/i+9ONHhfWVYRaGfCp7TYu5Bx0hZTIfmiW/qQ10Ygg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR10MB4642
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_10,2024-04-30_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 bulkscore=0 adultscore=0
 phishscore=0 mlxscore=0 suspectscore=0 mlxlogscore=999 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2404010000
 definitions=main-2405010078
X-Proofpoint-GUID: nEEYLxfjBIPD9_47D783veiaDzdEB9ov
X-Proofpoint-ORIG-GUID: nEEYLxfjBIPD9_47D783veiaDzdEB9ov

On 01/05/2024 02:47, Dave Chinner wrote:
> On Mon, Apr 29, 2024 at 05:47:42PM +0000, John Garry wrote:
>> Support atomic writes by producing a single BIO with REQ_ATOMIC flag set.
>>
>> We rely on the FS to guarantee extent alignment, such that an atomic write
>> should never straddle two or more extents. The FS should also check for
>> validity of an atomic write length/alignment.
>>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>>   fs/iomap/direct-io.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
>> index a3ed7cfa95bc..d7bdeb675068 100644
>> --- a/fs/iomap/direct-io.c
>> +++ b/fs/iomap/direct-io.c
>> @@ -275,6 +275,7 @@ static inline blk_opf_t iomap_dio_bio_opflags(struct iomap_dio *dio,
>>   static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>>   		struct iomap_dio *dio)
>>   {
>> +	bool is_atomic = dio->iocb->ki_flags & IOCB_ATOMIC;
>>   	const struct iomap *iomap = &iter->iomap;
>>   	struct inode *inode = iter->inode;
>>   	unsigned int zeroing_size, pad;
>> @@ -387,6 +388,9 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>>   		bio->bi_iter.bi_sector = iomap_sector(iomap, pos);
>>   		bio->bi_write_hint = inode->i_write_hint;
>>   		bio->bi_ioprio = dio->iocb->ki_ioprio;
>> +		if (is_atomic)
>> +			bio->bi_opf |= REQ_ATOMIC;
> 
> REQ_ATOMIC is only valid for write IO, isn't it?

yes, it is. We reject RWF_ATOMIC for a READ.

> 
> This should be added in iomap_dio_bio_opflags() after it is
> determined we are doing a write operation.  Regardless, it should be
> added in iomap_dio_bio_opflags(), not here. That also allows us to
> get rid of the is_atomic variable.

ok

> 
>> +
>>   		bio->bi_private = dio;
>>   		bio->bi_end_io = iomap_dio_bio_end_io;
>>   
>> @@ -403,6 +407,12 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>>   		}
>>   
>>   		n = bio->bi_iter.bi_size;
>> +		if (is_atomic && n != orig_count) {
>> +			/* This bio should have covered the complete length */
>> +			ret = -EINVAL;
>> +			bio_put(bio);
>> +			goto out;
>> +		}
> 
> What happens now if we've done zeroing IO before this? I suspect we
> might expose stale data if the partial block zeroing converts the
> unwritten extent in full...

We use iomap_dio.ref to ensure that __iomap_dio_rw() does not return 
until any zeroing and actual sub-io block write completes. See 
iomap_dio_zero() -> iomap_dio_submit_bio() -> atomic_inc(&dio->ref) 
callchain. I meant to add such info to the commit message, as you 
questioned this previously.

> 
>>   		if (dio->flags & IOMAP_DIO_WRITE) {
>>   			task_io_account_write(n);
>>   		} else {
> 
> Ignoring the error handling issues, this code might be better as:
> 
> 		if (dio->flags & IOMAP_DIO_WRITE) {
> 			if ((opflags & REQ_ATOMIC) && n != orig_count) {
> 				/* atomic writes are all or nothing */
> 				ret = -EIO
> 				bio_put(bio);
> 				goto out;
> 			}
> 		}
> 
> so that we are not putting atomic write error checks in the read IO
> submission path.
> 

Maybe, I'll look at a rework with the suggested change to use 
iomap_dio_bio_opflags() - I actually thought that I introduced a change 
to use iomap_dio_bio_opflags() previously...

BTW, we need to return -EINVAL, as this is what userspace expects for 
such an error.

Thanks,
John