[dm-devel] dm raid1: the device-mapper 'mirror' target errors written bios erroneously

[Mike Snitzer]

On Wed, Mar 23 2016 at  4:06pm -0400,
heinzm at redhat.com <heinzm at redhat.com> wrote:

> From: Heinz Mauelshagen <heinzm at redhat.com>
> 
> 	The device-mapper 'mirror' target errors correctly written
> 	bios erroneously in hold_bio() in case the mapped device is
> 	suspended and flushs are allowed.
> 
> 	Because the caller already copes with erroring such bios in
> 	case all mirror legs got failed, any getting to hold_bio()
> 	are good, thus the patch sets bio->bi_error to 0.
> 
> 	Resolves: rhbz1307111
> ---
>  drivers/md/dm-raid1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
> index b3ccf1e..2285ab2 100644
> --- a/drivers/md/dm-raid1.c
> +++ b/drivers/md/dm-raid1.c
> @@ -492,7 +492,7 @@ static void hold_bio(struct mirror_set *ms, struct bio *bio)
>  		if (dm_noflush_suspending(ms->ti))
>  			bio->bi_error = DM_ENDIO_REQUEUE;
>  		else
> -			bio->bi_error = -EIO;
> +			bio->bi_error = 0;
>  
>  		bio_endio(bio);
>  		return;
> -- 
> 2.5.0
> 

Given the comment in do_failures() I cannot see how completing the bio
without error from hold_bio is _always_ the right thing to do:
         * If a 'noflush' suspend is in progress, we can requeue
         * the I/O's to the core.  This give userspace a chance
         * to reconfigure the mirror, at which point the core
         * will reissue the writes.  If the 'noflush' flag is
         * not set, we have no choice but to return errors.

(that comment should likely be moved to hold_bio.. but I digress)

Should the hold_bio() caller pass in the error disposition?  So
do_failures() would pass -EIO and mirror_presuspend() would pass 0?

(though completing with success in mirror_presuspend() before we _know_
the flush-based suspend has completed seems wreckless in and of
itself -- but that is a secondary concern).