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
