Introduction

PATCH New NeilBrown 1 participant 9 messages Aug 22, 2005 MD RAID

lore

Share & tools

lore

NeilBrown

author

2005-08-22 01:59 UTC #0 lore raw

PATCH 1/1 Introduction

Following are 8 patches for md in 2.6.13-rc6-mm1.

The first patch could usefully go into 2.6.13. The remainder should wait for 2.6.14-rc1.

Number 001 simply calls md_wakeup_thread during raid startup to make sure that resync gets started. It is always perfectly safe to call this function -- as long as we don't call it continuously -- as an extra wakeup doesn't hurt.
The remainder fix:
bugs in code that is only in -mm as yet,
minor bugs in recently added 'bitmap' code,
fix a long standing shortcoming with 'linear' the only one person has
complained about.

Thanks,
NeilBrown

[PATCH md 001 of 8] Make sure resync gets started when array starts. [PATCH md 002 of 8] Support md/linear array with components greater than 2 terabytes. [PATCH md 003 of 8] raid1_quiesce is back to front, fix it.
[PATCH md 004 of 8] Make sure bitmap_daemon_work actually does work. [PATCH md 005 of 8] Do not set mddev->bitmap until bitmap is fully initialised [PATCH md 006 of 8] Allow hot-adding devices to arrays with non-persistant superblocks. [PATCH md 007 of 8] Allow md to load a superblock with feature-bit '1' set [PATCH md 008 of 8] Fix bitmap/read_sb_page so that it handles errors properly.

NeilBrown

author

2005-08-22 01:59 UTC #1 lore raw

PATCH 1/1 Make sure resync gets started when array starts.

We weren't actually waking up the md thread after setting MD_RECOVERY_NEEDED when assembling an array, so it is possible to lose a race and not actually start resync.

So add a call to md_wakeup_thread, and while we are at it, remove all the "if (mddev->thread)" guards as md_wake_thread does its own checking.

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

### Diffstat output

 ./drivers/md/md.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

drivers/md/md.c renamed from drivers/md/md.c~current~ +3 -4

