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

Darrick J. Wong
2021-03-18T04:34:32Z

On Mon, Mar 15, 2021 at 06:46:15PM +0000, Christoph Hellwig wrote:
> 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.

Sorry about that; I'll spell them out in the future.

> 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.

I'll have a look into that tomorrow morning. :)

> > 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.

Yeah, this area needs some simplification. Will do.

--D