Received-SPF: pass (google.com: domain of linux-kernel+bounces-165033-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <833f5821-a928-441f-848f-3a846111dcb7@oracle.com>
Date: Wed, 1 May 2024 09:30:37 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 09/21] xfs: Do not free EOF blocks 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-10-john.g.garry@oracle.com>
 <ZjF2jjtsA/C6ajtb@dread.disaster.area>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ZjF2jjtsA/C6ajtb@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?NXRDYlZEa1pscy8zYk1IUlp2UG9yNUtScEdLZmplenlFa09QcWV3Z2RUZ1U4?=
 =?utf-8?B?YTg0NlBUOC9zM1pCUzVsVzlBcGR6VjVWQUZocURkVCs5NW9zc013WWNieUwv?=
 =?utf-8?B?TWdZcklOcE56SE0rVzkzN1YzQllCeWpvemdoTVhPTnZEcWRDZXhTUFJydG5Z?=
 =?utf-8?B?YmdyVlFKS1B1U0tOS0kxTXllM0NsTjRhYWYvbHFpbXpXL1AwVnRDbVl0U280?=
 =?utf-8?B?TXFnVHZPVXNUYnhtSXJJTWZkUCs4SFJITEpDeStpWEZFK095UkxnekJGdDUv?=
 =?utf-8?B?WUZPWExTS2VMN2EzMFRYVDhselBMZjlqeHlNdUxRRlVZN1VXelRMdmRhVkE3?=
 =?utf-8?B?WUdENEh3a3kyV2FSMjJyUlN2VE4xY013anpCcHp2a1VyZWNYaWRnZEhiaTFS?=
 =?utf-8?B?eDg4cWR1ZlgxSmlac3JLeEpzRWZpcUpERDRBakk4c093OGxFcjZNbGgzUVZL?=
 =?utf-8?B?azE1UjFCcXErU050eWxoczVhbjdFVVFsUVZOT0NhT0JxeXRSeWdXelltZXdt?=
 =?utf-8?B?RzRBZll3czlpckJWamdCUmd6ZkNlR2tNNktDSVJ0RnZKZytsRi9nOEpZbnVM?=
 =?utf-8?B?YWpTOSsrYkljOFBEaXloZzAvTnMvQXoyNXg0K3JlMUFQaGJMbEE2SUZ0eng5?=
 =?utf-8?B?OVU3UjJYVEZmdDFycWFvcmZmcCtYNFFvd0JsbmVmTTBWYmhVdVlpRU9sVVBs?=
 =?utf-8?B?aUtyUWdFdkIxb1Zma2ZDMW12elFyYlZLQ253QVkvZGJLMTUvM2xBbFFCMjkx?=
 =?utf-8?B?TjZFcGRocU4vUjU3K3RJaFZ3cm5mbjZiTkJERGh6eEROOFc2OGovL1FMcUxY?=
 =?utf-8?B?NC9vOGUzQWg0NFpyZmdjUTNNei9UTlRYeUtxcERmS1J3V3RFQjBqdU1qZGRY?=
 =?utf-8?B?S0NUWVhoTEd2ejhEQ2JnOTdFZzhOMHdoWUkyemxpNnA5c29iTG9SZTdqcDFW?=
 =?utf-8?B?R2lkWVFxL2Nmd2lONGlhb2dSQ1d0MmN2bHg1ZEhGbnRUaElIWXdLWXVGNzIx?=
 =?utf-8?B?ZjliaUZkMytGWWUxVTJlaFBYWU1ObE5zYWNITU5CUEFQbFhFUzBobWx6bTcz?=
 =?utf-8?B?alIxdlVmZXU4SlNRL3ljL1h0ZUhmT09nWkI5V0l0cEc0N3kxUGZsTm5odWlH?=
 =?utf-8?B?REVyUi9YSmw3MDdod01EYWJ6bEpLYTVQM0doQ0tJNDRuTm5zVUZOR2ZXa0Fk?=
 =?utf-8?B?UGU5VFlVQWJHd3FIRkxNQnFzQjhiM2N6MVlxcVRMVVM0MUpaTExhdHhQVEZ5?=
 =?utf-8?B?NzZmd1FidVE5MmxVbGxyTkxZclZBSStCdms1R0xxSUt5VmFTSzVpOTg3NmR0?=
 =?utf-8?B?cE5OT2RMSkxmKzhpNFkrekxSVStVZTh5NmI2SkthVkVoVm1BelJuNDJtVGlJ?=
 =?utf-8?B?OEZJbGJVSnlXRkVIa2Y3T0NGbGpFSTZPeEpERGhQdVBGbXJrd3hBVG5TZTNN?=
 =?utf-8?B?aDljdnZDZ1VTeTZYekNQVkhoRUk1eTlTYjltQVMrS01jMWxiYkVvZzVoNzNE?=
 =?utf-8?B?ZUtIdytYdlFnQWplTjdtZlp0N3Yvd1VYeG43SDF2VklScFJ5Y05DS0x2eWNK?=
 =?utf-8?B?U0hEcXJCYnJrYmhnUmh3bVJEL2RjNEErYVpRQXF2eDZpYXVaTFlrYmJUdHk5?=
 =?utf-8?B?dGF6RnMvT3lndTNvaEplclJZNmhzNGxkS1ZLNWdYRFVpUmxFdHBpSytUM2s5?=
 =?utf-8?B?VG9VUVJpY3hQVWlUWGtGK2ZJbEhPZktEQzRtQWM1ZUF5N1BtVWR0cUhxTmdl?=
 =?utf-8?B?TXBwQ2JHK2VCWnhKeStNdms3bDU4TXVwOGlXL3NKTEZYdkVHMTFVa2VkR3hG?=
 =?utf-8?B?T090UUp5WGJiRHZYOFF1VTVqTDJONFZGWC9ZS3FFQXoyZUYwMmo2cmV5VmY0?=
 =?utf-8?B?OS9UQkdyUjA0dlJyZVNQTGl2K0ZRaDE0TU0yUEJJM0dZSWc0RWRVS3NnbFdz?=
 =?utf-8?B?NHdMUjdoeFRNS2ZlSGJCSXp5S3lSbG1GeEdqWWhRY0cvNUFsS2dySWtoM1h4?=
 =?utf-8?B?ZWNuaENSS1Q0NmdpWFlYSkphWTl5WnI0bnhFWXVhdldtbmlva2YxajZNZjVR?=
 =?utf-8?B?cDlsRVFUbFF1ZDEwM3VDMlgvdEFGamJTRHlJRVRBanFSTDloVEZHM0ZOTmF4?=
 =?utf-8?B?Y3VtODRtRTJCWWo2VTJHZzNER3VuVitaYi9UWjkzbTJxeXNJb0p5L3lwM2hJ?=
 =?utf-8?B?ZUE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	Em2AfeXH6QaBWL7EyW7nSDyDuUMwAiyBJBOsxe/OCFSBf3JavnfaTurPnYMuL/8SEgdbq61iopKF/NbQ0BQvN/CfEGVsr+ycCKtk5L0H8fn6R/rC4P50+LjVDlpiXqY4VOA4h67W4teqvohEyjAKWzlU3YjOO0dCZtXs0nhKUuWSZrW5ojTQ299A+XSWV3DLS6vEq29cvJjY4ANXPobVgVqX53ko9/j6wPFK+hCZOr+9rFSLZcM6sfuRRb/o22CYz9qW3eU0nmcARdBRhNp133PiYBD0qu/HZWNqMA2R8PWbpeW4aJ1SrewOafRI/KKSy5KGzRpaqMuR+1m7nmJTIj/kxoKV9yVj8aHgGT0+XEbhqSg4zBCMLOHxucl77oG/Q52NZ57rSE/j71jSMqvOEfQ23UfEtWfA20gh7SC+EZb6NPV7/V++pilDmmrmZS9fFevf8zOYe2AIYSxUgVWDKwIl9SX6rjP9W60CISJAzz/bcziYT0vVb9tjQPnJ2muLoSxO+WkqOO4aeZbRATO9h/EYbosOsToY2c/E/+k0wFuUqhDmus93dT3caqcAs04h3Iy/522VjKXPBk3XQY8yjizHjbbNgjlCRYWD0GI0ww8=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 92c7f599-f7b9-4fc9-6440-08dc69b8fd67
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 May 2024 08:30:42.7957
 (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: ZVlh+RZrxGhZeu8coV+ARTnq53K+QplzeyNc9LbFNiMBiKa34Kc256Jm8DdQ+e5BN7ZUnbzK9ryckxgCsRe9vA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR10MB7065
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_08,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-2405010059
X-Proofpoint-GUID: 2rXO11HfEE-llNzGAd1BBNKm7lxW75yc
X-Proofpoint-ORIG-GUID: 2rXO11HfEE-llNzGAd1BBNKm7lxW75yc

On 30/04/2024 23:54, Dave Chinner wrote:
> On Mon, Apr 29, 2024 at 05:47:34PM +0000, John Garry wrote:
>> For when forcealign is enabled, we want the EOF to be aligned as well, so
>> do not free EOF blocks.
> 
> This is doesn't match what the code does. The code is correct - it
> rounds the range to be trimmed up to the aligned offset beyond EOF
> and then frees them. The description needs to be updated to reflect
> this.

ok, fine

> 
>>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>>   fs/xfs/xfs_bmap_util.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index 19e11d1da660..f26d1570b9bd 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -542,8 +542,13 @@ xfs_can_free_eofblocks(
>>   	 * forever.
>>   	 */
>>   	end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip));
>> -	if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1)
>> +
>> +	/* Do not free blocks when forcing extent sizes */
>> +	if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1)
> 
> I see this sort of check all through the remaining patches.
> 
> Given there are significant restrictions on forced alignment,
> shouldn't this all the details be pushed inside the helper function?
> e.g.
> 
> /*
>   * Forced extent alignment is dependent on extent size hints being
>   * set to define the alignment. Alignment is only necessary when the
>   * extent size hint is larger than a single block.
>   *
>   * If reflink is enabled on the file or we are in always_cow mode,
>   * we can't easily do forced alignment.
>   *
>   * We don't support forced alignment on realtime files.
>   * XXX(dgc): why not?

There is no technical reason to not be able to support forcealign on RT, 
AFAIK. My idea is to support RT after non-RT is supported.

>   */
> static inline bool
> xfs_inode_has_forcealign(struct xfs_inode *ip)
> {
> 	if (!(ip->di_flags & XFS_DIFLAG_EXTSIZE))
> 		return false;
> 	if (ip->i_extsize <= 1)
> 		return false;
> 
> 	if (xfs_is_cow_inode(ip))
> 		return false;

Could we just include this in the forcealign validate checks? Currently 
we just check CoW extsize is zero there.

> 	if (ip->di_flags & XFS_DIFLAG_REALTIME)
> 		return false;

We check this in xfs_inode_validate_forcealign()

> 
> 	return ip->di_flags2 & XFS_DIFLAG2_FORCEALIGN;
> }
> 

So can we simply have:

static inline bool
xfs_inode_has_forcealign(struct xfs_inode *ip)
{

	if (!(ip->di_flags & XFS_DIFLAG_EXTSIZE))
		return false;
  	if (ip->i_extsize <= 1)
  		return false;
  	return ip->di_flags2 & XFS_DIFLAG2_FORCEALIGN;
}

Thanks,
John