xfs
[Top] [All Lists]

Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callba

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks
From: Ben Myers <bpm@xxxxxxx>
Date: Thu, 12 Jul 2012 18:04:21 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120702100034.921366796@xxxxxxxxxxxxxxxxxxxxxx>
References: <20120702100003.960640484@xxxxxxxxxxxxxxxxxxxxxx> <20120702100034.921366796@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
Hey Christoph,

On Mon, Jul 02, 2012 at 06:00:05AM -0400, Christoph Hellwig wrote:
> xfs_bdstrat_cb only adds a check for a shutdown filesystem over 
> xfs_buf_iorequest,
> but xfs_buf_iodone_callbacks just checked for a shut down filesystem a little
> earlier.  In addition the shutdown handling in xfs_bdstrat_cb is not very 
> suitable
> for this caller.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> ---
>  fs/xfs/xfs_buf.c      |   51 
> +++++++++++++++++++++-----------------------------
>  fs/xfs/xfs_buf_item.c |    2 -
>  2 files changed, 23 insertions(+), 30 deletions(-)
> 
> Index: xfs/fs/xfs/xfs_buf_item.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_buf_item.c    2012-06-26 23:20:22.326407387 +0200
> +++ xfs/fs/xfs/xfs_buf_item.c 2012-06-26 23:20:28.766407349 +0200
> @@ -954,7 +954,7 @@ xfs_buf_iodone_callbacks(
>  
>               if (!XFS_BUF_ISSTALE(bp)) {
>                       bp->b_flags |= XBF_WRITE | XBF_ASYNC | XBF_DONE;
> -                     xfs_bdstrat_cb(bp);
> +                     xfs_buf_iorequest(bp);
>               } else {
>                       xfs_buf_relse(bp);
>               }
> Index: xfs/fs/xfs/xfs_buf.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_buf.c 2012-06-26 23:20:23.686407379 +0200
> +++ xfs/fs/xfs/xfs_buf.c      2012-06-26 23:21:40.286406924 +0200
> @@ -989,27 +989,6 @@ xfs_buf_ioerror_alert(
>               (__uint64_t)XFS_BUF_ADDR(bp), func, bp->b_error, bp->b_length);
>  }
>  
> -int
> -xfs_bwrite(
> -     struct xfs_buf          *bp)
> -{
> -     int                     error;
> -
> -     ASSERT(xfs_buf_islocked(bp));
> -
> -     bp->b_flags |= XBF_WRITE;
> -     bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q);
> -
> -     xfs_bdstrat_cb(bp);
> -
> -     error = xfs_buf_iowait(bp);
> -     if (error) {
> -             xfs_force_shutdown(bp->b_target->bt_mount,
> -                                SHUTDOWN_META_IO_ERROR);
> -     }
> -     return error;
> -}
> -
>  /*
>   * Called when we want to stop a buffer from getting written or read.
>   * We attach the EIO error, muck with its flags, and call xfs_buf_ioend
> @@ -1079,14 +1058,7 @@ xfs_bioerror_relse(
>       return EIO;
>  }
>  
> -
> -/*
> - * All xfs metadata buffers except log state machine buffers
> - * get this attached as their b_bdstrat callback function.
> - * This is so that we can catch a buffer
> - * after prematurely unpinning it to forcibly shutdown the filesystem.
> - */
> -int
> +STATIC int

xfs/fs/xfs/xfs_buf.c:1062: error: static declaration of ‘xfs_bdstrat_cb’ 
follows non-static declaration
xfs/fs/xfs/xfs_buf.h:183: error: previous declaration of ‘xfs_bdstrat_cb’ was 
here

CONFIG_XFS_DEBUG was not set.

I'm happy to have caught that before pulling this in.  Most of the time I am
testing with CONFIG_XFS_DEBUG=y.  I'll fix it up.

Regards,
        Ben

>  xfs_bdstrat_cb(
>       struct xfs_buf  *bp)
>  {
> @@ -1107,6 +1079,27 @@ xfs_bdstrat_cb(
>       return 0;
>  }
>  
> +int
> +xfs_bwrite(
> +     struct xfs_buf          *bp)
> +{
> +     int                     error;
> +
> +     ASSERT(xfs_buf_islocked(bp));
> +
> +     bp->b_flags |= XBF_WRITE;
> +     bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q);
> +
> +     xfs_bdstrat_cb(bp);
> +
> +     error = xfs_buf_iowait(bp);
> +     if (error) {
> +             xfs_force_shutdown(bp->b_target->bt_mount,
> +                                SHUTDOWN_META_IO_ERROR);
> +     }
> +     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
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

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