Re: [PATCH 2/7] xfs: refactor the predicate part of xfs_free_eofblocks

[Christoph Hellwig <hch@infradead.org>]

On Thu, Jan 14, 2021 at 02:49:53PM -0800, Darrick J. Wong wrote:
> > > +	end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip));
> > > +	last_fsb = XFS_B_TO_FSB(mp, mp->m_super->s_maxbytes);
> > > +	if (last_fsb <= end_fsb)
> > > +		return 0;
> > 
> > Where does this strange magic come from?
> 
> It comes straight from xfs_free_eofblocks.
> 
> I /think/ the purpose of this is to avoid touching any file that's
> larger than the page cache supports (i.e. 16T on 32-bit) because inode
> inactivation causes pagecache invalidation, and trying to invalidate
> with too high a pgoff causes weird integer truncation problems.

Hmm.  At very least we need to document this, but we really should not
allow to even read in an inode successfully under this condition..

Screams for a nice little test case..

> > 	xfs_ilock(ip, XFS_ILOCK_SHARED);
> > 	if (ip->i_delayed_blks) {
> > 		*has = true;
> > 		goto out_unlock;
> > 	}
> > 	if (!(ip->i_df.if_flags & XFS_IFEXTENTS)) {
> > 		error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
> 
> Is it even worth reading in the bmap btree to clear posteof blocks if we
> haven't loaded it already?

True, we can return early in that case.  I'm also not sure what the
i_delayed_blks check is supposed to do, as delalloc extents do show
up in the iext tree just like normal extents.