Re: [PATCH] xfs: Fix file creation failure

[Darrick J. Wong]

On Wed, Jun 05, 2024 at 09:00:28AM +1000, Dave Chinner wrote:
> On Tue, Jun 04, 2024 at 03:11:21PM +0800, Zizhi Wo wrote:
> > We have an xfs image that contains only 2 AGs, the first AG is full and
> > the second AG is empty, then a concurrent file creation and little writing
> > could unexpectedly return -ENOSPC error since there is a race window that
> > the allocator could get the wrong agf->agf_longest.
> > 
> > Write file process steps:
> > 1) Find the entry that best meets the conditions, then calculate the start
> > address and length of the remaining part of the entry after allocation.
> > 2) Delete this entry. Because the second AG is empty, the btree in its agf
> > has only one record, and agf->agf_longest will be set to 0 after deletion.
> > 3) Insert the remaining unused parts of this entry based on the
> > calculations in 1), and update the agf->agf_longest.
> > 
> > Create file process steps:
> > 1) Check whether there are free inodes in the inode chunk.
> > 2) If there is no free inode, check whether there has space for creating
> > inode chunks, perform the no-lock judgment first.
> > 3) If the judgment succeeds, the judgment is performed again with agf lock
> > held. Otherwire, an error is returned directly.
> > 
> > If the write process is in step 2) but not go to 3) yet, the create file
> > process goes to 2) at this time, it will be mistaken for no space,
> > resulting in the file system still has space but the file creation fails.
> > 
> > 	Direct write				Create file
> > xfs_file_write_iter
> >  ...
> >  xfs_direct_write_iomap_begin
> >   xfs_iomap_write_direct
> >    ...
> >    xfs_alloc_ag_vextent_near
> >     xfs_alloc_cur_finish
> >      xfs_alloc_fixup_trees
> >       xfs_btree_delete
> >        xfs_btree_delrec
> > 	xfs_allocbt_update_lastrec
> > 	// longest = 0 because numrec == 0.
> > 	 agf->agf_longest = len = 0
> > 					   xfs_create
> > 					    ...
> > 					     xfs_dialloc
> > 					      ...
> > 					      xfs_alloc_fix_freelist
> > 					       xfs_alloc_space_available
> > 					-> as longest=0, it will return
> > 					false, no space for inode alloc.
> 
> Ok, so this is a another attempt to address the problem Ye Bin
> attempted to fix here:
> 
> https://lore.kernel.org/linux-xfs/20240419061848.1032366-1-yebin10@xxxxxxxxxx/
> 
> > Fix this issue by adding the bc_free_longest field to the xfs_btree_cur_t
> > structure to store the potential longest count that will be updated. The
> > assignment is done in xfs_alloc_fixup_trees() and xfs_free_ag_extent().
> 
> I outlined how this should be fixed in the above thread:
> 
> https://lore.kernel.org/linux-xfs/ZiWgRGWVG4aK1165@xxxxxxxxxxxxxxxxxxx/
> 
> This is what I said:
> 
> | What we actually want is for pag->pagf_longest not to change
> | transiently to zero in xfs_alloc_fixup_trees(). If the delrec that
> | zeroes the pagf_longest field is going to follow it up with an
> | insrec that will set it back to a valid value, we really should not
> | be doing the zeroing in the first place.
> | 
> | Further, the only btree that tracks the right edge of the btree is
> | the by-count allocbt. This isn't "generic" functionality, even
> | though it is implemented through the generic btree code. If we lift
> | ->update_lastrec from the generic code and do it directly in
> | xfs_alloc.c whenever we are finished with a by-count tree update
> | and the cursor points to a record in the right-most leaf of the
> | tree, then we run the lastrec update directly at that point. 
> | By decoupling the lastrec updates from the individual record
> | manipulations, we can make the transients disappear completely.
> 
> I'm not sure if this patch is an attempt to implement this - there
> is no reference in the commit description to this previous attempt
> to fix the issue, nor is the any discussion of why this particular
> solution was chosen.
> 
> In future, when you are trying to fix an issue that has previously
> been discussed/presented on the list, please reference it and
> provide a link to the previous discussions in the changelog for the
> new version of the patchset fixing the issue.

Aha, that's why this seemed oddly familiar.

--D

