On Mon, Jul 01, 2024 at 10:26:18AM +0800, Zhang Yi wrote: > On 2024/7/1 9:16, Dave Chinner wrote: > > On Tue, Jun 18, 2024 at 10:21:11PM +0800, Zhang Yi wrote: > >> @@ -917,7 +920,17 @@ xfs_setattr_size( > >> return error; > >> } > >> > >> - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp); > >> + /* > >> + * For realtime inode with more than one block rtextsize, we need the > >> + * block reservation for bmap btree block allocations/splits that can > >> + * happen since it could split the tail written extent and convert the > >> + * right beyond EOF one to unwritten. > >> + */ > >> + if (xfs_inode_has_bigrtalloc(ip)) > >> + resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0); > > > > .... should this be doing this generic check instead: > > > > if (xfs_inode_alloc_unitsize(ip) > 1) > > if (xfs_inode_alloc_unitsize(ip) > i_blocksize(inode)) ? > > > resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0); > > > > Yeah, it makes sense to me, but Christoph suggested to think about force > aligned allocations later, so I only dealt with the big RT inode case here. > I can revise it if John and Christoph don't object. Sorry, but I don't really care what either John or Christoph say on this matter: xfs_inode_has_bigrtalloc() is recently introduced technical debt that should not be propagated further. xfs_inode_has_bigrtalloc() needs to be replaced completely with xfs_inode_alloc_unitsize() and any conditional behaviour needed can be based on the return value from xfs_inode_alloc_unitsize(). That works for everything that has an allocation block size larger than one filesystem block, not just one specific RT case. Don't force John to have fix all these same RT bugs that are being fixed with xfs_inode_has_bigrtalloc() just because forced alignment stuff is not yet merged. Don't make John's life harder than it needs to be to get that stuff merged, and don't waste my time arguing about it: just fix the problem the right way the first time. -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
