On Tue, Jun 02, 2020 at 03:30:52PM -0700, Darrick J. Wong wrote:
> On Tue, Jun 02, 2020 at 07:42:37AM +1000, Dave Chinner wrote:
> > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> > index 2364a9aa2d71a..9739d64a46443 100644
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -1131,11 +1131,9 @@ xfs_buf_inode_iodone(
> > if (ret == 1)
> > return;
> > ASSERT(ret == 2);
> > - spin_lock(&bp->b_mount->m_ail->ail_lock);
> > list_for_each_entry(lip, &bp->b_li_list, li_bio_list) {
> > - xfs_set_li_failed(lip, bp);
> > + set_bit(XFS_LI_FAILED, &lip->li_flags);
>
> Hm. So if I read this right, for inode buffers we set/clear LI_FAILED
> directly (i.e. without messing with li_buf) because for inodes we want
> to manage the pointer directly without LI_FAILED messing with it. That
> way we can attach the buffer to the item when we dirty the inode, and
> release it when iflush is finished (or aborts). Dquots retain the old
> behavior (grab the buffer only while we're checkpointing a dquot item)
> which is why the v1 series crashed in xfs/438, so we have to leave
> xfs_set/clear_li_failed alone for now. Right?
Correct. The lip->li_buf pointer is now owned by the inode log item
for inodes, it's not a field that exists purely for buffer error
handling. Any time an inode is attached to the buffer, lip->li_buf
points to the buffer, and hence we no longer need to attach the
buffer to the log item when IO fails to be able to trigger retries.
> If so,
> Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
Thanks!
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
