On 8/30/07, Yuri Tikhonov <yur@xxxxxxxxxxx> wrote:
>
> Hi Dan,
>
> On Monday 27 August 2007 23:12, you wrote:
> > This still looks racy... I think the complete fix is to make the
> > R5_Wantfill and dev_q->toread accesses atomic. Please test the
> > following patch (also attached) and let me know if it fixes what you are
> > seeing:
>
> Your approach doesn't help, the Bonnie++ utility hangs up during the
> ReWriting stage.
>
Looking at it again I see that what I added would not affect the
failure you are seeing. However I noticed that you are using a broken
version of the stripe-queue and cache_arbiter patches. In the current
revisions the dev_q->flags field has been moved back to dev->flags
which fixes a data corruption issue and could potentially address the
hang you are seeing. The latest revisions are:
raid5: add the stripe_queue object for tracking raid io requests (rev2)
raid5: use stripe_queues to prioritize the "most deserving" requests (rev6)
> Note that before applying your patch I rolled my fix in the
> ops_complete_biofill() function back. Do I understand it right that your
> patch should be used *instead* of my one rather than *with* it ?
>
You understood correctly. The attached patch integrates your change
to keep R5_Wantfill set while also protecting the 'more_to_read' case.
Please try it on top of the latest stripe-queue changes [1] (instead
of the other proposed patches) .
> Regards, Yuri
Thanks,
Dan
[1] git fetch -f git://lost.foo-projects.org/~dwillia2/git/iop
md-for-linus:refs/heads/md-for-linus
raid5: fix the 'more_to_read' case in ops_complete_biofill
From: Dan Williams <dan.j.williams@xxxxxxxxx>
Prevent ops_complete_biofill from running concurrently with add_queue_bio
---
drivers/md/raid5.c | 33 +++++++++++++++++++--------------
1 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 2f9022d..1c591d3 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -828,22 +828,19 @@ static void ops_complete_biofill(void *stripe_head_ref)
struct r5dev *dev = &sh->dev[i];
struct r5_queue_dev *dev_q = &sq->dev[i];
- /* check if this stripe has new incoming reads */
+ /* 1/ acknowledge completion of a biofill operation
+ * 2/ check if we need to reply to a read request.
+ * 3/ check if we need to reschedule handle_stripe
+ */
if (dev_q->toread)
more_to_read++;
- /* acknowledge completion of a biofill operation */
- /* and check if we need to reply to a read request
- */
- if (test_bit(R5_Wantfill, &dev->flags) && !dev_q->toread) {
+ if (test_bit(R5_Wantfill, &dev->flags)) {
struct bio *rbi, *rbi2;
- clear_bit(R5_Wantfill, &dev->flags);
- /* The access to dev->read is outside of the
- * spin_lock_irq(&conf->device_lock), but is protected
- * by the STRIPE_OP_BIOFILL pending bit
- */
- BUG_ON(!dev->read);
+ if (!dev_q->toread)
+ clear_bit(R5_Wantfill, &dev->flags);
+
rbi = dev->read;
dev->read = NULL;
while (rbi && rbi->bi_sector <
@@ -899,8 +896,15 @@ static void ops_run_biofill(struct stripe_head *sh)
}
atomic_inc(&sh->count);
+
+ /* spin_lock prevents ops_complete_biofill from running concurrently
+ * with add_queue_bio in the synchronous case
+ */
+ spin_lock(&sq->lock);
async_trigger_callback(ASYNC_TX_DEP_ACK | ASYNC_TX_ACK, tx,
ops_complete_biofill, sh);
+ spin_unlock(&sq->lock);
+
}
static void ops_complete_compute5(void *stripe_head_ref)
@@ -2279,7 +2283,8 @@ static int add_queue_bio(struct stripe_queue *sq, struct bio *bi, int dd_idx,
(unsigned long long)bi->bi_sector,
(unsigned long long)sq->sector);
- spin_lock(&sq->lock);
+ /* prevent asynchronous completions from running */
+ spin_lock_bh(&sq->lock);
spin_lock_irq(&conf->device_lock);
sh = sq->sh;
if (forwrite) {
@@ -2306,7 +2311,7 @@ static int add_queue_bio(struct stripe_queue *sq, struct bio *bi, int dd_idx,
*bip = bi;
bi->bi_phys_segments ++;
spin_unlock_irq(&conf->device_lock);
- spin_unlock(&sq->lock);
+ spin_unlock_bh(&sq->lock);
pr_debug("added bi b#%llu to stripe s#%llu, disk %d.\n",
(unsigned long long)bi->bi_sector,
@@ -2339,7 +2344,7 @@ static int add_queue_bio(struct stripe_queue *sq, struct bio *bi, int dd_idx,
overlap:
set_bit(R5_Overlap, &sh->dev[dd_idx].flags);
spin_unlock_irq(&conf->device_lock);
- spin_unlock(&sq->lock);
+ spin_unlock_bh(&sq->lock);
return 0;
}
