Re: [PATCH 2/2] xfs: Properly retry failed inode items in case of error during buffer writeback

[Carlos Maiolino <cmaiolino@redhat.com>]

> > > > +			}
> > > > +
> > > > +			if (!(bp->b_flags & XBF_WRITE_FAIL)) {
> > > > +				rval = XFS_ITEM_FLUSHING;
> > > > +				xfs_buf_relse(bp);
> > > > +				goto out_unlock;
> > > > +			}
> 
> I think I glossed over this on my first pass, but I don't think we need
> to (or should) check XBF_WRITE_FAIL here or in the error handler. It's a
> flag used to control the internal retry and that is kind of irrelevant
> to this mechanism. Unless I'm missing something.. I don't think this
> state can occur..?
> 

Yup, right, it is not required, I'll remove it for the next version,

thanks for the review

> Brian
> 
> > > > +
> > > > +			while (lip != NULL) {
> > > > +				next = lip->li_bio_list;
> > > > +
> > > > +				if (lip->li_flags & XFS_LI_FAILED)
> > > > +					lip->li_flags &= XFS_LI_FAILED;
> > > 
> > > Eric already pointed out that you probably intend to clear the flag
> > > here..?
> > > 
> > 
> > Yup, my bad.
> > 
> > > > +				lip = next;
> > > > +			}
> > > 
> > > This whole hunk might be better off in a helper function (with the
> > > comment Eric suggested as well).
> > >
> > 
> > Agreed, a helper function can be used here and in dquot code as well, so I agree
> > that a helper function can be useful, I'll try to make it a common code for both
> > dquot and inode items.
> >  
> > > Those points and the ->iop_error() thing aside, this otherwise seems Ok
> > > to me.
> > >
> > 
> >  
> > > Brian
> > > 
> > > > +
> > > > +			if (!xfs_buf_delwri_queue(bp, buffer_list))
> > > > +				rval = XFS_ITEM_FLUSHING;
> > > > +
> > > > +			xfs_buf_relse(bp);
> > > > +			goto out_unlock;
> > > > +		}
> > > > +
> > > >  		rval = XFS_ITEM_FLUSHING;
> > > >  		goto out_unlock;
> > > > +
> > > >  	}
> > > >  
> > > >  	ASSERT(iip->ili_fields != 0 || XFS_FORCED_SHUTDOWN(ip->i_mount));
> > > > @@ -622,7 +673,8 @@ static const struct xfs_item_ops xfs_inode_item_ops = {
> > > >  	.iop_unlock	= xfs_inode_item_unlock,
> > > >  	.iop_committed	= xfs_inode_item_committed,
> > > >  	.iop_push	= xfs_inode_item_push,
> > > > -	.iop_committing = xfs_inode_item_committing
> > > > +	.iop_committing = xfs_inode_item_committing,
> > > > +	.iop_error	= xfs_inode_item_error
> > > >  };
> > > >  
> > > >  
> > > > -- 
> > > > 2.9.3
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > -- 
> > Carlos
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos