Re: [PATCH 02/11] xfs: refactor the predicate part of xfs_free_eofblocks

Christoph Hellwig
2021-03-15T18:47:22Z

Going further through the series actually made me go back to this one,
so a few more comments:

> /*
> + * Decide if this inode have post-EOF blocks. The caller is responsible
> + * for knowing / caring about the PREALLOC/APPEND flags.

Please spell out the XFS_DIFLAG_ here, as this really confused me. In
fact even with that it still confuses me, as "caller is responsible"
here really means: only call this if you previously called
xfs_can_free_eofblocks and it return true.

Which brings me to the structure of this: I think without much pain
we can ensure xfs_can_free_eofblocks is always called with the iolock,
in which case we really should merge xfs_can_free_eofblocks and this
new helper to avoid the rather confusing fact that we have two similarly
named helper doing similiar but not the same thing.

> int
> +xfs_has_eofblocks(
> + struct xfs_inode *ip,
> + bool *has)

I also think the calling convention can be simplified here. If an
error occurs we obviously do not want to free the eofblocks. So
instead of returning two calues we can just return a single bool.