xfs
[Top] [All Lists]

Re: [PATCH] xfs: lobotomise xfs_trans_read_buf_map()

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: lobotomise xfs_trans_read_buf_map()
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 2 Dec 2014 08:59:30 -0800
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1417473290-17544-1-git-send-email-david@xxxxxxxxxxxxx>
References: <1417473290-17544-1-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
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?

>       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.

>       }
> +
>       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.

>  #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.

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