On Tue, Dec 02, 2014 at 08:59:30AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 02, 2014 at 09:34:50AM +1100, Dave Chinner wrote:
> > The only way we can find a buffer that has not had IO completed on
> > it is if it had readahead issued on it, but we never do readahead on
> > buffers that we have already joined into a transaction. Hence this
> > condition cannot occur, and buffers locked and joined into a
> > transaction should always be marked done and not under IO.
>
> Should we add an ASSERT that would trigger when someone tries
> to issue readahead on a buffer with b_transp set?
Perhaps so.
> > bp = xfs_buf_read_map(target, map, nmaps, flags, ops);
> > - if (bp == NULL) {
> > - *bpp = NULL;
> > - return (flags & XBF_TRYLOCK) ?
> > - 0 : -ENOMEM;
> > + if (!bp) {
> > + if (!(flags & XBF_TRYLOCK))
> > + return -ENOMEM;
> > + return tp ? 0 : -EAGAIN;
>
> Can you fix the inconsistent return for the trylock case in a follow on
> patch? This difference doesn't look intentional to me, and I would
> be surprised if it's correctly handled in the callers.
Ok, I'll do an audit and make this common in a follow up patch. Just
to confirm:
if (!(flags & XBF_TRYLOCK))
return -ENOMEM;
return -EAGAIN;
is what you want to see, right?
> > }
> > +
> > if (bp->b_error) {
> > error = bp->b_error;
> > + if (!XFS_FORCED_SHUTDOWN(mp))
> > + xfs_buf_ioerror_alert(bp, __func__);
> > + bp->b_flags &= ~XBF_DONE;
> > xfs_buf_stale(bp);
> > - XFS_BUF_DONE(bp);
>
> The old non-tp case did a XFS_BUF_UNDONE, which you open code here,
> while the with-tp case did a XFS_BUF_DONE. I think this change needs
> a little explanation.
Consistency. A read failed on the buffer, so the contents are
undefined. XBF_DONE implies the contents of the buffer are valid
and so setting XBF_DONE is wrong. Further, the buffer is marked
stale, again indicating that the contents are invalid and that it
should never be written. This makes the XBF_DONE value redundant.
Hence it doesn't matter whether it is transaction context or not,
XBF_DONE should not be set on a stale buffer that failed a read....
I'll add a comment explaining this.
>
> > #ifdef DEBUG
> > - if (xfs_do_error && !(tp->t_flags & XFS_TRANS_DIRTY)) {
> > + if (xfs_do_error && (!tp || !(tp->t_flags & XFS_TRANS_DIRTY))) {
> > if (xfs_error_target == target) {
> > if (((xfs_req_num++) % xfs_error_mod) == 0) {
> > - xfs_force_shutdown(tp->t_mountp,
> > - SHUTDOWN_META_IO_ERROR);
> > xfs_buf_relse(bp);
> > - xfs_debug(mp, "Returning trans error!");
> > + xfs_debug(mp, "Returning error!");
> > return -EIO;
> > }
> > }
>
> I would suggest to kill this xfs_do_error error code, it's the last
> use of the never initialized xfs_do_error and xfs_error_target
> variables.
I was in two minds w.r.t. killing that code. My initial patch did
kill it, but I didn't in this rework. I'll update the patch to kill
it.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