> > Reported by: Ye Bin <yebin10@xxxxxxxxxx>
> > Signed-off-by: Zizhi Wo <wozizhi@xxxxxxxxxx>
> > ---
> >  fs/xfs/libxfs/xfs_alloc.c       | 14 ++++++++++++++
> >  fs/xfs/libxfs/xfs_alloc_btree.c |  9 ++++++++-
> >  fs/xfs/libxfs/xfs_btree.h       |  1 +
> >  3 files changed, 23 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index 6c55a6e88eba..86ba873d57a8 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -577,6 +577,13 @@ xfs_alloc_fixup_trees(
> >  		nfbno2 = rbno + rlen;
> >  		nflen2 = (fbno + flen) - nfbno2;
> >  	}
> > +
> > +	/*
> > +	 * Record the potential maximum free length in advance.
> > +	 */
> > +	if (nfbno1 != NULLAGBLOCK || nfbno2 != NULLAGBLOCK)
> > +		cnt_cur->bc_ag.bc_free_longest = XFS_EXTLEN_MAX(nflen1, nflen2);
> > +
> 
> Why do we store the length of a random extent being freed here?
> nflen1/2 almost always have nothing to do with the longest free
> space extent in the tree, they are just the new free space extents
> we are insering into a random location in the free space tree.
> 
> >  	/*
> >  	 * Delete the entry from the by-size btree.
> >  	 */
> > @@ -2044,6 +2051,13 @@ xfs_free_ag_extent(
> >  	 * Now allocate and initialize a cursor for the by-size tree.
> >  	 */
> >  	cnt_cur = xfs_cntbt_init_cursor(mp, tp, agbp, pag);
> > +	/*
> > +	 * Record the potential maximum free length in advance.
> > +	 */
> > +	if (haveleft)
> > +		cnt_cur->bc_ag.bc_free_longest = ltlen;
> > +	if (haveright)
> > +		cnt_cur->bc_ag.bc_free_longest = gtlen;
> 
> That doesn't look correct. At this point in the code, ltlen/gtlen
> are the sizes of the physically adjacent freespace extents that we
> are going to merge the newly freed extent with. i.e. the new
> freespace extent is going to have one of 4 possible values:
> 
> 	no merge: len
> 	left merge: ltlen + len
> 	right merge: gtlen + len
> 	both merge: ltlen + gtlen + len
> 
> So regardless of anything else, this code isn't setting the longest
> freespace extent in teh AGF to the lenght of the longest freespace
> extent in the filesystem.
> 
> Which leads me to ask: how did you test this code? This bug should
> have been triggering verifier, repair and scrub failures quite
> quickly with fstests....
> 
> >  	/*
> >  	 * Have both left and right contiguous neighbors.
> >  	 * Merge all three into a single free block.
> > diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
> > index 6ef5ddd89600..8e7d1e0f1a63 100644
> > --- a/fs/xfs/libxfs/xfs_alloc_btree.c
> > +++ b/fs/xfs/libxfs/xfs_alloc_btree.c
> > @@ -161,7 +161,14 @@ xfs_allocbt_update_lastrec(
> >  			rrp = XFS_ALLOC_REC_ADDR(cur->bc_mp, block, numrecs);
> >  			len = rrp->ar_blockcount;
> >  		} else {
> > -			len = 0;
> > +			/*
> > +			 * Update in advance to prevent file creation failure
> > +			 * for concurrent processes even though there is no
> > +			 * numrec currently.
> > +			 * And there's no need to worry as the value that no
> > +			 * less than bc_free_longest will be inserted later.
> > +			 */
> > +			len = cpu_to_be32(cur->bc_ag.bc_free_longest);
> >  		}
> 
> So this is in the LASTREC_DELREC case when the last record is
> removed from the btree. This is what causes the transient state
> as we do this when deleting a record to trim it and then re-insert
> the remainder back into the by-count btree.
> 
> Writing some random transient value into the AGF *and journalling
> it* means we creating a transient on-disk format structure
> corruption and potentially writing it to persistent storage (i.e.
> the journal). The structure is, at least, not consistent in memory
> because the free space tree is empty at this point in time, whilst
> the agf->longest field says it has a free space available. This
> could trip verifiers, be flagged as corruption by xfs_scrub/repair,
> etc.
> 
> Now, this *might be safe* because we *may* clean it up later in the
> transaction, but if this really is the last extent being removed
> from the btree and a cursor has previously been used to do other
> insert and free operations that set this field, then we trip over
> this stale inforamtion and write a corrupt structure to disk. That's
> not good.
> 
> As I said above, this "last record tracking" needs to be ripped out
> of the generic btree code because only the by-count btree uses it.
> Then it can be updated at the end of the by-count btree update
> process in the allocation code (i.e. after all record manipulations
> are done in the transaction) and that avoids this transient caused
> by updating the last record on every btree record update that is
> done.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@xxxxxxxxxxxxx
>