diff ./drivers/md/md.c~current~ ./drivers/md/md.c
@@ -256,8 +256,7 @@ static inline void mddev_unlock(mddev_t 
 {
 	up(&mddev->reconfig_sem);
 
-	if (mddev->thread)
-		md_wakeup_thread(mddev->thread);
+	md_wakeup_thread(mddev->thread);
 }
 
 mdk_rdev_t * find_rdev_nr(mddev_t *mddev, int nr)

@@ -1726,6 +1725,7 @@ static int do_md_run(mddev_t * mddev)
 	mddev->in_sync = 1;
 	
 	set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
+	md_wakeup_thread(mddev->thread);
 	
 	if (mddev->sb_dirty)
 		md_update_sb(mddev);

@@ -2255,8 +2255,7 @@ static int add_new_disk(mddev_t * mddev,
 			export_rdev(rdev);
 
 		set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
-		if (mddev->thread)
-			md_wakeup_thread(mddev->thread);
+		md_wakeup_thread(mddev->thread);
 		return err;
 	}

NeilBrown

author

2005-08-22 01:59 UTC #2 lore raw

PATCH 1/1 Support md/linear array with components greater than 2 terabytes.

In reply to NeilBrown

linear currently uses division by the size of the smallest componenet device to find which device a request goes to.
If that smallest device is larger than 2 terabytes, then the division will not work on some systems.

So we introduce a pre-shift, and take care not to make the hash table too large, much like the code in raid0.

Also get rid of conf->nr_zones, which is not needed.

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

### Diffstat output

 ./drivers/md/linear.c         |   99 ++++++++++++++++++++++++++++--------------
 ./include/linux/raid/linear.h |    4 -
 2 files changed, 70 insertions(+), 33 deletions(-)

drivers/md/linear.c renamed from drivers/md/linear.c~current~ +68 ~~-31~~

diff ./drivers/md/linear.c~current~ ./drivers/md/linear.c
@@ -38,7 +38,8 @@ static inline dev_info_t *which_dev(mdde
 	/*
 	 * sector_div(a,b) returns the remainer and sets a to a/b
 	 */
-	(void)sector_div(block, conf->smallest->size);
+	block >>= conf->preshift;
+	(void)sector_div(block, conf->hash_spacing);
 	hash = conf->hash_table[block];
 
 	while ((sector>>1) >= (hash->size + hash->offset))

@@ -47,7 +48,7 @@ static inline dev_info_t *which_dev(mdde
 }
 
 /**
- *	linear_mergeable_bvec -- tell bio layer if a two requests can be merged
+ *	linear_mergeable_bvec -- tell bio layer if two requests can be merged
  *	@q: request queue
  *	@bio: the buffer head that's been built up so far
  *	@biovec: the request that could be merged to it.

@@ -116,7 +117,7 @@ static int linear_run (mddev_t *mddev)
 	dev_info_t **table;
 	mdk_rdev_t *rdev;
 	int i, nb_zone, cnt;
-	sector_t start;
+	sector_t min_spacing;
 	sector_t curr_offset;
 	struct list_head *tmp;

@@ -127,11 +128,6 @@ static int linear_run (mddev_t *mddev)
 	memset(conf, 0, sizeof(*conf) + mddev->raid_disks*sizeof(dev_info_t));
 	mddev->private = conf;
 
-	/*
-	 * Find the smallest device.
-	 */
-
-	conf->smallest = NULL;
 	cnt = 0;
 	mddev->array_size = 0;

@@ -159,8 +155,6 @@ static int linear_run (mddev_t *mddev)
 		disk->size = rdev->size;
 		mddev->array_size += rdev->size;
 
-		if (!conf->smallest || (disk->size < conf->smallest->size))
-			conf->smallest = disk;
 		cnt++;
 	}
 	if (cnt != mddev->raid_disks) {

@@ -168,6 +162,36 @@ static int linear_run (mddev_t *mddev)
 		goto out;
 	}
 
+	min_spacing = mddev->array_size;
+	sector_div(min_spacing, PAGE_SIZE/sizeof(struct dev_info *));
+
+	/* min_spacing is the minimum spacing that will fit the hash
+	 * table in one PAGE.  This may be much smaller than needed.
+	 * We find the smallest non-terminal set of consecutive devices
+	 * that is larger than min_spacing as use the size of that as
+	 * the actual spacing
+	 */
+	conf->hash_spacing = mddev->array_size;
+	for (i=0; i < cnt-1 ; i++) {
+		sector_t sz = 0;
+		int j;
+		for (j=i; i<cnt-1 && sz < min_spacing ; j++)
+			sz += conf->disks[j].size;
+		if (sz >= min_spacing && sz < conf->hash_spacing)
+			conf->hash_spacing = sz;
+	}
+
+	/* hash_spacing may be too large for sector_div to work with,
+	 * so we might need to pre-shift
+	 */
+	conf->preshift = 0;
+	if (sizeof(sector_t) > sizeof(u32)) {
+		sector_t space = conf->hash_spacing;
+		while (space > (sector_t)(~(u32)0)) {
+			space >>= 1;
+			conf->preshift++;
+		}
+	}
 	/*
 	 * This code was restructured to work around a gcc-2.95.3 internal
 	 * compiler error.  Alter it with care.

@@ -177,39 +201,52 @@ static int linear_run (mddev_t *mddev)
 		unsigned round;
 		unsigned long base;
 
-		sz = mddev->array_size;
-		base = conf->smallest->size;
+		sz = mddev->array_size >> conf->preshift;
+		sz += 1; /* force round-up */
+		base = conf->hash_spacing >> conf->preshift;
 		round = sector_div(sz, base);
-		nb_zone = conf->nr_zones = sz + (round ? 1 : 0);
+		nb_zone = sz + (round ? 1 : 0);
 	}
