Received-SPF: pass (google.com: domain of linux-kernel+bounces-165183-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <fd036daf-8a7b-4ae1-8a94-0998bc6adaa5@oracle.com>
Date: Wed, 1 May 2024 12:24:39 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RFC v3 12/21] xfs: Only free full extents for forcealign
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-13-john.g.garry@oracle.com>
 <ZjGSiOt21g5JCOhf@dread.disaster.area>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ZjGSiOt21g5JCOhf@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?dURxdXA1dWdjVEZZSGptRjhWcElrZ3NLMy91bis4Rm04WkRSRmM1ZnRYck5I?=
 =?utf-8?B?TnlpdEw3MlM4SEpCV2xWdjR4STNmRzN2QjdSYUh1TGM4UnRvemtMa3hqQlBK?=
 =?utf-8?B?OGlJaDlIdktka3lIWU9KYno3NmpNS2xURk4vSThiTmp3eEtFY3UrUVhDMEFr?=
 =?utf-8?B?dk1LSnAyQlNVTnhuRFNDdkJVeUtvRXJEYTVYMnRjVXVqcUp0dmY0dzFvVjdH?=
 =?utf-8?B?S3A5azBJT2RZcXBIOEcycFNFamNhTzBWb28zUnE5aWFjZzRla0JJd3pNSk5M?=
 =?utf-8?B?N3NNSnFuWHpXSmRiZ1ZtcVczUUs1ZXZzUTg4NWNZeDl6N0JJTGZDS3RVWnlU?=
 =?utf-8?B?Q1Z4cTRZZjJUL05xUTJMTFowSDZ4WjF3eUo1VURsMFJuUU5uelZxcUVqVzFO?=
 =?utf-8?B?dXhacDhNaUVQeDl5enZkY2pnejVxaHpKbVBhdSs3dk40ZHFKRUJRYmNHb3Zn?=
 =?utf-8?B?VkhzZXZzRm9mbXpvMURzVXFyb2YzcWxXdEs3c3hEcjNaUWFrZW5FaEpFQUZ4?=
 =?utf-8?B?U3Q3TVVhamwrMGFEMml3bjZ6TldiT0ZUQWZqVEU3V2FPbUwxRGFyYlRxZHBV?=
 =?utf-8?B?bVJab01VZ3U1dytqWXk4TjRMeEVLTUhqYW5BT09Kd2NTenczMUFEWVpGMTJj?=
 =?utf-8?B?clIyMzcxSkZKd3hsbjgvc3piV1Z3blBEVyt0em4vV2FFZGJJMXhBQWtYQXJK?=
 =?utf-8?B?Mjk1OHJhREVSUE9YbmR4S0E2N2ZMTDZvTnVZT0NjQ0MrUGI1TVk5WnhSL210?=
 =?utf-8?B?U1JkenVnZFE4ZkN1L1lzL0d1NitucjBXNmxML0FpcVN6NjBRNm9GNHFDZ3Rt?=
 =?utf-8?B?L2Q2b2JCZ0hmV1cwcEFLNS9uR2syMU1UWi9tRk9mYVFnUnhtUjV2emk0VzlV?=
 =?utf-8?B?WXFEV0E2bEF1OUcwQjBaQndYOTBGZkVuUjZrYnhCMlpnMHVaY3pBbEllNjhy?=
 =?utf-8?B?ZlppU2tRUEdSKzhzdE5ma1QySkhtSHZ6dnVGZndqNU1QWWs4bDVxMjV4TWdE?=
 =?utf-8?B?QzRyZXh6bC9JZXFHMXVJbm9EVGZKbGEydWkwaHBPZ0ZtVGNqZjhiUHA4QWdW?=
 =?utf-8?B?WjFLTUowNnZpZk8xV0oxWWhnRmdFemZuaXBYaU1ZU3JMMGJzSVZjaHNUWFhG?=
 =?utf-8?B?ZEgrdGNaVTlNVFZiSzgrOHJWSCtkamFoYndVdXA2VU9maXNiNS9ZTWE4cWZ4?=
 =?utf-8?B?a1dzMEx2aWgvWkdYbTlBN2ZoNm13V09tZGlZYy9ha0lqTTVDL3dZME9nQlBh?=
 =?utf-8?B?bWxFMVFzTHI1WXp5ZzBGeElIcGFtQ3dGeXZSNFZOYnpXNHNtWThBR0wzUTlu?=
 =?utf-8?B?NzFCd1Z4Q0NQQTNPNkt2QkxVRTcwZjY4c1l1WGdTbjA3cktJS2FkbTllOFNw?=
 =?utf-8?B?UjlQMldheFpHRjB2ZCtXUVc1RDlVWWRPSWdFOEFOWDc5T1NqalhiNm1aOFRr?=
 =?utf-8?B?NUh3Y0RzZ0kzQm53Y1ozVzVuVDZIVEw5eXJ0NENLSHBkV2V1aXpEL0NQTi9q?=
 =?utf-8?B?Q25jd1Fna1dTdVBUTGdHOW90TE8xb3gvNmczd0NYQmgxRUExaFNTQ1Rlajkx?=
 =?utf-8?B?QUExVWY5eWlhb0cxMC9QdWdEb3E4ZWZKMjBPcnNOSWRrTldqZDA5ZXFaS2x6?=
 =?utf-8?B?Nnp6U2dJRFFRSkNEVVZDNkpXYVFZZWpaLytlMExyR0hZeG9EYmJtYmk1amlG?=
 =?utf-8?B?cm1oUWRUeXkyNWZJcGFkQVR3cDhUY2pRa0dQbjlvQXQyeEJvdWFPTTdRNGxx?=
 =?utf-8?B?dVRYeDVmcTJtMFFQN0Q2ak1PSXVqbGc3NzMwb0d4Y3NLZm5JZFdJV2c5V0dR?=
 =?utf-8?B?SjYySE5RalkwOFBtcm9ZMFVHUmdidnZzaGpISmN5OUJ1L2lUa3E3S1JLQ2wx?=
 =?utf-8?B?V1RNeTFrTlBOQUlxclI2MVY0TFhtNlUxZjdTWmd3YXJSUStWS25sT1ZsaEVJ?=
 =?utf-8?B?OW5Ya2I5dVQ0WWp2ODdJeUVyRmMxS1F6eGVLUCtLdXVCOHVyMmhocDBtWkMw?=
 =?utf-8?B?d0trelRMZFA3VlRzcjdzU0Jta3RMRHZFOFRyRnBjK25UaTlFR0duS3I1Skd4?=
 =?utf-8?B?WlhrYm81T2h6VlFxdk9ORVRPWXl2aTl0U3JIblEwQ1BpV1BQak9qdFBBUVBp?=
 =?utf-8?Q?OT9DFz2u0wwKsnQRkX8MeE6c0?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	F9HDO+w1PVAT46Dl3ZEIlgo5pGeaKGvLTxct8E/xdYT2HMcVwwnvTiN2OiH1gk5q0pHBGsKsENGEgKKl1LJDrCB9/TPx9eJdqxuB0Sf8GXZKp9FEiTZtC5MmbOECNB1MDLfgsIGbol62NfI8+N9T0WgmIdjBWL15gkOawwZ6jx0QRTlBHVCh8MgaBe+m3DbTmR0KxnwA4gL5dI+TOKZgtPoDM9tYKYnll13qQYJouUKaAgL9ssAFYAin/TWWLTELkuA4L02+5iUIgwg5SE8Jxjhq1Gt3D0aUcAXfbYAtThPSkjwQ3CiD9VrkwsxP+wKgutqRJUvL/JlJ9NysXaFeWBvE6i8ZRKXvr19tlVi4qCGr5LhJBCvFt0fdVAMpLtiRGy13HDVhbFRcKGCsTt0vqyHbRZ8Upigh4sxSiiD1L70idJiNyX3O1fXLEfa/+RTmck5zuLp+VyKQX8WLMfBkdPs/u2xUSo/kMy5wLkkzcnqR3DI0YYgEesl0FhygI3Qm8MowUnG7bT5uNJzZxbymCRvLVQH6dKqnGHshniWqOxJxhx72oNe6CQ2knUlMadbKpVeHczgUVeZdyPwO6L1wG/HTKY93JA62EOtPelez+bY=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c39232b4-da03-4c17-3bd4-08dc69d14d9b
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:24:45.2800
 (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: qBfVx8hhAes+Bh/zLVKqfqMbmTn0Mzf5qSxx00e47f3thN1OaJ1aXKUcNMNeE9zEZRJ43rk9ByO9w+9Zpltw3g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR10MB7618
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 spamscore=0 malwarescore=0
 mlxlogscore=999 bulkscore=0 suspectscore=0 adultscore=0 phishscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2405010080
X-Proofpoint-GUID: x0dX-TmeNRNZ39YCnKp7DYMHIQCk4D_j
X-Proofpoint-ORIG-GUID: x0dX-TmeNRNZ39YCnKp7DYMHIQCk4D_j

On 01/05/2024 01:53, Dave Chinner wrote:
> On Mon, Apr 29, 2024 at 05:47:37PM +0000, John Garry wrote:
>> Like we already do for rtvol, only free full extents for forcealign in
>> xfs_free_file_space().
>>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>>   fs/xfs/xfs_bmap_util.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index f26d1570b9bd..1dd45dfb2811 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -847,8 +847,11 @@ xfs_free_file_space(
>>   	startoffset_fsb = XFS_B_TO_FSB(mp, offset);
>>   	endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);
>>   
>> -	/* We can only free complete realtime extents. */
>> -	if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) {
>> +	/* Free only complete extents. */
>> +	if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1) {
>> +		startoffset_fsb = roundup_64(startoffset_fsb, ip->i_extsize);
>> +		endoffset_fsb = rounddown_64(endoffset_fsb, ip->i_extsize);
>> +	} else if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) {
>>   		startoffset_fsb = xfs_rtb_roundup_rtx(mp, startoffset_fsb);
>>   		endoffset_fsb = xfs_rtb_rounddown_rtx(mp, endoffset_fsb);
>>   	}
> 
> When you look at xfs_rtb_roundup_rtx() you'll find it's just a one
> line wrapper around roundup_64().
> 
> So lets get rid of the obfuscation that the one line RT wrapper
> introduces, and it turns into this:
> 
> 	rounding = 1;
> 	if (xfs_inode_has_forcealign(ip)
> 		rounding = ip->i_extsize;
> 	else if (XFS_IS_REALTIME_INODE(ip))
> 		rounding = mp->m_sb.sb_rextsize;
> 
> 	if (rounding > 1) {
> 		startoffset_fsb = roundup_64(startoffset_fsb, rounding);
> 		endoffset_fsb = rounddown_64(endoffset_fsb, rounding);
> 	}

ok, and the same idea for xfs_can_free_eofblocks() with 
xfs_rtb_roundup_rtx(), right?

> 
> What this points out is that the prep steps for fallocate operations
> also need to handle both forced alignment and rtextsize rounding,
> and it does neither right now.  xfs_flush_unmap_range() is the main
> offender here, but xfs_prepare_shift() also needs fixing.

When you say fix, is this something to spin off separately for RT? This 
series is big enough already...

> 
> Hence:
> 
> static inline xfs_extlen_t
> xfs_extent_alignment(
> 	struct xfs_inode	*ip)
> {
> 	if (xfs_inode_has_forcealign(ip))
> 		return ip->i_extsize;
> 	if (XFS_IS_REALTIME_INODE(ip))
> 		return mp->m_sb.sb_rextsize;
> 	return 1;
> }
> 
> 
> In xfs_flush_unmap_range():
> 
> 	/*
> 	 * Make sure we extend the flush out to extent alignment
> 	 * boundaries so any extent range overlapping the start/end
> 	 * of the modification we are about to do is clean and idle.
> 	 */
> 	rounding = XFS_FSB_TO_B(mp, xfs_extent_alignment(ip));
> 	rounding = max(rounding, PAGE_SIZE);
> 	...
> 
> in xfs_free_file_space()
> 
> 	/*
> 	 * Round the range we are going to free inwards to extent
> 	 * alignment boundaries so we don't free blocks outside the
> 	 * range requested.
> 	 */
> 	rounding = xfs_extent_alignment(ip);
> 	if (rounding > 1 ) {
> 		startoffset_fsb = roundup_64(startoffset_fsb, rounding);
> 		endoffset_fsb = rounddown_64(endoffset_fsb, rounding);
> 	}
> 
> and in xfs_prepare_shift()
> 
> 	/*
> 	 * Shift operations must stabilize the start block offset boundary along
> 	 * with the full range of the operation. If we don't, a COW writeback
> 	 * completion could race with an insert, front merge with the start
> 	 * extent (after split) during the shift and corrupt the file. Start
> 	 * with the aligned block just prior to the start to stabilize the boundary.
> 	 */
> 	rounding = XFS_FSB_TO_B(mp, xfs_extent_alignment(ip));
> 	offset = round_down(offset, rounding);
> 	if (offset)
> 		offset -= rounding;
> 
> Also, I think that the changes I suggested earlier to
> xfs_is_falloc_aligned() could use this xfs_extent_alignment()
> helper...
> 
> Overall this makes the code a whole lot easier to read and it also
> allows forced alignment to work correctly on RT devices...
> 

ok, fine

Thanks,
John