add a bi_error field to struct bio V3

add a bi_error field to struct bio V3

[Christoph Hellwig]

Bio error reporting has been a mess for a while, and the increasing
use of chained bios makes it worse.  Add a bi_error field to struct
bio to fix this.

Note that the rebase to 4.2-rc means a lot of context changes, so I've
dropped the Reviewed-by tags from V2 as it will need a re-review.

Re: [dm-devel] [PATCH] block: add a bi_error field to struct bio

[Hannes Reinecke]

On 07/20/2015 03:29 PM, Christoph Hellwig wrote:
> Currently we have two different ways to signal an I/O error on a BIO:
> 
>  (1) by clearing the BIO_UPTODATE flag
>  (2) by returning a Linux errno value to the bi_end_io callback
> 
> The first one has the drawback of only communicating a single possible
> error (-EIO), and the second one has the drawback of not beeing persistent
> when bios are queued up, and are not passed along from child to parent
> bio in the ever more popular chaining scenario.  Having both mechanisms
> available has the additional drawback of utterly confusing driver authors
> and introducing bugs where various I/O submitters only deal with one of
> them, and the others have to add boilerplate code to deal with both kinds
> of error returns.
> 
> So add a new bi_error field to store an errno value directly in struct
> bio and remove the existing mechanisms to clean all this up.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
Very good improvement.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH] block: add a bi_error field to struct bio

[NeilBrown]

On Mon, 20 Jul 2015 15:29:37 +0200 Christoph Hellwig <hch@lst.de> wrote:

> Currently we have two different ways to signal an I/O error on a BIO:
> 
>  (1) by clearing the BIO_UPTODATE flag
>  (2) by returning a Linux errno value to the bi_end_io callback
> 
> The first one has the drawback of only communicating a single possible
> error (-EIO), and the second one has the drawback of not beeing persistent
> when bios are queued up, and are not passed along from child to parent
> bio in the ever more popular chaining scenario.  Having both mechanisms
> available has the additional drawback of utterly confusing driver authors
> and introducing bugs where various I/O submitters only deal with one of
> them, and the others have to add boilerplate code to deal with both kinds
> of error returns.
> 
> So add a new bi_error field to store an errno value directly in struct
> bio and remove the existing mechanisms to clean all this up.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: NeilBrown <neilb@suse.com> (umem and md/raid).

i.e. these files.
>  drivers/block/umem.c                |  4 +--
>  drivers/md/faulty.c                 |  4 +--
>  drivers/md/linear.c                 |  2 +-
>  drivers/md/md.c                     | 18 +++++------
>  drivers/md/multipath.c              | 12 +++----
>  drivers/md/raid0.c                  |  2 +-
>  drivers/md/raid1.c                  | 53 ++++++++++++++++---------------
>  drivers/md/raid10.c                 | 55 +++++++++++++++-----------------
>  drivers/md/raid5.c                  | 52 +++++++++++++++----------------


Thanks,
NeilBrown

Re: [PATCH] block: add a bi_error field to struct bio

[Jens Axboe]

On 07/20/2015 07:29 AM, Christoph Hellwig wrote:
> Currently we have two different ways to signal an I/O error on a BIO:
>
>   (1) by clearing the BIO_UPTODATE flag
>   (2) by returning a Linux errno value to the bi_end_io callback
>
> The first one has the drawback of only communicating a single possible
> error (-EIO), and the second one has the drawback of not beeing persistent
> when bios are queued up, and are not passed along from child to parent
> bio in the ever more popular chaining scenario.  Having both mechanisms
> available has the additional drawback of utterly confusing driver authors
> and introducing bugs where various I/O submitters only deal with one of
> them, and the others have to add boilerplate code to deal with both kinds
> of error returns.
>
> So add a new bi_error field to store an errno value directly in struct
> bio and remove the existing mechanisms to clean all this up.

I think this is a good change, the only part I _really_ dislike is that 
this now bumps a struct bio from 2 cache lines to 3. Have you done any 
perf testing?

-- 
Jens Axboe

