On Wed, Jan 13, 2021 at 03:57:15PM +0100, Christoph Hellwig wrote:
> On Mon, Jan 11, 2021 at 03:23:28PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> >
> > Refactor the part of _free_eofblocks that decides if it's really going
> > to truncate post-EOF blocks into a separate helper function. The
> > upcoming deferred inode inactivation patch requires us to be able to
> > decide this prior to actual inactivation. No functionality changes.
>
> Is there any specific reason why the new xfs_has_eofblocks helper is in
> xfs_inode.c? That just makes following the logic a little harder.
I ... have no idea. This patch also isn't technically needed until we
get to the deferred inactivation patchset, but I'll reshuffle the deck
and move this function back to xfs_bmap_util.c.
> > +
> > +/*
> > + * Decide if this inode have post-EOF blocks. The caller is responsible
> > + * for knowing / caring about the PREALLOC/APPEND flags.
> > + */
> > +int
> > +xfs_has_eofblocks(
> > + struct xfs_inode *ip,
> > + bool *has)
> > +{
> > + struct xfs_bmbt_irec imap;
> > + struct xfs_mount *mp = ip->i_mount;
> > + xfs_fileoff_t end_fsb;
> > + xfs_fileoff_t last_fsb;
> > + xfs_filblks_t map_len;
> > + int nimaps;
> > + int error;
> > +
> > + *has = false;
> > + 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.
> > + map_len = last_fsb - end_fsb;
> > +
> > + nimaps = 1;
> > + xfs_ilock(ip, XFS_ILOCK_SHARED);
> > + error = xfs_bmapi_read(ip, end_fsb, map_len, &imap, &nimaps, 0);
> > + xfs_iunlock(ip, XFS_ILOCK_SHARED);
> > +
> > + if (error || nimaps == 0)
> > + return error;
> > +
> > + *has = imap.br_startblock != HOLESTARTBLOCK || ip->i_delayed_blks;
> > + return 0;
>
> I think this logic could be simplified at lot by using xfs_iext_lookup
> directly. Something like:
>
> *has = false;
>
> 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?
--D
> if (error)
> goto out_unlock;
> }
> if (xfs_iext_lookup_extent(ip, &ip->i_df, end_fsb, &icur, &imap))
> *has = true;
> out_unlock:
> xfs_iunlock(ip, XFS_ILOCK_SHARED);
> return error;
--D
