Skip to contentSkip to conversation

Introduction

PATCH New NeilBrown 5 participants 20 messages Nov 2, 2004 – Nov 8, 2004

Subsystems (3)
Block layer MD RAID Device mapper
lore
Share & tools
lore

NeilBrown

author
#0 lore raw

PATCH 1/1 Introduction

Following are 4 patches for md/raid against 2.6.10-rc1-mm2.

1/ Fix problem with linear arrays if component devices are > 2terabytes 2/ Fix data corruption in (experimental) RAID6 personality 3/ Fix possible oops with unplug_timer firing at the wrong time. 4/ Add new md personality "faulty".
"Faulty" can be used to inject faults and so test failure modes of other raid levels and of filesystes.

NeilBrown

author
#1 lore raw

PATCH 1/1 Fix problem with md/linear for devices larger than 2 terabytes

Some size fields were "int" instead of "sector_t".

Signed-off-by Neil Brown <[email protected]>

### Diffstat output

 ./drivers/md/linear.c         |    8 +++++---
 ./include/linux/raid/linear.h |    4 ++--
 2 files changed, 7 insertions(+), 5 deletions(-)
drivers/md/linear.c renamed from drivers/md/linear.c~current~ +5 -3
diff ./drivers/md/linear.c~current~ ./drivers/md/linear.c
@@ -116,7 +116,8 @@ static int linear_run (mddev_t *mddev)
 	linear_conf_t *conf;
 	struct linear_hash *table;
 	mdk_rdev_t *rdev;
-	int size, i, nb_zone, cnt;
+	int i, nb_zone, cnt;
+	sector_t size;
 	unsigned int curr_offset;
 	struct list_head *tmp;
 

@@ -265,10 +266,11 @@ static int linear_make_request (request_
 		char b[BDEVNAME_SIZE];
 
 		printk("linear_make_request: Block %llu out of bounds on "
-			"dev %s size %ld offset %ld\n",
+			"dev %s size %llu offset %llu\n",
 			(unsigned long long)block,
 			bdevname(tmp_dev->rdev->bdev, b),
-			tmp_dev->size, tmp_dev->offset);
+			(unsigned long long)tmp_dev->size,
+		        (unsigned long long)tmp_dev->offset);
 		bio_io_error(bio, bio->bi_size);
 		return 0;
 	}
include/linux/raid/linear.h renamed from include/linux/raid/linear.h~current~ +2 -2
diff ./include/linux/raid/linear.h~current~ ./include/linux/raid/linear.h
@@ -5,8 +5,8 @@
 
 struct dev_info {
 	mdk_rdev_t	*rdev;
-	unsigned long	size;
-	unsigned long	offset;
+	sector_t	size;
+	sector_t	offset;
 };
 
 typedef struct dev_info dev_info_t;
author
#2 lore raw

PATCH 1/1 Fix raid6 problem

In reply to NeilBrown

Sometimes it didn't read all (working) drives before a parity calculation.

Signed-off-by Neil Brown <[email protected]>

### Diffstat output

 ./drivers/md/raid6main.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)