Re: [PATCH] block: add a bi_error field to struct bio

[Jens Axboe]

On 07/22/2015 12:51 PM, Jens Axboe wrote:
> On 07/20/2015 07:29 AM, Christoph Hellwig wrote:
>> Currently we have two different ways to signal an I/O error on a BIO:
>>
>>   (1) by clearing the BIO_UPTODATE flag
>>   (2) by returning a Linux errno value to the bi_end_io callback
>>
>> The first one has the drawback of only communicating a single possible
>> error (-EIO), and the second one has the drawback of not beeing
>> persistent
>> when bios are queued up, and are not passed along from child to parent
>> bio in the ever more popular chaining scenario.  Having both mechanisms
>> available has the additional drawback of utterly confusing driver authors
>> and introducing bugs where various I/O submitters only deal with one of
>> them, and the others have to add boilerplate code to deal with both kinds
>> of error returns.
>>
>> So add a new bi_error field to store an errno value directly in struct
>> bio and remove the existing mechanisms to clean all this up.
>
> I think this is a good change, the only part I _really_ dislike is that
> this now bumps a struct bio from 2 cache lines to 3. Have you done any
> perf testing?

One possible solution would be to shrink bi_flags to an unsigned int, no 
problems fitting that in. Then we could stuff bi_error in that (new) 
hole, and we would end up having the same size again.

-- 
Jens Axboe

Re: [PATCH] block: add a bi_error field to struct bio

[Christoph Hellwig]

On Wed, Jul 22, 2015 at 03:59:46PM -0600, Jens Axboe wrote:
> One possible solution would be to shrink bi_flags to an unsigned int, no 
> problems fitting that in. Then we could stuff bi_error in that (new) hole, 
> and we would end up having the same size again.

As long as we use set/test/clear_bt on bi_flags that won't work unfortunately.

Re: [PATCH] block: add a bi_error field to struct bio

[Jens Axboe]

On 07/24/2015 04:49 AM, Christoph Hellwig wrote:
> On Wed, Jul 22, 2015 at 03:59:46PM -0600, Jens Axboe wrote:
>> One possible solution would be to shrink bi_flags to an unsigned int, no
>> problems fitting that in. Then we could stuff bi_error in that (new) hole,
>> and we would end up having the same size again.
>
> As long as we use set/test/clear_bt on bi_flags that won't work unfortunately.

Right, I don't think we need to do that though. If you look at the flags 
usage, it's all over the map. Some use test/set_bit, some set it just by 
OR'ing the mask. There's no reason we can't make this work without 
relying on set/test_bit, and then shrink it to an unsigned int.

-- 
Jens Axboe

Re: [PATCH] block: add a bi_error field to struct bio

[Christoph Hellwig]

On Fri, Jul 24, 2015 at 10:36:45AM -0600, Jens Axboe wrote:
> Right, I don't think we need to do that though. If you look at the flags 
> usage, it's all over the map. Some use test/set_bit, some set it just by 
> OR'ing the mask. There's no reason we can't make this work without relying 
> on set/test_bit, and then shrink it to an unsigned int.

Yes, the current mess doesn't look kosher.  The bvec pool bits don't
really make it better.

But do we really need the cmpxchg hack? Seems like most flags aren't
exposed to concurrency at all, althugh this would need a careful audit.

Re: [PATCH] block: add a bi_error field to struct bio

[Jens Axboe]

On 07/28/2015 05:12 AM, Christoph Hellwig wrote:
> On Fri, Jul 24, 2015 at 10:36:45AM -0600, Jens Axboe wrote:
>> Right, I don't think we need to do that though. If you look at the flags
>> usage, it's all over the map. Some use test/set_bit, some set it just by
>> OR'ing the mask. There's no reason we can't make this work without relying
>> on set/test_bit, and then shrink it to an unsigned int.
>
> Yes, the current mess doesn't look kosher.  The bvec pool bits don't
> really make it better.
>
> But do we really need the cmpxchg hack? Seems like most flags aren't
> exposed to concurrency at all, althugh this would need a careful audit.

