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