-			
-	conf->hash_table = kmalloc (sizeof (dev_info_t*) * nb_zone,
+	BUG_ON(nb_zone > PAGE_SIZE / sizeof(struct dev_info *));
+
+	conf->hash_table = kmalloc (sizeof (struct dev_info *) * nb_zone,
 					GFP_KERNEL);
 	if (!conf->hash_table)
 		goto out;
 
 	/*
 	 * Here we generate the linear hash table
+	 * First calculate the device offsets.
 	 */
+	conf->disks[0].offset = 0;
+	for (i=1; i<mddev->raid_disks; i++)
+		conf->disks[i].offset =
+			conf->disks[i-1].offset +
+			conf->disks[i-1].size;
+
 	table = conf->hash_table;
-	start = 0;
 	curr_offset = 0;
-	for (i = 0; i < cnt; i++) {
-		dev_info_t *disk = conf->disks + i;
-
-		disk->offset = curr_offset;
-		curr_offset += disk->size;
-
-		/* 'curr_offset' is the end of this disk
-		 * 'start' is the start of table
+	i = 0;
+	for (curr_offset = 0;
+	     curr_offset < mddev->array_size;
+	     curr_offset += conf->hash_spacing) {
+
+		while (i < mddev->raid_disks-1 &&
+		       curr_offset >= conf->disks[i+1].offset)
+			i++;
+
+		*table ++ = conf->disks + i;
+	}
+
+	if (conf->preshift) {
+		conf->hash_spacing >>= conf->preshift;
+		/* round hash_spacing up so that when we divide by it,
+		 * we err on the side of "too-low", which is safest.
 		 */
-		while (start < curr_offset) {
-			*table++ = disk;
-			start += conf->smallest->size;
-		}
+		conf->hash_spacing++;
 	}
-	if (table-conf->hash_table != nb_zone)
-		BUG();
+
+	BUG_ON(table - conf->hash_table > nb_zone);
 
 	blk_queue_merge_bvec(mddev->queue, linear_mergeable_bvec);
 	mddev->queue->unplug_fn = linear_unplug;

@@ -299,7 +336,7 @@ static void linear_status (struct seq_fi
 	sector_t s = 0;
   
 	seq_printf(seq, "      ");
-	for (j = 0; j < conf->nr_zones; j++)
+	for (j = 0; j < mddev->raid_disks; j++)
 	{
 		char b[BDEVNAME_SIZE];
 		s += conf->smallest_size;

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
@@ -14,8 +14,8 @@ typedef struct dev_info dev_info_t;
 struct linear_private_data
 {
 	dev_info_t		**hash_table;
-	dev_info_t		*smallest;
-	int			nr_zones;
+	sector_t		hash_spacing;
+	int			preshift; /* shift before dividing by hash_spacing */
 	dev_info_t		disks[0];
 };

NeilBrown

author

2005-08-22 01:59 UTC #3 lore raw

PATCH 1/1 raid1_quiesce is back to front, fix it.

In reply to NeilBrown

A state of 0 mean 'not quiesced'
A state of 1 means 'is quiesced'

The original code got this wrong.

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

### Diffstat output

 ./drivers/md/raid1.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/md/raid1.c renamed from drivers/md/raid1.c~current~ +2 -2

diff ./drivers/md/raid1.c~current~ ./drivers/md/raid1.c
@@ -1708,14 +1708,14 @@ void raid1_quiesce(mddev_t *mddev, int s
 	conf_t *conf = mddev_to_conf(mddev);
 
 	switch(state) {
-	case 0:
+	case 1:
 		spin_lock_irq(&conf->resync_lock);
 		conf->barrier++;
 		wait_event_lock_irq(conf->wait_idle, !conf->nr_pending,
 				    conf->resync_lock, raid1_unplug(mddev->queue));
 		spin_unlock_irq(&conf->resync_lock);
 		break;
-	case 1:
+	case 0:
 		spin_lock_irq(&conf->resync_lock);
 		conf->barrier--;
 		spin_unlock_irq(&conf->resync_lock);

NeilBrown

author

2005-08-22 01:59 UTC #4 lore raw

PATCH 1/1 Make sure bitmap_daemon_work actually does work.

In reply to NeilBrown

The 'lastrun' time wasn't being initialised, so it could be half a jiffie-cycle before it seemed to be time to do work again.

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

### Diffstat output

 ./drivers/md/bitmap.c |    1 +
 1 files changed, 1 insertion(+)

drivers/md/bitmap.c renamed from drivers/md/bitmap.c~current~ +1 -0

diff ./drivers/md/bitmap.c~current~ ./drivers/md/bitmap.c
@@ -522,6 +522,7 @@ success:
 	/* assign fields using values from superblock */
 	bitmap->chunksize = chunksize;
 	bitmap->daemon_sleep = daemon_sleep;
+	bitmap->daemon_lastrun = jiffies;
 	bitmap->max_write_behind = write_behind;
 	bitmap->flags |= sb->state;
 	bitmap->events_cleared = le64_to_cpu(sb->events_cleared);

NeilBrown

author

2005-08-22 01:59 UTC #5 lore raw

PATCH 1/1 Do not set mddev->bitmap until bitmap is fully initialised

In reply to NeilBrown

When hot-adding a bitmap, bitmap_daemon_work could get called while the bitmap is being created, so don't set mddev->bitmap until the bitmap is ready.
This requires freeing the bitmap inside bitmap_create if creation failed part-way through.

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

### Diffstat output

 ./drivers/md/bitmap.c |   33 ++++++++++++++++++++++++---------
 1 files changed, 24 insertions(+), 9 deletions(-)

drivers/md/bitmap.c renamed from drivers/md/bitmap.c~current~ +24 -9

diff ./drivers/md/bitmap.c~current~ ./drivers/md/bitmap.c
@@ -1503,17 +1503,14 @@ void bitmap_flush(mddev_t *mddev)
 /*
  * free memory that was allocated
  */
-void bitmap_destroy(mddev_t *mddev)
+static void bitmap_free(struct bitmap *bitmap)
 {
 	unsigned long k, pages;
 	struct bitmap_page *bp;
-	struct bitmap *bitmap = mddev->bitmap;
 
 	if (!bitmap) /* there was no bitmap */
 		return;
 
-	mddev->bitmap = NULL; /* disconnect from the md device */
-
 	/* release the bitmap file and kill the daemon */
 	bitmap_file_put(bitmap);

@@ -1531,6 +1528,17 @@ void bitmap_destroy(mddev_t *mddev)
 	kfree(bp);
 	kfree(bitmap);
 }
+void bitmap_destroy(mddev_t *mddev)
+{
+	struct bitmap *bitmap = mddev->bitmap;
+
+	if (!bitmap) /* there was no bitmap */
+		return;
+
+	mddev->bitmap = NULL; /* disconnect from the md device */
+
+	bitmap_free(bitmap);
+}
 
 /*
  * initialize the bitmap structure

@@ -1561,15 +1569,15 @@ int bitmap_create(mddev_t *mddev)
 
 	spin_lock_init(&bitmap->lock);
 	bitmap->mddev = mddev;
-	mddev->bitmap = bitmap;
 
 	spin_lock_init(&bitmap->write_lock);
 	INIT_LIST_HEAD(&bitmap->complete_pages);
 	init_waitqueue_head(&bitmap->write_wait);
 	bitmap->write_pool = mempool_create(WRITE_POOL_SIZE, write_pool_alloc,
 				write_pool_free, NULL);
+	err = -ENOMEM;
 	if (!bitmap->write_pool)
-		return -ENOMEM;
+		goto error;
 
 	bitmap->file = file;
 	bitmap->offset = mddev->bitmap_offset;

@@ -1577,7 +1585,7 @@ int bitmap_create(mddev_t *mddev)
 	/* read superblock from bitmap file (this sets bitmap->chunksize) */
 	err = bitmap_read_sb(bitmap);
 	if (err)
-		return err;
+		goto error;
 
 	bitmap->chunkshift = find_first_bit(&bitmap->chunksize,
 					sizeof(bitmap->chunksize));

@@ -1601,8 +1609,9 @@ int bitmap_create(mddev_t *mddev)
 #else
 	bitmap->bp = kmalloc(pages * sizeof(*bitmap->bp), GFP_KERNEL);
 #endif
+	err = -ENOMEM;
 	if (!bitmap->bp)
-		return -ENOMEM;
+		goto error;
 	memset(bitmap->bp, 0, pages * sizeof(*bitmap->bp));
 
 	bitmap->flags |= BITMAP_ACTIVE;

@@ -1617,16 +1626,22 @@ int bitmap_create(mddev_t *mddev)
 	err = bitmap_init_from_disk(bitmap, start);
 
 	if (err)
-		return err;
+		goto error;
 
 	printk(KERN_INFO "created bitmap (%lu pages) for device %s\n",
 		pages, bmname(bitmap));
 
+	mddev->bitmap = bitmap;
+
 	/* kick off the bitmap daemons */
 	err = bitmap_start_daemons(bitmap);
 	if (err)
 		return err;
 	return bitmap_update_sb(bitmap);
+
+ error:
+	bitmap_free(bitmap);
+	return err;
 }
 
 /* the bitmap API -- for raid personalities */

NeilBrown

author

2005-08-22 01:59 UTC #6 lore raw

PATCH 1/1 Allow hot-adding devices to arrays with non-persistant superblocks.

In reply to NeilBrown

It is possibly (and occasionally useful) to have a raid1 without persistent superblocks. The code in add_new_disk for adding a device to such an array always tries to read a superblock. This will obviously fail.
So do the appropriate test and call md_import_device with appropriate args.

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

### Diffstat output

 ./drivers/md/md.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

drivers/md/md.c renamed from drivers/md/md.c~current~ +5 -2

diff ./drivers/md/md.c~current~ ./drivers/md/md.c
@@ -2225,8 +2225,11 @@ static int add_new_disk(mddev_t * mddev,
 			       mdname(mddev));
 			return -EINVAL;
 		}
-		rdev = md_import_device(dev, mddev->major_version,
-					mddev->minor_version);
+		if (mddev->persistent)
+			rdev = md_import_device(dev, mddev->major_version,
+						mddev->minor_version);
+		else
+			rdev = md_import_device(dev, -1, -1);
 		if (IS_ERR(rdev)) {
 			printk(KERN_WARNING 
 				"md: md_import_device returned %ld\n",

NeilBrown

author

2005-08-22 01:59 UTC #7 lore raw

PATCH 1/1 Allow md to load a superblock with feature-bit '1' set

In reply to NeilBrown

As this is used to flag an internal bitmap.

Also, introduce symbolic names for feature bits.

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

### Diffstat output

 ./drivers/md/md.c           |    6 +++---
 ./include/linux/raid/md_p.h |    5 +++++
 2 files changed, 8 insertions(+), 3 deletions(-)

drivers/md/md.c renamed from drivers/md/md.c~current~ +3 -3

diff ./drivers/md/md.c~current~ ./drivers/md/md.c
@@ -875,7 +875,7 @@ static int super_1_load(mdk_rdev_t *rdev
 	    sb->major_version != cpu_to_le32(1) ||
 	    le32_to_cpu(sb->max_dev) > (4096-256)/2 ||
 	    le64_to_cpu(sb->super_offset) != (rdev->sb_offset<<1) ||
-	    sb->feature_map != 0)
+	    (le32_to_cpu(sb->feature_map) & ~MD_FEATURE_ALL) != 0)
 		return -EINVAL;
 
 	if (calc_sb_1_csum(sb) != sb->sb_csum) {

@@ -954,7 +954,7 @@ static int super_1_validate(mddev_t *mdd
 
 		mddev->max_disks =  (4096-256)/2;
 
-		if ((le32_to_cpu(sb->feature_map) & 1) &&
+		if ((le32_to_cpu(sb->feature_map) & MD_FEATURE_BITMAP_OFFSET) &&
 		    mddev->bitmap_file == NULL ) {
 			if (mddev->level != 1) {
 				printk(KERN_WARNING "md: bitmaps only supported for raid1\n");

@@ -1029,7 +1029,7 @@ static void super_1_sync(mddev_t *mddev,
 
 	if (mddev->bitmap && mddev->bitmap_file == NULL) {
 		sb->bitmap_offset = cpu_to_le32((__u32)mddev->bitmap_offset);
-		sb->feature_map = cpu_to_le32(1);
+		sb->feature_map = cpu_to_le32(MD_FEATURE_BITMAP_OFFSET);
 	}
 
 	max_dev = 0;

include/linux/raid/md_p.h renamed from include/linux/raid/md_p.h~current~ +5 -0

diff ./include/linux/raid/md_p.h~current~ ./include/linux/raid/md_p.h
@@ -238,5 +238,10 @@ struct mdp_superblock_1 {
 	__u16	dev_roles[0];	/* role in array, or 0xffff for a spare, or 0xfffe for faulty */
 };
 
+/* feature_map bits */
+#define MD_FEATURE_BITMAP_OFFSET	1
+
+#define	MD_FEATURE_ALL			1
+
 #endif

NeilBrown

author

2005-08-22 01:59 UTC #8 lore raw

PATCH 1/1 Fix bitmap/read_sb_page so that it handles errors properly.

In reply to NeilBrown

read_sb_page assumed that if sync_page_io fails, the device would be marked faultly. However it isn't. So in the face of error, read_sb_page would loop forever.

Redo the logic so that this cannot happen.

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

### Diffstat output

 ./drivers/md/bitmap.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

drivers/md/bitmap.c renamed from drivers/md/bitmap.c~current~ +10 -9

diff ./drivers/md/bitmap.c~current~ ./drivers/md/bitmap.c
@@ -270,19 +270,20 @@ static struct page *read_sb_page(mddev_t
 
 	if (!page)
 		return ERR_PTR(-ENOMEM);
-	do {
-		ITERATE_RDEV(mddev, rdev, tmp)
-			if (rdev->in_sync && !rdev->faulty)
-				goto found;
-		return ERR_PTR(-EIO);
 
-	found:
+	ITERATE_RDEV(mddev, rdev, tmp) {
+		if (! rdev->in_sync || rdev->faulty)
+			continue;
+
 		target = (rdev->sb_offset << 1) + offset + index * (PAGE_SIZE/512);
 
-	} while (!sync_page_io(rdev->bdev, target, PAGE_SIZE, page, READ));
+		if (sync_page_io(rdev->bdev, target, PAGE_SIZE, page, READ)) {
+			page->index = index;
+			return page;
+		}
+	}
+	return ERR_PTR(-EIO);
 
-	page->index = index;
-	return page;
 }
 
 static int write_sb_page(mddev_t *mddev, long offset, struct page *page, int wait)