[lkml]   [2004]   [Jul]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
Patch in this message
SubjectRe: [RFC][PATCH] ataraid_end_request hides errors (all? 2.4 kernels)

I had already written the subject off as "nobody cares"; I'm very
glad you responded. Some more comments are below and I'm attaching
a new patch.

> The correct thing seems to pass IO error ("!uptodate") to the upper
> ->b_end_io callback only in case both components of an IO operation
> have
> failed. As long as at least one of the mirrors have successfully
> finished the IO, we should continue operation, providing reliability,

I think you are talking about the RAID-1 read case. In this case
the ataraid subdrivers just pick one disk among all the possible
mirrors and submit the request to that. ataraid_end_request() is
not involved. The fact that most ataraid subdrivers won't resubmit
the IO to a different mirror if it fails is a very small problem
compared to what I'm picking on here.

And that is the following. For RAID1 writes, the IOs have to be
submitted to all the mirrors. If the IO for _any_ of them fails,
the upper levels should be notified of the error, because the
data integrity is broken across mirrors. (With the exception of
those few subdrivers that are capable of keeping working with the
volume in "degraded" mode.) Ataraid_end_request() is widely used
for the RAID1 write case and thus carries the danger of never
notifying the upper levels that anything went wrong.

Furthermore, for RAID0 reads and writes, the ataraid subdrivers
widely employ the ataraid_split_request() function in order to
get the IO size to fit in just one strip and thus be applicable
to just one disk. The ataraid_split_request() just splits the
request in halves and may get invoked again on each part if they
still don't fit. Each pair of new IOs that are produced this way
are tied together through the use of ataraid_end_request(). Thus,
in theory there could be a whole binary tree of requests formed
from the "original request". And again, the trouble from using
ataraid_end_request() is that most of these IOs in the tree could
go wrong, but as long as the last one succeeds, a chain of
ataraid_end_request()-s will happily report a successful IO
completion to each other and eventually back to the upper levels.
The userland application may never learn about the massive data
corruption that has just occured. Again, it is my strong belief
that if _any_ "component IO" fails, the whole "original IO" should
be failed.

Therefore, the patch keeps track of cumulative IO status by using
an otherwise unused bit from the b_state field in each IO's "parent
buffer_head". Other solutions exist and I mentioned it before, but I
think this is the least intrusive and does not require extra memory.

> About error reporting, indeed the current scheme is very bad and
> doesnt
> correctly report errors. We should at least print some warning on
> ataraid_end_request() informing of IO errors.

I added a printk to the patch to follow your suggestion,
but I think the main thing is to let the upper levels know.

> Its not OK to mess with BH_PrivateStart, no, jbd/xfs use it
> for their own purposes.

That's very unfortunate, because the comments in fs.h for 2.4 kernels
and in buffer_head.h for 2.6 kernels make it look like all the bits
starting with BH_PrivateStart are fair game. Any chance of replacing
the inviting comment with a big warning, or better yet, officially
incorporating all the bits used in jbd.h into the b_state?

Best regards,

Martins Krikis

P.S. The patch is untested. I don't have a setup to do this easily.

--- linux/drivers/ide/raid/ataraid.c.orig 2004-07-13 23:45:21.000000000 -0400
+++ linux/drivers/ide/raid/ataraid.c 2004-07-26 18:52:21.000000000 -0400
@@ -34,7 +34,8 @@

#include "ataraid.h"

+#define BH_IOFailure (BH_PrivateStart + 5) /* jbd.h uses +0 to +4 */
static int ataraid_hardsect_size[256];
static int ataraid_blksize_size[256];

@@ -153,7 +154,17 @@ void ataraid_end_request(struct buffer_h
if (private==NULL)

+ if (!uptodate) {
+ set_bit(BH_IOFailure, &private->parent->b_state);
+ printk(KERN_ERR "ataraid: IO error on major %d minor %d\n",
+ MAJOR(bh->b_rdev), MINOR(bh->b_rdev));
+ }
if (atomic_dec_and_test(&private->count)) {
+ if (test_bit(BH_IOFailure, &private->parent->b_state)) {
+ uptodate = 0; /* fail the completed original I/O */
+ clear_bit(BH_IOFailure, &private->parent->b_state);
+ }
private->parent = NULL;
@@ -194,6 +205,8 @@ static void ataraid_split_request(reques

bh2->b_data += bh->b_size/2;

+ clear_bit(BH_IOFailure, &bh->b_state); /* this bit tracks success */
 \ /
  Last update: 2005-03-22 14:04    [from the cache]
©2003-2011 Jasper Spaans