Received-SPF: pass (google.com: domain of linux-kernel+bounces-166187-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <70bc1a7f-1218-46d6-a8e8-96b667d393b7@oracle.com>
Date: Thu, 2 May 2024 10:12:36 +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>
 <d39c46b7-185c-4175-b909-2ba307c177c9@oracle.com>
 <ZjLvw3Y5m9AxH71u@dread.disaster.area>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ZjLvw3Y5m9AxH71u@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?VTR4d0ZMQ1hqSVI5RWtaL244bGVMbTBYd3FIamRNWlQzZlI4b2RzZ1NpdVZK?=
 =?utf-8?B?Z1lyZlVkRzhjNGI4Q0tsTnFGUkw1ZHZWRGorek5TQ3FJOE5FbU5QZ1pOeDlv?=
 =?utf-8?B?VHphZHdlbjE2VXl0NHMwaCtzczdaOHpIYVR5Y2VFUk9jVjlOYTdoTmxjZ2dD?=
 =?utf-8?B?STdYdHVBQWhzS0U4Y0pHRFMxTjJWbXNmQlRKZkg4LzV6a3hmZHFTQ2ZhVlVS?=
 =?utf-8?B?RUxqYk9FczFhOWIwOGhBUWNNd2tYc3YvNE5uVTM0UG1wdWIrNEllREpvTHZD?=
 =?utf-8?B?VmNHRzloQTlOUVZWRlBhSjM5cFY0ZDhtTHM4T2g0SjRUblZ0dkJFOTNzd2tu?=
 =?utf-8?B?dDVxTERVZ1JCZjIwYTFZdENOd3BqRWtWV2d0WFEzcWpMR0MrM1lqMUVmMTUw?=
 =?utf-8?B?TmtjVHFtSHFpNXkrb3dpZ2FCU3FrL2crM0JBcHpGT2dtZXNFbno0NVByNllU?=
 =?utf-8?B?bnd5clB1bFVSTUlpUlFmdUFXWGJMUTNzUFBxZ3FOVThaSStjdStVZW1tZjVp?=
 =?utf-8?B?YjluWGtWbFJsbndUK3RMWjJWbWcvZG0vYTVyU092VlQrQXpDSGxUNndkMXBU?=
 =?utf-8?B?U01vQ09lMlVYOUx6cTZVbjZBVnFabXE4dHJEOFdyS1R0eU4yUEVIUGJNemtk?=
 =?utf-8?B?T3VDZUNCZmEvWGU5Vmg4RWlaQWFjcldsbG14ZVA5OVlyMHFweWZWdDlTQy9Y?=
 =?utf-8?B?NkgrU0JTcXBsVVdXMXdGcEZ2UmExUmJoa2VvK2NFK2lwNGZaWjhlWVBBVk13?=
 =?utf-8?B?SUlmQ1NjTHl4WmpNb21VZzhxa2FOcnV5NG5FdG1HSG5tYkYxeS9OcTBNbDBX?=
 =?utf-8?B?TmNpQnp5R2MwZXBuSk91WHFxSVNHMFNDczFiMngweFl3bVEvYUVLZG13V1cz?=
 =?utf-8?B?aTVFcWRleDVTTE50bFcyM0s5UmJWeWhDK2FsSlVva1ZJSFhFZWUrbHBscmR6?=
 =?utf-8?B?MndNS0s0M3Vvd3VPdnZSZ0pNVVErUU1rc3hNaEk3Z1c2MXVyL0MySUIvc2FH?=
 =?utf-8?B?TWw2TWRrV0luUFI3NGVqMml6RjBkbVdQWWlRbmJqM3NwUmlBSlNiYTdwRTdo?=
 =?utf-8?B?aUZqWHFDZmNUQWFPcCtqOUtaZSs1M3VVTmltQ1UrUFhIUHFoUGRXQysxVEY4?=
 =?utf-8?B?YUtnNWg3UklGTmJONEl6eXZQVFdEUXNsZXZzakczUVA3dzZ3QUIvWUx1TlJx?=
 =?utf-8?B?UzdSWEhvbnpCOU9EY1hBMjNHSmE0OElmL0RROElHM08xSUVPZkt4QTlvVnNF?=
 =?utf-8?B?S0k2ZSswTkdzYlNsdFhMeFRjSlBacGMyd2pYdmVPaTA1U2lWWnB2NUtnUy9I?=
 =?utf-8?B?R3pmckJibytlRjQzUDNqUzFpeGxjcloxQnE4TCtVTjAxRUN4YzBYWUE0UGdE?=
 =?utf-8?B?b0JJU0lsMm1RQTVocVRoaEVqenBPckhIbnpuMFN3QURxQTNlenQrNFdBb2tE?=
 =?utf-8?B?b0xaRVkrQlZ1QTdON0xPZGh5azJrYisrYkYzQ3lyVTIxUlVoRHl2RlJNZEpZ?=
 =?utf-8?B?cWRxVmJDdjh2Z2FKWkpuZUNnaDE4eDludGdXRXRObVVRWTFRTzBmUkZVT0NU?=
 =?utf-8?B?eWVmYmZjZTM0T2svY25iMmNXY2JCejdrZW1NSFNyMXl0YXlpc0ZINVJXdWtW?=
 =?utf-8?B?SHZ6djMwMlQ3SU9TY1ZoMzdPSWtXaytMZXlubnpQTllESm81ZUJKU0g5SVE2?=
 =?utf-8?B?MVJMNWIrTEdGNTZxTWhOR28rbGpKQ1lTbE1WbXBXWnlUV1IzU1BWRFUyakVZ?=
 =?utf-8?B?L2k0R3ZxSzBjKzNWenFSWUNOemVDQjlUdzBReUJBL2w1c0h4QnFLeFZyT3VU?=
 =?utf-8?B?YVFGSkxMT1B0SDBPTFZyb2tES1FIZm5OUkRvYXNjaDkzTGlxd0FzQWNlbFFv?=
 =?utf-8?B?WGlGcUFMQlhtK3NhMnFSTDhXc2haZ2RPcmJnSmx4R2ptcE1HSk5oanlYMm9u?=
 =?utf-8?B?RjQyL2c1T0pMZ3BOYnl4UWZxS2F0Z0hZMG92NEQrdHdOQ1k3a1FJT2ZTaEow?=
 =?utf-8?B?cVc3NXlDT0FjQnV4Snk0ZTdWUWswa2RyTng3RFNnamQxUlMyTFUvNmt5a0ds?=
 =?utf-8?B?S3RzUmZnWXhnMVBPRnhsNkVTUys4RW1tc3lBU0Judjg5Z0c4cFFMMWpnYVNw?=
 =?utf-8?B?QUlSV3p3OHpxNkd4SlVuN3hIVzdzdFFLdkFrMUVwcE5TeXRFcmxETE1yZ2Iv?=
 =?utf-8?B?OWc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	SPuPmj8h+h0L0ZkEAK1QgbLvCQPY53v6k/cBEsIOLca9goktXj/6V5J5g0K6XjLApvU9CoaA+JNpZGvqfr47/ntKPX+MVSXgd3prfZ7qhm6eW57Mg1VV9cZasNVKVv1pw5mDJC81IN5RbW44OCwlCLFJtZaMDQc3ii6YRYWCHR0KB9rZxKlnoHvdSqmc/ExUGhCJKeilc0qeWI4WDTcJa8p9x/x9ftelMHzgDCXfXqsTzYlJmOMIexSPaV7MXqK/qrNTS7CFPzERhseAc8fyQJQIcgyqM7c96d+GvGPj/AuDn78eLD6JoGb6wvJqJjzTpwpOApx4RrB+2NWcRpzNdZhOnyrSUlBImeoemBPN1RRUbBox0lacIfnykd2yjZoRa0loTVRvAP+CrJYrx5aiJIpfW0lwzWnlKsOzAXcr0/a4NIDxc23Je4aHnk4NSD0fZ5XCNGZmSSzuez3Ysd9gBS7PYuRC5RzDM9eXzRsZALtPoG9gDhE4QvrUQQO8kBGfzp82oVnsWWDlWFR0/LQku24KoEXgDxkMoYQbB671TKFvr9Nkxw/MrlI0kzp0kNiuE+mr2hXHKuLqZ1SAAA12m7/WBMUGfvqP9qgEqozQkAA=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ba4697ec-caa5-4a18-2781-08dc6a880516
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 May 2024 09:12:41.5105
 (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: gKyv/LsqK8ik1SlurbAC1hn2R+AzvH2t4BnULhh1ncQzSnYcQWGACQnAa6KiJga8iXzfk81xJGoEiYUWsHUYag==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB5676
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_16,2024-05-02_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-2405020055
X-Proofpoint-GUID: _Urs7rg-u3vz-OgQSVRyYz7_cooE-weu
X-Proofpoint-ORIG-GUID: _Urs7rg-u3vz-OgQSVRyYz7_cooE-weu

On 02/05/2024 02:43, Dave Chinner wrote:
> On Wed, May 01, 2024 at 12:08:34PM +0100, John Garry wrote:
>> 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>
>>>> ---
> ...
>>>> +
>>>>    		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.
> 
> Yes, I get that. But my point is that we may have only done -part-
> of a block unaligned IO.
> 
> This is effectively a failure from a bio_iov_iter_get_pages() call.
> What does the bio_iov_iter_get_pages() failure case do that this new
> failure case not do? Why does this case have different failure
> handling?
> 

So you are saying that if we fail here (that is the (is_atomic && n != 
orig_count) check), anything unwritten in the atomic write region and 
zerotail region could expose stale data, right?

If so, I would say that we need to zero the complete unwritten atomic 
write and zerotail regions - similar to the bio_iov_iter_get_pages() 
failure handling - and still report an -EINVAL error.

How does that sound?

Thanks,
John