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

Darrick J. Wong
2021-03-19T01:49:54Z

On Wed, Mar 17, 2021 at 09:33:29PM -0700, Darrick J. Wong wrote:
> 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. :)

The only change that was necessary was moving the can_free_eofblocks
call in the blockgc code until after we've taken the IOLOCK.

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

I moved all the stuff in this function upwards into
xfs_can_free_eofblocks and it seems to work ok.

--D

>
> --D