xfs
[Top] [All Lists]

Re: [PATCH] xfs: lobotomise xfs_trans_read_buf_map()

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: lobotomise xfs_trans_read_buf_map()
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 3 Dec 2014 09:45:18 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20141202165930.GA28571@xxxxxxxxxxxxx>
References: <1417473290-17544-1-git-send-email-david@xxxxxxxxxxxxx> <20141202165930.GA28571@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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

<Prev in Thread] Current Thread [Next in Thread>