On Thu, Dec 05, 2024 at 08:18:02PM +0800, Jinliang Zheng wrote: > 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. Yes, but xfs_bmap_adjacent_valid() does not tell us that. > /* > * 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 For people reading along: This sets the allocation target to the end of the previous physical extent. > /* > * 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 And this adjusts for the file offset of the new EOF allocation being a distance beyond the previous extent. i.e. file offset: 0 EOF ap->offset layout: +--prev--+-----hole-----+--new EOF allocation--+ After allocation: file offset: 0 oEOF offset EOF layout: +--prev--+-----hole-----+--new EOF allocation--+ physical: +--used--+-----free-----+-------used-----------+ And now when the write to fill the file offset hole (e.g. because of racing concurrent extending AIO+DIO writes being issued out of order), we end up with this non-EOF NEAR allocation being set up over the hole in the file: file offset: 0 ap->offset EOF layout: +--prev--+-----hole-----+--------next----------+ ap->blkno And the NEAR allocation will find the exact free space we left to fill that hole, resulting in a file that looks like this: file offset: 0 EOF layout: +----------------------------------------------+ physical: +----------------------------------------------+ i.e. a single contiguous extent. > return true; And it's important to note that xfs_bmap_adjacent returns true if it selects a new target for exact allocation. > } > > 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. Well, yes, that is exactly what the code was -trying- to do. It was using ap->offset as a proxy for "there is a previous extent" rather than an explicit check for "do we need exact allocation" As you've rightly pointed out - this code is not correct in all situations, nor optimal for all situations. What I've been trying to point out to you is that your solution is not optimal, either. > Therefore, we need xfs_bmap_adjacent() to determine whether xfs_bmap_adjacent() > has indeed modified ap->blkno. It already does, but we ignore it. If we want use exact allocation only when we are doing EOF allocation: Perhaps: - xfs_bmap_adjacent(ap); + if (!xfs_bmap_adjacent(ap)) + ap->eof = false; And then in xfs_bmap_btalloc_at_eof() all we need is - if (ap->offset) { + if (ap->eof) { i.e. we only do exact allocation at EOF when xfs_bmap_adjacent has set a target we want exact allocation for. (note: don't confuse ap->eof and ap->aeof) > > 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. This *is* a general scenario. Every single extending allocation goes through this path. > 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? I think you missed the significance of the gap (file offset) adjustment. Write a couple of TB beyond EOF and see what happens. Then allocate a file in the last AG that is a runt, and try to write a distance beyond EOF that will land the target blkno between the size of the runt AG and mp->m_sb.sb_agcount.... Hint: runt AG length < AGBNO(ap->blkno) < mp->m_sb.sb_agcount. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
