Re: [PATCH v4 19/25] xfs: remove unwritten extents after preallocations in fsverity metadata

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Mar 13, 2026 at 12:17:15PM +0100, Andrey Albershteyn wrote:
> On 2026-03-12 07:52:50, Darrick J. Wong wrote:
> > On Thu, Mar 12, 2026 at 02:50:57PM +0100, Andrey Albershteyn wrote:
> > > On 2026-03-09 18:29:14, Darrick J. Wong wrote:
> > > > > XFS preallocates spaces during writes. In normal I/O this space, if
> > > > > unused, is removed by truncate. For files with fsverity, XFS does not
> > > > > use truncate as fsverity metadata is stored past EOF.
> > > > > 
> > > > > After we're done with writing fsverity metadata iterate over extents in
> > > > > that region and remove any unwritten ones. These would be preallocation
> > > > > leftovers in the merkle tree holes and past fsverity descriptor.
> > > > > 
> > > > > Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> > > > > ---
> > > 
> > > > There's an upper limit on the number of blocks you can unmap/free in a
> > > > single transaction.  Maybe move the xfs_trans_{alloc,commit} into the
> > > > loop body?
> > > > 
> > > > Oh wait, you skip the written extents.  Ok, so maybe just roll it after
> > > > you've done a bunmapi.
> > > 
> > > I see, I will add a rolling transaction. Btw, why skipping the
> > > written extents let's us use rolling here?
> > 
> > Hrmm.  At first I thought: why can't xfs_fsverity_cancel_unwritten
> > allocate (and commit) the transaction inside the loop body?  Then I
> > thought "well, it's only conditionally unmapping things, and it's sort
> > of a pain to allocate a transaction only then to find out if you
> > actually want to run one".
> > 
> > OTOH there could be billions of extents in the data fork, so holding the
> > ILOCK for that many transactions isn't a good thing because tr_write
> > only preallocates space for a certain number of transaction rolls.
> > fsverity already holds IOLOCK_EXCL so there can't be any other programs
> > using the file.
> > 
> > So now I arrive back at "The transaction allocation and commit/cancel
> > should be inside the loop body, since crashing midway through wouldn't
> > result in user-visible changes to the file data".
> > 
> > > > Do you need to purge the cow fork too?
> > > 
> > > hmm, what case are you thinking about here? The fsverity is written
> > > in past EOF region, I don't see how COW extent could be left there.
> > > Can they somehow be left mapped for blocks past i_size?
> > 
> > I was talking about speculative cow fork preallocations below i_size.
> > They'll eventually get purged by blockgc, but you could give the space
> > back once you've committed enabling the fsverity file flag.
> 
> hmm, if they're purged by blockgc, then why to do it here? I'm

Since you've made the file read-only by turning on fsverity, I think
it's appropriate to return the COW fork preallocations to the free space
pool.  Granted, you're correct that blockgc will eventually call
xfs_reflink_cancel_cow_range for you either due to timeout or because
something hit ENOSPC and kicked blockgc.  So I guess its not a
requirement to land this series; I was just surprised not to see
xfs_reflink_cancel_cow_range done explicitly here.

> iterating and removing unwritten extents here only because
> xfs_inode_free_eofblocks() will skip fsverity inodes, the check in
> xfs_can_free_eofblocks():
> 
> 	if (IS_VERITY(VFS_I(ip)))
> 		return false;
> 
> The skipping is done to not remove all the fsverity metadata
> extents. With preallocations enabled there could be unwritten
> extents left in the metadata.

<nod> Cleaning out the unwritten extents from the data fork when
enabling fsverity makes sense to me and is totally fine.

> Am I missing something?

Nah, but we might be talking past each other a bit at this point. :/

--D