drivers/md/raid6main.c renamed from drivers/md/raid6main.c~current~ +9 -8
diff ./drivers/md/raid6main.c~current~ ./drivers/md/raid6main.c
@@ -734,7 +734,6 @@ static void compute_parity(struct stripe
 	case READ_MODIFY_WRITE:
 		BUG();		/* READ_MODIFY_WRITE N/A for RAID-6 */
 	case RECONSTRUCT_WRITE:
-	case UPDATE_PARITY:	/* Is this right? */
 		for (i= disks; i-- ;)
 			if ( i != pd_idx && i != qd_idx && sh->dev[i].towrite ) {
 				chosen = sh->dev[i].towrite;

@@ -770,7 +769,8 @@ static void compute_parity(struct stripe
 		i = d0_idx;
 		do {
 			ptrs[count++] = page_address(sh->dev[i].page);
-
+			if (count <= disks-2 && !test_bit(R5_UPTODATE, &sh->dev[i].flags))
+				printk("block %d/%d not uptodate on parity calc\n", i,count);
 			i = raid6_next_disk(i, disks);
 		} while ( i != d0_idx );
 //		break;

@@ -818,7 +818,7 @@ static void compute_block_1(struct strip
 			if (test_bit(R5_UPTODATE, &sh->dev[i].flags))
 				ptr[count++] = p;
 			else
-				PRINTK("compute_block() %d, stripe %llu, %d"
+				printk("compute_block() %d, stripe %llu, %d"
 				       " not present\n", dd_idx,
 				       (unsigned long long)sh->sector, i);
 

@@ -875,6 +875,9 @@ static void compute_block_2(struct strip
 		do {
 			ptrs[count++] = page_address(sh->dev[i].page);
 			i = raid6_next_disk(i, disks);
+			if (i != dd_idx1 && i != dd_idx2 &&
+			    !test_bit(R5_UPTODATE, &sh->dev[i].flags))
+				printk("compute_2 with missing block %d/%d\n", count, i);
 		} while ( i != d0_idx );
 
 		if ( failb == disks-2 ) {

@@ -1157,17 +1160,15 @@ static void handle_stripe(struct stripe_
 	 * parity, or to satisfy requests
 	 * or to load a block that is being partially written.
 	 */
-	if (to_read || non_overwrite || (syncing && (uptodate < disks))) {
+	if (to_read || non_overwrite || (to_write && failed) || (syncing && (uptodate < disks))) {
 		for (i=disks; i--;) {
 			dev = &sh->dev[i];
 			if (!test_bit(R5_LOCKED, &dev->flags) && !test_bit(R5_UPTODATE, &dev->flags) &&
 			    (dev->toread ||
 			     (dev->towrite && !test_bit(R5_OVERWRITE, &dev->flags)) ||
 			     syncing ||
-			     (failed >= 1 && (sh->dev[failed_num[0]].toread ||
-					 (sh->dev[failed_num[0]].towrite && !test_bit(R5_OVERWRITE, &sh->dev[failed_num[0]].flags)))) ||
-			     (failed >= 2 && (sh->dev[failed_num[1]].toread ||
-					 (sh->dev[failed_num[1]].towrite && !test_bit(R5_OVERWRITE, &sh->dev[failed_num[1]].flags))))
+			     (failed >= 1 && (sh->dev[failed_num[0]].toread || to_write)) ||
+			     (failed >= 2 && (sh->dev[failed_num[1]].toread || to_write))
 				    )
 				) {
 				/* we would like to get this block, possibly
author
#3 lore raw

PATCH 1/1 Delete unplug timer before shutting down md array.

In reply to NeilBrown

As the unplug timer can potentially fire at any time, and and it access data that is released by the md ->stop function, we need to del_timer_sync before releasing that data.

Signed-off-by Neil Brown <[email protected]>

### Diffstat output

 ./drivers/md/linear.c    |    1 +
 ./drivers/md/multipath.c |    1 +
 ./drivers/md/raid0.c     |    1 +
 ./drivers/md/raid1.c     |    1 +
 ./drivers/md/raid10.c    |    1 +
 ./drivers/md/raid5.c     |    1 +
 ./drivers/md/raid6main.c |    1 +
 7 files changed, 7 insertions(+)
drivers/md/linear.c renamed from drivers/md/linear.c~current~ +1 -0
diff ./drivers/md/linear.c~current~ ./drivers/md/linear.c
@@ -231,6 +231,7 @@ static int linear_stop (mddev_t *mddev)
 {
 	linear_conf_t *conf = mddev_to_conf(mddev);
   
+	del_timer_sync(&mddev->queue->unplug_timer); /* the unplug fn references 'conf'*/
 	kfree(conf->hash_table);
 	kfree(conf);
drivers/md/multipath.c renamed from drivers/md/multipath.c~current~ +1 -0
diff ./drivers/md/multipath.c~current~ ./drivers/md/multipath.c
@@ -566,6 +566,7 @@ static int multipath_stop (mddev_t *mdde
 
 	md_unregister_thread(mddev->thread);
 	mddev->thread = NULL;
+	del_timer_sync(&mddev->queue->unplug_timer); /* the unplug fn references 'conf'*/
 	mempool_destroy(conf->pool);
 	kfree(conf->multipaths);
 	kfree(conf);
drivers/md/raid0.c renamed from drivers/md/raid0.c~current~ +1 -0
diff ./drivers/md/raid0.c~current~ ./drivers/md/raid0.c
@@ -385,6 +385,7 @@ static int raid0_stop (mddev_t *mddev)
 {
 	raid0_conf_t *conf = mddev_to_conf(mddev);
 
+	del_timer_sync(&mddev->queue->unplug_timer); /* the unplug fn references 'conf'*/
 	kfree (conf->hash_table);
 	conf->hash_table = NULL;
 	kfree (conf->strip_zone);
drivers/md/raid1.c renamed from drivers/md/raid1.c~current~ +1 -0
diff ./drivers/md/raid1.c~current~ ./drivers/md/raid1.c
@@ -1293,6 +1293,7 @@ static int stop(mddev_t *mddev)
 
 	md_unregister_thread(mddev->thread);
 	mddev->thread = NULL;
+	del_timer_sync(&mddev->queue->unplug_timer); /* the unplug fn references 'conf'*/
 	if (conf->r1bio_pool)
 		mempool_destroy(conf->r1bio_pool);
 	if (conf->mirrors)
drivers/md/raid10.c renamed from drivers/md/raid10.c~current~ +1 -0
diff ./drivers/md/raid10.c~current~ ./drivers/md/raid10.c
@@ -1744,6 +1744,7 @@ static int stop(mddev_t *mddev)
 
 	md_unregister_thread(mddev->thread);
 	mddev->thread = NULL;
+	del_timer_sync(&mddev->queue->unplug_timer); /* the unplug fn references 'conf'*/
 	if (conf->r10bio_pool)
 		mempool_destroy(conf->r10bio_pool);
 	if (conf->mirrors)
drivers/md/raid5.c renamed from drivers/md/raid5.c~current~ +1 -0
diff ./drivers/md/raid5.c~current~ ./drivers/md/raid5.c
@@ -1707,6 +1707,7 @@ static int stop (mddev_t *mddev)
 	mddev->thread = NULL;
 	shrink_stripes(conf);
 	free_pages((unsigned long) conf->stripe_hashtbl, HASH_PAGES_ORDER);
+	del_timer_sync(&mddev->queue->unplug_timer); /* the unplug fn references 'conf'*/
 	kfree(conf);
 	mddev->private = NULL;
 	return 0;
drivers/md/raid6main.c renamed from drivers/md/raid6main.c~current~ +1 -0
diff ./drivers/md/raid6main.c~current~ ./drivers/md/raid6main.c
@@ -1878,6 +1878,7 @@ static int stop (mddev_t *mddev)
 	mddev->thread = NULL;
 	shrink_stripes(conf);
 	free_pages((unsigned long) conf->stripe_hashtbl, HASH_PAGES_ORDER);
+	del_timer_sync(&mddev->queue->unplug_timer); /* the unplug fn references 'conf'*/
 	kfree(conf);
 	mddev->private = NULL;
 	return 0;
#4 lore raw

Re: [PATCH md 3 of 4] Delete unplug timer before shutting down md array.

Quoted 4 lines · NeilBrown

As the unplug timer can potentially fire at any time, and
and it access data that is released by the md ->stop function, we need to del_timer_sync before releasing that data.

raid really shouldn't know about unplug_timer.

Is there some missing block level API?

#5 lore raw

Re: [PATCH md 3 of 4] Delete unplug timer before shutting down md array.

Quoted 9 lines · Andrew Morton
Quoted 4 lines · NeilBrown

As the unplug timer can potentially fire at any time, and
and it access data that is released by the md ->stop function, we need to del_timer_sync before releasing that data.

raid really shouldn't know about unplug_timer.

I mean, if we're actually destroying the queue then blk_cleanup_queue() should have done the right thing here. If we're not destroying the queue then an unplug timer expiry should be benign.

Bit confused as to what's happening here.

author
#6 lore raw

Re: [PATCH md 3 of 4] Delete unplug timer before shutting down md array.

Quoted 8 lines · On Monday November 1, [email protected] wrote:
Quoted 7 lines · Andrew Morton
Quoted 4 lines · NeilBrown

As the unplug timer can potentially fire at any time, and
and it access data that is released by the md ->stop function, we need to del_timer_sync before releasing that data.

Quoted 8 lines
Quoted 2 lines

raid really shouldn't know about unplug_timer.

I mean, if we're actually destroying the queue then blk_cleanup_queue() should have done the right thing here. If we're not destroying the queue then an unplug timer expiry should be benign.

Bit confused as to what's happening here.

Not surprising. It's a bit ugly.

md has the misfortune of working in a way that doesn't fit very well with some aspects of the blkdev layer.

To create an md array, you open /dev/mdX, and use some ioctls to set up the array. Thus the block device must exist internally as a block device before any details about it are available.

Similarly, to shut down an array, you open /dev/mdX and use an ioctl to shut it down. This it will still be open (though only once) after it has been shut down.

(as I understand it) I can only safely use blk_cleanup_queue on the queue after (or during) the last close. Thus I cannot use it while shutting down an array.
However I need to release the md personality as part of shutting down the array, so I have to disable the unplug function (which is handled by the personality).

I could change ->unplug_fn to point to something safe, but there is still a race with the timer going off at just the wrong time.

Maybe an API that allowed by to change ->make_request_fn and ->unplug_fn (and possibly others) and then call some interface, and be sure that no internal use of those functions would occur after given interface was called.
This interface could then just do the del_timer_sync.

NeilBrown

#7 lore raw

Re: [PATCH md 3 of 4] Delete unplug timer before shutting down md array.

(Added Jens)

Quoted 39 lines · Neil Brown
Quoted 8 lines · On Monday November 1, [email protected] wrote:
Quoted 7 lines · Andrew Morton
Quoted 4 lines · NeilBrown

As the unplug timer can potentially fire at any time, and
and it access data that is released by the md ->stop function, we need to del_timer_sync before releasing that data.

Quoted 8 lines
Quoted 2 lines

raid really shouldn't know about unplug_timer.

I mean, if we're actually destroying the queue then blk_cleanup_queue() should have done the right thing here. If we're not destroying the queue then an unplug timer expiry should be benign.

Bit confused as to what's happening here.

Not surprising. It's a bit ugly.

md has the misfortune of working in a way that doesn't fit very well with some aspects of the blkdev layer.

To create an md array, you open /dev/mdX, and use some ioctls to set up the array. Thus the block device must exist internally as a block device before any details about it are available.

Similarly, to shut down an array, you open /dev/mdX and use an ioctl to shut it down. This it will still be open (though only once) after it has been shut down.

(as I understand it) I can only safely use blk_cleanup_queue on the queue after (or during) the last close. Thus I cannot use it while shutting down an array.
However I need to release the md personality as part of shutting down the array, so I have to disable the unplug function (which is handled by the personality).

Quoted 2 lines

I could change ->unplug_fn to point to something safe, but there is still a race with the timer going off at just the wrong time.

hm, OK. So perhaps using /dev/mdX as a control channel for creating and destroying /dev/mdX was unfortunate design. Ho hum.

Quoted 5 lines

Maybe an API that allowed by to change ->make_request_fn and ->unplug_fn (and possibly others) and then call some interface, and be sure that no internal use of those functions would occur after given interface was called.
This interface could then just do the del_timer_sync.

Something like that would be nicer, I think - a centralised point with nicely apologetic commentary rather than mysterious open-coded poking at block internals ;)

#8 lore raw

Re: [PATCH md 3 of 4] Delete unplug timer before shutting down md array.

Quoted 18 lines · Andrew Morton
Quoted 18 lines

md has the misfortune of working in a way that doesn't fit very well with some aspects of the blkdev layer.

To create an md array, you open /dev/mdX, and use some ioctls to set up the array. Thus the block device must exist internally as a block device before any details about it are available.

Similarly, to shut down an array, you open /dev/mdX and use an ioctl to shut it down. This it will still be open (though only once) after it has been shut down.

(as I understand it) I can only safely use blk_cleanup_queue on the queue after (or during) the last close. Thus I cannot use it while shutting down an array.
However I need to release the md personality as part of shutting down the array, so I have to disable the unplug function (which is handled by the personality).

Quoted 5 lines
Quoted 2 lines

I could change ->unplug_fn to point to something safe, but there is still a race with the timer going off at just the wrong time.

hm, OK. So perhaps using /dev/mdX as a control channel for creating and destroying /dev/mdX was unfortunate design. Ho hum.

would it be unfeasible to create a custom (misc) character device, say /dev/mdadm and use that to control arrays instead of /dev/md0?

Regards,
L.

Signature · 6 lines
Luca Berra -- [email protected]
       Communication Media & Services S.r.l.
/"\
\ /     ASCII RIBBON CAMPAIGN
 X        AGAINST HTML MAIL
/ \
author
#9 lore raw

Re: [PATCH md 3 of 4] Delete unplug timer before shutting down md array.

Quoted 6 lines · On Tuesday November 2, [email protected] wrote:
Quoted 2 lines

hm, OK. So perhaps using /dev/mdX as a control channel for creating and destroying /dev/mdX was unfortunate design. Ho hum.

would it be unfeasible to create a custom (misc) character device, say /dev/mdadm and use that to control arrays instead of /dev/md0?

Sure it would. Though I would rather so sort of filesystem interface.

But it is unreasonable to remove the current interface without a reasonable transition period. So the current code needs to be made to work.

NeilBrown

#10 lore raw

Re: [PATCH md 3 of 4] Delete unplug timer before shutting down md array.

In reply to NeilBrown
Quoted 4 lines · NeilBrown

As the unplug timer can potentially fire at any time, and and it access data that is released by the md ->stop function, we need to del_timer_sync before releasing that data.

I don't think I saw an update to this patch so I just knocked up the below simple conversion. If anyone can think up a nice description of what blk_sync_queue() should do, it would be appreciated ;)

Signed-off-by Andrew Morton <[email protected]>

---

 25-akpm/drivers/block/ll_rw_blk.c |   12 ++++++++++--
 25-akpm/drivers/md/linear.c       |    2 +-
 25-akpm/drivers/md/multipath.c    |    2 +-
 25-akpm/drivers/md/raid0.c        |    2 +-
 25-akpm/drivers/md/raid1.c        |    2 +-
 25-akpm/drivers/md/raid10.c       |    2 +-
 25-akpm/drivers/md/raid5.c        |    2 +-
 25-akpm/drivers/md/raid6main.c    |    2 +-
 25-akpm/include/linux/blkdev.h    |    1 +
 9 files changed, 18 insertions(+), 9 deletions(-)
drivers/block/ll_rw_blk.c renamed from drivers/block/ll_rw_blk.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup +10 -2
diff -puN drivers/block/ll_rw_blk.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup drivers/block/ll_rw_blk.c
@@ -1358,10 +1358,19 @@ void blk_stop_queue(request_queue_t *q)
 	blk_remove_plug(q);
 	set_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
 }
-
 EXPORT_SYMBOL(blk_stop_queue);
 
 /**
+ * blk_sync_queue - prepare to destroy a queue
+ * @q: the queue
+ */
+void blk_sync_queue(struct request_queue *q)
+{
+	del_timer_sync(&q->unplug_timer);
+}
+EXPORT_SYMBOL(blk_sync_queue);
+
+/**
  * blk_run_queue - run a single device queue
  * @q:	The queue to run
  */

@@ -1374,7 +1383,6 @@ void blk_run_queue(struct request_queue 
 	q->request_fn(q);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 }
-
 EXPORT_SYMBOL(blk_run_queue);
 
 /**
drivers/md/linear.c renamed from drivers/md/linear.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup +1 -1
diff -puN drivers/md/linear.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup drivers/md/linear.c
@@ -231,7 +231,7 @@ static int linear_stop (mddev_t *mddev)
 {
 	linear_conf_t *conf = mddev_to_conf(mddev);
   
-	del_timer_sync(&mddev->queue->unplug_timer); /* the unplug fn references 'conf'*/
+	blk_sync_queue(&mddev->queue); /* the unplug fn references 'conf'*/
 	kfree(conf->hash_table);
 	kfree(conf);
drivers/md/multipath.c renamed from drivers/md/multipath.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup +1 -1
diff -puN drivers/md/multipath.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup drivers/md/multipath.c
@@ -566,7 +566,7 @@ static int multipath_stop (mddev_t *mdde
 
 	md_unregister_thread(mddev->thread);
 	mddev->thread = NULL;
-	del_timer_sync(&mddev->queue->unplug_timer); /* the unplug fn references 'conf'*/
+	blk_sync_queue(&mddev->queue); /* the unplug fn references 'conf'*/
 	mempool_destroy(conf->pool);
 	kfree(conf->multipaths);
 	kfree(conf);
drivers/md/raid0.c renamed from drivers/md/raid0.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup +1 -1
diff -puN drivers/md/raid0.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup drivers/md/raid0.c
@@ -385,7 +385,7 @@ static int raid0_stop (mddev_t *mddev)
 {
 	raid0_conf_t *conf = mddev_to_conf(mddev);
 
-	del_timer_sync(&mddev->queue->unplug_timer); /* the unplug fn references 'conf'*/
+	blk_sync_queue(&mddev->queue); /* the unplug fn references 'conf'*/
 	kfree (conf->hash_table);
 	conf->hash_table = NULL;
 	kfree (conf->strip_zone);
drivers/md/raid10.c renamed from drivers/md/raid10.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup +1 -1
diff -puN drivers/md/raid10.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup drivers/md/raid10.c
@@ -1744,7 +1744,7 @@ static int stop(mddev_t *mddev)
 
 	md_unregister_thread(mddev->thread);
 	mddev->thread = NULL;
-	del_timer_sync(&mddev->queue->unplug_timer); /* the unplug fn references 'conf'*/
+	blk_sync_queue(&mddev->queue); /* the unplug fn references 'conf'*/
 	if (conf->r10bio_pool)
 		mempool_destroy(conf->r10bio_pool);
 	if (conf->mirrors)
drivers/md/raid1.c renamed from drivers/md/raid1.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup +1 -1
diff -puN drivers/md/raid1.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup drivers/md/raid1.c
@@ -1293,7 +1293,7 @@ static int stop(mddev_t *mddev)
 
 	md_unregister_thread(mddev->thread);
 	mddev->thread = NULL;
-	del_timer_sync(&mddev->queue->unplug_timer); /* the unplug fn references 'conf'*/
+	blk_sync_queue(&mddev->queue); /* the unplug fn references 'conf'*/
 	if (conf->r1bio_pool)
 		mempool_destroy(conf->r1bio_pool);
 	if (conf->mirrors)
drivers/md/raid5.c renamed from drivers/md/raid5.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup +1 -1
diff -puN drivers/md/raid5.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup drivers/md/raid5.c
@@ -1707,7 +1707,7 @@ static int stop (mddev_t *mddev)
 	mddev->thread = NULL;
 	shrink_stripes(conf);
 	free_pages((unsigned long) conf->stripe_hashtbl, HASH_PAGES_ORDER);
-	del_timer_sync(&mddev->queue->unplug_timer); /* the unplug fn references 'conf'*/
+	blk_sync_queue(&mddev->queue); /* the unplug fn references 'conf'*/
 	kfree(conf);
 	mddev->private = NULL;
 	return 0;
drivers/md/raid6main.c renamed from drivers/md/raid6main.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup +1 -1
diff -puN drivers/md/raid6main.c~md-delete-unplug-timer-before-shutting-down-md-array-cleanup drivers/md/raid6main.c
@@ -1878,7 +1878,7 @@ static int stop (mddev_t *mddev)
 	mddev->thread = NULL;
 	shrink_stripes(conf);
 	free_pages((unsigned long) conf->stripe_hashtbl, HASH_PAGES_ORDER);
-	del_timer_sync(&mddev->queue->unplug_timer); /* the unplug fn references 'conf'*/
+	blk_sync_queue(&mddev->queue); /* the unplug fn references 'conf'*/
 	kfree(conf);
 	mddev->private = NULL;
 	return 0;
include/linux/blkdev.h renamed from include/linux/blkdev.h~md-delete-unplug-timer-before-shutting-down-md-array-cleanup +1 -0
diff -puN include/linux/blkdev.h~md-delete-unplug-timer-before-shutting-down-md-array-cleanup include/linux/blkdev.h
@@ -522,6 +522,7 @@ extern int blk_hw_contig_segment(request
 extern int scsi_cmd_ioctl(struct file *, struct gendisk *, unsigned int, void __user *);
 extern void blk_start_queue(request_queue_t *q);
 extern void blk_stop_queue(request_queue_t *q);
+extern void blk_sync_queue(struct request_queue *q);
 extern void __blk_stop_queue(request_queue_t *q);
 extern void blk_run_queue(request_queue_t *);
 extern void blk_queue_activity_fn(request_queue_t *, activity_fn *, void *);

_

#11 lore raw

Re: [PATCH md 3 of 4] Delete unplug timer before shutting down md array.

Quoted 9 lines · Andrew Morton
Quoted 4 lines · NeilBrown

As the unplug timer can potentially fire at any time, and and it access data that is released by the md ->stop function, we need to del_timer_sync before releasing that data.

I don't think I saw an update to this patch so I just knocked up the below simple conversion. If anyone can think up a nice description of what blk_sync_queue() should do, it would be appreciated ;)

I don't see much merrit in this patch, you have to prevent new requests from setting it off again. Basically, from my understanding, Neil needs a way to atomically kill the timer and the unplug function. Correct?

Signature · 1 line
Jens Axboe
#12 lore raw

Re: [PATCH md 3 of 4] Delete unplug timer before shutting down md array.

Quoted 7 lines · Jens Axboe
Quoted 6 lines · Andrew Morton
Quoted 4 lines · NeilBrown

As the unplug timer can potentially fire at any time, and and it access data that is released by the md ->stop function, we need to del_timer_sync before releasing that data.

Quoted 7 lines
Quoted 3 lines

I don't think I saw an update to this patch so I just knocked up the below simple conversion. If anyone can think up a nice description of what blk_sync_queue() should do, it would be appreciated ;)

I don't see much merrit in this patch, you have to prevent new requests from setting it off again. Basically, from my understanding, Neil needs a way to atomically kill the timer and the unplug function. Correct?

Actually, with the online io scheduler switching we have a way to do this already:

	blk_wait_queue_drained(q, 1);
	blk_sync_queue(q);

Then you are sure the queue is idle, switch unplug (or ->make_request_fn) and:

	blk_finish_queue_drain(q);

We need to move the ->rl waitqueue initializers to blk_queue_make_request() then.

Would this work for you, Neil?

Signature · 1 line
Jens Axboe
author
#13 lore raw

Re: [PATCH md 3 of 4] Delete unplug timer before shutting down md array.

Quoted 16 lines · On Friday November 5, [email protected] wrote:
Quoted 7 lines · Jens Axboe
Quoted 6 lines · Andrew Morton
Quoted 4 lines · NeilBrown

As the unplug timer can potentially fire at any time, and and it access data that is released by the md ->stop function, we need to del_timer_sync before releasing that data.

Quoted 7 lines
Quoted 3 lines

I don't think I saw an update to this patch so I just knocked up the below simple conversion. If anyone can think up a nice description of what blk_sync_queue() should do, it would be appreciated ;)

I don't see much merrit in this patch, you have to prevent new requests from setting it off again. Basically, from my understanding, Neil needs a way to atomically kill the timer and the unplug function. Correct?

Quoted 6 lines

Actually, with the online io scheduler switching we have a way to do this already:

	blk_wait_queue_drained(q, 1);
	blk_sync_queue(q);

I couldn't find blk_sync_queue in 2.6.10-rc1-mm2 ....

Quoted 2 lines

Would this work for you, Neil?

I need it to do:

	del_timer_sync(&q->unplug_timer);
	kblockd_flush();

(I now realise that the patch I started with wasn't enough, as kblockd_flush is needed as well).

Anything that I can that eventually does that should be OK.

NeilBrown

#14 lore raw

Re: [PATCH md 3 of 4] Delete unplug timer before shutting down md array.

Quoted 18 lines · Neil Brown
Quoted 16 lines · On Friday November 5, [email protected] wrote:
Quoted 14 lines · Jens Axboe
Quoted 9 lines · Andrew Morton
Quoted 4 lines · NeilBrown

As the unplug timer can potentially fire at any time, and and it access data that is released by the md ->stop function, we need to del_timer_sync before releasing that data.

I don't think I saw an update to this patch so I just knocked up the below simple conversion. If anyone can think up a nice description of what blk_sync_queue() should do, it would be appreciated ;)

I don't see much merrit in this patch, you have to prevent new requests from setting it off again. Basically, from my understanding, Neil needs a way to atomically kill the timer and the unplug function. Correct?

Quoted 8 lines
Quoted 6 lines

Actually, with the online io scheduler switching we have a way to do this already:

	blk_wait_queue_drained(q, 1);
	blk_sync_queue(q);

I couldn't find blk_sync_queue in 2.6.10-rc1-mm2 ....

I added it in -mm3 to replace all the open-coded del_timer_sync()s.

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10-rc1/2.6.10-rc1-mm3/broken-out/md-delete-unplug-timer-before-shutting-down-md-array-cleanup.patch

Quoted 12 lines
Quoted 2 lines

Would this work for you, Neil?

I need it to do:

	del_timer_sync(&q->unplug_timer);
	kblockd_flush();

(I now realise that the patch I started with wasn't enough, as kblockd_flush is needed as well).

Anything that I can that eventually does that should be OK.

We should be able to stick all the above inside block_sync_queue()

author
#15 lore raw

Re: [PATCH md 3 of 4] Delete unplug timer before shutting down md array.

Quoted 3 lines · On Sunday November 7, [email protected] wrote:
Quoted 1 line

I couldn't find blk_sync_queue in 2.6.10-rc1-mm2 ....

I added it in -mm3 to replace all the open-coded del_timer_sync()s.

Yeh... right... of course ... I saw that mail...

I think I missed the trust of Jens' mail.

I now see the point was that:

.... you have to prevent new requests from setting it (the timer) off again.

That isn't a problem in my case. By the time I to want to remove the timer, I know that no-one has the device open, and so no requests are going to arrive.
The blk_wait_queue_drained has no relevance for md as we don't use the same sort of queue.
So I think blk_sync_queue is all that is needed.

Quoted 2 lines

We should be able to stick all the above inside block_sync_queue()

Maybe:

###Comments for ChangeSet
Document blk_sync_queue and complete it's functionality.

blk_sync_queue needs to call kblock_flush as the unplug function is not actually called by the timer, but rather by a work_queue that the timer triggers.

Also, use blk_sync_queue to abstract identical functionality from blk_cleanup_queue.

Signed-off-by Neil Brown <[email protected]>

### Diffstat output

 ./drivers/block/ll_rw_blk.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)
drivers/block/ll_rw_blk.c renamed from drivers/block/ll_rw_blk.c~current~ +11 -3
diff ./drivers/block/ll_rw_blk.c~current~ ./drivers/block/ll_rw_blk.c
@@ -1361,12 +1361,21 @@ void blk_stop_queue(request_queue_t *q)
 EXPORT_SYMBOL(blk_stop_queue);
 
 /**
- * blk_sync_queue - prepare to destroy a queue
+ * blk_sync_queue - cancel any pending callbacks a queue
  * @q: the queue
+ *
+ * Description:
+ *     The block layer may perform asynchronous callback activity
+ *     on a queue, such as calling the unplug function after a timeout.
+ *     A block device may call blk_sync_queue to ensure that any
+ *     such activity is cancelled, thus allowing it to release resources
+ *     the the callbacks might use.
+ *
  */
 void blk_sync_queue(struct request_queue *q)
 {
 	del_timer_sync(&q->unplug_timer);
+	kblockd_flush();
 }
 EXPORT_SYMBOL(blk_sync_queue);
 

@@ -1410,8 +1419,7 @@ void blk_cleanup_queue(request_queue_t *
 	if (q->elevator)
 		elevator_exit(q->elevator);
 
-	del_timer_sync(&q->unplug_timer);
-	kblockd_flush();
+	blk_sync_queue(q);
 
 	if (rl->rq_pool)
 		mempool_destroy(rl->rq_pool);
#16 lore raw

Re: [PATCH md 3 of 4] Delete unplug timer before shutting down md array.

In reply to Andrew Morton
Quoted 26 lines · Andrew Morton
Quoted 25 lines · Neil Brown
Quoted 22 lines · On Friday November 5, [email protected] wrote:
Quoted 7 lines · Jens Axboe
Quoted 6 lines · Andrew Morton
Quoted 4 lines · NeilBrown

As the unplug timer can potentially fire at any time, and and it access data that is released by the md ->stop function, we need to del_timer_sync before releasing that data.

Quoted 7 lines
Quoted 3 lines

I don't think I saw an update to this patch so I just knocked up the below simple conversion. If anyone can think up a nice description of what blk_sync_queue() should do, it would be appreciated ;)

I don't see much merrit in this patch, you have to prevent new requests from setting it off again. Basically, from my understanding, Neil needs a way to atomically kill the timer and the unplug function. Correct?

Actually, with the online io scheduler switching we have a way to do this already:

	blk_wait_queue_drained(q, 1);
	blk_sync_queue(q);
Quoted 5 lines
Quoted 1 line

I couldn't find blk_sync_queue in 2.6.10-rc1-mm2 ....

I added it in -mm3 to replace all the open-coded del_timer_sync()s.

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10-rc1/2.6.10-rc1-mm3/broken-out/md-delete-unplug-timer-before-shutting-down-md-array-cleanup.patch

I seem to be missing Neils reply, this is the first I see of it. Strange.

Quoted 6 lines
Quoted 6 lines
Quoted 2 lines

Would this work for you, Neil?

I need it to do:

	del_timer_sync(&q->unplug_timer);
	kblockd_flush();

blk_sync_queue() should do both. I'm unsure of whether we should wrap blk_wait_queue_drained() and that into a seperate helper. There needs to be an unfreeze as well, of course. Perhaps blk_freeze_queue() and blk_unfreeze_queue()?

Signature · 1 line
Jens Axboe
author
#17 lore raw

PATCH 1/1 "Faulty" personality of md

In reply to NeilBrown

The 'faulty' personality provides a layer over any block device in which errors may be synthesised.

A variety of errors are possible including transient and persistent read and write errors, and read errors that persist until the next write.

There error mode can be changed on a live array.

Accessing this personality requires mdadm 2.8.0 or later.

Signed-off-by Neil Brown <[email protected]>

### Diffstat output

 ./drivers/md/Kconfig        |    9 +
 ./drivers/md/Makefile       |    1 
 ./drivers/md/faulty.c       |  343 ++++++++++++++++++++++++++++++++++++++++++++
 ./drivers/md/md.c           |   13 +
 ./include/linux/raid/md_k.h |    7 
 5 files changed, 371 insertions(+), 2 deletions(-)
drivers/md/Kconfig renamed from drivers/md/Kconfig~current~ +9 -0
diff ./drivers/md/Kconfig~current~ ./drivers/md/Kconfig
@@ -164,6 +164,15 @@ config MD_MULTIPATH
 
 	  If unsure, say N.
 
+config MD_FAULTY
+	tristate "Faulty test module for MD"
+	depends on BLK_DEV_MD
+	help
+	  The "faulty" module allows for a block device that occasionally returns
+	  read or write errors.  It is useful for testing.
+
+	  In unsure, say N.
+
 config BLK_DEV_DM
 	tristate "Device mapper support"
 	depends on MD
drivers/md/Makefile renamed from drivers/md/Makefile~current~ +1 -0
diff ./drivers/md/Makefile~current~ ./drivers/md/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_MD_RAID10)		+= raid10.o
 obj-$(CONFIG_MD_RAID5)		+= raid5.o xor.o
 obj-$(CONFIG_MD_RAID6)		+= raid6.o xor.o
 obj-$(CONFIG_MD_MULTIPATH)	+= multipath.o
+obj-$(CONFIG_MD_FAULTY)		+= faulty.o
 obj-$(CONFIG_BLK_DEV_MD)	+= md.o
 obj-$(CONFIG_BLK_DEV_DM)	+= dm-mod.o
 obj-$(CONFIG_DM_CRYPT)		+= dm-crypt.o
drivers/md/faulty.c renamed from drivers/md/faulty.c~current~ +343 -0

343 lines not on this page · raw message

drivers/md/md.c renamed from drivers/md/md.c~current~ +12 -1
diff ./drivers/md/md.c~current~ ./drivers/md/md.c
@@ -2397,16 +2397,27 @@ static int update_array_info(mddev_t *md
 /*	    mddev->patch_version != info->patch_version || */
 	    mddev->ctime         != info->ctime         ||
 	    mddev->level         != info->level         ||
-	    mddev->layout        != info->layout        ||
+/*	    mddev->layout        != info->layout        || */
 	    !mddev->persistent	 != info->not_persistent||
 	    mddev->chunk_size    != info->chunk_size    )
 		return -EINVAL;
 	/* Check there is only one change */
 	if (mddev->size != info->size) cnt++;
 	if (mddev->raid_disks != info->raid_disks) cnt++;
+	if (mddev->layout != info->layout) cnt++;
 	if (cnt == 0) return 0;
 	if (cnt > 1) return -EINVAL;
 
+	if (mddev->layout != info->layout) {
+		/* Change layout
+		 * we don't need to do anything at the md level, the
+		 * personality will take care of it all.
+		 */
+		if (mddev->pers->reconfig == NULL)
+			return -EINVAL;
+		else
+			return mddev->pers->reconfig(mddev, info->layout, -1);
+	}
 	if (mddev->size != info->size) {
 		mdk_rdev_t * rdev;
 		struct list_head *tmp;
include/linux/raid/md_k.h renamed from include/linux/raid/md_k.h~current~ +6 -1
diff ./include/linux/raid/md_k.h~current~ ./include/linux/raid/md_k.h
@@ -25,10 +25,12 @@
 #define MULTIPATH         7UL
 #define RAID6		  8UL
 #define	RAID10		  9UL
-#define MAX_PERSONALITY   10UL
+#define FAULTY		  10UL
+#define MAX_PERSONALITY   11UL
 
 #define	LEVEL_MULTIPATH		(-4)
 #define	LEVEL_LINEAR		(-1)
+#define	LEVEL_FAULTY		(-5)
 
 #define MaxSector (~(sector_t)0)
 #define MD_THREAD_NAME_MAX 14

@@ -36,6 +38,7 @@
 static inline int pers_to_level (int pers)
 {
 	switch (pers) {
+		case FAULTY:		return LEVEL_FAULTY;
 		case MULTIPATH:		return LEVEL_MULTIPATH;
 		case HSM:		return -3;
 		case TRANSLUCENT:	return -2;

@@ -53,6 +56,7 @@ static inline int pers_to_level (int per
 static inline int level_to_pers (int level)
 {
 	switch (level) {
+		case LEVEL_FAULTY: return FAULTY;
 		case LEVEL_MULTIPATH: return MULTIPATH;
 		case -3: return HSM;
 		case -2: return TRANSLUCENT;

@@ -290,6 +294,7 @@ struct mdk_personality_s
 	int (*sync_request)(mddev_t *mddev, sector_t sector_nr, int go_faster);
 	int (*resize) (mddev_t *mddev, sector_t sectors);
 	int (*reshape) (mddev_t *mddev, int raid_disks);
+	int (*reconfig) (mddev_t *mddev, int layout, int chunk_size);
 };
#18 lore raw

Re: [PATCH md 4 of 4] "Faulty" personality of md

Quoted 8 lines · NeilBrown

The 'faulty' personality provides a layer over any block device in which errors may be synthesised.

A variety of errors are possible including transient and persistent read and write errors, and read errors that persist until the next write.

There error mode can be changed on a live array.

Duplicates functionality already present with the device-mapper error target, or functionality present with the (not yet merged) flakey dm target.

I think we should strive for convergence, not for more duplication.

Sincerely,
Lars Marowsky-Brée <[email protected]>

Signature · 8 lines
High Availability & Clustering
SUSE Labs, Research and Development
SUSE LINUX AG - A Novell company

-
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#19 lore raw

Re: [PATCH md 4 of 4] "Faulty" personality of md

Quoted 9 lines · On Tuesday November 2, [email protected] wrote:
Quoted 7 lines · NeilBrown

The 'faulty' personality provides a layer over any block device in which errors may be synthesised.

A variety of errors are possible including transient and persistent read and write errors, and read errors that persist until the next write.

Quoted 5 lines
Quoted 1 line

There error mode can be changed on a live array.

Duplicates functionality already present with the device-mapper error target, or functionality present with the (not yet merged) flakey dm target.

so this means you won't need to merge your "flakey" dm target?? (I couldn't find the "error' target after a quick look).

Quoted 2 lines

I think we should strive for convergence, not for more duplication.

Why? (genuine question)

And if we should, why does dm support raid1 and multipath? (Actually I would be quite happy to discard the md/multipath code).

I'm somewhat ambivalent about convergence, but if you think is it a good thing and want to work towards it, I'm open to co-operation.

However, that doesn't involve saying "We've already done that" with (I think) an implication that what I had submitted therefore isn't wanted.
Rather, it involves saying "how can we unite what we have" and making some practical suggestions.

As I said, I'm open to suggestions, but it isn't something that I particularly want to push.

NeilBrown