xfs
[Top] [All Lists]

Re: [PATCH 1/6] xfs: don't try to mark uncached buffers stale on error.

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/6] xfs: don't try to mark uncached buffers stale on error.
From: Ben Myers <bpm@xxxxxxx>
Date: Mon, 16 Dec 2013 16:44:32 -0600
Cc: Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131213130230.GA22594@xxxxxxxxxxxxx>
References: <1386826478-13846-1-git-send-email-david@xxxxxxxxxxxxx> <1386826478-13846-2-git-send-email-david@xxxxxxxxxxxxx> <20131213130230.GA22594@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Fri, Dec 13, 2013 at 05:02:30AM -0800, Christoph Hellwig wrote:
> I think the real problem here is not nessecarily marking uncached
> buffers as stale, but marking unlocked buffers as stale.  This kinda
> overlaps because we only really ever do I/O on unlocked buffers if they
> are uncached too as it would be safe otherwise.
> 
> I think this is easily fixable by never calling xfsbdstrat on unlocked
> buffers, and as an extension simply killing xfsbdstrat as it's already
> fairly useless.  The patch below replaces all calls of xfsbdstrat with
> trivial error handling for the callers that have local uncached buffers,
> and opencodes it in the one remaining other caller.  There's a lot left
> to be cleaned up in this area, but this seems like the least invasive
> patch that doesn't cause more of a mess.
> 
> ---
> From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> Subject: [PATCH] xfs: remove xfsbdstrat
> 
> The xfsbdstrat helper is a small but useless wrapper for xfs_buf_iorequest 
> that
> handles the case of a shut down filesystem.  Most of the users have private,
> uncached buffers that can just be freed in this case, but the complex error
> handling in xfs_bioerror_relse messes up the case when it's called without
> a locked buffer.
> 
> Remove xfsbdstrat and opencode the error handling in the callers.  All but
> one can simply return an error and don't need to deal with buffer state,
> and the one caller that cares about the buffer state could do with a major
> cleanup as well, but we'll defer that to later.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 5887e41..1394106 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1187,7 +1187,12 @@ xfs_zero_remaining_bytes(
>               XFS_BUF_UNWRITE(bp);
>               XFS_BUF_READ(bp);
>               XFS_BUF_SET_ADDR(bp, xfs_fsb_to_db(ip, imap.br_startblock));
> -             xfsbdstrat(mp, bp);
> +
> +             if (XFS_FORCED_SHUTDOWN(mp)) {
> +                     error = XFS_ERROR(EIO);
> +                     break;
> +             }
> +             xfs_buf_iorequest(bp);
>               error = xfs_buf_iowait(bp);
>               if (error) {
>                       xfs_buf_ioerror_alert(bp,
> @@ -1200,7 +1205,12 @@ xfs_zero_remaining_bytes(
>               XFS_BUF_UNDONE(bp);
>               XFS_BUF_UNREAD(bp);
>               XFS_BUF_WRITE(bp);
> -             xfsbdstrat(mp, bp);
> +
> +             if (XFS_FORCED_SHUTDOWN(mp)) {
> +                     error = XFS_ERROR(EIO);
> +                     break;
> +             }
> +             xfs_buf_iorequest(bp);
>               error = xfs_buf_iowait(bp);
>               if (error) {
>                       xfs_buf_ioerror_alert(bp,
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index ce01c1a..544315e 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -698,7 +698,11 @@ xfs_buf_read_uncached(
>       bp->b_flags |= XBF_READ;
>       bp->b_ops = ops;
>  
> -     xfsbdstrat(target->bt_mount, bp);
> +     if (XFS_FORCED_SHUTDOWN(target->bt_mount)) {
> +             xfs_buf_relse(bp);
> +             return NULL;
> +     }
> +     xfs_buf_iorequest(bp);
>       xfs_buf_iowait(bp);
>       return bp;
>  }
> @@ -1089,7 +1093,7 @@ xfs_bioerror(
>   * This is meant for userdata errors; metadata bufs come with
>   * iodone functions attached, so that we can track down errors.
>   */
> -STATIC int
> +int
>  xfs_bioerror_relse(
>       struct xfs_buf  *bp)
>  {
> @@ -1164,25 +1168,6 @@ xfs_bwrite(
>       return error;
>  }
>  
> -/*
> - * Wrapper around bdstrat so that we can stop data from going to disk in case
> - * we are shutting down the filesystem.  Typically user data goes thru this
> - * path; one of the exceptions is the superblock.
> - */
> -void
> -xfsbdstrat(
> -     struct xfs_mount        *mp,
> -     struct xfs_buf          *bp)
> -{
> -     if (XFS_FORCED_SHUTDOWN(mp)) {
> -             trace_xfs_bdstrat_shut(bp, _RET_IP_);
> -             xfs_bioerror_relse(bp);
> -             return;
> -     }
> -
> -     xfs_buf_iorequest(bp);
> -}
> -
>  STATIC void
>  _xfs_buf_ioend(
>       xfs_buf_t               *bp,
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index e656833..7e41b08 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -269,9 +269,6 @@ extern void xfs_buf_unlock(xfs_buf_t *);
>  
>  /* Buffer Read and Write Routines */
>  extern int xfs_bwrite(struct xfs_buf *bp);
> -
> -extern void xfsbdstrat(struct xfs_mount *, struct xfs_buf *);
> -
>  extern void xfs_buf_ioend(xfs_buf_t *,       int);
>  extern void xfs_buf_ioerror(xfs_buf_t *, int);
>  extern void xfs_buf_ioerror_alert(struct xfs_buf *, const char *func);
> @@ -282,6 +279,8 @@ extern void xfs_buf_iomove(xfs_buf_t *, size_t, size_t, 
> void *,
>  #define xfs_buf_zero(bp, off, len) \
>           xfs_buf_iomove((bp), (off), (len), NULL, XBRW_ZERO)
>  
> +extern int xfs_bioerror_relse(struct xfs_buf *);
> +
>  static inline int xfs_buf_geterror(xfs_buf_t *bp)
>  {
>       return bp ? bp->b_error : ENOMEM;
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 07ab52c..73c1493 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -193,7 +193,10 @@ xlog_bread_noalign(
>       bp->b_io_length = nbblks;
>       bp->b_error = 0;
>  
> -     xfsbdstrat(log->l_mp, bp);
> +     if (XFS_FORCED_SHUTDOWN(log->l_mp))
> +             return XFS_ERROR(EIO);
> +
> +     xfs_buf_iorequest(bp);
>       error = xfs_buf_iowait(bp);
>       if (error)
>               xfs_buf_ioerror_alert(bp, __func__);
> @@ -4408,7 +4411,11 @@ xlog_do_recover(
>       XFS_BUF_READ(bp);
>       XFS_BUF_UNASYNC(bp);
>       bp->b_ops = &xfs_sb_buf_ops;
> -     xfsbdstrat(log->l_mp, bp);
> +
> +     if (XFS_FORCED_SHUTDOWN(log->l_mp))
> +             return XFS_ERROR(EIO);
> +

I think we need a

xfs_buf_relse(bp);

before returning.  Looks good otherwise.

Reviewed-by: Ben Myers <bpm@xxxxxxx>

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