On Wed, 4 Dec 2024 07:40:20 +1100, Dave Chinner wrote: > On Sat, Nov 31, 2024 at 07:11:32PM +0800, Jinliang Zheng wrote: > > When we call create(), lseek() and write() sequentially, offset != 0 > > cannot be used as a judgment condition for whether the file already > > has extents. > > > > Furthermore, when xfs_bmap_adjacent() has not given a better blkno, > > it is not necessary to use exact EOF block allocation. > > > > Signed-off-by: Jinliang Zheng <alexjlzheng@xxxxxxxxxxx> > > --- > > Changelog: > > - V2: Fix the entry condition > > - V1: https://lore.kernel.org/linux-xfs/ZyFJm7xg7Msd6eVr@xxxxxxxxxxxxxxxxxxx/T/#t > > --- > > fs/xfs/libxfs/xfs_bmap.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > > index 36dd08d13293..c1e5372b6b2e 100644 > > --- a/fs/xfs/libxfs/xfs_bmap.c > > +++ b/fs/xfs/libxfs/xfs_bmap.c > > @@ -3531,12 +3531,14 @@ xfs_bmap_btalloc_at_eof( > > int error; > > > > /* > > - * If there are already extents in the file, try an exact EOF block > > - * allocation to extend the file as a contiguous extent. If that fails, > > - * or it's the first allocation in a file, just try for a stripe aligned > > - * allocation. > > + * If there are already extents in the file, and xfs_bmap_adjacent() has > > + * given a better blkno, try an exact EOF block allocation to extend the > > + * file as a contiguous extent. If that fails, or it's the first > > + * allocation in a file, just try for a stripe aligned allocation. > > */ > > - if (ap->offset) { > > + if (ap->prev.br_startoff != NULLFILEOFF && > > + !isnullstartblock(ap->prev.br_startblock) && > > + xfs_bmap_adjacent_valid(ap, ap->blkno, ap->prev.br_startblock)) { > > There's no need for calling xfs_bmap_adjacent_valid() here - > we know that ap->blkno is valid because the > bounds checking has already been done by xfs_bmap_adjacent(). I'm sorry that I didn't express it clearly, what I meant here is: if we want to extend the file as a contiguous extent, then ap->blkno must be a better choice given by xfs_bmap_adjacent() than other default values. /* * If allocating at eof, and there's a previous real block, * try to use its last block as our starting point. */ if (ap->eof && ap->prev.br_startoff != NULLFILEOFF && !isnullstartblock(ap->prev.br_startblock) && xfs_bmap_adjacent_valid(ap, ap->prev.br_startblock + ap->prev.br_blockcount, ap->prev.br_startblock)) { ap->blkno = ap->prev.br_startblock + ap->prev.br_blockcount; <--- better A /* * Adjust for the gap between prevp and us. */ adjust = ap->offset - (ap->prev.br_startoff + ap->prev.br_blockcount); if (adjust && xfs_bmap_adjacent_valid(ap, ap->blkno + adjust, ap->prev.br_startblock)) ap->blkno += adjust; <--- better B return true; } Only when we reach 'better A' or 'better B' of xfs_bmap_adjacent() above, it is worth trying to use xfs_alloc_vextent_EXACT_bno(). Otherwise, NEAR is more suitable than EXACT. Therefore, we need xfs_bmap_adjacent() to determine whether xfs_bmap_adjacent() has indeed modified ap->blkno. > > Actually, for another patch, the bounds checking in > xfs_bmap_adjacent_valid() is incorrect. What happens if the last AG > is a runt? i.e. it open codes xfs_verify_fsbno() and gets it wrong. For general scenarios, I agree. But here, the parameters x and y of xfs_bmap_adjacent_valid() are both derived from ap->prev. Is it possible that it exceeds mp->m_sb.sb_agcount or mp->m_sb.sb_agblocks? Thank you :) Jinliang Zheng > > -Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx
