On Tue, Jan 14, 2020 at 12:40:11AM -0800, Christoph Hellwig wrote:
> > Fortunately for us, remote attribute values are written to disk with
> > xfs_bwrite(), which is to say that they are not logged. Fix the problem
> > by removing all places where we could end up creating a buffer log item
> > for a remote attribute value and leave a note explaining why.
>
> This is stil missing a comment that you are using a suitable helper
> for marking the buffer stale, and why rmeoving the HOLEBLOCK check
> is safe (which I now tink it is based on looking at the caller).
Oops, I forgot to update the changelog.
> > - 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);
> > @@ -411,7 +428,7 @@ xfs_attr_rmtval_get(
> > error = xfs_attr_rmtval_copyout(mp, bp, args->dp->i_ino,
> > &offset, &valuelen,
> > &dst);
> > - xfs_trans_brelse(args->trans, bp);
> > + xfs_buf_relse(bp);
>
> FYI, I don't think mixing xfs_trans_read_buf and xfs_buf_relse is a good
> pattern.
Yeah, you're right. I didn't want to go opencoding the !bp or
bp->b_error > 0 cases that happen in xfs_trans_buf_read to make this bug
fix an even bigger pile of patches, but maybe it's just time to clean up
xfs_buf_read() to return error values like most everywhere else.
> > @@ -48,8 +45,8 @@ xfs_attr3_leaf_freextent(
> > * Roll through the "value", invalidating the attribute value's
> > * blocks.
> > */
> > - tblkno = blkno;
> > - tblkcnt = blkcnt;
> > + tblkno = lp->valueblk;
> > + tblkcnt = lp->valuelen;
>
> Nit: these could be easily initialized on the declaration lines. Or
> even better if you keep the old calling conventions of passing the
> blockno and count by value, in which case we don't need the extra local
> variables at all.
>
> > @@ -174,9 +155,7 @@ xfs_attr3_leaf_inactive(
> > */
> > error = 0;
> > for (lp = list, i = 0; i < count; i++, lp++) {
> > - tmp = xfs_attr3_leaf_freextent(trans, dp,
> > - lp->valueblk, lp->valuelen);
> > -
> > + tmp = xfs_attr3_rmt_inactive(dp, lp);
>
> So given that we don't touch the transaction I don't think we even
> need the memory allocation to defer the marking stale of the buffer
> until after the xfs_trans_brelse. But that could be a separate
> patch, especially if the block/count calling conventions are kept as-is.
These last two I'll clean up in a followup patch that gets rid of the
pointless local variables in the first function and the pointless memory
allocation in the second function.
--D
