Received-SPF: pass (google.com: domain of linux-kernel+bounces-165159-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <05016d8f-cc25-4148-bf78-6567cc2dfbc4@oracle.com>
Date: Wed, 1 May 2024 11:54:43 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RFC v3 11/21] xfs: Unmap blocks according to 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-12-john.g.garry@oracle.com>
 <ZjGIktQV12qas14f@dread.disaster.area>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <ZjGIktQV12qas14f@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?bERqVzQ5Si91OWZWdDJaUHNqWk9jTTJ1TGlYSHVGcVlRSnBQT0ZqOFFPdi8w?=
 =?utf-8?B?Q2pYWU5Xc0Q3UmJndTFXTXhuVEVpcTJSeGZ4K0xTY3hmbVFoRk9ka1p2Z1oy?=
 =?utf-8?B?U2sxYjNra1FrTmJYMlFYVkxDYXl1cHU5OWFJR012bDVSZ2lPZ2tacXpBam9t?=
 =?utf-8?B?emtwaWpNN2lJMEo0VmYzUitBM3NZejF2azhraHNTZkxvZHBkKytDTTcyU0Iy?=
 =?utf-8?B?a0dlNUdsdlRPV3ltK21rUmpDTjJKOHdpWnRKZ3JXRUhBOEhkZ3N2cGtHOHdp?=
 =?utf-8?B?ZlFpNVFYM2czV1FLOHVDcVY3ZTJMMHE0K3h1UTJ0c3BUTklkMC90dHhKOFdN?=
 =?utf-8?B?Slg0R3hzTGFGMkpXNG14eDJpQkF4Y01VeFN4WHNnYWRBQUtoMnNUcEpsa0Nz?=
 =?utf-8?B?NXQyY0RESE4rNkZ6NnBlc1k4c0ovY1UzZ3NDS25pVFpBSm0xUnNLeTJVWWpQ?=
 =?utf-8?B?RjROSXIxTHNXOXpyVktudE00WWlaa21nM3JZck9VTWpqRHRXeHhHckcyK2dS?=
 =?utf-8?B?YzcrNEwyQjNLc3NIZG11U2FKaytUYW1abzlrUGI0VGhYOGZpR2k2TEYzUHY5?=
 =?utf-8?B?bVlKZEhpODdOZGpXR1lJTVNHWlJ1YXlOemFrdTgzcHdnSU5LWFRVY0pXOFJE?=
 =?utf-8?B?Y21QRHZvZk1UWDN0RHV1WnpNanVCaHBZbm9nWGg0OXJaWG1BWm1uMGx2aW1H?=
 =?utf-8?B?M09DSnkycFlDbUZQWk1yMml0Z2NFa29BWHF6clBtQnE0UWhQUFEvRUpCdndD?=
 =?utf-8?B?RXhrajZpTnNqSEZQRUlsUWtvempiZi9uRnh6eE5PR2dGeWk0b0F0Vms3elZ4?=
 =?utf-8?B?MjNycVBOUEcwSXRjWHJ2M2liZm5ZbUpYRmhtN1lLTXJ2SEp2RGRuY1BaQ0My?=
 =?utf-8?B?cUpZNGxwMWY0OTR1WnZnZGZSYU85aERVN3lzQzVFcjZZckJ6bVF0N0xQYnZl?=
 =?utf-8?B?dUxSWk1xQlRKRDU1LzdhTXgrZlpMSEhJVURZMUFqaDAwNzRmTm5TRDkveGFG?=
 =?utf-8?B?cDFQL1Jycy8rejBuMXVYSHVuVDlsT2loZ2FvczJKa2MrN2l2aGZyTkFDNkFl?=
 =?utf-8?B?bTNWR3IwUlYwRnBjU0NSQ3hFUEhaam43dFlRVWRZYlpTRExrNG1jcUxsZFFV?=
 =?utf-8?B?aXM3QzhlTWJkSzNXRW9WZ01SQmNRblRacWp2VUZNSGtaTzRsbTRucjU5L1lJ?=
 =?utf-8?B?OGFGc1NJcSt1WW1DcTU4QXdkUWFmeG9yU3cxRnNLUG02S09iZ0gyb2NhcHlx?=
 =?utf-8?B?YkUwVitKMDB0bmt1NUszUjF1Z3BBSjhURHFhTEtIcDJ2TkNNMjI1WlRlbnU3?=
 =?utf-8?B?Rjl5dDh2SWEvYnU0alJ5b0xpbWhuS3dkRnNrK0p2V1lya3ErbVc0enU4cmtD?=
 =?utf-8?B?d29qNS9BL3ZaSXdDZmtvUGk4N2YvYzJac3ppVmlKRnZHZ2FOcVFWMmJ1QU9E?=
 =?utf-8?B?Q2tjaTZNVmYvNnJ3ZFRCbDJHSmljcGZ2enY2d1VMcHMzdE9IYWhGczZSRkJl?=
 =?utf-8?B?ODMrVHBsRTlZK3FrTEJRWHV5ZStpNUp4NjU5SzNnMjFnVXVoM2ZEWWVZZ3Rs?=
 =?utf-8?B?RlVWZWJsb2dWS2huQ0d1L2gycFZSaEhIdWRtLzVOUVFITnhDRXNUTjJYM3Mw?=
 =?utf-8?B?TjdXdHBPMFllNkVTTGV6U09vSy83aElsV3EvMFRtc3AzTlMyVmRrWUNaT3BQ?=
 =?utf-8?B?Y0FXejlWM0dSeWF2MWdydnh3ZS81cUY4NitraStNQTRVQ0pLSDZ3OWxpSldY?=
 =?utf-8?B?UU80MnE1RnlQaGlPbDRHRWVIR0FpUmhFR09NQXcvREh1ajdVbFJtQ1ZESzNX?=
 =?utf-8?B?Z2pvTHVFbngzWnZuTFBqdFNDUGhCNmp0Sk53THJWS2hhK294cGlWbE9ZOWJW?=
 =?utf-8?B?Z04zRHRTTW9XN1hiTGF4VDljdmR1UHZMTG13bFl1cmNjVWFzWG1sdmhMSlRU?=
 =?utf-8?B?azZ6MXFNUVNTUDFFYnA5d055Ymppc2k3WmRBU0Z3bzY3KzZiaGN1UmRPYm1o?=
 =?utf-8?B?ZjVXV2owUUlURk1KL25rUmtFbDZmODQ3WGgrYjM0WE1XREtFNVd4Y0MyeDgx?=
 =?utf-8?B?dEZ0V1o3WU5BanE5SU1kdUpsUTF4b0oxU2RYYytvU3Rndy8yNFNlUzY1OWtT?=
 =?utf-8?B?aThMeE1IQXk2MC92NFpWRmtYdExBdzgydHdpTnFGMDI1ZjFDenNVVm1OWTV0?=
 =?utf-8?B?WXc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	NJaF6ZcVN1j38/oOzVfEb4o2qkHfWCLMajw7r5N+E0KfUhUVIuwfAZERWcA+UsKby/skeBYCziRMZjGLRKt/OVFoKVNywmSQu1h9MGZax1NlLskSjgOjGrwX07A60Y4IlsLKJsj4N5/yn7W7eM0192/x5o6NBAJj5c6ycNw7zENxCx56EEAnRG0cR+Z6YB7wPlR2czkk2L+ZqkhmQ7tYuhmoA3WatvUCaSf30uv+YMPP5ApIORHhklnh5c3jw/bmHO28diHLOB7wphcMcToT66q1MuCO3ElVkzhaVBY4XL3hnQrYtxkOP+Wbgb2F6CgONsdJSE0n9c+gvcYNOt1ZQXhbsAa38NyJhANuXr57+UbqHGssO8z/Y86v+fxqGM0wYhh5156RQ2MeWf5i6h0Kr7VObbj907L1fmVQmIRO85ksFmhma1eKln7dBfiaClZB0WNcZ8J6aXCSwkLQDNCXma4uzuN+Zvl8StESrhHgxD3Hl2sEHa3QulOMS//3T8fGXctMoFO1eJLTGrWDiPYYVr8ObqoMeuRL3WaQIJuMwvmfDjx653mLnF5D1ywSGz9nUbPQFqPOWpyqyOpcQqkS3avCcIL/Lug/EydQXOym5eI=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dcabe19d-4d1b-436d-cc5c-08dc69cd1f19
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:54:49.2825
 (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: Kh0Ke8thfVfjx/r6adRwkraRBAnin1ABLBrYYiWzjlEgOteVjbGztNGHgXhfZs0vABk7wkqt4nOySjKhXBAxSA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR10MB6836
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-2405010077
X-Proofpoint-ORIG-GUID: MO895rLumaoCCS7RboV3R-5emnzm9mAq
X-Proofpoint-GUID: MO895rLumaoCCS7RboV3R-5emnzm9mAq

On 01/05/2024 01:10, Dave Chinner wrote:
> On Mon, Apr 29, 2024 at 05:47:36PM +0000, John Garry wrote:
>> For when forcealign is enabled, blocks in an inode need to be unmapped
>> according to extent alignment, like what is already done for rtvol.
>>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_bmap.c | 39 +++++++++++++++++++++++++++++++++------
>>   fs/xfs/xfs_inode.h       |  5 +++++
>>   2 files changed, 38 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
>> index 4f39a43d78a7..4a78ab193753 100644
>> --- a/fs/xfs/libxfs/xfs_bmap.c
>> +++ b/fs/xfs/libxfs/xfs_bmap.c
>> @@ -5339,6 +5339,15 @@ xfs_bmap_del_extent_real(
>>   	return 0;
>>   }
>>   
>> +/* Return the offset of an block number within an extent for forcealign. */
>> +static xfs_extlen_t
>> +xfs_forcealign_extent_offset(
>> +	struct xfs_inode	*ip,
>> +	xfs_fsblock_t		bno)
>> +{
>> +	return bno & (ip->i_extsize - 1);
>> +}
>> +
>>   /*
>>    * Unmap (remove) blocks from a file.
>>    * If nexts is nonzero then the number of extents to remove is limited to
>> @@ -5361,6 +5370,7 @@ __xfs_bunmapi(
>>   	struct xfs_bmbt_irec	got;		/* current extent record */
>>   	struct xfs_ifork	*ifp;		/* inode fork pointer */
>>   	int			isrt;		/* freeing in rt area */
>> +	int			isforcealign;	/* freeing for file inode with forcealign */
>>   	int			logflags;	/* transaction logging flags */
>>   	xfs_extlen_t		mod;		/* rt extent offset */
>>   	struct xfs_mount	*mp = ip->i_mount;
>> @@ -5397,7 +5407,10 @@ __xfs_bunmapi(
>>   		return 0;
>>   	}
>>   	XFS_STATS_INC(mp, xs_blk_unmap);
>> -	isrt = xfs_ifork_is_realtime(ip, whichfork);
>> +	isrt = (whichfork == XFS_DATA_FORK) && XFS_IS_REALTIME_INODE(ip);
> 
> Why did you change this check? What's wrong with
> xfs_ifork_is_realtime(), and if there is something wrong, why
> shouldn't xfs_ifork_is_relatime() get fixed?

oops, I should have not made that change. I must have changed it when 
debugging and not reverted it.

> 
>> +	isforcealign = (whichfork == XFS_DATA_FORK) &&
>> +			xfs_inode_has_forcealign(ip) &&
>> +			xfs_inode_has_extsize(ip) && ip->i_extsize > 1;
> 
> This is one of the reasons why I said xfs_inode_has_forcealign()
> should be checking that extent size hints should be checked in that
> helper....

Right. In this particular case, I found that directories may be 
considered as well if we don't check for xfs_inode_has_extsize() (which 
we don't want).

> 
>>   	end = start + len;
>>   
>>   	if (!xfs_iext_lookup_extent_before(ip, ifp, &end, &icur, &got)) {
>> @@ -5459,11 +5472,15 @@ __xfs_bunmapi(
>>   		if (del.br_startoff + del.br_blockcount > end + 1)
>>   			del.br_blockcount = end + 1 - del.br_startoff;
>>   
>> -		if (!isrt || (flags & XFS_BMAPI_REMAP))
>> +		if ((!isrt && !isforcealign) || (flags & XFS_BMAPI_REMAP))
>>   			goto delete;
>>   
>> -		mod = xfs_rtb_to_rtxoff(mp,
>> -				del.br_startblock + del.br_blockcount);
>> +		if (isrt)
>> +			mod = xfs_rtb_to_rtxoff(mp,
>> +					del.br_startblock + del.br_blockcount);
>> +		else if (isforcealign)
>> +			mod = xfs_forcealign_extent_offset(ip,
>> +					del.br_startblock + del.br_blockcount);
> 
> There's got to be a cleaner way to do this.
> 
> We already know that either isrt or isforcealign must be set here,
> so there's no need for the "else if" construct.

right

> 
> Also, forcealign should take precedence over realtime, so that
> forcealign will work on realtime devices as well. I'd change this
> code to call a wrapper like:
> 
> 		mod = xfs_bunmapi_align(ip, del.br_startblock + del.br_blockcount);
> 
> static xfs_extlen_t
> xfs_bunmapi_align(
> 	struct xfs_inode	*ip,
> 	xfs_fsblock_t		bno)
> {
> 	if (!XFS_INODE_IS_REALTIME(ip)) {
> 		ASSERT(xfs_inode_has_forcealign(ip))
> 		if (is_power_of_2(ip->i_extsize))
> 			return bno & (ip->i_extsize - 1);
> 		return do_div(bno, ip->i_extsize);
> 	}
> 	return xfs_rtb_to_rtxoff(ip->i_mount, bno);
> }

ok, that's neater

Thanks,
John