Re: [PATCH] [WIP] Propagate error state from buffers to the objects attached

[Brian Foster <bfoster@redhat.com>]

On Thu, Jan 12, 2017 at 03:22:09PM +0100, Carlos Maiolino wrote:
> Hi folks,
> 
> this is a new version of my possible fix to the error propagation model plus the
> fix to the locked AIL items, both things are merged together since I'm using the
> fix to the locked items to test the error propagation model, I'll split them
> between different patches and also include dquot fixes when we agree that I'm on
> the right path.
> 
> This new version adds Brian's suggestion to use a new flag for the xfs_log_item
> instead of a callback. and uses the flag into xfs_inode_item_push, to resubmit
> the buffer or not.
> 
> Also, the logic about when to set the flag has been moved into
> xfs_buf_iodone_callback_error, so we only check that after the last buffer
> resubmission.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_buf_item.c   |  6 ++++++
>  fs/xfs/xfs_inode_item.c | 33 +++++++++++++++++++++++++++++++--
>  fs/xfs/xfs_trans.h      |  4 +++-
>  3 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 2975cb2..16896d5 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -1123,6 +1123,12 @@ xfs_buf_iodone_callback_error(
>  	/* still a transient error, higher layers will retry */
>  	xfs_buf_ioerror(bp, 0);
>  	xfs_buf_relse(bp);
> +
> +	/*
> +	 * Notify log item that the buffer has been failed so it can be retried
> +	 * later if needed
> +	 */
> +	lip->li_flags |= XFS_LI_FAILED;

Looks like the right idea to me. We might want to set this before we
release the buffer though. Perhaps move it a few lines up and combine
this comment with the "transient error" comment above..?

>  	return true;
>  
>  	/*
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index d90e781..308aa27 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -517,8 +517,37 @@ xfs_inode_item_push(
>  	 * the AIL.
>  	 */
>  	if (!xfs_iflock_nowait(ip)) {
> -		rval = XFS_ITEM_FLUSHING;
> -		goto out_unlock;
> +		int error;
> +		struct xfs_dinode *dip;
> +
> +		/* Buffer carrying this item has been failed, we must resubmit
> +		 * the buffer or the item will be locked forever
> +		 */

I'd suggest to reference the flush lock specifically, in particular how
it's locked once and remains so until the flushed content makes it to
disk (across I/O failures and retries if necessary).

Note that the comment above the 'if (!xfs_iflock_nowait())' above will
probably need an update as well.

> +		if (lip->li_flags & XFS_LI_FAILED) {
> +			printk("#### ITEM BUFFER FAILED PREVIOUSLY, inode: %llu\n",
> +			       ip->i_ino);
> +			error = xfs_imap_to_bp(ip->i_mount, NULL, &ip->i_imap,
> +					       &dip, &bp, XBF_TRYLOCK, 0);
> +
> +			if (error) {
> +				rval = XFS_ITEM_FLUSHING;
> +				goto out_unlock;
> +			}
> +
> +			if (!(bp->b_flags & XBF_WRITE_FAIL)) {
> +				rval = XFS_ITEM_FLUSHING;
> +				xfs_buf_relse(bp);
> +				goto out_unlock;
> +			}
> +
> +			if (!xfs_buf_delwri_queue(bp, buffer_list)) {
> +				printk("#### QUEUEING AGAIN\n");
> +				rval = XFS_ITEM_FLUSHING;
> +			}
> +

We need to clear the LI_FAILED state once we've queued it for retry. We
may also want to do the same for all other LI_FAILED log items attached
to the buffer so we don't lock the buffer a bunch of times. For
example, add a helper to walk the log items and clear the failure flag.
(That could be a separate patch in the series though).

Those things aside this looks like it's on the right track to me. Thanks
Carlos.

Brian

> +			xfs_buf_relse(bp);
> +			goto out_unlock;
> +		}
>  	}
>  
>  	ASSERT(iip->ili_fields != 0 || XFS_FORCED_SHUTDOWN(ip->i_mount));
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 61b7fbd..d62d174 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -66,10 +66,12 @@ typedef struct xfs_log_item {
>  
>  #define	XFS_LI_IN_AIL	0x1
>  #define XFS_LI_ABORTED	0x2
> +#define	XFS_LI_FAILED	0x3
>  
>  #define XFS_LI_FLAGS \
>  	{ XFS_LI_IN_AIL,	"IN_AIL" }, \
> -	{ XFS_LI_ABORTED,	"ABORTED" }
> +	{ XFS_LI_ABORTED,	"ABORTED"}, \
> +	{ XFS_LI_FAILED,	"FAILED" }
>  
>  struct xfs_item_ops {
>  	void (*iop_size)(xfs_log_item_t *, int *, int *);
> -- 
> 2.9.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html