Hi Christoph,
On Fri, Aug 11, 2017 at 06:09:57AM -0700, Christoph Hellwig wrote:
> > }
> > #endif
> > +
> > +static inline void
> > +xfs_clear_li_failed(
> > + struct xfs_log_item *lip)
> > +{
> > + struct xfs_buf *bp = lip->li_buf;
> > +
> > + ASSERT(lip->li_flags & XFS_LI_IN_AIL);
> > + lockdep_assert_held(&lip->li_ailp->xa_lock);
> > +
> > + if (lip->li_flags & XFS_LI_FAILED) {
> > + lip->li_flags &= ~XFS_LI_FAILED;
> > + lip->li_buf = NULL;
> > + xfs_buf_rele(bp);
>
> This means we now nest pag->pag_buf_lock and bp->b_lock inside
> ailp->xa_lock. I couldn't find any place where we take
> it in a different order, but it makes me feel a little nervous.
>
If it makes you feel less nervous, I plan to remove the xa_lock there, and use
atomic operations instead, to set/clear the lip->li_flags, as discussed in
previous versions.
> Also I think the set/clear failed helper might better be out of line
> instead of bloating the callers.
>
Sure, I have no problem in making it a not inline function, My goal in making it
inlined was to not add another call in the stack just to remove the flags, but,
maybe a stupid question, but, what would be the benefit in making it a regular
function instead of inline?
> Except for that the patch looks fine to me.
Thanks for the review.
Cheers
--
Carlos
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
