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