I actually don't think that we do need it at all. With the uptodate bit 
gone, we really should not have any concurrency issues on it at all. 
CHAIN and REFFED need serialization, but that is already done previous 
to this change.

-- 
Jens Axboe

[RFC] add a bi_error field

[Christoph Hellwig]

Bio error reporting has been a mess for a while, and the increasing
use of chained bios makes it worse.

This series attempts to add a proper error field to struct bio
to sort this out.  It's working fine for me, but MD and btrfs were
doing pretty nasty things with the BIO_UPTODATE flag, so I would
appreciate some detailed review there.

Re: [dm-devel] [PATCH] block: add a bi_error field to struct bio

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> The first one has the drawback of only communicating a single
Christoph> possible error (-EIO), and the second one has the drawback of
Christoph> not beeing persistent when bios are queued up, and are not
Christoph> passed along from child to parent bio in the ever more
Christoph> popular chaining scenario.

Christoph> So add a new bi_error field to store an errno value directly
Christoph> in struct bio and remove the existing mechanisms to clean all
Christoph> this up.

Having the error status separate from the bio has been a major headache.
I am entirely in favor of this patch.

It was a big chunk of changes to read through but I did not spot any
obvious problems or polarity reversals. It would be nice to get the
respective fs/md/target driver folks to check their portions, though.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [dm-devel] [PATCH] block: add a bi_error field to struct bio

[Neil Brown]

On Wed,  3 Jun 2015 15:42:39 +0200
Christoph Hellwig <hch@lst.de> wrote:

> Currently we have two different ways to signal an I/O error on a BIO:
> 
>  (1) by clearing the BIO_UPTODATE flag
>  (2) by returning a Linux errno value to the bi_end_io callback
> 
> The first one has the drawback of only communicating a single possible
> error (-EIO), and the second one has the drawback of not beeing persistent
> when bios are queued up, and are not passed along from child to parent
> bio in the ever more popular chaining scenario.  Having both mechanisms
> available has the additional drawback of utterly confusing driver authors
> and introducing bugs where various I/O submitters only deal with one of
> them, and the others have to add boilerplate code to deal with both kinds
> of error returns.
> 
> So add a new bi_error field to store an errno value directly in struct
> bio and remove the existing mechanisms to clean all this up.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I really like this clean up.
It is unfortunate that the patch is so big, but I guess it has to be.

It mostly looks good, but review is hard and testing is harder :-(

I found:

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index f80f1af..1bad16f 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
....
> @@ -1800,7 +1799,7 @@ static void end_sync_write(struct bio *bio, int error)
>  			reschedule_retry(r1_bio);
>  		else {
>  			put_buf(r1_bio);
> -			md_done_sync(mddev, s, uptodate);
> +			md_done_sync(mddev, s, !bio->bi_error);
>  		}
>  	}
>  }

This introduces a use-after-free.  put_buf(r1_bio) can  result in bio_put on
'bio'.
It is safe to move the put_buf call after the md_done_sync(), but it is
probably best to leave the 'update' variable as it.  i.e. Just change:

-	int uptodate = test_bit(BIO_UPTODATE, &bio->bi_flags);
+	int uptodate = !bio->bi_error;


I can't see any other problems with the md changes.

Reviewed-by: NeilBrown <neilb@suse.de> (md/raid parts)

Thanks,
NeilBrown

Re: [dm-devel] [PATCH] block: add a bi_error field to struct bio

[Christoph Hellwig]

On Wed, Jun 10, 2015 at 12:50:54PM +1000, Neil Brown wrote:
> This introduces a use-after-free.  put_buf(r1_bio) can  result in bio_put on
> 'bio'.
> It is safe to move the put_buf call after the md_done_sync(), but it is
> probably best to leave the 'update' variable as it.  i.e. Just change:
> 
> -	int uptodate = test_bit(BIO_UPTODATE, &bio->bi_flags);
> +	int uptodate = !bio->bi_error;
> 
> 
> I can't see any other problems with the md changes.

Thanks, I'll keep the local uptodate variable for now.