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
