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
