Received-SPF: pass (google.com: domain of linux-kernel+bounces-165139-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <f6a618e6-a00e-483d-bd6b-c7d58ff5b2fa@oracle.com>
Date: Wed, 1 May 2024 11:23:14 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 14/21] iomap: Sub-extent zeroing
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-15-john.g.garry@oracle.com>
 <ZjGVuBi6XeJYo4Ca@dread.disaster.area>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ZjGVuBi6XeJYo4Ca@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?UE5oZC81bEVpMXNGSWhwN0Y2L1JIV2pmcTU4MVBZYXhyaDg2T0dsaVFkN0sx?=
 =?utf-8?B?N3k2Q2NlRWo0Rml0Mkw0cGFqYVkyeFpRc0c1OFFUbGsyVDcyQnMyVUc3ZmhF?=
 =?utf-8?B?RkZyQlV1aWZzV0V2Vjl6L241ZjUzZktDSWF4bGRLTzJmcTN5Nzlpdm5vV3FF?=
 =?utf-8?B?dHBGMWhnYnlPdXVxWTBQT3FNZFBXNWd5LzNpZks0WTVKT3dFK2loaWdTbkcx?=
 =?utf-8?B?VjdKOHZuQ1FLRnhJWUFXWGxoamd2ODRvSDNrai9wY29iaDJiTnIvQlhZSzY3?=
 =?utf-8?B?RW9FdTk2eVRhL2Y3dHVGN3RrWXJ2SnhTVzI1RjM1N2E3aWc2MC90V3dGUWlx?=
 =?utf-8?B?RzlUUmNPUUhjZkdNa1ZsL3RKL3h1UDZIUmVtNkZxUnBLaWlFQ3BSY2ZnSWE5?=
 =?utf-8?B?WHdrSm9vekp1N285aTZ6eHVzcTB3NnJzcWc1SGxTMWdoRmhxdkdIdWF3N0du?=
 =?utf-8?B?eXErNUlhZ2J2cUphQ0UvRlBDelp5dnNmMEwxcUFuY1pwTytENDk5Rk1vaXp1?=
 =?utf-8?B?OE5HQkVGUUpTd3dIUjgwQVpFeE5Yc2dwQ0M1cnArZ0d0QnY1WWJrWGd4djVh?=
 =?utf-8?B?c3ZRYkVQUFRtNmt4RnlYTHVwcjQ3dzZMbGlqM0hlRXk3RVA5MjlUSzBxdk1D?=
 =?utf-8?B?VmZLd1grV08rekVMNFZ4RGdvc0gyWEkzYVo2NU5qMGRqTTJSQ1oxK0NIejR2?=
 =?utf-8?B?NW5BRlhTMmdxdGRlZ0NndUNxT3JmZXk2REQzNi9BbGM4a1B3RjlWenNSUE1o?=
 =?utf-8?B?akRuR1VNQlcvWkdtUWtTQXBIa1NQbnRMVXpuanZHcmtYblNVQnpTOTRuenZs?=
 =?utf-8?B?N293ZW0rNkJHb1E2bmNlNE9BY3ZEZ01pc01SUndlN0VsZXAyOGhiaGZybk5N?=
 =?utf-8?B?OFdQR2tjUXd6ZW9Tck9KMU9VbEdmdG5zc1RtWWhvSHE3YXlnZTRGNE1DNmdP?=
 =?utf-8?B?YzNORWtBcGk2UXZua0VpblJ3Q0g3QVNJMVRqOEVJZXQ4NjJLcEJmRUdXcG43?=
 =?utf-8?B?ZVNkMG1HVldnK2MraldwQnpKbFNrclZUcXVWbERmUTZ6OGZ6QnhuQ2xMWGZN?=
 =?utf-8?B?VUJ5dnJWZkZLWFFENGgvYTZpZXNuRkxsOXNaRW5EN2hwWFZuWDZLTFFZRFZX?=
 =?utf-8?B?K3FDNTFEUTlMV2Q2RjEyeEJ3ZXlMU1FlVHRkcGxxL0Q2Z2lhS0g3MTdDcTFi?=
 =?utf-8?B?VUZBdzhKbGdtdUJyWC9JMU9Fb0pvb2w3TFhLQjJMYTVaUEsyMW1Yd2w4T2Ey?=
 =?utf-8?B?aTJpbHVUODdsUEE4NHVIUEtJZFZyanZaSGZFTFk5MHBNQkRJeFBSVTZxbHVr?=
 =?utf-8?B?bVl3NjRvVG9NempBMFpRSDNOd3hMMjNsTjc5dDFmSGdaT3RpQlgzNy9ZRnB2?=
 =?utf-8?B?YkUwa0pjcDJKaGxxMGEzWExvTElraU9iVEtTcnlIZ2djazVwaXhCdDM4eWFP?=
 =?utf-8?B?Qmx3b1dqZE5NemJ3dFZNSC8vc1NIODd6SDFRL29tOUxlNlZnWHM0L2daalA2?=
 =?utf-8?B?R01yRTZOMXhVSVVQeE83VEhJRysyM0kxTlVXZ0FoR2U5akVkcDRZVEJ0bWpW?=
 =?utf-8?B?RGQvOW5iUytXMlJiOHBnTFpUR1VHSnFLbGpsS2IzMnBQeDgwTmIrNWtlY0tw?=
 =?utf-8?B?TDRReEtDUFNHc3RnNTRsQnVuS1duWUJDeUxXZGhuUVcwUGovRmduNXd3UEd6?=
 =?utf-8?B?QXBwd2Z3eThZWXVCNHNudy9wWVlQaWVFdVA1UHJXVDdnMnRuWTFpN1FWL3pY?=
 =?utf-8?B?WlQvYmNwWGV5MDBHT0V6NWRINGlyckRNR0lTR0JWQ2QxWjlCdjl2M2RUclpT?=
 =?utf-8?B?REE5UVA1QytlQVlzSkp0anhrZHZVendXcXNpQnNCQkRpdGRCeEw4UDVsVW5J?=
 =?utf-8?B?M1NBNFk3ejdFaDAxU3lzOWk0QjV6bEMzcElRc1RxNEJMMHpqWDVBSkY2ZUx3?=
 =?utf-8?B?a3JzOUpXU3Q5QlNiaU1hK3I5aHF4RzhkT21aM3NDeFhybU1kbFErSmx5eFZj?=
 =?utf-8?B?Z0p0T2VvU1A0VnRhMW1IclNwanE3ajdIdFBXK0V2UkxJL2lZdmd3ek1zR1dR?=
 =?utf-8?B?djBTdUhFU0g5eUdaNXhZSnFFSVRheE15SlJEVzI5YTZaMnoyRWR2VGVBd0VK?=
 =?utf-8?B?REt3VHdNa3ZmKzJub3Z3cUovekV6a0J6T1l5TDVHZWxsVElvanNUK3ZuY1oy?=
 =?utf-8?B?SkE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	iuim8AvclCNKJOkpWU0isXy37ZWo0C52Oxrv9v5iSu/O3qZ1PYxFahpwWqRoAFjBhjgt6m35WITbZ08SdM16oFANBy823rERNKUGw9TJdQSz9/oaOoef8ilK9/RHXBCit7Pl+vAljEY7EXJmFu8nKQ+HL0Ft6oJ+Wzu842U9IGQKLJpD2BDDHX1smqH5rg0btOgNeF2XiinHUUFEPyKFXBKkRzAsCAKqRnWA/oW9iaTfmY2Vcs/8lutXbBIgP1s4Y1iPLh1DGWvMYsJIdLCUGOhQPGKBE78q2i/NLw8IbU3FEM8OF3/sJWQ9CgXKarTgkBxpANYe+VOUrOuht/2Fuj81z9bLuFQCE0tI3vNrxHj9nIy2ukO/r7xjVZD3J6IVNmKI3d6KiWTrPHjbSDFI9Kab8u/2su76y4Tx5UzKaVOySlpNkZxkuuzjWsUCrEBGwOdru5wlFcMWTooVLeIagcWzpx+o6TPPgeNy1fj6gHWVhx87RXFXsbwRr3QdU5WBSa4P5YOIkVeFmd9agyZ4JF73pFdeH5SGxVht1ZL9l33lArCf8OcVFW5SsoqfFnNC1vmifiielqBHOtLeM6uwv5bkS25NwLGN1+oEQ4wCG3Q=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 191307d0-d67d-4167-e4d1-08dc69c8b8c6
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:23:19.6203
 (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: JptDwRr7fIaT3Dwp4vE5lO3a0/vevVWeWvfJRmZy/0oaOm7fCcJTcodFBNZMAKJBPyasNKrKYle1WJdNMLVg4A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR10MB6881
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-2405010073
X-Proofpoint-GUID: lWdW62ODyFIiY4U72HjnrJ6u64mU2SdZ
X-Proofpoint-ORIG-GUID: lWdW62ODyFIiY4U72HjnrJ6u64mU2SdZ

On 01/05/2024 02:07, Dave Chinner wrote:
> On Mon, Apr 29, 2024 at 05:47:39PM +0000, John Garry wrote:
>> For FS_XFLAG_FORCEALIGN support, we want to treat any sub-extent IO like
>> sub-fsblock DIO, in that we will zero the sub-extent when the mapping is
>> unwritten.
>>
>> This will be important for atomic writes support, in that atomically
>> writing over a partially written extent would mean that we would need to
>> do the unwritten extent conversion write separately, and the write could
>> no longer be atomic.
>>
>> It is the task of the FS to set iomap.extent_size per iter to indicate
>> sub-extent zeroing required.
>>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
> 
> Shouldn't this be done before the XFS feature is enabled in the
> series?

Well, it is done before XFS iomap zeroing support patch. But I can move 
this patch to the very beginning of the series.

> 
>> ---
>>   fs/iomap/direct-io.c  | 17 +++++++++++------
>>   include/linux/iomap.h |  1 +
>>   2 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
>> index f3b43d223a46..a3ed7cfa95bc 100644
>> --- a/fs/iomap/direct-io.c
>> +++ b/fs/iomap/direct-io.c
>> @@ -277,7 +277,7 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>>   {
>>   	const struct iomap *iomap = &iter->iomap;
>>   	struct inode *inode = iter->inode;
>> -	unsigned int fs_block_size = i_blocksize(inode), pad;
>> +	unsigned int zeroing_size, pad;
>>   	loff_t length = iomap_length(iter);
>>   	loff_t pos = iter->pos;
>>   	blk_opf_t bio_opf;
>> @@ -288,6 +288,11 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>>   	size_t copied = 0;
>>   	size_t orig_count;
>>   
>> +	if (iomap->extent_size)
>> +		zeroing_size = iomap->extent_size;
>> +	else
>> +		zeroing_size = i_blocksize(inode);
> 
> Oh, the dissonance!
> 
> iomap->extent_size isn't an extent size at all.

Right, it's a poorly chosen name

> 
> The size of the extent the iomap returns is iomap->length. This new
> variable is the IO specific "block size" that should be assumed by
> the dio code to determine if padding should be done.
> 
> IOWs, I think we should add an "io_block_size" field to the iomap,
> and every filesystem that supports iomap should set it to the
> filesystem block size (i_blocksize(inode)). Then the changes to the
> iomap code end up just being:
> 
> 
> -	unsigned int fs_block_size = i_blocksize(inode), pad;
> +	unsigned int fs_block_size = iomap->io_block_size, pad;
> 
> And the patch that introduces that infrastructure change will also
> change all the filesystem implementations to unconditionally set
> iomap->io_block_size to i_blocksize().

ok

> 
> Then, in a separate patch, you can add XFS support for large IO
> block sizes when we have either a large rtextsize or extent size
> hints set.

I hadn't been considering large rtextsize for this. I suppose that it 
could be added.

> 
>> +
>>   	if ((pos | length) & (bdev_logical_block_size(iomap->bdev) - 1) ||
>>   	    !bdev_iter_is_aligned(iomap->bdev, dio->submit.iter))
>>   		return -EINVAL;
>> @@ -354,8 +359,8 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>>   		dio->iocb->ki_flags &= ~IOCB_HIPRI;
>>   
>>   	if (need_zeroout) {
>> -		/* zero out from the start of the block to the write offset */
>> -		pad = pos & (fs_block_size - 1);
>> +		/* zero out from the start of the region to the write offset */
>> +		pad = pos & (zeroing_size - 1);
>>   		if (pad)
>>   			iomap_dio_zero(iter, dio, pos - pad, pad);
>>   	}
>> @@ -428,10 +433,10 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>>   zero_tail:
>>   	if (need_zeroout ||
>>   	    ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) {
>> -		/* zero out from the end of the write to the end of the block */
>> -		pad = pos & (fs_block_size - 1);
>> +		/* zero out from the end of the write to the end of the region */
>> +		pad = pos & (zeroing_size - 1);
>>   		if (pad)
>> -			iomap_dio_zero(iter, dio, pos, fs_block_size - pad);
>> +			iomap_dio_zero(iter, dio, pos, zeroing_size - pad);
>>   	}
>>   out:
>>   	/* Undo iter limitation to current extent */
>> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
>> index 6fc1c858013d..42623b1cdc04 100644
>> --- a/include/linux/iomap.h
>> +++ b/include/linux/iomap.h
>> @@ -97,6 +97,7 @@ struct iomap {
>>   	u64			length;	/* length of mapping, bytes */
>>   	u16			type;	/* type of mapping */
>>   	u16			flags;	/* flags for mapping */
>> +	unsigned int		extent_size;
> 
> This needs a descriptive comment. At minimum, it should tell the
> reader what units are used for the variable.  If it is bytes, then
> it needs to be a u64, because XFS can have extent size hints well
> beyond 2^32 bytes in length.
> 

ok

Thanks,
John