Received-SPF: pass (google.com: domain of linux-kernel+bounces-165191-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: <0eb8b5b6-1a59-445c-8ac1-1de2a1c0ce4a@oracle.com>
Date: Wed, 1 May 2024 12:36:02 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 15/21] fs: xfs: 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-16-john.g.garry@oracle.com>
 <ZjGbkAuGj0MhXAZ/@dread.disaster.area>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ZjGbkAuGj0MhXAZ/@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?TnV5Ym03T3ZNNWY4KzFUYkJrbHI4RVhnbmZzeWxhVnZiNzdDUW5NKzhzbHBp?=
 =?utf-8?B?YzFoVWlOR3kwUkFlMWs2dUVTaEVvdjVjUXBiTllYc2ZRVlNHSkphYUgwbEZl?=
 =?utf-8?B?VzFxTjZxT1FRUDZPV2s0OGxBbDZmL2hmbUVGamVlSit2WVVNYmU2ejVoVnpG?=
 =?utf-8?B?UGRGcWV6Qnd6WjdkNmtHRW16UUcySVBKQzdlR1R3cVRadVVnVDdKWUJ3MnN0?=
 =?utf-8?B?S0ZqUUF1YzV0VnMwZFYxWDlPVmhpT1Y2eVNHMVNKMWEvR2l4UFZ2dHdQODI2?=
 =?utf-8?B?VVc1YlhQRmxIWEkxaGd4N3RsVjEzaTNidUh0M0RWYmRwTnhKQ3BBbzV4eWNP?=
 =?utf-8?B?bTFCMU1JQUYyS05Sb0pHS3p3TW4weVROSE95NE8xc2FGR29vdlBXQ1lLalJ6?=
 =?utf-8?B?QVMrYld2RVVNOTdFYXhOS2NmUXFNTkVoMmNwb2lNa0pJU3ZSZTV3NEpDVXRW?=
 =?utf-8?B?VUt3TUFEWHNtZnVQRENVMUZ5MnI3L2VuN0pJcUhlL2tUOTRqYTU2S25uNzQz?=
 =?utf-8?B?M05YaUo4cU5KYlBTVkFqSDlFdzN6NHEyTisrQXZiUUl6dzNZOG50SzMyVnhQ?=
 =?utf-8?B?ZC9uSW1MNnFSWGRZUGtVY0lCWkFha1hSV0FYUHh6NVFmQmc0c1hmM1YvVFlG?=
 =?utf-8?B?SGthRk5HeWdYcks1aGNxVGd1QUkwVFplcm1ObWp3ZzcyTGJwcnQ5a0JxMFBV?=
 =?utf-8?B?d1MxT0pidkVEbTAwOG1jOWMyYmlMdCtjQXUvc21LcDY4Wk1NUHVmY1ZmVGVS?=
 =?utf-8?B?YkszOEJLTUxJeEx5cG1xejBBbGlQUFk5eXYvdEF0U0ppQTJUZ25PZzV0Umtl?=
 =?utf-8?B?WitGYVoxWHNtc2pkVmNLeG83RjcvU0IwUDBxQS9tSVUrVUZLWndVaG9uT25o?=
 =?utf-8?B?eXhJS0JqZ3ZnY3hMT01FM0VZRXcrKzI1Ti9wbGNZTjUzcnFQSzVRZHFPZmVu?=
 =?utf-8?B?RGZCeWpxOGVyaHlvVkFhS3JUVzlVSmVlbzVPMXhORk5QUENnWVVBRHdlV3Rn?=
 =?utf-8?B?b29abDBhcXo4dW9WZW5oRnZHbDVzWmQzOGU3N2FNNGhFdmlyeFJpRS9NTmhw?=
 =?utf-8?B?ZDBmbVRrUE1NSEs4TUMxcTEzSlExZUlrUm1KSDRpRHB5Lyt5YVpIVmR1TU4x?=
 =?utf-8?B?alFrVnJFM283TVBBQS8wN25FMFpUWGppWEpRTTh2c24wZG5NdVdEUnBJRGJG?=
 =?utf-8?B?eTlJVTY4Nmo3TmRsbHRUMldJVUxLSDVyMEM2VmM1UzdFams5Ym51RkFZb25p?=
 =?utf-8?B?dG9OeU1PY25jeTZUWU90aXI0VFdOenp3WEJDNHRYTGRpSERyQ0hZYW5yclBx?=
 =?utf-8?B?YzBKSmM1cTN5aUtjT21LN2RTTnA0R05xOUU3bDlHOXFCdndRN3lvN0VtcFBV?=
 =?utf-8?B?YVVVaXJjMjh6TGgxa1VQSWIrLzVJc0QvS1hVQnY3NjlTaEFZalQ4Y2w4Nmd1?=
 =?utf-8?B?aGNPelBPSEs5dnNTUHJQRjJac3VLbEZkOXZQZDlvQU9NOXFaQmRjb1g0YS9D?=
 =?utf-8?B?ZGVFbnk1bStOVEYxREswRDdudVZZbTJHNHN2NjVjQ051bXFrVElVczNBU29B?=
 =?utf-8?B?K28wcDhuMTI5QzVYcWhIUXRGNTBlczRXdGp2Q3BnblI3ZnVmQXhBcFFzNjJv?=
 =?utf-8?B?UnN4TTUrWlVYY3l1Umt5S1NmM3ZraWVwWlZtZnBDS0dCMkw2aDJRUGNxZUY5?=
 =?utf-8?B?aFMvNlBRMEFjMUJTNjlpdjZnNGtvY3RpeFUvMXQrR0VlOWZqMTR6bWRuNzEw?=
 =?utf-8?B?STRlSFJIWm9LVU9yb3d2WWY1ZytFQkQ1OUJ3UUtDY1A4bmprbTZ5YmY5cXJR?=
 =?utf-8?B?cEdMdG05TXJlb0xWenBOVHNyTHJNcGNMeFp4cGVYUkpVWUN5NkN6V1VGZENo?=
 =?utf-8?B?KzJneDlmTCtMTzJYcm5qektUK0R1VlRoWC9VdFpFSUtQTkI2YnRTTWE0MEdp?=
 =?utf-8?B?bW1rc2pLcm5XQWEyU0dLS3dVS255dnAzeU5oVG5Md1dyVTF5U0ZvMnU1MERE?=
 =?utf-8?B?aEt6dmRGRHF1NWEvVTkvVnNacEViT3FOemFOcUx5anl6RXhoMGRwOHEwZkVZ?=
 =?utf-8?B?c3UwcW9ObjAxbndHVGhlMWRtMndsUUJPZzFZaGoyRU5mQWZIcml2V2dIZDYy?=
 =?utf-8?Q?HL0s4isemGD9UFTf6qW6XPLGZ?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	H0MBkU1MdI1TU48Ph710ciJ5XB7Y0RcRJVq40Y8/DRwimAebGs7ynp9ikjShoMzaJh+dbLhFJD+yjINQcBXfEpUzrLmlDWFyII+Tz11YFUuxzKc25YQyAtZeQ2w3t/nWKHY4H9zV/BOwYnthI/MoNbmynV8xmP1zv0MRR7d9m00VKXloHCbA/mOLlMuixDURIwUfPxY93tqOAF09HSmEy/fNqud52DQfpF653fp6xZYtx6QasDFRYv+da/JfMWbmy7Sh9JdDjm+zuF3LNb/engZFPDZLwd7G6x71SsMse42FyJ9JGDFtGLpW+AXTbYt5yEeFslJXHY6I9gI95JdOj0ARonOpgHxDNX66VWE3gsZwrq8XFouhodeZdvdkpWF9VAbamokIdh6hKr+aboXwawoBOCm45f2wITrLyAs2LoSdIQdYLMm3lE1y3Kyp51GxT6O7BZ55al4crhPe5SmFlnsmhd1HtuQuvDvj91PdqdFlhcCS2TnqQNdM0yEHCHybeI+rtlndTYdTWoS6u/Im187qPpUsObUN6W0tVWKpZv3sHO5jVMMwRYXgmOMRVX8q+8BJGel9UadlE5cMCevSvz/DeXKmrj3wp6mvACI4TT8=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 05f3be51-390b-4575-1dcc-08dc69d2ea08
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:36:17.2123
 (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: yFfIfxjIyf2FuOvMyJBXHaOPb13/KkgXk48ac9Py51i9S/e4pRmjUoi3w+oonFiR2S48SA4jFw/qz1qKWAf7QQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB4266
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_11,2024-04-30_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 phishscore=0 bulkscore=0
 suspectscore=0 malwarescore=0 spamscore=0 adultscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2404010000
 definitions=main-2405010080
X-Proofpoint-ORIG-GUID: CnRu1_afS67ps0ou2lBVmi_jB_XfxrOG
X-Proofpoint-GUID: CnRu1_afS67ps0ou2lBVmi_jB_XfxrOG

On 01/05/2024 02:32, Dave Chinner wrote:
> On Mon, Apr 29, 2024 at 05:47:40PM +0000, John Garry wrote:
>> Set iomap->extent_size when sub-extent zeroing is required.
>>
>> We treat a sub-extent write same as an unaligned write, so we can leverage
>> the existing sub-FSblock unaligned write support, i.e. try a shared lock
>> with IOMAP_DIO_OVERWRITE_ONLY flag, if this fails then try the exclusive
>> lock.
>>
>> In xfs_iomap_write_unwritten(), FSB calcs are now based on the extsize.
> 
> If forcedalign is set, should we just reject unaligned DIOs?

Why would we? That's very restrictive. Indeed, we got to the point of 
adding the sub-extent zeroing just for supporting that.

> 
> .....
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>>   fs/xfs/xfs_file.c  | 35 ++++++++++++++++++++++-------------
>>   fs/xfs/xfs_iomap.c | 13 +++++++++++--
>>   2 files changed, 33 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
>> index e81e01e6b22b..ee4f94cf6f4e 100644
>> --- a/fs/xfs/xfs_file.c
>> +++ b/fs/xfs/xfs_file.c
>> @@ -620,18 +620,19 @@ xfs_file_dio_write_aligned(
>>    * Handle block unaligned direct I/O writes
> 
>   * Handle unaligned direct IO writes.
> 
>>    *
>>    * In most cases direct I/O writes will be done holding IOLOCK_SHARED, allowing
>> - * them to be done in parallel with reads and other direct I/O writes.  However,
>> - * if the I/O is not aligned to filesystem blocks, the direct I/O layer may need
>> - * to do sub-block zeroing and that requires serialisation against other direct
>> - * I/O to the same block.  In this case we need to serialise the submission of
>> - * the unaligned I/O so that we don't get racing block zeroing in the dio layer.
>> - * In the case where sub-block zeroing is not required, we can do concurrent
>> - * sub-block dios to the same block successfully.
>> + * them to be done in parallel with reads and other direct I/O writes.
>> + * However if the I/O is not aligned to filesystem blocks/extent, the direct
>> + * I/O layer may need to do sub-block/extent zeroing and that requires
>> + * serialisation against other direct I/O to the same block/extent.  In this
>> + * case we need to serialise the submission of the unaligned I/O so that we
>> + * don't get racing block/extent zeroing in the dio layer.
>> + * In the case where sub-block/extent zeroing is not required, we can do
>> + * concurrent sub-block/extent dios to the same block/extent successfully.
>>    *
>>    * Optimistically submit the I/O using the shared lock first, but use the
>>    * IOMAP_DIO_OVERWRITE_ONLY flag to tell the lower layers to return -EAGAIN
>> - * if block allocation or partial block zeroing would be required.  In that case
>> - * we try again with the exclusive lock.
>> + * if block/extent allocation or partial block/extent zeroing would be
>> + * required.  In that case we try again with the exclusive lock.
> 
> Rather than changing every "block" to "block/extent", leave the bulk
> of the comment unchanged and add another paragraph to it that says
> something like:
> 
>   * If forced extent alignment is turned on, then serialisation
>   * constraints are extended from filesystem block alignment
>   * to extent alignment boundaries. In this case, we treat any
>   * non-extent-aligned DIO the same as a sub-block DIO.

ok, fine

> 
>>    */
>>   static noinline ssize_t
>>   xfs_file_dio_write_unaligned(
>> @@ -646,9 +647,9 @@ xfs_file_dio_write_unaligned(
>>   	ssize_t			ret;
>>   
>>   	/*
>> -	 * Extending writes need exclusivity because of the sub-block zeroing
>> -	 * that the DIO code always does for partial tail blocks beyond EOF, so
>> -	 * don't even bother trying the fast path in this case.
>> +	 * Extending writes need exclusivity because of the sub-block/extent
>> +	 * zeroing that the DIO code always does for partial tail blocks
>> +	 * beyond EOF, so don't even bother trying the fast path in this case.
>>   	 */
>>   	if (iocb->ki_pos > isize || iocb->ki_pos + count >= isize) {
>>   		if (iocb->ki_flags & IOCB_NOWAIT)
>> @@ -714,11 +715,19 @@ xfs_file_dio_write(
>>   	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
>>   	struct xfs_buftarg      *target = xfs_inode_buftarg(ip);
>>   	size_t			count = iov_iter_count(from);
>> +	struct xfs_mount	*mp = ip->i_mount;
>> +	unsigned int		blockmask;
>>   
>>   	/* direct I/O must be aligned to device logical sector size */
>>   	if ((iocb->ki_pos | count) & target->bt_logical_sectormask)
>>   		return -EINVAL;
>> -	if ((iocb->ki_pos | count) & ip->i_mount->m_blockmask)
>> +
>> +	if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1)
>> +		blockmask = XFS_FSB_TO_B(mp, ip->i_extsize) - 1;
>> +	else
>> +		blockmask = mp->m_blockmask;
> 
> 	alignmask = XFS_FSB_TO_B(mp, xfs_inode_alignment(ip)) - 1;

Do you mean xfs_extent_alignment() instead of xfs_inode_alignment()?

> 
> Note that this would consider sub rt_extsize IO as unaligned,

> which
> may be undesirable. In that case, we should define a second helper
> such as xfs_inode_io_alignment() that doesn't take into account RT
> extent sizes because we can still do filesystem block sized
> unwritten extent conversion on those devices. The same IO-specific
> wrapper would be used for the other cases in this patch, too.

ok, fine

> 
>> +
>> +	if ((iocb->ki_pos | count) & blockmask)
>>   		return xfs_file_dio_write_unaligned(ip, iocb, from);
>>   	return xfs_file_dio_write_aligned(ip, iocb, from);
>>   }
>> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
>> index 4087af7f3c9f..1a3692bbc84d 100644
>> --- a/fs/xfs/xfs_iomap.c
>> +++ b/fs/xfs/xfs_iomap.c
>> @@ -138,6 +138,8 @@ xfs_bmbt_to_iomap(
>>   
>>   	iomap->validity_cookie = sequence_cookie;
>>   	iomap->folio_ops = &xfs_iomap_folio_ops;
>> +	if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1)
>> +		iomap->extent_size = XFS_FSB_TO_B(mp, ip->i_extsize);
> 
> 	iomap->io_block_size = XFS_FSB_TO_B(mp, xfs_inode_alignment(ip));
> 
>>   	return 0;
>>   }
>>   
>> @@ -570,8 +572,15 @@ xfs_iomap_write_unwritten(
>>   
>>   	trace_xfs_unwritten_convert(ip, offset, count);
>>   
>> -	offset_fsb = XFS_B_TO_FSBT(mp, offset);
>> -	count_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)offset + count);
>> +	if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1) {
>> +		xfs_extlen_t extsize_bytes = mp->m_sb.sb_blocksize * ip->i_extsize;
>> +
>> +		offset_fsb = XFS_B_TO_FSBT(mp, round_down(offset, extsize_bytes));
>> +		count_fsb = XFS_B_TO_FSB(mp, round_up(offset + count, extsize_bytes));
>> +	} else {
>> +		offset_fsb = XFS_B_TO_FSBT(mp, offset);
>> +		count_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)offset + count);
>> +	}
> 
> More places we can use a xfs_inode_alignment() helper.
> 
> 	offset_fsb = XFS_B_TO_FSBT(mp, offset);
> 	count_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)offset + count);
> 	rounding = XFS_FSB_TO_B(mp, xfs_inode_alignment(ip));
> 	if (rounding > 1) {
> 		 offset_fsb = rounddown_64(offset_fsb, rounding);
> 		 count_fsb = roundup_64(count_fsb, rounding);
> 	}

ok, but again I assume you mean xfs_extent_alignment()

Thanks,
John