Re: [PATCH v3 09/21] xfs: Do not free EOF blocks for forcealign

[John Garry]

On 02/05/2024 02:11, Dave Chinner wrote:
> > > static inline bool
> > > xfs_inode_has_forcealign(struct xfs_inode *ip)
> > > {
> > > 	if (!(ip->di_flags & XFS_DIFLAG_EXTSIZE))
> > > 		return false;
> > > 	if (ip->i_extsize <= 1)
> > > 		return false;
> > >
> > > 	if (xfs_is_cow_inode(ip))
> > > 		return false;
> > Could we just include this in the forcealign validate checks? Currently we
> > just check CoW extsize is zero there.
> Checking COW extsize is zero doesn't tell us anything useful about
> whether the inode might have shared extents, or that the filesystem
> has had the sysfs "always cow" debug knob turned on. That changes
> filesystem behaviour at mount time and has nothing to do with the
> on-disk format constraints.
>
> And now that I think about it, checking for COW extsize is
> completely the wrong thing to do because it doesn't get used until
> an extent is shared and a COW trigger is hit. So the presence of COW
> extsize has zero impact on whether we can use forced alignment or
> not.

ok

> IOWs, we have to check for shared extents or always cow here,
> because even a file with correctly set up forced alignment needs to
> have forced alignment disabled when always_cow is enabled. Every
> write is going to use the COW path and AFAICT we don't support
> forced alignment through that path yet.

ok

> > > 	if (ip->di_flags & XFS_DIFLAG_REALTIME)
> > > 		return false;
> > We check this in xfs_inode_validate_forcealign()
> That's kinda my point - we have a random smattering of different
> checks at different layers and in different contexts. i.e.  There's
> no one function that performs -all- the "can we do forced alignment"
> checks that allow forced alignment to be used. This simply adds all
> those checks in the one place and ensures that even if other code
> gets checks wrong, we won't use forcealign inappropriately.

Fine, I can do that if you think it is the best strategy.

Thanks,
John