Re: [PATCH 1/3] xfs: refactor remote attr value buffer invalidation

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Jan 08, 2020 at 12:49:22AM -0800, Christoph Hellwig wrote:
> The refactor in the subject is very misleading.  You are not refactoring
> code, but fixing a bug.

Ok, I'll make that clearer.

> > -			error = xfs_trans_read_buf(mp, args->trans,
> > +			error = xfs_trans_read_buf(mp, NULL,
> >  						   mp->m_ddev_targp,
> >  						   dblkno, dblkcnt, 0, &bp,
> >  						   &xfs_attr3_rmt_buf_ops);
> 
> xfs_trans_read_buf with an always NULL tp is a strange interface.  Any
> reason not to use xfs_buf_read directly?

If the remote value checksum fails validation, xfs_trans_read_buf will
collapse EFSBADCRC to EFSCORRUPTED.  It'll also take care of releasing
the buffer.

I agree that xfs_buf_read is a more logical choice here, but it doesn't
do those things and I think we'd be better off changing xfs_buf_read
(and _buf_get) to return EFSBADCRC/EFSCORRUPTED/ENOMEM.

> > +/* Mark stale any buffers for the remote value. */
> > +void
> > +xfs_attr_rmtval_stale(
> > +	struct xfs_inode	*ip,
> > +	struct xfs_bmbt_irec	*map)
> > +{
> > +	struct xfs_mount	*mp = ip->i_mount;
> > +	struct xfs_buf		*bp;
> > +	xfs_daddr_t		dblkno;
> > +	int			dblkcnt;
> > +
> > +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> > +	if (map->br_startblock == HOLESTARTBLOCK)
> > +		return;
> > +
> > +	dblkno = XFS_FSB_TO_DADDR(mp, map->br_startblock),
> > +	dblkcnt = XFS_FSB_TO_BB(mp, map->br_blockcount);
> 
> Now this helper seems like a real refactoring in that it splits out a
> common helper.  It matches one o the call sites exactly, while the
> other has a major change, so I think it shouldn't just be one extra
> patch, but instead of two extra patche to clearly document the changes.

Ok.

> > -		/*
> > -		 * If it's a hole, these are already unmapped
> > -		 * so there's nothing to invalidate.
> > -		 */
> > -		if (map.br_startblock != HOLESTARTBLOCK) {
> 
> Isn't this something we should keep in the caller?  That way the actual
> invalide helper can assert that the map contains neither a hole or
> a delaystartblock.

Yeah, we could keep that in the caller.

> > -			bp = xfs_trans_get_buf(*trans,
> > -					dp->i_mount->m_ddev_targp,
> > -					dblkno, dblkcnt, 0);
> > -			if (!bp)
> > -				return -ENOMEM;
> > -			xfs_trans_binval(*trans, bp);
> 
> And this is a pretty big change in that we now trylock and never read
> a buffer from disk if it isn't in core.  That change looks fine to me
> from trying to understand what is going on, but it clearly needs to
> be split out and documented.

<nod>

"Find any incore buffers associated with the remote attr value and mark
them stale so they go away."

> > -			/*
> > -			 * Roll to next transaction.
> > -			 */
> > -			error = xfs_trans_roll_inode(trans, dp);
> > -			if (error)
> > -				return error;
> > -		}
> > +		xfs_attr_rmtval_stale(dp, &map);
> >  
> >  		tblkno += map.br_blockcount;
> >  		tblkcnt -= map.br_blockcount;
> >  	}
> >  
> > -	return 0;
> > +	return xfs_trans_roll_inode(trans, dp);
> 
> xfs_attr3_leaf_freextent not doesn't do anything with the trans but
> rolling it.  I think you can drop both the roll and the trans argument.

Yeah, I was 90% convinced of that too.  That'll be another prep patch.

--D