Re: [PATCH 1/2] xfs: rearrange code in xfs_inode_item_precommit

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

On Fri, Sep 19, 2025 at 08:19:52AM -0700, Christoph Hellwig wrote:
> On Thu, Sep 18, 2025 at 08:12:53AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > There are similar extsize checks and updates done inside and outside
> > the inode item lock, which could all be done under a single top
> > level logic branch outside the ili_lock. The COW extsize fixup can
> > potentially miss updating the XFS_ILOG_CORE in ili_fsync_fields, so
> > moving this code up above the ili_fsync_fields update could also be
> > considered a fix.
> > 
> > Further, to make the next change a bit cleaner, move where we
> > calculate the on-disk flag mask to after we attach the cluster
> > buffer to the the inode log item.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_inode_item.c | 65 ++++++++++++++++++-----------------------
> >  1 file changed, 29 insertions(+), 36 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> > index afb6cadf7793..318e7c68ec72 100644
> > --- a/fs/xfs/xfs_inode_item.c
> > +++ b/fs/xfs/xfs_inode_item.c
> > @@ -131,46 +131,28 @@ xfs_inode_item_precommit(
> >  	}
> >  
> >  	/*
> > +	 * Inode verifiers do not check that the extent size hints are an
> > +	 * integer multiple of the rt extent size on a directory with
> > +	 * rtinherit flags set.  If we're logging a directory that is
> > +	 * misconfigured in this way, clear the bad hints.
> >  	 */
> 
> Not directly related to this patch, but why are we not checking for
> that in the inode verifier?  Even if we can't reject that value,
> it seems like we should fix that up when reading an inode into memory
> instead of in a pre-commit hook?

growfs can change rtextsize when adding a rt section to the filesystem,
and we're not allowed to break userspace (even though I'd wager nobody
has actually done this in the past 20 years).

--D

> The patch itself looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>