Skip to contentSkip to conversation

dm-raid456 support using md/raid5.c, now with dirty-log

PATCH In review NeilBrown 2 participants 36 messages Jun 1, 2010 – Jun 21, 2010

Subsystems (2)
lore
Share & tools
lore

NeilBrown

author
#0 lore raw

cover dm-raid456 support using md/raid5.c, now with dirty-log

Nearly two months ago I posted my first serious attempt and making md/raid5.c work as a dm target. I met with cautious approval I think, but as it didn't interact with dirty-logs yet it wasn't really read for prime time.

It has taken longer than I hoped, but here is version 2, now with dirty-log integration.
I have done a modest amount of testing, watching the bits in the log getting cleared and set just as you would expect, and watching the resync complete instantly when the dirty-log shows that all regions are clean.

There is not even a hint of cluster support yet, but that shouldn't be necessary for initial submission to mainline.

A significant difference to Heinz' dm-raid45 is that I only honour the table options that lvm actually sets. The extra ones don't really fit with md/raid5 and I don't think they need to be table options. If any are needed they might work OK as messages (???).

There are a number of changes to core-dm in here including:
- support for targets to be unplugged when the device is
- support for targets to report congestion beyond the congestion of component devices
- support for the dirty-log to cover and extent different from the size of the target (For raid5 it must be the size of the components).
I have tried to fit these to the general style of dm as best as I can.

There is certainly room for more testing and review, but I would like to see this entering -next soon with a view to seeing it merged in the next merge window.

Is this reasonable? Achievable?

Comments?

These patches can all be found on the "md-dm-raid45" branch of
git://neil.brown.name/md/
or at http://neil.brown.name/git?p=md;a=shortlog;h=refs/heads/md-dm-raid45

Thanks,
NeilBrown

---

NeilBrown (24):
md: reduce dependence on sysfs.
md/raid5: factor out code for changing size of stripe cache. md/raid5: ensure we create a unique name for kmem_cache when mddev has no gendisk
md: be more careful setting MD_CHANGE_CLEAN
md: split out md_rdev_init
md: export various start/stop interfaces
md/dm: create dm-raid456 module using md/raid5
dm-raid456: add support for raising events to userspace.
raid5: Don't set read-ahead when there is no queue
dm-raid456: add congestion checking.
md/raid5: add simple plugging infrastructure.
md/plug: optionally use plugger to unplug an array during resync/recovery.
dm-raid456: support unplug
dm-raid456: add support for setting IO hints.
dm-raid456: add suspend/resume method
dm-raid456: add message handler.
md/bitmap: white space clean up and similar.
md/bitmap: reduce dependence on sysfs.
md/bitmap: clean up plugging calls.
md/bitmap: optimise scanning of empty bitmaps.
dm-dirty-log: allow log size to be different from target size. md/bitmap: prepare for storing write-intent-bitmap via dm-dirty-log. md/bitmap: separate out loading a bitmap from initialising the structures.
dm-raid456: switch to use dm_dirty_log for tracking dirty regions.

 drivers/md/Kconfig                 |    8 +
 drivers/md/Makefile                |    1 
 drivers/md/bitmap.c                |  508 +++++++++++++++++---------------
 drivers/md/bitmap.h                |    6 
 drivers/md/dm-log-userspace-base.c |   11 -
 drivers/md/dm-log.c                |   18 +
 drivers/md/dm-raid1.c              |    4 
 drivers/md/dm-raid456.c            |  576 ++++++++++++++++++++++++++++++++++++
 drivers/md/dm-table.c              |   19 +
 drivers/md/md.c                    |  234 +++++++++------
 drivers/md/md.h                    |   51 +++
 drivers/md/raid5.c                 |  169 ++++++-----
 drivers/md/raid5.h                 |    8 -
 include/linux/device-mapper.h      |   13 +
 include/linux/dm-dirty-log.h       |    3 
 15 files changed, 1235 insertions(+), 394 deletions(-)
 create mode 100644 drivers/md/dm-raid456.c
author
#1 lore raw

PATCH 1/24 md: reduce dependence on sysfs.

We will want md devices to live as dm targets where sysfs is not visible. So allow md to not connect to sysfs.

---
 drivers/md/md.c    |  101 ++++++++++++++++++++++++----------------------------
 drivers/md/md.h    |   12 ++++++
 drivers/md/raid5.c |    8 ++--
 3 files changed, 62 insertions(+), 59 deletions(-)
drivers/md/md.c +47 -54
index 46b3a04..6fbd1c7 100644
@@ -541,14 +541,16 @@ static void mddev_unlock(mddev_t * mddev)
 		mutex_lock(&mddev->open_mutex);
 		mutex_unlock(&mddev->reconfig_mutex);
 
-		if (to_remove != &md_redundancy_group)
-			sysfs_remove_group(&mddev->kobj, to_remove);
-		if (mddev->pers == NULL ||
-		    mddev->pers->sync_request == NULL) {
-			sysfs_remove_group(&mddev->kobj, &md_redundancy_group);
-			if (mddev->sysfs_action)
-				sysfs_put(mddev->sysfs_action);
-			mddev->sysfs_action = NULL;
+		if (mddev->kobj.sd) {
+			if (to_remove != &md_redundancy_group)
+				sysfs_remove_group(&mddev->kobj, to_remove);
+			if (mddev->pers == NULL ||
+			    mddev->pers->sync_request == NULL) {
+				sysfs_remove_group(&mddev->kobj, &md_redundancy_group);
+				if (mddev->sysfs_action)
+					sysfs_put(mddev->sysfs_action);
+				mddev->sysfs_action = NULL;
+			}
 		}
 		mutex_unlock(&mddev->open_mutex);
 	} else
@@ -1811,11 +1813,9 @@ static int bind_rdev_to_array(mdk_rdev_t * rdev, mddev_t * mddev)
 		goto fail;
 
 	ko = &part_to_dev(rdev->bdev->bd_part)->kobj;
-	if ((err = sysfs_create_link(&rdev->kobj, ko, "block"))) {
-		kobject_del(&rdev->kobj);
-		goto fail;
-	}
-	rdev->sysfs_state = sysfs_get_dirent(rdev->kobj.sd, NULL, "state");
+	if (sysfs_create_link(&rdev->kobj, ko, "block"))
+		/* failure here is OK */;
+	rdev->sysfs_state = sysfs_get_dirent_safe(rdev->kobj.sd, "state");
 
 	list_add_rcu(&rdev->same_set, &mddev->disks);
 	bd_claim_by_disk(rdev->bdev, rdev->bdev->bd_holder, mddev->gendisk);
@@ -2333,8 +2333,8 @@ state_store(mdk_rdev_t *rdev, const char *buf, size_t len)
 		set_bit(In_sync, &rdev->flags);
 		err = 0;
 	}
-	if (!err && rdev->sysfs_state)
-		sysfs_notify_dirent(rdev->sysfs_state);
+	if (!err)
+		sysfs_notify_dirent_safe(rdev->sysfs_state);
 	return err ? err : len;
 }
 static struct rdev_sysfs_entry rdev_state =
@@ -2429,14 +2429,10 @@ slot_store(mdk_rdev_t *rdev, const char *buf, size_t len)
 			rdev->raid_disk = -1;
 			return err;
 		} else
-			sysfs_notify_dirent(rdev->sysfs_state);
+			sysfs_notify_dirent_safe(rdev->sysfs_state);
 		sprintf(nm, "rd%d", rdev->raid_disk);
 		if (sysfs_create_link(&rdev->mddev->kobj, &rdev->kobj, nm))
-			printk(KERN_WARNING
-			       "md: cannot register "
-			       "%s for %s\n",
-			       nm, mdname(rdev->mddev));
-
+			/* failure here is OK */;
 		/* don't wakeup anyone, leave that to userspace. */
 	} else {
 		if (slot >= rdev->mddev->raid_disks)
@@ -2446,7 +2442,7 @@ slot_store(mdk_rdev_t *rdev, const char *buf, size_t len)
 		clear_bit(Faulty, &rdev->flags);
 		clear_bit(WriteMostly, &rdev->flags);
 		set_bit(In_sync, &rdev->flags);
-		sysfs_notify_dirent(rdev->sysfs_state);
+		sysfs_notify_dirent_safe(rdev->sysfs_state);
 	}
 	return len;
 }
@@ -3411,7 +3407,7 @@ array_state_store(mddev_t *mddev, const char *buf, size_t len)
 	if (err)
 		return err;
 	else {
-		sysfs_notify_dirent(mddev->sysfs_state);
+		sysfs_notify_dirent_safe(mddev->sysfs_state);
 		return len;
 	}
 }
@@ -3709,7 +3705,7 @@ action_store(mddev_t *mddev, const char *page, size_t len)
 	}
 	set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
 	md_wakeup_thread(mddev->thread);
-	sysfs_notify_dirent(mddev->sysfs_action);
+	sysfs_notify_dirent_safe(mddev->sysfs_action);
 	return len;
 }
 
@@ -4255,13 +4251,14 @@ static int md_alloc(dev_t dev, char *name)
 		       disk->disk_name);
 		error = 0;
 	}
-	if (sysfs_create_group(&mddev->kobj, &md_bitmap_group))
+	if (mddev->kobj.sd &&
+	    sysfs_create_group(&mddev->kobj, &md_bitmap_group))
 		printk(KERN_DEBUG "pointless warning\n");
  abort:
 	mutex_unlock(&disks_mutex);
-	if (!error) {
+	if (!error && mddev->kobj.sd) {
 		kobject_uevent(&mddev->kobj, KOBJ_ADD);
-		mddev->sysfs_state = sysfs_get_dirent(mddev->kobj.sd, NULL, "array_state");
+		mddev->sysfs_state = sysfs_get_dirent_safe(mddev->kobj.sd, "array_state");
 	}
 	mddev_put(mddev);
 	return error;
@@ -4299,7 +4296,7 @@ static void md_safemode_timeout(unsigned long data)
 	if (!atomic_read(&mddev->writes_pending)) {
 		mddev->safemode = 1;
 		if (mddev->external)
-			sysfs_notify_dirent(mddev->sysfs_state);
+			sysfs_notify_dirent_safe(mddev->sysfs_state);
 	}
 	md_wakeup_thread(mddev->thread);
 }
@@ -4371,7 +4368,7 @@ static int md_run(mddev_t *mddev)
 				return -EINVAL;
 			}
 		}
-		sysfs_notify_dirent(rdev->sysfs_state);
+		sysfs_notify_dirent_safe(rdev->sysfs_state);
 	}
 
 	spin_lock(&pers_lock);
@@ -4470,11 +4467,12 @@ static int md_run(mddev_t *mddev)
 		return err;
 	}
 	if (mddev->pers->sync_request) {
-		if (sysfs_create_group(&mddev->kobj, &md_redundancy_group))
+		if (mddev->kobj.sd &&
+		    sysfs_create_group(&mddev->kobj, &md_redundancy_group))
 			printk(KERN_WARNING
 			       "md: cannot register extra attributes for %s\n",
 			       mdname(mddev));
-		mddev->sysfs_action = sysfs_get_dirent(mddev->kobj.sd, NULL, "sync_action");
+		mddev->sysfs_action = sysfs_get_dirent_safe(mddev->kobj.sd, "sync_action");
 	} else if (mddev->ro == 2) /* auto-readonly not meaningful */
 		mddev->ro = 0;
 
@@ -4492,8 +4490,7 @@ static int md_run(mddev_t *mddev)
 			char nm[20];
 			sprintf(nm, "rd%d", rdev->raid_disk);
 			if (sysfs_create_link(&mddev->kobj, &rdev->kobj, nm))
-				printk("md: cannot register %s for %s\n",
-				       nm, mdname(mddev));
+				/* failure here is OK */;
 		}
 	
 	set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
@@ -4505,9 +4502,8 @@ static int md_run(mddev_t *mddev)
 	md_wakeup_thread(mddev->sync_thread); /* possibly kick off a reshape */
 
 	md_new_event(mddev);
-	sysfs_notify_dirent(mddev->sysfs_state);
-	if (mddev->sysfs_action)
-		sysfs_notify_dirent(mddev->sysfs_action);
+	sysfs_notify_dirent_safe(mddev->sysfs_state);
+	sysfs_notify_dirent_safe(mddev->sysfs_action);
 	sysfs_notify(&mddev->kobj, NULL, "degraded");
 	return 0;
 }
@@ -4547,7 +4543,7 @@ static int restart_array(mddev_t *mddev)
 	set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
 	md_wakeup_thread(mddev->thread);
 	md_wakeup_thread(mddev->sync_thread);
-	sysfs_notify_dirent(mddev->sysfs_state);
+	sysfs_notify_dirent_safe(mddev->sysfs_state);
 	return 0;
 }
 
@@ -4671,7 +4667,7 @@ static int md_set_readonly(mddev_t *mddev, int is_open)
 		mddev->ro = 1;
 		set_disk_ro(mddev->gendisk, 1);
 		clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
-		sysfs_notify_dirent(mddev->sysfs_state);
+		sysfs_notify_dirent_safe(mddev->sysfs_state);
 		err = 0;	
 	}
 out:
@@ -4704,7 +4700,7 @@ static int do_md_stop(mddev_t * mddev, int mode, int is_open)
 		mddev->queue->backing_dev_info.congested_fn = NULL;
 
 		/* tell userspace to handle 'inactive' */
-		sysfs_notify_dirent(mddev->sysfs_state);
+		sysfs_notify_dirent_safe(mddev->sysfs_state);
 
 		list_for_each_entry(rdev, &mddev->disks, same_set)
 			if (rdev->raid_disk >= 0) {
@@ -4750,7 +4746,7 @@ static int do_md_stop(mddev_t * mddev, int mode, int is_open)
 	err = 0;
 	blk_integrity_unregister(disk);
 	md_new_event(mddev);
-	sysfs_notify_dirent(mddev->sysfs_state);
+	sysfs_notify_dirent_safe(mddev->sysfs_state);
 	return err;
 }
 
@@ -5112,7 +5108,7 @@ static int add_new_disk(mddev_t * mddev, mdu_disk_info_t *info)
 		if (err)
 			export_rdev(rdev);
 		else
-			sysfs_notify_dirent(rdev->sysfs_state);
+			sysfs_notify_dirent_safe(rdev->sysfs_state);
 
 		md_update_sb(mddev, 1);
 		if (mddev->degraded)
@@ -5787,7 +5783,7 @@ static int md_ioctl(struct block_device *bdev, fmode_t mode,
 	if (_IOC_TYPE(cmd) == MD_MAJOR && mddev->ro && mddev->pers) {
 		if (mddev->ro == 2) {
 			mddev->ro = 0;
-			sysfs_notify_dirent(mddev->sysfs_state);
+			sysfs_notify_dirent_safe(mddev->sysfs_state);
 			set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
 			md_wakeup_thread(mddev->thread);
 		} else {
@@ -6032,7 +6028,7 @@ void md_error(mddev_t *mddev, mdk_rdev_t *rdev)
 	mddev->pers->error_handler(mddev,rdev);
 	if (mddev->degraded)
 		set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
-	sysfs_notify_dirent(rdev->sysfs_state);
+	sysfs_notify_dirent_safe(rdev->sysfs_state);
 	set_bit(MD_RECOVERY_INTR, &mddev->recovery);
 	set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
 	md_wakeup_thread(mddev->thread);
@@ -6493,7 +6489,7 @@ void md_write_start(mddev_t *mddev, struct bio *bi)
 		spin_unlock_irq(&mddev->write_lock);
 	}
 	if (did_change)
-		sysfs_notify_dirent(mddev->sysfs_state);
+		sysfs_notify_dirent_safe(mddev->sysfs_state);
 	wait_event(mddev->sb_wait,
 		   !test_bit(MD_CHANGE_CLEAN, &mddev->flags) &&
 		   !test_bit(MD_CHANGE_PENDING, &mddev->flags));
@@ -6536,7 +6532,7 @@ int md_allow_write(mddev_t *mddev)
 			mddev->safemode = 1;
 		spin_unlock_irq(&mddev->write_lock);
 		md_update_sb(mddev, 0);
-		sysfs_notify_dirent(mddev->sysfs_state);
+		sysfs_notify_dirent_safe(mddev->sysfs_state);
 	} else
 		spin_unlock_irq(&mddev->write_lock);
 
@@ -6922,10 +6918,7 @@ static int remove_and_add_spares(mddev_t *mddev)
 					sprintf(nm, "rd%d", rdev->raid_disk);
 					if (sysfs_create_link(&mddev->kobj,
 							      &rdev->kobj, nm))
-						printk(KERN_WARNING
-						       "md: cannot register "
-						       "%s for %s\n",
-						       nm, mdname(mddev));
+						/* failure here is OK */;
 					spares++;
 					md_new_event(mddev);
 					set_bit(MD_CHANGE_DEVS, &mddev->flags);
@@ -7018,7 +7011,7 @@ void md_check_recovery(mddev_t *mddev)
 				mddev->safemode = 0;
 			spin_unlock_irq(&mddev->write_lock);
 			if (did_change)
-				sysfs_notify_dirent(mddev->sysfs_state);
+				sysfs_notify_dirent_safe(mddev->sysfs_state);
 		}
 
 		if (mddev->flags)
@@ -7057,7 +7050,7 @@ void md_check_recovery(mddev_t *mddev)
 			mddev->recovery = 0;
 			/* flag recovery needed just to double check */
 			set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
-			sysfs_notify_dirent(mddev->sysfs_action);
+			sysfs_notify_dirent_safe(mddev->sysfs_action);
 			md_new_event(mddev);
 			goto unlock;
 		}
@@ -7119,7 +7112,7 @@ void md_check_recovery(mddev_t *mddev)
 				mddev->recovery = 0;
 			} else
 				md_wakeup_thread(mddev->sync_thread);
-			sysfs_notify_dirent(mddev->sysfs_action);
+			sysfs_notify_dirent_safe(mddev->sysfs_action);
 			md_new_event(mddev);
 		}
 	unlock:
@@ -7128,7 +7121,7 @@ void md_check_recovery(mddev_t *mddev)
 			if (test_and_clear_bit(MD_RECOVERY_RECOVER,
 					       &mddev->recovery))
 				if (mddev->sysfs_action)
-					sysfs_notify_dirent(mddev->sysfs_action);
+					sysfs_notify_dirent_safe(mddev->sysfs_action);
 		}
 		mddev_unlock(mddev);
 	}
@@ -7136,7 +7129,7 @@ void md_check_recovery(mddev_t *mddev)
 
 void md_wait_for_blocked_rdev(mdk_rdev_t *rdev, mddev_t *mddev)
 {
-	sysfs_notify_dirent(rdev->sysfs_state);
+	sysfs_notify_dirent_safe(rdev->sysfs_state);
 	wait_event_timeout(rdev->blocked_wait,
 			   !test_bit(Blocked, &rdev->flags),
 			   msecs_to_jiffies(5000));
drivers/md/md.h +12 -0
index 7ab5ea1..1f680e4 100644
@@ -379,6 +379,18 @@ struct md_sysfs_entry {
 };
 extern struct attribute_group md_bitmap_group;
 
+static inline struct sysfs_dirent *sysfs_get_dirent_safe(struct sysfs_dirent *sd, char *name)
+{
+	if (sd)
+		return sysfs_get_dirent(sd, NULL, name);
+	return sd;
+}
+static inline void sysfs_notify_dirent_safe(struct sysfs_dirent *sd)
+{
+	if (sd)
+		sysfs_notify_dirent(sd);
+}
+
 static inline char * mdname (mddev_t * mddev)
 {
 	return mddev->gendisk ? mddev->gendisk->disk_name : "mdX";
drivers/md/raid5.c +3 -5
index d2c0f94..8882da3 100644
@@ -5070,7 +5070,8 @@ static int run(mddev_t *mddev)
 	/* Ok, everything is just fine now */
 	if (mddev->to_remove == &raid5_attrs_group)
 		mddev->to_remove = NULL;
-	else if (sysfs_create_group(&mddev->kobj, &raid5_attrs_group))
+	else if (mddev->kobj.sd &&
+	    sysfs_create_group(&mddev->kobj, &raid5_attrs_group))
 		printk(KERN_WARNING
 		       "md/raid:%s: failed to create sysfs attributes.\n",
 		       mdname(mddev));
@@ -5451,10 +5452,7 @@ static int raid5_start_reshape(mddev_t *mddev)
 				sprintf(nm, "rd%d", rdev->raid_disk);
 				if (sysfs_create_link(&mddev->kobj,
 						      &rdev->kobj, nm))
-					printk(KERN_WARNING
-					       "md/raid:%s: failed to create "
-					       " link %s\n",
-					       mdname(mddev), nm);
+					/* Failure here is OK */;
 			} else
 				break;
 		}
author
#2 lore raw

PATCH 2/24 md/raid5: factor out code for changing size of stripe cache.

In reply to NeilBrown

Separate the actual 'change' code from the sysfs interface so that it can eventually be called internally.

---
 drivers/md/raid5.c |   38 +++++++++++++++++++++++++-------------
 1 files changed, 25 insertions(+), 13 deletions(-)
drivers/md/raid5.c +25 -13
index 8882da3..8647705 100644
@@ -4480,23 +4480,15 @@ raid5_show_stripe_cache_size(mddev_t *mddev, char *page)
 		return 0;
 }
 
-static ssize_t
-raid5_store_stripe_cache_size(mddev_t *mddev, const char *page, size_t len)
+static int
+raid5_set_cache_size(mddev_t *mddev, int size)
 {
 	raid5_conf_t *conf = mddev->private;
-	unsigned long new;
 	int err;
 
-	if (len >= PAGE_SIZE)
+	if (size <= 16 || size > 32768)
 		return -EINVAL;
-	if (!conf)
-		return -ENODEV;
-
-	if (strict_strtoul(page, 10, &new))
-		return -EINVAL;
-	if (new <= 16 || new > 32768)
-		return -EINVAL;
-	while (new < conf->max_nr_stripes) {
+	while (size < conf->max_nr_stripes) {
 		if (drop_one_stripe(conf))
 			conf->max_nr_stripes--;
 		else
@@ -4505,11 +4497,31 @@ raid5_store_stripe_cache_size(mddev_t *mddev, const char *page, size_t len)
 	err = md_allow_write(mddev);
 	if (err)
 		return err;
-	while (new > conf->max_nr_stripes) {
+	while (size > conf->max_nr_stripes) {
 		if (grow_one_stripe(conf))
 			conf->max_nr_stripes++;
 		else break;
 	}
+	return 0;
+}
+
+static ssize_t
+raid5_store_stripe_cache_size(mddev_t *mddev, const char *page, size_t len)
+{
+	raid5_conf_t *conf = mddev->private;
+	unsigned long new;
+	int err;
+
+	if (len >= PAGE_SIZE)
+		return -EINVAL;
+	if (!conf)
+		return -ENODEV;
+
+	if (strict_strtoul(page, 10, &new))
+		return -EINVAL;
+	err = raid5_set_cache_size(mddev, new);
+	if (err)
+		return err;
 	return len;
 }
 
author
#3 lore raw

PATCH 3/24 md/raid5: ensure we create a unique name for kmem_cache when mddev has no gendisk

In reply to NeilBrown

We will shortly allow md devices with no gendisk (they are attached to a dm-target instead). That will cause mdname() to return 'mdX'. There is one place where mdname really needs to be unique: when creating the name for a slab cache.
So in that case, if there is no gendisk, you the address of the mddev formatted in HEX to provide a unique name.

---
 drivers/md/raid5.c |   12 ++++++++----
 drivers/md/raid5.h |    2 +-
 2 files changed, 9 insertions(+), 5 deletions(-)
drivers/md/raid5.c +8 -4
index 8647705..74e36a2 100644
@@ -1269,10 +1269,14 @@ static int grow_stripes(raid5_conf_t *conf, int num)
 	struct kmem_cache *sc;
 	int devs = max(conf->raid_disks, conf->previous_raid_disks);
 
-	sprintf(conf->cache_name[0],
-		"raid%d-%s", conf->level, mdname(conf->mddev));
-	sprintf(conf->cache_name[1],
-		"raid%d-%s-alt", conf->level, mdname(conf->mddev));
+	if (conf->mddev->gendisk)
+		sprintf(conf->cache_name[0],
+			"raid%d-%s", conf->level, mdname(conf->mddev));
+	else
+		sprintf(conf->cache_name[0],
+			"raid%d-%p", conf->level, conf->mddev);
+	sprintf(conf->cache_name[1], "%s-alt", conf->cache_name[0]);
+
 	conf->active_name = 0;
 	sc = kmem_cache_create(conf->cache_name[conf->active_name],
 			       sizeof(struct stripe_head)+(devs-1)*sizeof(struct r5dev),
drivers/md/raid5.h +1 -1
index 0f86f5e..bb7ab92 100644
@@ -388,7 +388,7 @@ struct raid5_private_data {
 	 * two caches.
 	 */
 	int			active_name;
-	char			cache_name[2][20];
+	char			cache_name[2][32];
 	struct kmem_cache		*slab_cache; /* for allocating stripes */
 
 	int			seq_flush, seq_write;
author
#4 lore raw

PATCH 4/24 md: be more careful setting MD_CHANGE_CLEAN

In reply to NeilBrown

When MD_CHANGE_CLEAN is set we might block in md_write_start. So we should only set it when fairly sure that something will clear it.

There are two places where it is set so as to encourage a metadata update to record the progress of resync/recovery. This should only be done if the internal metadata update mechanisms are in use, which can be tested by by inspecting '->persistent'.

---
 drivers/md/bitmap.c |    3 ++-
 drivers/md/md.c     |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)
drivers/md/bitmap.c +2 -1
index 1742435..4518994 100644
@@ -1526,7 +1526,8 @@ void bitmap_cond_end_sync(struct bitmap *bitmap, sector_t sector)
 		   atomic_read(&bitmap->mddev->recovery_active) == 0);
 
 	bitmap->mddev->curr_resync_completed = bitmap->mddev->curr_resync;
-	set_bit(MD_CHANGE_CLEAN, &bitmap->mddev->flags);
+	if (bitmap->mddev->persistent)
+		set_bit(MD_CHANGE_CLEAN, &bitmap->mddev->flags);
 	sector &= ~((1ULL << CHUNK_BLOCK_SHIFT(bitmap)) - 1);
 	s = 0;
 	while (s < sector && s < bitmap->mddev->resync_max_sectors) {
drivers/md/md.c +2 -1
index 6fbd1c7..2337cb2 100644
@@ -6726,7 +6726,8 @@ void md_do_sync(mddev_t *mddev)
 				   atomic_read(&mddev->recovery_active) == 0);
 			mddev->curr_resync_completed =
 				mddev->curr_resync;
-			set_bit(MD_CHANGE_CLEAN, &mddev->flags);
+			if (mddev->persistent)
+				set_bit(MD_CHANGE_CLEAN, &mddev->flags);
 			sysfs_notify(&mddev->kobj, NULL, "sync_completed");
 		}
 
author
#5 lore raw

PATCH 5/24 md: split out md_rdev_init

In reply to NeilBrown

This functionality will be needed separately in a subsequent patch, so split it into it's own exported function.

---
 drivers/md/md.c |   34 +++++++++++++++++++---------------
 drivers/md/md.h |    1 +
 2 files changed, 20 insertions(+), 15 deletions(-)
drivers/md/md.c +19 -15
index 2337cb2..d8e8a8c 100644
@@ -2690,6 +2690,24 @@ static struct kobj_type rdev_ktype = {
 	.default_attrs	= rdev_default_attrs,
 };
 
+void md_rdev_init(mdk_rdev_t *rdev)
+{
+	rdev->desc_nr = -1;
+	rdev->saved_raid_disk = -1;
+	rdev->raid_disk = -1;
+	rdev->flags = 0;
+	rdev->data_offset = 0;
+	rdev->sb_events = 0;
+	rdev->last_read_error.tv_sec  = 0;
+	rdev->last_read_error.tv_nsec = 0;
+	atomic_set(&rdev->nr_pending, 0);
+	atomic_set(&rdev->read_errors, 0);
+	atomic_set(&rdev->corrected_errors, 0);
+
+	INIT_LIST_HEAD(&rdev->same_set);
+	init_waitqueue_head(&rdev->blocked_wait);
+}
+EXPORT_SYMBOL_GPL(md_rdev_init);
 /*
  * Import a device. If 'super_format' >= 0, then sanity check the superblock
  *
@@ -2713,6 +2731,7 @@ static mdk_rdev_t *md_import_device(dev_t newdev, int super_format, int super_mi
 		return ERR_PTR(-ENOMEM);
 	}
 
+	md_rdev_init(rdev);
 	if ((err = alloc_disk_sb(rdev)))
 		goto abort_free;
 
@@ -2722,18 +2741,6 @@ static mdk_rdev_t *md_import_device(dev_t newdev, int super_format, int super_mi
 
 	kobject_init(&rdev->kobj, &rdev_ktype);
 
-	rdev->desc_nr = -1;
-	rdev->saved_raid_disk = -1;
-	rdev->raid_disk = -1;
-	rdev->flags = 0;
-	rdev->data_offset = 0;
-	rdev->sb_events = 0;
-	rdev->last_read_error.tv_sec  = 0;
-	rdev->last_read_error.tv_nsec = 0;
-	atomic_set(&rdev->nr_pending, 0);
-	atomic_set(&rdev->read_errors, 0);
-	atomic_set(&rdev->corrected_errors, 0);
-
 	size = rdev->bdev->bd_inode->i_size >> BLOCK_SIZE_BITS;
 	if (!size) {
 		printk(KERN_WARNING 
@@ -2762,9 +2769,6 @@ static mdk_rdev_t *md_import_device(dev_t newdev, int super_format, int super_mi
 		}
 	}
 
-	INIT_LIST_HEAD(&rdev->same_set);
-	init_waitqueue_head(&rdev->blocked_wait);
-
 	return rdev;
 
 abort_free:
drivers/md/md.h +1 -0
index 1f680e4..a9cde80 100644
@@ -484,4 +484,5 @@ extern void md_integrity_add_rdev(mdk_rdev_t *rdev, mddev_t *mddev);
 extern int strict_strtoul_scaled(const char *cp, unsigned long *res, int scale);
 extern void restore_bitmap_write_access(struct file *file);
 
+extern void md_rdev_init(mdk_rdev_t *rdev);
 #endif /* _MD_MD_H */
author
#6 lore raw

PATCH 6/24 md: export various start/stop interfaces

In reply to NeilBrown

export entry points for starting and stopping md arrays. This will be used by a module to make md/raid5 work under dm.
Also stop calling md_stop_writes from md_stop, as that won't work well with dm - it will want to call the two separately.

---
 drivers/md/md.c |   15 +++++++++------
 drivers/md/md.h |    4 ++++
 2 files changed, 13 insertions(+), 6 deletions(-)
drivers/md/md.c +9 -6
index d8e8a8c..4eccf4e 100644
@@ -416,7 +416,7 @@ static void mddev_put(mddev_t *mddev)
 	spin_unlock(&all_mddevs_lock);
 }
 
-static void mddev_init(mddev_t *mddev)
+void mddev_init(mddev_t *mddev)
 {
 	mutex_init(&mddev->open_mutex);
 	mutex_init(&mddev->reconfig_mutex);
@@ -436,6 +436,7 @@ static void mddev_init(mddev_t *mddev)
 	mddev->resync_max = MaxSector;
 	mddev->level = LEVEL_NONE;
 }
+EXPORT_SYMBOL_GPL(mddev_init);
 
 static mddev_t * mddev_find(dev_t unit)
 {
@@ -4307,7 +4308,7 @@ static void md_safemode_timeout(unsigned long data)
 
 static int start_dirty_degraded;
 
-static int md_run(mddev_t *mddev)
+int md_run(mddev_t *mddev)
 {
 	int err;
 	mdk_rdev_t *rdev;
@@ -4511,6 +4512,7 @@ static int md_run(mddev_t *mddev)
 	sysfs_notify(&mddev->kobj, NULL, "degraded");
 	return 0;
 }
+EXPORT_SYMBOL_GPL(md_run);
 
 static int do_md_run(mddev_t *mddev)
 {
@@ -4620,7 +4622,7 @@ static void md_clean(mddev_t *mddev)
 	mddev->bitmap_info.max_write_behind = 0;
 }
 
-static void md_stop_writes(mddev_t *mddev)
+void md_stop_writes(mddev_t *mddev)
 {
 	if (mddev->sync_thread) {
 		set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
@@ -4640,11 +4642,10 @@ static void md_stop_writes(mddev_t *mddev)
 		md_update_sb(mddev, 1);
 	}
 }
+EXPORT_SYMBOL_GPL(md_stop_writes);
 
-static void md_stop(mddev_t *mddev)
+void md_stop(mddev_t *mddev)
 {
-	md_stop_writes(mddev);
-
 	mddev->pers->stop(mddev);
 	if (mddev->pers->sync_request && mddev->to_remove == NULL)
 		mddev->to_remove = &md_redundancy_group;
@@ -4652,6 +4653,7 @@ static void md_stop(mddev_t *mddev)
 	mddev->pers = NULL;
 	clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
 }
+EXPORT_SYMBOL_GPL(md_stop);
 
 static int md_set_readonly(mddev_t *mddev, int is_open)
 {
@@ -4698,6 +4700,7 @@ static int do_md_stop(mddev_t * mddev, int mode, int is_open)
 		if (mddev->ro)
 			set_disk_ro(disk, 0);
 
+		md_stop_writes(mddev);
 		md_stop(mddev);
 		mddev->queue->merge_bvec_fn = NULL;
 		mddev->queue->unplug_fn = NULL;
drivers/md/md.h +4 -0
index a9cde80..8e19e86 100644
@@ -484,5 +484,9 @@ extern void md_integrity_add_rdev(mdk_rdev_t *rdev, mddev_t *mddev);
 extern int strict_strtoul_scaled(const char *cp, unsigned long *res, int scale);
 extern void restore_bitmap_write_access(struct file *file);
 
+extern void mddev_init(mddev_t *mddev);
+extern int md_run(mddev_t *mddev);
+extern void md_stop(mddev_t *mddev);
+extern void md_stop_writes(mddev_t *mddev);
 extern void md_rdev_init(mdk_rdev_t *rdev);
 #endif /* _MD_MD_H */
author
#7 lore raw

PATCH 7/24 md/dm: create dm-raid456 module using md/raid5

In reply to NeilBrown
---
 drivers/md/Kconfig      |    8 +
 drivers/md/Makefile     |    1 
 drivers/md/dm-raid456.c |  437 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 446 insertions(+), 0 deletions(-)
 create mode 100644 drivers/md/dm-raid456.c
drivers/md/Kconfig +8 -0
index 4a6feac..3465363 100644
@@ -256,6 +256,14 @@ config DM_MIRROR
          Allow volume managers to mirror logical volumes, also
          needed for live data migration tools such as 'pvmove'.
 
+config DM_RAID456
+       tristate "RAID 4/5/6 target (EXPERIMENTAL)"
+       depends on BLK_DEV_DM && MD_RAID456 && EXPERIMENTAL
+       ---help---
+       A dm target that supports RAID4 RAID5 and RAID6 mapping
+
+       If unsure, say N.
+
 config DM_LOG_USERSPACE
 	tristate "Mirror userspace logging (EXPERIMENTAL)"
 	depends on DM_MIRROR && EXPERIMENTAL && NET
drivers/md/Makefile +1 -0
index e355e7f..0734fba 100644
@@ -44,6 +44,7 @@ obj-$(CONFIG_DM_SNAPSHOT)	+= dm-snapshot.o
 obj-$(CONFIG_DM_MIRROR)		+= dm-mirror.o dm-log.o dm-region-hash.o
 obj-$(CONFIG_DM_LOG_USERSPACE)	+= dm-log-userspace.o
 obj-$(CONFIG_DM_ZERO)		+= dm-zero.o
+obj-$(CONFIG_DM_RAID456)	+= dm-raid456.o
 
 quiet_cmd_unroll = UNROLL  $@
       cmd_unroll = $(AWK) -f$(srctree)/$(src)/unroll.awk -vN=$(UNROLL) \
drivers/md/dm-raid456.c new file +437 -0

437 lines not on this page · raw message

author
#8 lore raw

PATCH 8/24 dm-raid456: add support for raising events to userspace.

In reply to NeilBrown

Userspace needs to know about failure events. DM handles though through the DM_DEV_WAIT_CMD ioctl.

So allow md_error to be given some work to do on an error, and arrange that work to signal dm.

---
 drivers/md/dm-raid456.c |    8 ++++++++
 drivers/md/md.c         |    2 ++
 drivers/md/md.h         |    1 +
 3 files changed, 11 insertions(+), 0 deletions(-)
drivers/md/dm-raid456.c +8 -0
index 5185a8f..d54f901 100644
@@ -139,6 +139,13 @@ static int dev_parms(struct raid_set *rs, char **argv)
 	return 0;
 }
 
+static void do_table_event(struct work_struct *ws)
+{
+	struct raid_set *rs = container_of(ws, struct raid_set,
+					   md.event_work);
+	dm_table_event(rs->ti->table);
+}
+
 /*
  * Construct a RAID4/5/6 mapping:
  * Args:
@@ -290,6 +297,7 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	if (rs->md.raid_disks - in_sync > rt->parity_devs)
 		goto err;
 
+	INIT_WORK(&rs->md.event_work, do_table_event);
 	ti->split_io = rs->md.chunk_sectors;
 	ti->private = rs;
 
drivers/md/md.c +2 -0
index 4eccf4e..da860f7 100644
@@ -6039,6 +6039,8 @@ void md_error(mddev_t *mddev, mdk_rdev_t *rdev)
 	set_bit(MD_RECOVERY_INTR, &mddev->recovery);
 	set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
 	md_wakeup_thread(mddev->thread);
+	if (mddev->event_work.func)
+		schedule_work(&mddev->event_work);
 	md_new_event_inintr(mddev);
 }
 
drivers/md/md.h +1 -0
index 8e19e86..6318175 100644
@@ -315,6 +315,7 @@ struct mddev_s
 	struct bio *barrier;
 	atomic_t flush_pending;
 	struct work_struct barrier_work;
+	struct work_struct event_work;
 };
 
 
author
#9 lore raw

PATCH 9/24 raid5: Don't set read-ahead when there is no queue

In reply to NeilBrown

dm-raid456 does not provide a 'queue' for raid5 to use, so we must make raid5 stop depending on the queue.

First: read_ahead
dm handles read-ahead adjustment fully in userspace, so simply don't do any readahead adjustments if there is no queue.

Also re-arrange code slightly so all the accesses to ->queue are together.

Finally, move the blk_queue_merge_bvec function into the 'if' as the ->split_io setting in dm-raid456 has the same effect.

---
 drivers/md/raid5.c |   32 +++++++++++++++++---------------
 1 files changed, 17 insertions(+), 15 deletions(-)
drivers/md/raid5.c +17 -15
index 74e36a2..8839573 100644
@@ -5072,16 +5072,6 @@ static int run(mddev_t *mddev)
 							"reshape");
 	}
 
-	/* read-ahead size must cover two whole stripes, which is
-	 * 2 * (datadisks) * chunksize where 'n' is the number of raid devices
-	 */
-	{
-		int data_disks = conf->previous_raid_disks - conf->max_degraded;
-		int stripe = data_disks *
-			((mddev->chunk_sectors << 9) / PAGE_SIZE);
-		if (mddev->queue->backing_dev_info.ra_pages < 2 * stripe)
-			mddev->queue->backing_dev_info.ra_pages = 2 * stripe;
-	}
 
 	/* Ok, everything is just fine now */
 	if (mddev->to_remove == &raid5_attrs_group)
@@ -5089,8 +5079,23 @@ static int run(mddev_t *mddev)
 	else if (mddev->kobj.sd &&
 	    sysfs_create_group(&mddev->kobj, &raid5_attrs_group))
 		printk(KERN_WARNING
-		       "md/raid:%s: failed to create sysfs attributes.\n",
+		       "raid5: failed to create sysfs attributes for %s\n",
 		       mdname(mddev));
+	md_set_array_sectors(mddev, raid5_size(mddev, 0, 0));
+
+	if (mddev->queue) {
+		/* read-ahead size must cover two whole stripes, which
+		 * is 2 * (datadisks) * chunksize where 'n' is the
+		 * number of raid devices
+		 */
+		int data_disks = conf->previous_raid_disks - conf->max_degraded;
+		int stripe = data_disks *
+			((mddev->chunk_sectors << 9) / PAGE_SIZE);
+		if (mddev->queue->backing_dev_info.ra_pages < 2 * stripe)
+			mddev->queue->backing_dev_info.ra_pages = 2 * stripe;
+
+		blk_queue_merge_bvec(mddev->queue, raid5_mergeable_bvec);
+	}
 
 	mddev->queue->queue_lock = &conf->device_lock;
 
@@ -5098,9 +5103,6 @@ static int run(mddev_t *mddev)
 	mddev->queue->backing_dev_info.congested_data = mddev;
 	mddev->queue->backing_dev_info.congested_fn = raid5_congested;
 
-	md_set_array_sectors(mddev, raid5_size(mddev, 0, 0));
-
-	blk_queue_merge_bvec(mddev->queue, raid5_mergeable_bvec);
 	chunk_size = mddev->chunk_sectors << 9;
 	blk_queue_io_min(mddev->queue, chunk_size);
 	blk_queue_io_opt(mddev->queue, chunk_size *
@@ -5523,7 +5525,7 @@ static void end_reshape(raid5_conf_t *conf)
 		/* read-ahead size must cover two whole stripes, which is
 		 * 2 * (datadisks) * chunksize where 'n' is the number of raid devices
 		 */
-		{
+		if (conf->mddev->queue) {
 			int data_disks = conf->raid_disks - conf->max_degraded;
 			int stripe = data_disks * ((conf->chunk_sectors << 9)
 						   / PAGE_SIZE);
author
#10 lore raw

PATCH 10/24 dm-raid456: add congestion checking.

In reply to NeilBrown

dm currently implements congestion checking by checking on congestion in each component device.

For raid456 we need to also check if the stripe cache is congested. So add support to dm for a target to register a congestion checker, then registered such a checker for dm-raid456.

We add support for multiple callbacks as we will need one for unplug too.

Finally, we move the setting for congested_fn for the mddev->queue into the "if (mddev->queue)" protected branch as it is not needed for dm-raid456 now.

---
 drivers/md/dm-raid456.c       |   13 +++++++++++++
 drivers/md/dm-table.c         |   15 +++++++++++++++
 drivers/md/raid5.c            |   22 +++++++++++++++-------
 drivers/md/raid5.h            |    1 +
 include/linux/device-mapper.h |   12 ++++++++++++
 5 files changed, 56 insertions(+), 7 deletions(-)
drivers/md/dm-raid456.c +13 -0
index d54f901..0e3922a 100644
@@ -17,6 +17,7 @@ struct raid_set {
 	struct dm_target *ti;
 	struct mddev_s md;
 	struct raid_type *raid_type;
+	struct target_callbacks callbacks;
 	struct raid_dev dev[0];
 };
 
@@ -146,6 +147,13 @@ static void do_table_event(struct work_struct *ws)
 	dm_table_event(rs->ti->table);
 }
 
+static int raid_is_congested(void *v, int bits)
+{
+	struct target_callbacks *cb = v;
+	struct raid_set *rs = container_of(cb, struct raid_set,
+					   callbacks);
+	return md_raid5_congested(&rs->md, bits);
+}
 /*
  * Construct a RAID4/5/6 mapping:
  * Args:
@@ -309,6 +317,10 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
 
 	if (errnum)
 		goto err;
+
+	rs->callbacks.congested_fn = raid_is_congested;
+	dm_table_add_callbacks(ti->table, &rs->callbacks);
+
 	return 0;
 err:
 	if (rs)
@@ -321,6 +333,7 @@ static void raid_dtr(struct dm_target *ti)
 {
 	struct raid_set *rs = ti->private;
 
+	list_del_init(&rs->callbacks.list);
 	md_stop(&rs->md);
 	context_free(rs);
 }
drivers/md/dm-table.c +15 -0
index 9924ea2..b856340 100644
@@ -68,6 +68,8 @@ struct dm_table {
 	void (*event_fn)(void *);
 	void *event_context;
 
+	struct list_head target_callbacks;
+
 	struct dm_md_mempools *mempools;
 };
 
@@ -202,6 +204,7 @@ int dm_table_create(struct dm_table **result, fmode_t mode,
 		return -ENOMEM;
 
 	INIT_LIST_HEAD(&t->devices);
+	INIT_LIST_HEAD(&t->target_callbacks);
 	atomic_set(&t->holders, 0);
 
 	if (!num_targets)
@@ -1174,10 +1177,18 @@ int dm_table_resume_targets(struct dm_table *t)
 	return 0;
 }
 
+void dm_table_add_callbacks(struct dm_table *t,
+			    struct target_callbacks *cb)
+{
+	list_add(&cb->list, &t->target_callbacks);
+}
+EXPORT_SYMBOL_GPL(dm_table_add_callbacks);
+
 int dm_table_any_congested(struct dm_table *t, int bdi_bits)
 {
 	struct dm_dev_internal *dd;
 	struct list_head *devices = dm_table_get_devices(t);
+	struct target_callbacks *cb;
 	int r = 0;
 
 	list_for_each_entry(dd, devices, list) {
@@ -1192,6 +1203,10 @@ int dm_table_any_congested(struct dm_table *t, int bdi_bits)
 				     bdevname(dd->dm_dev.bdev, b));
 	}
 
+	list_for_each_entry(cb, &t->target_callbacks, list)
+		if (cb->congested_fn)
+			r |= cb->congested_fn(cb, bdi_bits);
+
 	return r;
 }
 
drivers/md/raid5.c +15 -7
index 8839573..c0746af 100644
@@ -3592,17 +3592,14 @@ static void raid5_unplug_device(struct request_queue *q)
 	unplug_slaves(mddev);
 }
 
-static int raid5_congested(void *data, int bits)
+int md_raid5_congested(mddev_t *mddev, int bits)
 {
-	mddev_t *mddev = data;
 	raid5_conf_t *conf = mddev->private;
 
 	/* No difference between reads and writes.  Just check
 	 * how busy the stripe_cache is
 	 */
 
-	if (mddev_congested(mddev, bits))
-		return 1;
 	if (conf->inactive_blocked)
 		return 1;
 	if (conf->quiesce)
@@ -3612,6 +3609,15 @@ static int raid5_congested(void *data, int bits)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(md_raid5_congested);
+
+static int raid5_congested(void *data, int bits)
+{
+	mddev_t *mddev = data;
+
+	return mddev_congested(mddev, bits) ||
+		md_raid5_congested(mddev, bits);
+}
 
 /* We want read requests to align with chunks where possible,
  * but write requests don't need to.
@@ -5095,13 +5101,14 @@ static int run(mddev_t *mddev)
 			mddev->queue->backing_dev_info.ra_pages = 2 * stripe;
 
 		blk_queue_merge_bvec(mddev->queue, raid5_mergeable_bvec);
+
+		mddev->queue->backing_dev_info.congested_data = mddev;
+		mddev->queue->backing_dev_info.congested_fn = raid5_congested;
 	}
 
 	mddev->queue->queue_lock = &conf->device_lock;
 
 	mddev->queue->unplug_fn = raid5_unplug_device;
-	mddev->queue->backing_dev_info.congested_data = mddev;
-	mddev->queue->backing_dev_info.congested_fn = raid5_congested;
 
 	chunk_size = mddev->chunk_sectors << 9;
 	blk_queue_io_min(mddev->queue, chunk_size);
@@ -5131,7 +5138,8 @@ static int stop(mddev_t *mddev)
 
 	md_unregister_thread(mddev->thread);
 	mddev->thread = NULL;
-	mddev->queue->backing_dev_info.congested_fn = NULL;
+	if (mddev->queue)
+		mddev->queue->backing_dev_info.congested_fn = NULL;
 	blk_sync_queue(mddev->queue); /* the unplug fn references 'conf'*/
 	free_conf(conf);
 	mddev->private = NULL;
drivers/md/raid5.h +1 -0
index bb7ab92..936caf8 100644
@@ -497,4 +497,5 @@ static inline int algorithm_is_DDF(int layout)
 {
 	return layout >= 8 && layout <= 10;
 }
+extern int md_raid5_congested(mddev_t *mddev, int bits);
 #endif
include/linux/device-mapper.h +12 -0
index 1381cd9..2b0f538 100644
@@ -187,6 +187,12 @@ struct dm_target {
 	char *error;
 };
 
+/* Each target can link one of these into the table */
+struct target_callbacks {
+	struct list_head list;
+	congested_fn *congested_fn;
+};
+
 int dm_register_target(struct target_type *t);
 void dm_unregister_target(struct target_type *t);
 
@@ -263,6 +269,12 @@ int dm_table_add_target(struct dm_table *t, const char *type,
 			sector_t start, sector_t len, char *params);
 
 /*
+ * Target_ctr should call this if they need to add any
+ * callback
+ */
+void dm_table_add_callbacks(struct dm_table *t,
+			    struct target_callbacks *cb);
+/*
  * Finally call this to make the table ready for use.
  */
 int dm_table_complete(struct dm_table *t);
author
#11 lore raw

PATCH 11/24 md/raid5: add simple plugging infrastructure.

In reply to NeilBrown

md/raid5 uses the plugging infrastructure provided by the block layer and 'struct request_queue'. However when we plug raid5 under dm there is no request queue so we cannot use that.

So create a similar infrastructure that is much lighter weight and use it for raid5.

---
 drivers/md/md.c    |   45 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/md/md.h    |   20 ++++++++++++++++++++
 drivers/md/raid5.c |   39 +++++++++++++++++++++++++--------------
 drivers/md/raid5.h |    3 +++
 4 files changed, 93 insertions(+), 14 deletions(-)
drivers/md/md.c +45 -0
index da860f7..e44b9c6 100644
@@ -384,6 +384,51 @@ void md_barrier_request(mddev_t *mddev, struct bio *bio)
 }
 EXPORT_SYMBOL(md_barrier_request);
 
+/* Support for plugging.
+ * This mirrors the plugging support in request_queue, but does not
+ * require having a whole queue
+ */
+static void plugger_work(struct work_struct *work)
+{
+	struct plug_handle *plug =
+		container_of(work, struct plug_handle, unplug_work);
+	plug->unplug_fn(plug);
+}
+static void plugger_timeout(unsigned long data)
+{
+	struct plug_handle *plug = (void *)data;
+	kblockd_schedule_work(NULL, &plug->unplug_work);
+}
+void plugger_init(struct plug_handle *plug,
+		  void (*unplug_fn)(struct plug_handle *))
+{
+	plug->unplug_flag = 0;
+	plug->unplug_fn = unplug_fn;
+	init_timer(&plug->unplug_timer);
+	plug->unplug_timer.function = plugger_timeout;
+	plug->unplug_timer.data = (unsigned long)plug;
+	INIT_WORK(&plug->unplug_work, plugger_work);
+}
+EXPORT_SYMBOL_GPL(plugger_init);
+
+void plugger_set_plug(struct plug_handle *plug)
+{
+	if (!test_and_set_bit(PLUGGED_FLAG, &plug->unplug_flag))
+		mod_timer(&plug->unplug_timer, jiffies + msecs_to_jiffies(3)+1);
+}
+EXPORT_SYMBOL_GPL(plugger_set_plug);
+
+int plugger_remove_plug(struct plug_handle *plug)
+{
+	if (test_and_clear_bit(PLUGGED_FLAG, &plug->unplug_flag)) {
+		del_timer(&plug->unplug_timer);
+		return 1;
+	} else
+		return 0;
+}
+EXPORT_SYMBOL_GPL(plugger_remove_plug);
+
+
 static inline mddev_t *mddev_get(mddev_t *mddev)
 {
 	atomic_inc(&mddev->active);
drivers/md/md.h +20 -0
index 6318175..f210482 100644
@@ -29,6 +29,26 @@
 typedef struct mddev_s mddev_t;
 typedef struct mdk_rdev_s mdk_rdev_t;
 
+/* generic plugging support - like that provided with request_queue,
+ * but does not require a request_queue
+ */
+struct plug_handle {
+	void			(*unplug_fn)(struct plug_handle *);
+	struct timer_list	unplug_timer;
+	struct work_struct	unplug_work;
+	unsigned long		unplug_flag;
+};
+#define	PLUGGED_FLAG 1
+void plugger_init(struct plug_handle *plug,
+		  void (*unplug_fn)(struct plug_handle *));
+void plugger_set_plug(struct plug_handle *plug);
+int plugger_remove_plug(struct plug_handle *plug);
+static inline void plugger_flush(struct plug_handle *plug)
+{
+	del_timer_sync(&plug->unplug_timer);
+	cancel_work_sync(&plug->unplug_work);
+}
+
 /*
  * MD's 'extended' device
  */
drivers/md/raid5.c +25 -14
index c0746af..6b1802c 100644
@@ -201,11 +201,11 @@ static void __release_stripe(raid5_conf_t *conf, struct stripe_head *sh)
 		if (test_bit(STRIPE_HANDLE, &sh->state)) {
 			if (test_bit(STRIPE_DELAYED, &sh->state)) {
 				list_add_tail(&sh->lru, &conf->delayed_list);
-				blk_plug_device(conf->mddev->queue);
+				plugger_set_plug(&conf->plug);
 			} else if (test_bit(STRIPE_BIT_DELAY, &sh->state) &&
 				   sh->bm_seq - conf->seq_write > 0) {
 				list_add_tail(&sh->lru, &conf->bitmap_list);
-				blk_plug_device(conf->mddev->queue);
+				plugger_set_plug(&conf->plug);
 			} else {
 				clear_bit(STRIPE_BIT_DELAY, &sh->state);
 				list_add_tail(&sh->lru, &conf->handle_list);
@@ -365,7 +365,7 @@ static struct stripe_head *__find_stripe(raid5_conf_t *conf, sector_t sector,
 }
 
 static void unplug_slaves(mddev_t *mddev);
-static void raid5_unplug_device(struct request_queue *q);
+static void raid5_unplug_device(raid5_conf_t *conf);
 
 static struct stripe_head *
 get_active_stripe(raid5_conf_t *conf, sector_t sector,
@@ -395,7 +395,7 @@ get_active_stripe(raid5_conf_t *conf, sector_t sector,
 						     < (conf->max_nr_stripes *3/4)
 						     || !conf->inactive_blocked),
 						    conf->device_lock,
-						    raid5_unplug_device(conf->mddev->queue)
+						    raid5_unplug_device(conf)
 					);
 				conf->inactive_blocked = 0;
 			} else
@@ -3532,7 +3532,7 @@ static void raid5_activate_delayed(raid5_conf_t *conf)
 			list_add_tail(&sh->lru, &conf->hold_list);
 		}
 	} else
-		blk_plug_device(conf->mddev->queue);
+		plugger_set_plug(&conf->plug);
 }
 
 static void activate_bit_delay(raid5_conf_t *conf)
@@ -3573,23 +3573,33 @@ static void unplug_slaves(mddev_t *mddev)
 	rcu_read_unlock();
 }
 
-static void raid5_unplug_device(struct request_queue *q)
+static void raid5_unplug_device(raid5_conf_t *conf)
 {
-	mddev_t *mddev = q->queuedata;
-	raid5_conf_t *conf = mddev->private;
 	unsigned long flags;
 
 	spin_lock_irqsave(&conf->device_lock, flags);
 
-	if (blk_remove_plug(q)) {
+	if (plugger_remove_plug(&conf->plug)) {
 		conf->seq_flush++;
 		raid5_activate_delayed(conf);
 	}
-	md_wakeup_thread(mddev->thread);
+	md_wakeup_thread(conf->mddev->thread);
 
 	spin_unlock_irqrestore(&conf->device_lock, flags);
 
-	unplug_slaves(mddev);
+	unplug_slaves(conf->mddev);
+}
+
+static void raid5_unplug(struct plug_handle *plug)
+{
+	raid5_conf_t *conf = container_of(plug, raid5_conf_t, plug);
+	raid5_unplug_device(conf);
+}
+
+static void raid5_unplug_queue(struct request_queue *q)
+{
+	mddev_t *mddev = q->queuedata;
+	raid5_unplug_device(mddev->private);
 }
 
 int md_raid5_congested(mddev_t *mddev, int bits)
@@ -3999,7 +4009,7 @@ static int make_request(mddev_t *mddev, struct bio * bi)
 				 * add failed due to overlap.  Flush everything
 				 * and wait a while
 				 */
-				raid5_unplug_device(mddev->queue);
+				raid5_unplug_device(conf);
 				release_stripe(sh);
 				schedule();
 				goto retry;
@@ -5089,6 +5099,7 @@ static int run(mddev_t *mddev)
 		       mdname(mddev));
 	md_set_array_sectors(mddev, raid5_size(mddev, 0, 0));
 
+	plugger_init(&conf->plug, raid5_unplug);
 	if (mddev->queue) {
 		/* read-ahead size must cover two whole stripes, which
 		 * is 2 * (datadisks) * chunksize where 'n' is the
@@ -5108,7 +5119,7 @@ static int run(mddev_t *mddev)
 
 	mddev->queue->queue_lock = &conf->device_lock;
 
-	mddev->queue->unplug_fn = raid5_unplug_device;
+	mddev->queue->unplug_fn = raid5_unplug_queue;
 
 	chunk_size = mddev->chunk_sectors << 9;
 	blk_queue_io_min(mddev->queue, chunk_size);
@@ -5140,7 +5151,7 @@ static int stop(mddev_t *mddev)
 	mddev->thread = NULL;
 	if (mddev->queue)
 		mddev->queue->backing_dev_info.congested_fn = NULL;
-	blk_sync_queue(mddev->queue); /* the unplug fn references 'conf'*/
+	plugger_flush(&conf->plug); /* the unplug fn references 'conf'*/
 	free_conf(conf);
 	mddev->private = NULL;
 	mddev->to_remove = &raid5_attrs_group;
drivers/md/raid5.h +3 -0
index 936caf8..edf90a8 100644
@@ -398,6 +398,9 @@ struct raid5_private_data {
 					    * (fresh device added).
 					    * Cleared when a sync completes.
 					    */
+
+	struct plug_handle	plug;
+
 	/* per cpu variables */
 	struct raid5_percpu {
 		struct page	*spare_page; /* Used when checking P/Q in raid6 */
author
#12 lore raw

PATCH 12/24 md/plug: optionally use plugger to unplug an array during resync/recovery.

In reply to NeilBrown

If an array doesn't have a 'queue' then md_do_sync cannot unplug it.
In that case it will have a 'plugger', so make that available to the mddev, and us it to unplug the array if needed.

---
 drivers/md/md.c    |   15 ++++++++++++---
 drivers/md/md.h    |    2 ++
 drivers/md/raid5.c |    1 +
 3 files changed, 15 insertions(+), 3 deletions(-)
drivers/md/md.c +12 -3
index e44b9c6..ea6577b 100644
@@ -4665,6 +4665,7 @@ static void md_clean(mddev_t *mddev)
 	mddev->bitmap_info.chunksize = 0;
 	mddev->bitmap_info.daemon_sleep = 0;
 	mddev->bitmap_info.max_write_behind = 0;
+	mddev->plug = NULL;
 }
 
 void md_stop_writes(mddev_t *mddev)
@@ -6597,6 +6598,14 @@ int md_allow_write(mddev_t *mddev)
 }
 EXPORT_SYMBOL_GPL(md_allow_write);
 
+static void md_unplug(mddev_t *mddev)
+{
+	if (mddev->queue)
+		blk_unplug(mddev->queue);
+	if (mddev->plug)
+		mddev->plug->unplug_fn(mddev->plug);
+}
+
 #define SYNC_MARKS	10
 #define	SYNC_MARK_STEP	(3*HZ)
 void md_do_sync(mddev_t *mddev)
@@ -6775,7 +6784,7 @@ void md_do_sync(mddev_t *mddev)
 		     >= mddev->resync_max - mddev->curr_resync_completed
 			    )) {
 			/* time to update curr_resync_completed */
-			blk_unplug(mddev->queue);
+			md_unplug(mddev);
 			wait_event(mddev->recovery_wait,
 				   atomic_read(&mddev->recovery_active) == 0);
 			mddev->curr_resync_completed =
@@ -6853,7 +6862,7 @@ void md_do_sync(mddev_t *mddev)
 		 * about not overloading the IO subsystem. (things like an
 		 * e2fsck being done on the RAID array should execute fast)
 		 */
-		blk_unplug(mddev->queue);
+		md_unplug(mddev);
 		cond_resched();
 
 		currspeed = ((unsigned long)(io_sectors-mddev->resync_mark_cnt))/2
@@ -6872,7 +6881,7 @@ void md_do_sync(mddev_t *mddev)
 	 * this also signals 'finished resyncing' to md_stop
 	 */
  out:
-	blk_unplug(mddev->queue);
+	md_unplug(mddev);
 
 	wait_event(mddev->recovery_wait, !atomic_read(&mddev->recovery_active));
 
drivers/md/md.h +2 -0
index f210482..0bf8c18 100644
@@ -325,6 +325,8 @@ struct mddev_s
 	struct list_head		all_mddevs;
 
 	struct attribute_group		*to_remove;
+	struct plug_handle		*plug; /* if used by personality */
+
 	/* Generic barrier handling.
 	 * If there is a pending barrier request, all other
 	 * writes are blocked while the devices are flushed.
drivers/md/raid5.c +1 -0
index 6b1802c..74bc7a6 100644
@@ -5100,6 +5100,7 @@ static int run(mddev_t *mddev)
 	md_set_array_sectors(mddev, raid5_size(mddev, 0, 0));
 
 	plugger_init(&conf->plug, raid5_unplug);
+	mddev->plug = &conf->plug;
 	if (mddev->queue) {
 		/* read-ahead size must cover two whole stripes, which
 		 * is 2 * (datadisks) * chunksize where 'n' is the
author
#13 lore raw

PATCH 13/24 dm-raid456: support unplug

In reply to NeilBrown

In a similar manner to congestion checking, per-target unplug support for raid456 under dm.

---
 drivers/md/dm-raid456.c       |    9 +++++++++
 drivers/md/dm-table.c         |    4 ++++
 drivers/md/raid5.c            |   11 +++++------
 drivers/md/raid5.h            |    1 +
 include/linux/device-mapper.h |    1 +
 5 files changed, 20 insertions(+), 6 deletions(-)
drivers/md/dm-raid456.c +9 -0
index 0e3922a..46a5e29 100644
@@ -154,6 +154,14 @@ static int raid_is_congested(void *v, int bits)
 					   callbacks);
 	return md_raid5_congested(&rs->md, bits);
 }
+static void raid_unplug(void *v)
+{
+	struct target_callbacks *cb = v;
+	struct raid_set *rs = container_of(cb, struct raid_set,
+					   callbacks);
+	raid5_unplug_device(rs->md.private);
+}
+
 /*
  * Construct a RAID4/5/6 mapping:
  * Args:
@@ -289,6 +297,7 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
 			goto err;
 		clear_bit(In_sync, &rs->dev[rebuildA].rdev.flags);
 		rs->dev[rebuildA].rdev.recovery_offset = 0;
+	rs->callbacks.unplug_fn = raid_unplug;
 	}
 	if (rebuildB >= 0) {
 		if (rs->dev[rebuildB].dev == NULL)
drivers/md/dm-table.c +4 -0
index b856340..cad4992 100644
@@ -1228,6 +1228,7 @@ void dm_table_unplug_all(struct dm_table *t)
 {
 	struct dm_dev_internal *dd;
 	struct list_head *devices = dm_table_get_devices(t);
+	struct target_callbacks *cb;
 
 	list_for_each_entry(dd, devices, list) {
 		struct request_queue *q = bdev_get_queue(dd->dm_dev.bdev);
@@ -1240,6 +1241,9 @@ void dm_table_unplug_all(struct dm_table *t)
 				     dm_device_name(t->md),
 				     bdevname(dd->dm_dev.bdev, b));
 	}
+	list_for_each_entry(cb, &t->target_callbacks, list)
+		if (cb->unplug_fn)
+			cb->unplug_fn(cb);
 }
 
 struct mapped_device *dm_table_get_md(struct dm_table *t)
drivers/md/raid5.c +5 -6
index 74bc7a6..efcea17 100644
@@ -365,7 +365,6 @@ static struct stripe_head *__find_stripe(raid5_conf_t *conf, sector_t sector,
 }
 
 static void unplug_slaves(mddev_t *mddev);
-static void raid5_unplug_device(raid5_conf_t *conf);
 
 static struct stripe_head *
 get_active_stripe(raid5_conf_t *conf, sector_t sector,
@@ -3573,7 +3572,7 @@ static void unplug_slaves(mddev_t *mddev)
 	rcu_read_unlock();
 }
 
-static void raid5_unplug_device(raid5_conf_t *conf)
+void raid5_unplug_device(raid5_conf_t *conf)
 {
 	unsigned long flags;
 
@@ -3589,6 +3588,7 @@ static void raid5_unplug_device(raid5_conf_t *conf)
 
 	unplug_slaves(conf->mddev);
 }
+EXPORT_SYMBOL_GPL(raid5_unplug_device);
 
 static void raid5_unplug(struct plug_handle *plug)
 {
@@ -5116,11 +5116,10 @@ static int run(mddev_t *mddev)
 
 		mddev->queue->backing_dev_info.congested_data = mddev;
 		mddev->queue->backing_dev_info.congested_fn = raid5_congested;
-	}
-
-	mddev->queue->queue_lock = &conf->device_lock;
 
-	mddev->queue->unplug_fn = raid5_unplug_queue;
+		mddev->queue->queue_lock = &conf->device_lock;
+		mddev->queue->unplug_fn = raid5_unplug_queue;
+	}
 
 	chunk_size = mddev->chunk_sectors << 9;
 	blk_queue_io_min(mddev->queue, chunk_size);
drivers/md/raid5.h +1 -0
index edf90a8..fa3938a 100644
@@ -501,4 +501,5 @@ static inline int algorithm_is_DDF(int layout)
 	return layout >= 8 && layout <= 10;
 }
 extern int md_raid5_congested(mddev_t *mddev, int bits);
+extern void raid5_unplug_device(raid5_conf_t *conf);
 #endif
include/linux/device-mapper.h +1 -0
index 2b0f538..5b8ac19 100644
@@ -191,6 +191,7 @@ struct dm_target {
 struct target_callbacks {
 	struct list_head list;
 	congested_fn *congested_fn;
+	void (*unplug_fn)(void *);
 };
 
 int dm_register_target(struct target_type *t);
author
#14 lore raw

PATCH 14/24 dm-raid456: add support for setting IO hints.

In reply to NeilBrown
---
 drivers/md/dm-raid456.c |   33 +++++++++++++++++++++++++++++++++
 drivers/md/raid5.c      |   19 ++++++++++---------
 2 files changed, 43 insertions(+), 9 deletions(-)
drivers/md/dm-raid456.c +33 -0
index 46a5e29..0b72fa4 100644
@@ -435,6 +435,37 @@ static int raid_status(struct dm_target *ti, status_type_t type,
 	return 0;
 }
 
+static int raid_iterate_devices(struct dm_target *ti,
+				iterate_devices_callout_fn fn,
+				void *data)
+{
+	struct raid_set *rs = ti->private;
+	int ret = 0;
+	unsigned i = 0;
+
+	for (i = 0; !ret && i < rs->md.raid_disks; i++)
+		if (rs->dev[i].dev)
+			ret = fn(ti,
+				 rs->dev[i].dev,
+				 rs->dev[i].rdev.data_offset,
+				 rs->md.dev_sectors,
+				 data);
+
+	return ret;
+}
+
+static void raid_io_hints(struct dm_target *ti,
+			  struct queue_limits *limits)
+{
+	struct raid_set *rs = ti->private;
+	unsigned chunk_size = rs->md.chunk_sectors << 9;
+	raid5_conf_t *conf = rs->md.private;
+
+	blk_limits_io_min(limits, chunk_size);
+	blk_limits_io_opt(limits, chunk_size *
+			  (conf->raid_disks - conf->max_degraded));
+}
+
 static struct target_type raid_target = {
 	.name = "raid45",
 	.version = {1, 0, 0},
@@ -443,6 +474,8 @@ static struct target_type raid_target = {
 	.dtr = raid_dtr,
 	.map = raid_map,
 	.status = raid_status,
+	.iterate_devices = raid_iterate_devices,
+	.io_hints = raid_io_hints,
 };
 
 static int __init dm_raid_init(void)
drivers/md/raid5.c +10 -9
index efcea17..be5cab8 100644
@@ -4904,7 +4904,7 @@ static int only_parity(int raid_disk, int algo, int raid_disks, int max_degraded
 static int run(mddev_t *mddev)
 {
 	raid5_conf_t *conf;
-	int working_disks = 0, chunk_size;
+	int working_disks = 0;
 	int dirty_parity_disks = 0;
 	mdk_rdev_t *rdev;
 	sector_t reshape_offset = 0;
@@ -5102,6 +5102,7 @@ static int run(mddev_t *mddev)
 	plugger_init(&conf->plug, raid5_unplug);
 	mddev->plug = &conf->plug;
 	if (mddev->queue) {
+		int chunk_size;
 		/* read-ahead size must cover two whole stripes, which
 		 * is 2 * (datadisks) * chunksize where 'n' is the
 		 * number of raid devices
@@ -5119,16 +5120,16 @@ static int run(mddev_t *mddev)
 
 		mddev->queue->queue_lock = &conf->device_lock;
 		mddev->queue->unplug_fn = raid5_unplug_queue;
-	}
 
-	chunk_size = mddev->chunk_sectors << 9;
-	blk_queue_io_min(mddev->queue, chunk_size);
-	blk_queue_io_opt(mddev->queue, chunk_size *
-			 (conf->raid_disks - conf->max_degraded));
+		chunk_size = mddev->chunk_sectors << 9;
+		blk_queue_io_min(mddev->queue, chunk_size);
+		blk_queue_io_opt(mddev->queue, chunk_size *
+				 (conf->raid_disks - conf->max_degraded));
 
-	list_for_each_entry(rdev, &mddev->disks, same_set)
-		disk_stack_limits(mddev->gendisk, rdev->bdev,
-				  rdev->data_offset << 9);
+		list_for_each_entry(rdev, &mddev->disks, same_set)
+			disk_stack_limits(mddev->gendisk, rdev->bdev,
+					  rdev->data_offset << 9);
+	}
 
 	return 0;
 abort:
author
#15 lore raw

PATCH 15/24 dm-raid456: add suspend/resume method

In reply to NeilBrown

These just call in to the md methods.

---
 drivers/md/dm-raid456.c |   22 ++++++++++++++++++++++
 drivers/md/md.c         |    6 ++++--
 drivers/md/md.h         |    3 +++
 3 files changed, 29 insertions(+), 2 deletions(-)
drivers/md/dm-raid456.c +22 -0
index 0b72fa4..044faae 100644
@@ -466,6 +466,25 @@ static void raid_io_hints(struct dm_target *ti,
 			  (conf->raid_disks - conf->max_degraded));
 }
 
+static void raid_presuspend(struct dm_target *ti)
+{
+	struct raid_set *rs = ti->private;
+	md_stop_writes(&rs->md);
+}
+
+static void raid_postsuspend(struct dm_target *ti)
+{
+	struct raid_set *rs = ti->private;
+	mddev_suspend(&rs->md);
+}
+
+static void raid_resume(struct dm_target *ti)
+{
+	struct raid_set *rs = ti->private;
+
+	mddev_resume(&rs->md);
+}
+
 static struct target_type raid_target = {
 	.name = "raid45",
 	.version = {1, 0, 0},
@@ -476,6 +495,9 @@ static struct target_type raid_target = {
 	.status = raid_status,
 	.iterate_devices = raid_iterate_devices,
 	.io_hints = raid_io_hints,
+	.presuspend = raid_presuspend,
+	.postsuspend = raid_postsuspend,
+	.resume = raid_resume,
 };
 
 static int __init dm_raid_init(void)
drivers/md/md.c +4 -2
index ea6577b..21f3c35 100644
@@ -261,7 +261,7 @@ static int md_make_request(struct request_queue *q, struct bio *bio)
  * Once ->stop is called and completes, the module will be completely
  * unused.
  */
-static void mddev_suspend(mddev_t *mddev)
+void mddev_suspend(mddev_t *mddev)
 {
 	BUG_ON(mddev->suspended);
 	mddev->suspended = 1;
@@ -269,13 +269,15 @@ static void mddev_suspend(mddev_t *mddev)
 	wait_event(mddev->sb_wait, atomic_read(&mddev->active_io) == 0);
 	mddev->pers->quiesce(mddev, 1);
 }
+EXPORT_SYMBOL_GPL(mddev_suspend);
 
-static void mddev_resume(mddev_t *mddev)
+void mddev_resume(mddev_t *mddev)
 {
 	mddev->suspended = 0;
 	wake_up(&mddev->sb_wait);
 	mddev->pers->quiesce(mddev, 0);
 }
+EXPORT_SYMBOL_GPL(mddev_resume);
 
 int mddev_congested(mddev_t *mddev, int bits)
 {
drivers/md/md.h +3 -0
index 0bf8c18..6f2c568 100644
@@ -512,4 +512,7 @@ extern int md_run(mddev_t *mddev);
 extern void md_stop(mddev_t *mddev);
 extern void md_stop_writes(mddev_t *mddev);
 extern void md_rdev_init(mdk_rdev_t *rdev);
+
+extern void mddev_suspend(mddev_t *mddev);
+extern void mddev_resume(mddev_t *mddev);
 #endif /* _MD_MD_H */
author
#16 lore raw

PATCH 16/24 dm-raid456: add message handler.

In reply to NeilBrown

Support messages to:
- change the size of the stripe cache
- change the speed limiter on resync.

---
 drivers/md/dm-raid456.c |   26 ++++++++++++++++++++++++++
 drivers/md/raid5.c      |    3 ++-
 drivers/md/raid5.h      |    1 +
 3 files changed, 29 insertions(+), 1 deletions(-)
drivers/md/dm-raid456.c +26 -0
index 044faae..3fda954 100644
@@ -485,6 +485,31 @@ static void raid_resume(struct dm_target *ti)
 	mddev_resume(&rs->md);
 }
 
+/* Parse and handle a message from userspace
+ * Messages are:
+ *    stripecache  N  (pages per devices)
+ *    minspeed  N          (kibibytes per seconds)
+ */
+static int raid_message(struct dm_target *ti, unsigned argc, char **argv)
+{
+	struct raid_set *rs = ti->private;
+
+	if (argc == 2 && strcmp(argv[0], "stripecache") == 0) {
+		unsigned long size;
+		if (strict_strtoul(argv[1], 10, &size))
+			return -EINVAL;
+		return raid5_set_cache_size(&rs->md, size);
+	}
+	if (argc == 2 && strcmp(argv[0], "minspeed") == 0) {
+		unsigned long speed;
+		if (strict_strtoul(argv[1], 10, &speed))
+			return -EINVAL;
+		rs->md.sync_speed_min = speed;
+		return 0;
+	}
+	return -EINVAL;
+}
+
 static struct target_type raid_target = {
 	.name = "raid45",
 	.version = {1, 0, 0},
@@ -498,6 +523,7 @@ static struct target_type raid_target = {
 	.presuspend = raid_presuspend,
 	.postsuspend = raid_postsuspend,
 	.resume = raid_resume,
+	.message = raid_message,
 };
 
 static int __init dm_raid_init(void)
drivers/md/raid5.c +2 -1
index be5cab8..8ac122d 100644
@@ -4500,7 +4500,7 @@ raid5_show_stripe_cache_size(mddev_t *mddev, char *page)
 		return 0;
 }
 
-static int
+int
 raid5_set_cache_size(mddev_t *mddev, int size)
 {
 	raid5_conf_t *conf = mddev->private;
@@ -4524,6 +4524,7 @@ raid5_set_cache_size(mddev_t *mddev, int size)
 	}
 	return 0;
 }
+EXPORT_SYMBOL(raid5_set_cache_size);
 
 static ssize_t
 raid5_store_stripe_cache_size(mddev_t *mddev, const char *page, size_t len)
drivers/md/raid5.h +1 -0
index fa3938a..292a9c6 100644
@@ -502,4 +502,5 @@ static inline int algorithm_is_DDF(int layout)
 }
 extern int md_raid5_congested(mddev_t *mddev, int bits);
 extern void raid5_unplug_device(raid5_conf_t *conf);
+extern int raid5_set_cache_size(mddev_t *mddev, int size);
 #endif
author
#17 lore raw

PATCH 17/24 md/bitmap: white space clean up and similar.

In reply to NeilBrown

Fixes some whitespace problems
Fixed some checkpatch.pl complaints.
Replaced kmalloc ... memset(0), with kzalloc
Fixed an unlikely memory leak on an error path.
Reformatted a number of 'if/else' sets, sometimes
replacing goto with an else clause.
Removed some old comments and commented-out code.

---
 drivers/md/bitmap.c |  301 +++++++++++++++++++++++----------------------------
 1 files changed, 135 insertions(+), 166 deletions(-)
drivers/md/bitmap.c +135 -166
index 4518994..67fb32d 100644
@@ -13,7 +13,6 @@
  * Still to do:
  *
  * flush after percent set rather than just time based. (maybe both).
- * wait if count gets too high, wake when it drops to half.
  */
 
 #include <linux/blkdev.h>
@@ -51,9 +50,6 @@
 #define INJECT_FATAL_FAULT_3 0 /* undef */
 #endif
 
-//#define DPRINTK PRINTK /* set this NULL to avoid verbose debug output */
-#define DPRINTK(x...) do { } while(0)
-
 #ifndef PRINTK
 #  if DEBUG > 0
 #    define PRINTK(x...) printk(KERN_DEBUG x)
@@ -62,12 +58,11 @@
 #  endif
 #endif
 
-static inline char * bmname(struct bitmap *bitmap)
+static inline char *bmname(struct bitmap *bitmap)
 {
 	return bitmap->mddev ? mdname(bitmap->mddev) : "mdX";
 }
 
-
 /*
  * just a placeholder - calls kmalloc for bitmap pages
  */
@@ -78,7 +73,7 @@ static unsigned char *bitmap_alloc_page(struct bitmap *bitmap)
 #ifdef INJECT_FAULTS_1
 	page = NULL;
 #else
-	page = kmalloc(PAGE_SIZE, GFP_NOIO);
+	page = kzalloc(PAGE_SIZE, GFP_NOIO);
 #endif
 	if (!page)
 		printk("%s: bitmap_alloc_page FAILED\n", bmname(bitmap));
@@ -107,7 +102,8 @@ static void bitmap_free_page(struct bitmap *bitmap, unsigned char *page)
  * if we find our page, we increment the page's refcount so that it stays
  * allocated while we're using it
  */
-static int bitmap_checkpage(struct bitmap *bitmap, unsigned long page, int create)
+static int bitmap_checkpage(struct bitmap *bitmap,
+			    unsigned long page, int create)
 __releases(bitmap->lock)
 __acquires(bitmap->lock)
 {
@@ -121,7 +117,6 @@ __acquires(bitmap->lock)
 		return -EINVAL;
 	}
 
-
 	if (bitmap->bp[page].hijacked) /* it's hijacked, don't try to alloc */
 		return 0;
 
@@ -131,43 +126,34 @@ __acquires(bitmap->lock)
 	if (!create)
 		return -ENOENT;
 
-	spin_unlock_irq(&bitmap->lock);
-
 	/* this page has not been allocated yet */
 
-	if ((mappage = bitmap_alloc_page(bitmap)) == NULL) {
+	spin_unlock_irq(&bitmap->lock);
+	mappage = bitmap_alloc_page(bitmap);
+	spin_lock_irq(&bitmap->lock);
+
+	if (mappage == NULL) {
 		PRINTK("%s: bitmap map page allocation failed, hijacking\n",
 			bmname(bitmap));
 		/* failed - set the hijacked flag so that we can use the
 		 * pointer as a counter */
-		spin_lock_irq(&bitmap->lock);
 		if (!bitmap->bp[page].map)
 			bitmap->bp[page].hijacked = 1;
-		goto out;
-	}
-
-	/* got a page */
-
-	spin_lock_irq(&bitmap->lock);
-
-	/* recheck the page */
-
-	if (bitmap->bp[page].map || bitmap->bp[page].hijacked) {
+	} else if (bitmap->bp[page].map ||
+		   bitmap->bp[page].hijacked) {
 		/* somebody beat us to getting the page */
 		bitmap_free_page(bitmap, mappage);
 		return 0;
-	}
+	} else {
 
-	/* no page was in place and we have one, so install it */
+		/* no page was in place and we have one, so install it */
 
-	memset(mappage, 0, PAGE_SIZE);
-	bitmap->bp[page].map = mappage;
-	bitmap->missing_pages--;
-out:
+		bitmap->bp[page].map = mappage;
+		bitmap->missing_pages--;
+	}
 	return 0;
 }
 
-
 /* if page is completely empty, put it back on the free list, or dealloc it */
 /* if page was hijacked, unmark the flag so it might get alloced next time */
 /* Note: lock should be held when calling this */
@@ -183,26 +169,15 @@ static void bitmap_checkfree(struct bitmap *bitmap, unsigned long page)
 	if (bitmap->bp[page].hijacked) { /* page was hijacked, undo this now */
 		bitmap->bp[page].hijacked = 0;
 		bitmap->bp[page].map = NULL;
-		return;
+	} else {
+		/* normal case, free the page */
+		ptr = bitmap->bp[page].map;
+		bitmap->bp[page].map = NULL;
+		bitmap->missing_pages++;
+		bitmap_free_page(bitmap, ptr);
 	}
-
-	/* normal case, free the page */
-
-#if 0
-/* actually ... let's not.  We will probably need the page again exactly when
- * memory is tight and we are flusing to disk
- */
-	return;
-#else
-	ptr = bitmap->bp[page].map;
-	bitmap->bp[page].map = NULL;
-	bitmap->missing_pages++;
-	bitmap_free_page(bitmap, ptr);
-	return;
-#endif
 }
 
-
 /*
  * bitmap file handling - read and write the bitmap file and its superblock
  */
@@ -220,11 +195,14 @@ static struct page *read_sb_page(mddev_t *mddev, loff_t offset,
 
 	mdk_rdev_t *rdev;
 	sector_t target;
+	int did_alloc = 0;
 
-	if (!page)
+	if (!page) {
 		page = alloc_page(GFP_KERNEL);
-	if (!page)
-		return ERR_PTR(-ENOMEM);
+		if (!page)
+			return ERR_PTR(-ENOMEM);
+		did_alloc = 1;
+	}
 
 	list_for_each_entry(rdev, &mddev->disks, same_set) {
 		if (! test_bit(In_sync, &rdev->flags)
@@ -242,6 +220,8 @@ static struct page *read_sb_page(mddev_t *mddev, loff_t offset,
 			return page;
 		}
 	}
+	if (did_alloc)
+		put_page(page);
 	return ERR_PTR(-EIO);
 
 }
@@ -286,49 +266,51 @@ static int write_sb_page(struct bitmap *bitmap, struct page *page, int wait)
 	mddev_t *mddev = bitmap->mddev;
 
 	while ((rdev = next_active_rdev(rdev, mddev)) != NULL) {
-			int size = PAGE_SIZE;
-			loff_t offset = mddev->bitmap_info.offset;
-			if (page->index == bitmap->file_pages-1)
-				size = roundup(bitmap->last_page_size,
-					       bdev_logical_block_size(rdev->bdev));
-			/* Just make sure we aren't corrupting data or
-			 * metadata
-			 */
-			if (mddev->external) {
-				/* Bitmap could be anywhere. */
-				if (rdev->sb_start + offset + (page->index *(PAGE_SIZE/512)) >
-				    rdev->data_offset &&
-				    rdev->sb_start + offset < 
-				    rdev->data_offset + mddev->dev_sectors +
-				    (PAGE_SIZE/512))
-					goto bad_alignment;
-			} else if (offset < 0) {
-				/* DATA  BITMAP METADATA  */
-				if (offset
-				    + (long)(page->index * (PAGE_SIZE/512))
-				    + size/512 > 0)
-					/* bitmap runs in to metadata */
-					goto bad_alignment;
-				if (rdev->data_offset + mddev->dev_sectors
-				    > rdev->sb_start + offset)
-					/* data runs in to bitmap */
-					goto bad_alignment;
-			} else if (rdev->sb_start < rdev->data_offset) {
-				/* METADATA BITMAP DATA */
-				if (rdev->sb_start
-				    + offset
-				    + page->index*(PAGE_SIZE/512) + size/512
-				    > rdev->data_offset)
-					/* bitmap runs in to data */
-					goto bad_alignment;
-			} else {
-				/* DATA METADATA BITMAP - no problems */
-			}
-			md_super_write(mddev, rdev,
-				       rdev->sb_start + offset
-				       + page->index * (PAGE_SIZE/512),
-				       size,
-				       page);
+		int size = PAGE_SIZE;
+		loff_t offset = mddev->bitmap_info.offset;
+		if (page->index == bitmap->file_pages-1)
+			size = roundup(bitmap->last_page_size,
+				       bdev_logical_block_size(rdev->bdev));
+		/* Just make sure we aren't corrupting data or
+		 * metadata
+		 */
+		if (mddev->external) {
+			/* Bitmap could be anywhere. */
+			if (rdev->sb_start + offset + (page->index
+						       * (PAGE_SIZE/512))
+			    > rdev->data_offset
+			    &&
+			    rdev->sb_start + offset
+			    < (rdev->data_offset + mddev->dev_sectors
+			     + (PAGE_SIZE/512)))
+				goto bad_alignment;
+		} else if (offset < 0) {
+			/* DATA  BITMAP METADATA  */
+			if (offset
+			    + (long)(page->index * (PAGE_SIZE/512))
+			    + size/512 > 0)
+				/* bitmap runs in to metadata */
+				goto bad_alignment;
+			if (rdev->data_offset + mddev->dev_sectors
+			    > rdev->sb_start + offset)
+				/* data runs in to bitmap */
+				goto bad_alignment;
+		} else if (rdev->sb_start < rdev->data_offset) {
+			/* METADATA BITMAP DATA */
+			if (rdev->sb_start
+			    + offset
+			    + page->index*(PAGE_SIZE/512) + size/512
+			    > rdev->data_offset)
+				/* bitmap runs in to data */
+				goto bad_alignment;
+		} else {
+			/* DATA METADATA BITMAP - no problems */
+		}
+		md_super_write(mddev, rdev,
+			       rdev->sb_start + offset
+			       + page->index * (PAGE_SIZE/512),
+			       size,
+			       page);
 	}
 
 	if (wait)
@@ -364,10 +346,9 @@ static void write_page(struct bitmap *bitmap, struct page *page, int wait)
 			bh = bh->b_this_page;
 		}
 
-		if (wait) {
+		if (wait)
 			wait_event(bitmap->write_wait,
 				   atomic_read(&bitmap->pending_writes)==0);
-		}
 	}
 	if (bitmap->flags & BITMAP_WRITE_ERROR)
 		bitmap_file_kick(bitmap);
@@ -424,7 +405,7 @@ static struct page *read_page(struct file *file, unsigned long index,
 	struct buffer_head *bh;
 	sector_t block;
 
-	PRINTK("read bitmap file (%dB @ %Lu)\n", (int)PAGE_SIZE,
+	PRINTK("read bitmap file (%dB @ %llu)\n", (int)PAGE_SIZE,
 			(unsigned long long)index << PAGE_SHIFT);
 
 	page = alloc_page(GFP_KERNEL);
@@ -478,7 +459,7 @@ static struct page *read_page(struct file *file, unsigned long index,
 	}
 out:
 	if (IS_ERR(page))
-		printk(KERN_ALERT "md: bitmap read error: (%dB @ %Lu): %ld\n",
+		printk(KERN_ALERT "md: bitmap read error: (%dB @ %llu): %ld\n",
 			(int)PAGE_SIZE,
 			(unsigned long long)index << PAGE_SHIFT,
 			PTR_ERR(page));
@@ -664,11 +645,14 @@ static int bitmap_mask_state(struct bitmap *bitmap, enum bitmap_state bits,
 	sb = kmap_atomic(bitmap->sb_page, KM_USER0);
 	old = le32_to_cpu(sb->state) & bits;
 	switch (op) {
-		case MASK_SET: sb->state |= cpu_to_le32(bits);
-				break;
-		case MASK_UNSET: sb->state &= cpu_to_le32(~bits);
-				break;
-		default: BUG();
+	case MASK_SET:
+		sb->state |= cpu_to_le32(bits);
+		break;
+	case MASK_UNSET:
+		sb->state &= cpu_to_le32(~bits);
+		break;
+	default:
+		BUG();
 	}
 	kunmap_atomic(sb, KM_USER0);
 	return old;
@@ -710,12 +694,12 @@ static inline unsigned long file_page_offset(struct bitmap *bitmap, unsigned lon
 static inline struct page *filemap_get_page(struct bitmap *bitmap,
 					unsigned long chunk)
 {
-	if (file_page_index(bitmap, chunk) >= bitmap->file_pages) return NULL;
+	if (file_page_index(bitmap, chunk) >= bitmap->file_pages)
+		return NULL;
 	return bitmap->filemap[file_page_index(bitmap, chunk)
 			       - file_page_index(bitmap, 0)];
 }
 
-
 static void bitmap_file_unmap(struct bitmap *bitmap)
 {
 	struct page **map, *sb_page;
@@ -766,7 +750,6 @@ static void bitmap_file_put(struct bitmap *bitmap)
 	}
 }
 
-
 /*
  * bitmap_file_kick - if an error occurs while manipulating the bitmap file
  * then it is no longer reliable, so we stop using it and we mark the file
@@ -785,7 +768,6 @@ static void bitmap_file_kick(struct bitmap *bitmap)
 				ptr = d_path(&bitmap->file->f_path, path,
 					     PAGE_SIZE);
 
-
 			printk(KERN_ALERT
 			      "%s: kicking failed bitmap file %s from array!\n",
 			      bmname(bitmap), IS_ERR(ptr) ? "" : ptr);
@@ -803,9 +785,9 @@ static void bitmap_file_kick(struct bitmap *bitmap)
 }
 
 enum bitmap_page_attr {
-	BITMAP_PAGE_DIRTY = 0, // there are set bits that need to be synced
-	BITMAP_PAGE_CLEAN = 1, // there are bits that might need to be cleared
-	BITMAP_PAGE_NEEDWRITE=2, // there are cleared bits that need to be synced
+	BITMAP_PAGE_DIRTY = 0,     /* there are set bits that need to be synced */
+	BITMAP_PAGE_CLEAN = 1,     /* there are bits that might need to be cleared */
+	BITMAP_PAGE_NEEDWRITE = 2, /* there are cleared bits that need to be synced */
 };
 
 static inline void set_page_attr(struct bitmap *bitmap, struct page *page,
@@ -840,15 +822,15 @@ static void bitmap_file_set_bit(struct bitmap *bitmap, sector_t block)
 	void *kaddr;
 	unsigned long chunk = block >> CHUNK_BLOCK_SHIFT(bitmap);
 
-	if (!bitmap->filemap) {
+	if (!bitmap->filemap)
 		return;
-	}
 
 	page = filemap_get_page(bitmap, chunk);
-	if (!page) return;
+	if (!page)
+		return;
 	bit = file_page_offset(bitmap, chunk);
 
- 	/* set the bit */
+	/* set the bit */
 	kaddr = kmap_atomic(page, KM_USER0);
 	if (bitmap->flags & BITMAP_HOSTENDIAN)
 		set_bit(bit, kaddr);
@@ -859,7 +841,6 @@ static void bitmap_file_set_bit(struct bitmap *bitmap, sector_t block)
 
 	/* record page number so it gets flushed to disk when unplug occurs */
 	set_page_attr(bitmap, page, BITMAP_PAGE_DIRTY);
-
 }
 
 /* this gets called when the md device is ready to unplug its underlying
@@ -892,7 +873,7 @@ void bitmap_unplug(struct bitmap *bitmap)
 			wait = 1;
 		spin_unlock_irqrestore(&bitmap->lock, flags);
 
-		if (dirty | need_write)
+		if (dirty || need_write)
 			write_page(bitmap, page, 0);
 	}
 	if (wait) { /* if any writes were performed, we need to wait on them */
@@ -905,6 +886,7 @@ void bitmap_unplug(struct bitmap *bitmap)
 	if (bitmap->flags & BITMAP_WRITE_ERROR)
 		bitmap_file_kick(bitmap);
 }
+EXPORT_SYMBOL(bitmap_unplug);
 
 static void bitmap_set_memory_bits(struct bitmap *bitmap, sector_t offset, int needed);
 /* * bitmap_init_from_disk -- called at bitmap_create time to initialize
@@ -947,7 +929,6 @@ static int bitmap_init_from_disk(struct bitmap *bitmap, sector_t start)
 	if (!bitmap->mddev->bitmap_info.external)
 		bytes += sizeof(bitmap_super_t);
 
-	
 	num_pages = (bytes + PAGE_SIZE - 1) / PAGE_SIZE;
 
 	if (file && i_size_read(file->f_mapping->host) < bytes) {
@@ -966,7 +947,7 @@ static int bitmap_init_from_disk(struct bitmap *bitmap, sector_t start)
 
 	/* We need 4 bits per page, rounded up to a multiple of sizeof(unsigned long) */
 	bitmap->filemap_attr = kzalloc(
-		roundup( DIV_ROUND_UP(num_pages*4, 8), sizeof(unsigned long)),
+		roundup(DIV_ROUND_UP(num_pages*4, 8), sizeof(unsigned long)),
 		GFP_KERNEL);
 	if (!bitmap->filemap_attr)
 		goto err;
@@ -1021,7 +1002,7 @@ static int bitmap_init_from_disk(struct bitmap *bitmap, sector_t start)
 			if (outofdate) {
 				/*
 				 * if bitmap is out of date, dirty the
-			 	 * whole page and write it out
+				 * whole page and write it out
 				 */
 				paddr = kmap_atomic(page, KM_USER0);
 				memset(paddr + offset, 0xff,
@@ -1052,7 +1033,7 @@ static int bitmap_init_from_disk(struct bitmap *bitmap, sector_t start)
 		}
 	}
 
- 	/* everything went OK */
+	/* everything went OK */
 	ret = 0;
 	bitmap_mask_state(bitmap, BITMAP_STALE, MASK_UNSET);
 
@@ -1080,21 +1061,16 @@ void bitmap_write_all(struct bitmap *bitmap)
 	 */
 	int i;
 
-	for (i=0; i < bitmap->file_pages; i++)
+	for (i = 0; i < bitmap->file_pages; i++)
 		set_page_attr(bitmap, bitmap->filemap[i],
 			      BITMAP_PAGE_NEEDWRITE);
 }
 
-
 static void bitmap_count_page(struct bitmap *bitmap, sector_t offset, int inc)
 {
 	sector_t chunk = offset >> CHUNK_BLOCK_SHIFT(bitmap);
 	unsigned long page = chunk >> PAGE_COUNTER_SHIFT;
 	bitmap->bp[page].count += inc;
-/*
-	if (page == 0) printk("count page 0, offset %llu: %d gives %d\n",
-			      (unsigned long long)offset, inc, bitmap->bp[page].count);
-*/
 	bitmap_checkfree(bitmap, page);
 }
 static bitmap_counter_t *bitmap_get_counter(struct bitmap *bitmap,
@@ -1197,14 +1173,11 @@ void bitmap_daemon_work(mddev_t *mddev)
 					 (sector_t)j << CHUNK_BLOCK_SHIFT(bitmap),
 					 &blocks, 0);
 		if (bmc) {
-/*
-  if (j < 100) printk("bitmap: j=%lu, *bmc = 0x%x\n", j, *bmc);
-*/
 			if (*bmc)
 				bitmap->allclean = 0;
 
 			if (*bmc == 2) {
-				*bmc=1; /* maybe clear the bit next time */
+				*bmc = 1; /* maybe clear the bit next time */
 				set_page_attr(bitmap, page, BITMAP_PAGE_CLEAN);
 			} else if (*bmc == 1 && !bitmap->need_sync) {
 				/* we can clear the bit */
@@ -1243,7 +1216,7 @@ void bitmap_daemon_work(mddev_t *mddev)
 
  done:
 	if (bitmap->allclean == 0)
-		bitmap->mddev->thread->timeout = 
+		bitmap->mddev->thread->timeout =
 			bitmap->mddev->bitmap_info.daemon_sleep;
 	mutex_unlock(&mddev->bitmap_info.mutex);
 }
@@ -1265,7 +1238,7 @@ __acquires(bitmap->lock)
 
 	if (bitmap_checkpage(bitmap, page, create) < 0) {
 		csize = ((sector_t)1) << (CHUNK_BLOCK_SHIFT(bitmap));
-		*blocks = csize - (offset & (csize- 1));
+		*blocks = csize - (offset & (csize - 1));
 		return NULL;
 	}
 	/* now locked ... */
@@ -1276,12 +1249,12 @@ __acquires(bitmap->lock)
 		int hi = (pageoff > PAGE_COUNTER_MASK);
 		csize = ((sector_t)1) << (CHUNK_BLOCK_SHIFT(bitmap) +
 					  PAGE_COUNTER_SHIFT - 1);
-		*blocks = csize - (offset & (csize- 1));
+		*blocks = csize - (offset & (csize - 1));
 		return  &((bitmap_counter_t *)
 			  &bitmap->bp[page].map)[hi];
 	} else { /* page is allocated */
 		csize = ((sector_t)1) << (CHUNK_BLOCK_SHIFT(bitmap));
-		*blocks = csize - (offset & (csize- 1));
+		*blocks = csize - (offset & (csize - 1));
 		return (bitmap_counter_t *)
 			&(bitmap->bp[page].map[pageoff]);
 	}
@@ -1289,7 +1262,8 @@ __acquires(bitmap->lock)
 
 int bitmap_startwrite(struct bitmap *bitmap, sector_t offset, unsigned long sectors, int behind)
 {
-	if (!bitmap) return 0;
+	if (!bitmap)
+		return 0;
 
 	if (behind) {
 		int bw;
@@ -1328,10 +1302,10 @@ int bitmap_startwrite(struct bitmap *bitmap, sector_t offset, unsigned long sect
 			continue;
 		}
 
-		switch(*bmc) {
+		switch (*bmc) {
 		case 0:
 			bitmap_file_set_bit(bitmap, offset);
-			bitmap_count_page(bitmap,offset, 1);
+			bitmap_count_page(bitmap, offset, 1);
 			blk_plug_device_unlocked(bitmap->mddev->queue);
 			/* fall through */
 		case 1:
@@ -1345,16 +1319,19 @@ int bitmap_startwrite(struct bitmap *bitmap, sector_t offset, unsigned long sect
 		offset += blocks;
 		if (sectors > blocks)
 			sectors -= blocks;
-		else sectors = 0;
+		else
+			sectors = 0;
 	}
 	bitmap->allclean = 0;
 	return 0;
 }
+EXPORT_SYMBOL(bitmap_startwrite);
 
 void bitmap_endwrite(struct bitmap *bitmap, sector_t offset, unsigned long sectors,
 		     int success, int behind)
 {
-	if (!bitmap) return;
+	if (!bitmap)
+		return;
 	if (behind) {
 		if (atomic_dec_and_test(&bitmap->behind_writes))
 			wake_up(&bitmap->behind_wait);
@@ -1391,18 +1368,20 @@ void bitmap_endwrite(struct bitmap *bitmap, sector_t offset, unsigned long secto
 			wake_up(&bitmap->overflow_wait);
 
 		(*bmc)--;
-		if (*bmc <= 2) {
+		if (*bmc <= 2)
 			set_page_attr(bitmap,
 				      filemap_get_page(bitmap, offset >> CHUNK_BLOCK_SHIFT(bitmap)),
 				      BITMAP_PAGE_CLEAN);
-		}
+
 		spin_unlock_irqrestore(&bitmap->lock, flags);
 		offset += blocks;
 		if (sectors > blocks)
 			sectors -= blocks;
-		else sectors = 0;
+		else
+			sectors = 0;
 	}
 }
+EXPORT_SYMBOL(bitmap_endwrite);
 
 static int __bitmap_start_sync(struct bitmap *bitmap, sector_t offset, int *blocks,
 			       int degraded)
@@ -1455,14 +1434,14 @@ int bitmap_start_sync(struct bitmap *bitmap, sector_t offset, int *blocks,
 	}
 	return rv;
 }
+EXPORT_SYMBOL(bitmap_start_sync);
 
 void bitmap_end_sync(struct bitmap *bitmap, sector_t offset, int *blocks, int aborted)
 {
 	bitmap_counter_t *bmc;
 	unsigned long flags;
-/*
-	if (offset == 0) printk("bitmap_end_sync 0 (%d)\n", aborted);
-*/	if (bitmap == NULL) {
+
+	if (bitmap == NULL) {
 		*blocks = 1024;
 		return;
 	}
@@ -1471,26 +1450,23 @@ void bitmap_end_sync(struct bitmap *bitmap, sector_t offset, int *blocks, int ab
 	if (bmc == NULL)
 		goto unlock;
 	/* locked */
-/*
-	if (offset == 0) printk("bitmap_end sync found 0x%x, blocks %d\n", *bmc, *blocks);
-*/
 	if (RESYNC(*bmc)) {
 		*bmc &= ~RESYNC_MASK;
 
 		if (!NEEDED(*bmc) && aborted)
 			*bmc |= NEEDED_MASK;
 		else {
-			if (*bmc <= 2) {
+			if (*bmc <= 2)
 				set_page_attr(bitmap,
 					      filemap_get_page(bitmap, offset >> CHUNK_BLOCK_SHIFT(bitmap)),
 					      BITMAP_PAGE_CLEAN);
-			}
 		}
 	}
  unlock:
 	spin_unlock_irqrestore(&bitmap->lock, flags);
 	bitmap->allclean = 0;
 }
+EXPORT_SYMBOL(bitmap_end_sync);
 
 void bitmap_close_sync(struct bitmap *bitmap)
 {
@@ -1507,6 +1483,7 @@ void bitmap_close_sync(struct bitmap *bitmap)
 		sector += blocks;
 	}
 }
+EXPORT_SYMBOL(bitmap_close_sync);
 
 void bitmap_cond_end_sync(struct bitmap *bitmap, sector_t sector)
 {
@@ -1537,6 +1514,7 @@ void bitmap_cond_end_sync(struct bitmap *bitmap, sector_t sector)
 	bitmap->last_end_sync = jiffies;
 	sysfs_notify(&bitmap->mddev->kobj, NULL, "sync_completed");
 }
+EXPORT_SYMBOL(bitmap_cond_end_sync);
 
 static void bitmap_set_memory_bits(struct bitmap *bitmap, sector_t offset, int needed)
 {
@@ -1553,9 +1531,9 @@ static void bitmap_set_memory_bits(struct bitmap *bitmap, sector_t offset, int n
 		spin_unlock_irq(&bitmap->lock);
 		return;
 	}
-	if (! *bmc) {
+	if (!*bmc) {
 		struct page *page;
-		*bmc = 1 | (needed?NEEDED_MASK:0);
+		*bmc = 1 | (needed ? NEEDED_MASK : 0);
 		bitmap_count_page(bitmap, offset, 1);
 		page = filemap_get_page(bitmap, offset >> CHUNK_BLOCK_SHIFT(bitmap));
 		set_page_attr(bitmap, page, BITMAP_PAGE_CLEAN);
@@ -1720,9 +1698,9 @@ int bitmap_create(mddev_t *mddev)
 	bitmap->chunkshift = ffz(~mddev->bitmap_info.chunksize);
 
 	/* now that chunksize and chunkshift are set, we can use these macros */
- 	chunks = (blocks + CHUNK_BLOCK_RATIO(bitmap) - 1) >>
+	chunks = (blocks + CHUNK_BLOCK_RATIO(bitmap) - 1) >>
 			CHUNK_BLOCK_SHIFT(bitmap);
- 	pages = (chunks + PAGE_COUNTER_RATIO - 1) / PAGE_COUNTER_RATIO;
+	pages = (chunks + PAGE_COUNTER_RATIO - 1) / PAGE_COUNTER_RATIO;
 
 	BUG_ON(!pages);
 
@@ -1775,11 +1753,11 @@ static ssize_t
 location_show(mddev_t *mddev, char *page)
 {
 	ssize_t len;
-	if (mddev->bitmap_info.file) {
+	if (mddev->bitmap_info.file)
 		len = sprintf(page, "file");
-	} else if (mddev->bitmap_info.offset) {
+	else if (mddev->bitmap_info.offset)
 		len = sprintf(page, "%+lld", (long long)mddev->bitmap_info.offset);
-	} else
+	else
 		len = sprintf(page, "none");
 	len += sprintf(page+len, "\n");
 	return len;
@@ -1868,7 +1846,7 @@ timeout_show(mddev_t *mddev, char *page)
 	ssize_t len;
 	unsigned long secs = mddev->bitmap_info.daemon_sleep / HZ;
 	unsigned long jifs = mddev->bitmap_info.daemon_sleep % HZ;
-	
+
 	len = sprintf(page, "%lu", secs);
 	if (jifs)
 		len += sprintf(page+len, ".%03u", jiffies_to_msecs(jifs));
@@ -2050,12 +2028,3 @@ struct attribute_group md_bitmap_group = {
 	.attrs = md_bitmap_attrs,
 };
 
-
-/* the bitmap API -- for raid personalities */
-EXPORT_SYMBOL(bitmap_startwrite);
-EXPORT_SYMBOL(bitmap_endwrite);
-EXPORT_SYMBOL(bitmap_start_sync);
-EXPORT_SYMBOL(bitmap_end_sync);
-EXPORT_SYMBOL(bitmap_unplug);
-EXPORT_SYMBOL(bitmap_close_sync);
-EXPORT_SYMBOL(bitmap_cond_end_sync);
author
#18 lore raw

PATCH 18/24 md/bitmap: reduce dependence on sysfs.

In reply to NeilBrown

For dm-raid45 we will want to use bitmaps in dm-targets which don't have entries in sysfs, so cope with the mddev not living in sysfs.

---
 drivers/md/bitmap.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
drivers/md/bitmap.c +4 -3
index 67fb32d..8af4d65 100644
@@ -1358,7 +1358,7 @@ void bitmap_endwrite(struct bitmap *bitmap, sector_t offset, unsigned long secto
 		    bitmap->events_cleared < bitmap->mddev->events) {
 			bitmap->events_cleared = bitmap->mddev->events;
 			bitmap->need_sync = 1;
-			sysfs_notify_dirent(bitmap->sysfs_can_clear);
+			sysfs_notify_dirent_safe(bitmap->sysfs_can_clear);
 		}
 
 		if (!success && ! (*bmc & NEEDED_MASK))
@@ -1643,7 +1643,7 @@ int bitmap_create(mddev_t *mddev)
 	struct file *file = mddev->bitmap_info.file;
 	int err;
 	sector_t start;
-	struct sysfs_dirent *bm;
+	struct sysfs_dirent *bm = NULL;
 
 	BUILD_BUG_ON(sizeof(bitmap_super_t) != 256);
 
@@ -1664,7 +1664,8 @@ int bitmap_create(mddev_t *mddev)
 
 	bitmap->mddev = mddev;
 
-	bm = sysfs_get_dirent(mddev->kobj.sd, NULL, "bitmap");
+	if (mddev->kobj.sd)
+		bm = sysfs_get_dirent(mddev->kobj.sd, NULL, "bitmap");
 	if (bm) {
 		bitmap->sysfs_can_clear = sysfs_get_dirent(bm, NULL, "can_clear");
 		sysfs_put(bm);
author
#19 lore raw

PATCH 19/24 md/bitmap: clean up plugging calls.

In reply to NeilBrown

1/ use md_unplug in bitmap.c as we will soon be using bitmaps under
arrays with no queue attached.

2/ Don't bother plugging the queue when we set a bit in the bitmap.
The reason for this was to encourage as many bits as possible to get set before we unplug and write stuff out.
However every personality already plugs the queue after bitmap_startwrite either directly (raid1/raid10) or be setting STRIPE_BIT_DELAY which causes the queue to be plugged later (raid5).

---
 drivers/md/bitmap.c |    3 +--
 drivers/md/md.c     |    2 +-
 drivers/md/md.h     |    1 +
 3 files changed, 3 insertions(+), 3 deletions(-)
drivers/md/bitmap.c +1 -2
index 8af4d65..3f04699 100644
@@ -1296,7 +1296,7 @@ int bitmap_startwrite(struct bitmap *bitmap, sector_t offset, unsigned long sect
 			prepare_to_wait(&bitmap->overflow_wait, &__wait,
 					TASK_UNINTERRUPTIBLE);
 			spin_unlock_irq(&bitmap->lock);
-			blk_unplug(bitmap->mddev->queue);
+			md_unplug(bitmap->mddev);
 			schedule();
 			finish_wait(&bitmap->overflow_wait, &__wait);
 			continue;
@@ -1306,7 +1306,6 @@ int bitmap_startwrite(struct bitmap *bitmap, sector_t offset, unsigned long sect
 		case 0:
 			bitmap_file_set_bit(bitmap, offset);
 			bitmap_count_page(bitmap, offset, 1);
-			blk_plug_device_unlocked(bitmap->mddev->queue);
 			/* fall through */
 		case 1:
 			*bmc = 2;
drivers/md/md.c +1 -1
index 21f3c35..f24efd2 100644
@@ -6600,7 +6600,7 @@ int md_allow_write(mddev_t *mddev)
 }
 EXPORT_SYMBOL_GPL(md_allow_write);
 
-static void md_unplug(mddev_t *mddev)
+void md_unplug(mddev_t *mddev)
 {
 	if (mddev->queue)
 		blk_unplug(mddev->queue);
drivers/md/md.h +1 -0
index 6f2c568..a97e4b0 100644
@@ -506,6 +506,7 @@ extern int md_integrity_register(mddev_t *mddev);
 extern void md_integrity_add_rdev(mdk_rdev_t *rdev, mddev_t *mddev);
 extern int strict_strtoul_scaled(const char *cp, unsigned long *res, int scale);
 extern void restore_bitmap_write_access(struct file *file);
+extern void md_unplug(mddev_t *mddev);
 
 extern void mddev_init(mddev_t *mddev);
 extern int md_run(mddev_t *mddev);
author
#20 lore raw

PATCH 20/24 md/bitmap: optimise scanning of empty bitmaps.

In reply to NeilBrown

A bitmap is stored as one page per 2048 bits. If none of the bits are set, the page is not allocated.

When bitmap_get_counter finds that a page isn't allocate, it just reports that one bit work of space isn't flagged, rather than reporting that 2048 bits worth of space are unflagged.
This can cause searches for flagged bits (e.g. bitmap_close_sync) to do more work than is really necessary.

So change bitmap_get_counter (when creating) to report a number of blocks that more accurately reports the range of the device for which no counter currently exists.

---
 drivers/md/bitmap.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)
drivers/md/bitmap.c +13 -10
index 3f04699..29a3c86 100644
@@ -1235,29 +1235,32 @@ __acquires(bitmap->lock)
 	unsigned long page = chunk >> PAGE_COUNTER_SHIFT;
 	unsigned long pageoff = (chunk & PAGE_COUNTER_MASK) << COUNTER_BYTE_SHIFT;
 	sector_t csize;
+	int err;
+
+	err = bitmap_checkpage(bitmap, page, create);
 
-	if (bitmap_checkpage(bitmap, page, create) < 0) {
+	if (bitmap->bp[page].hijacked ||
+	    bitmap->bp[page].map == NULL)
+		csize = ((sector_t)1) << (CHUNK_BLOCK_SHIFT(bitmap) +
+					  PAGE_COUNTER_SHIFT - 1);
+	else
 		csize = ((sector_t)1) << (CHUNK_BLOCK_SHIFT(bitmap));
-		*blocks = csize - (offset & (csize - 1));
+	*blocks = csize - (offset & (csize - 1));
+
+	if (err < 0)
 		return NULL;
-	}
+
 	/* now locked ... */
 
 	if (bitmap->bp[page].hijacked) { /* hijacked pointer */
 		/* should we use the first or second counter field
 		 * of the hijacked pointer? */
 		int hi = (pageoff > PAGE_COUNTER_MASK);
-		csize = ((sector_t)1) << (CHUNK_BLOCK_SHIFT(bitmap) +
-					  PAGE_COUNTER_SHIFT - 1);
-		*blocks = csize - (offset & (csize - 1));
 		return  &((bitmap_counter_t *)
 			  &bitmap->bp[page].map)[hi];
-	} else { /* page is allocated */
-		csize = ((sector_t)1) << (CHUNK_BLOCK_SHIFT(bitmap));
-		*blocks = csize - (offset & (csize - 1));
+	} else /* page is allocated */
 		return (bitmap_counter_t *)
 			&(bitmap->bp[page].map[pageoff]);
-	}
 }
 
 int bitmap_startwrite(struct bitmap *bitmap, sector_t offset, unsigned long sectors, int behind)
author
#21 lore raw

PATCH 21/24 dm-dirty-log: allow log size to be different from target size.

In reply to NeilBrown

With RAID1, the dirty log covers the size of the target - the number of bits is the target size divided by the region size.

For RAID5 and similar the dirty log needs to cover the parity blocks, so it is best to base the dirty log size on the size of the component devices rather than the size of the array.

So when creating a dirty log, allow the log_size to be specified separately from the target, and in raid1, set it to the target length.

---
 drivers/md/dm-log-userspace-base.c |   11 +++++++----
 drivers/md/dm-log.c                |   18 +++++++++++-------
 drivers/md/dm-raid1.c              |    4 ++--
 include/linux/dm-dirty-log.h       |    3 ++-
 4 files changed, 22 insertions(+), 14 deletions(-)
drivers/md/dm-log-userspace-base.c +7 -4
index 1ed0094..935a49b 100644
@@ -94,7 +94,7 @@ retry:
 	return -ESRCH;
 }
 
-static int build_constructor_string(struct dm_target *ti,
+static int build_constructor_string(sector_t log_size,
 				    unsigned argc, char **argv,
 				    char **ctr_str)
 {
@@ -114,7 +114,7 @@ static int build_constructor_string(struct dm_target *ti,
 		return -ENOMEM;
 	}
 
-	str_size = sprintf(str, "%llu", (unsigned long long)ti->len);
+	str_size = sprintf(str, "%llu", (unsigned long long)log_size);
 	for (i = 0; i < argc; i++)
 		str_size += sprintf(str + str_size, " %s", argv[i]);
 
@@ -136,6 +136,7 @@ static int build_constructor_string(struct dm_target *ti,
  * else.
  */
 static int userspace_ctr(struct dm_dirty_log *log, struct dm_target *ti,
+			 sector_t log_size,
 			 unsigned argc, char **argv)
 {
 	int r = 0;
@@ -171,7 +172,9 @@ static int userspace_ctr(struct dm_dirty_log *log, struct dm_target *ti,
 	spin_lock_init(&lc->flush_lock);
 	INIT_LIST_HEAD(&lc->flush_list);
 
-	str_size = build_constructor_string(ti, argc - 1, argv + 1, &ctr_str);
+	str_size = build_constructor_string(log_size,
+					    argc - 1, argv + 1,
+					    &ctr_str);
 	if (str_size < 0) {
 		kfree(lc);
 		return str_size;
@@ -197,7 +200,7 @@ static int userspace_ctr(struct dm_dirty_log *log, struct dm_target *ti,
 	}
 
 	lc->region_size = (uint32_t)rdata;
-	lc->region_count = dm_sector_div_up(ti->len, lc->region_size);
+	lc->region_count = dm_sector_div_up(log_size, lc->region_size);
 
 out:
 	if (r) {
drivers/md/dm-log.c +11 -7
index 5a08be0..a232c14 100644
@@ -146,6 +146,7 @@ EXPORT_SYMBOL(dm_dirty_log_type_unregister);
 
 struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
 			struct dm_target *ti,
+			sector_t log_size,
 			int (*flush_callback_fn)(struct dm_target *ti),
 			unsigned int argc, char **argv)
 {
@@ -164,7 +165,7 @@ struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
 
 	log->flush_callback_fn = flush_callback_fn;
 	log->type = type;
-	if (type->ctr(log, ti, argc, argv)) {
+	if (type->ctr(log, ti, log_size, argc, argv)) {
 		kfree(log);
 		put_type(type);
 		return NULL;
@@ -335,9 +336,9 @@ static int read_header(struct log_c *log)
 	return 0;
 }
 
-static int _check_region_size(struct dm_target *ti, uint32_t region_size)
+static int _check_region_size(sector_t log_size, uint32_t region_size)
 {
-	if (region_size < 2 || region_size > ti->len)
+	if (region_size < 2 || region_size > log_size)
 		return 0;
 
 	if (!is_power_of_2(region_size))
@@ -353,6 +354,7 @@ static int _check_region_size(struct dm_target *ti, uint32_t region_size)
  *--------------------------------------------------------------*/
 #define BYTE_SHIFT 3
 static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
+			      sector_t log_size,
 			      unsigned int argc, char **argv,
 			      struct dm_dev *dev)
 {
@@ -382,12 +384,12 @@ static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
 	}
 
 	if (sscanf(argv[0], "%u", &region_size) != 1 ||
-	    !_check_region_size(ti, region_size)) {
+	    !_check_region_size(log_size, region_size)) {
 		DMWARN("invalid region size %s", argv[0]);
 		return -EINVAL;
 	}
 
-	region_count = dm_sector_div_up(ti->len, region_size);
+	region_count = dm_sector_div_up(log_size, region_size);
 
 	lc = kmalloc(sizeof(*lc), GFP_KERNEL);
 	if (!lc) {
@@ -507,9 +509,10 @@ static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
 }
 
 static int core_ctr(struct dm_dirty_log *log, struct dm_target *ti,
+		    sector_t log_size,
 		    unsigned int argc, char **argv)
 {
-	return create_log_context(log, ti, argc, argv, NULL);
+	return create_log_context(log, ti, log_size, argc, argv, NULL);
 }
 
 static void destroy_log_context(struct log_c *lc)
@@ -533,6 +536,7 @@ static void core_dtr(struct dm_dirty_log *log)
  * argv contains log_device region_size followed optionally by [no]sync
  *--------------------------------------------------------------*/
 static int disk_ctr(struct dm_dirty_log *log, struct dm_target *ti,
+		    sector_t log_size,
 		    unsigned int argc, char **argv)
 {
 	int r;
@@ -547,7 +551,7 @@ static int disk_ctr(struct dm_dirty_log *log, struct dm_target *ti,
 	if (r)
 		return r;
 
-	r = create_log_context(log, ti, argc - 1, argv + 1, dev);
+	r = create_log_context(log, ti, log_size, argc - 1, argv + 1, dev);
 	if (r) {
 		dm_put_device(ti, dev);
 		return r;
drivers/md/dm-raid1.c +2 -2
index ddda531..ea732fc 100644
@@ -968,8 +968,8 @@ static struct dm_dirty_log *create_dirty_log(struct dm_target *ti,
 		return NULL;
 	}
 
-	dl = dm_dirty_log_create(argv[0], ti, mirror_flush, param_count,
-				 argv + 2);
+	dl = dm_dirty_log_create(argv[0], ti, ti->len, mirror_flush,
+				 param_count, argv + 2);
 	if (!dl) {
 		ti->error = "Error creating mirror dirty log";
 		return NULL;
include/linux/dm-dirty-log.h +2 -1
index 7084503..641419f 100644
@@ -33,6 +33,7 @@ struct dm_dirty_log_type {
 	struct list_head list;
 
 	int (*ctr)(struct dm_dirty_log *log, struct dm_target *ti,
+		   sector_t log_size,
 		   unsigned argc, char **argv);
 	void (*dtr)(struct dm_dirty_log *log);
 
@@ -137,7 +138,7 @@ int dm_dirty_log_type_unregister(struct dm_dirty_log_type *type);
  * type->constructor/destructor() directly.
  */
 struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
-			struct dm_target *ti,
+			struct dm_target *ti, sector_t log_size,
 			int (*flush_callback_fn)(struct dm_target *ti),
 			unsigned argc, char **argv);
 void dm_dirty_log_destroy(struct dm_dirty_log *log);
#22 lore raw

Re: [PATCH 21/24] dm-dirty-log: allow log size to be different from target size.

Neil,

I had a first review through your patch series, which look mostly good to me.

I've got 2 points so far before I go for tests and further review:

o there's actually no need to change the dm-dirty-log interface in an incompatible way to allow for what's needed (see patch attached on top of your series) which we can't do in RHEL/SUSE/... anyway.

Notwithstanding, we need a discussion on dm-devel to justify if we should change the API upstream in order to avoid such workaround as in my attached patched.

o any reason you limit the dm-dirty-log type to 'core' ?

Heinz

Quoted 23 lines · NeilBrown

With RAID1, the dirty log covers the size of the target - the number of bits is the target size divided by the region size.

For RAID5 and similar the dirty log needs to cover the parity blocks, so it is best to base the dirty log size on the size of the component devices rather than the size of the array.

So when creating a dirty log, allow the log_size to be specified separately from the target, and in raid1, set it to the target length.

---
 drivers/md/dm-log-userspace-base.c |   11 +++++++----
 drivers/md/dm-log.c                |   18 +++++++++++-------
 drivers/md/dm-raid1.c              |    4 ++--
 include/linux/dm-dirty-log.h       |    3 ++-
 4 files changed, 22 insertions(+), 14 deletions(-)
drivers/md/dm-log-userspace-base.c +0 -0
index 1ed0094..935a49b 100644
@@ -94,7 +94,7 @@ retry:

<SNIP>

 drivers/md/dm-log.c          |   18 +++++++-----------
 drivers/md/dm-raid1.c        |    4 ++--
 drivers/md/dm-raid456.c      |    8 +++++---
 include/linux/dm-dirty-log.h |    3 +--
 4 files changed, 15 insertions(+), 18 deletions(-)
drivers/md/dm-log.c +7 -11
index a232c14..5a08be0 100644
@@ -146,7 +146,6 @@ EXPORT_SYMBOL(dm_dirty_log_type_unregister);
 
 struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
 			struct dm_target *ti,
-			sector_t log_size,
 			int (*flush_callback_fn)(struct dm_target *ti),
 			unsigned int argc, char **argv)
 {
@@ -165,7 +164,7 @@ struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
 
 	log->flush_callback_fn = flush_callback_fn;
 	log->type = type;
-	if (type->ctr(log, ti, log_size, argc, argv)) {
+	if (type->ctr(log, ti, argc, argv)) {
 		kfree(log);
 		put_type(type);
 		return NULL;
@@ -336,9 +335,9 @@ static int read_header(struct log_c *log)
 	return 0;
 }
 
-static int _check_region_size(sector_t log_size, uint32_t region_size)
+static int _check_region_size(struct dm_target *ti, uint32_t region_size)
 {
-	if (region_size < 2 || region_size > log_size)
+	if (region_size < 2 || region_size > ti->len)
 		return 0;
 
 	if (!is_power_of_2(region_size))
@@ -354,7 +353,6 @@ static int _check_region_size(sector_t log_size, uint32_t region_size)
  *--------------------------------------------------------------*/
 #define BYTE_SHIFT 3
 static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
-			      sector_t log_size,
 			      unsigned int argc, char **argv,
 			      struct dm_dev *dev)
 {
@@ -384,12 +382,12 @@ static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
 	}
 
 	if (sscanf(argv[0], "%u", &region_size) != 1 ||
-	    !_check_region_size(log_size, region_size)) {
+	    !_check_region_size(ti, region_size)) {
 		DMWARN("invalid region size %s", argv[0]);
 		return -EINVAL;
 	}
 
-	region_count = dm_sector_div_up(log_size, region_size);
+	region_count = dm_sector_div_up(ti->len, region_size);
 
 	lc = kmalloc(sizeof(*lc), GFP_KERNEL);
 	if (!lc) {
@@ -509,10 +507,9 @@ static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
 }
 
 static int core_ctr(struct dm_dirty_log *log, struct dm_target *ti,
-		    sector_t log_size,
 		    unsigned int argc, char **argv)
 {
-	return create_log_context(log, ti, log_size, argc, argv, NULL);
+	return create_log_context(log, ti, argc, argv, NULL);
 }
 
 static void destroy_log_context(struct log_c *lc)
@@ -536,7 +533,6 @@ static void core_dtr(struct dm_dirty_log *log)
  * argv contains log_device region_size followed optionally by [no]sync
  *--------------------------------------------------------------*/
 static int disk_ctr(struct dm_dirty_log *log, struct dm_target *ti,
-		    sector_t log_size,
 		    unsigned int argc, char **argv)
 {
 	int r;
@@ -551,7 +547,7 @@ static int disk_ctr(struct dm_dirty_log *log, struct dm_target *ti,
 	if (r)
 		return r;
 
-	r = create_log_context(log, ti, log_size, argc - 1, argv + 1, dev);
+	r = create_log_context(log, ti, argc - 1, argv + 1, dev);
 	if (r) {
 		dm_put_device(ti, dev);
 		return r;
drivers/md/dm-raid1.c +2 -2
index ea732fc..ddda531 100644
@@ -968,8 +968,8 @@ static struct dm_dirty_log *create_dirty_log(struct dm_target *ti,
 		return NULL;
 	}
 
-	dl = dm_dirty_log_create(argv[0], ti, ti->len, mirror_flush,
-				 param_count, argv + 2);
+	dl = dm_dirty_log_create(argv[0], ti, mirror_flush, param_count,
+				 argv + 2);
 	if (!dl) {
 		ti->error = "Error creating mirror dirty log";
 		return NULL;
drivers/md/dm-raid456.c +5 -3
index 3dcbc4a..33d2be8 100644
@@ -192,7 +192,7 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	int recovery = 1;
 	long raid_devs;
 	long rebuildA, rebuildB;
-	sector_t sectors_per_dev, chunks;
+	sector_t sectors_per_dev, chunks, ti_len_sav;
 	struct raid_set *rs = NULL;
 	int in_sync, i;
 	struct dm_dirty_log *log = NULL;
@@ -281,8 +281,10 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	if (sector_div(chunks, chunk_size))
 		goto err;
 
-	log = dm_dirty_log_create(log_argv[0], ti, sectors_per_dev,
-				  NULL, log_cnt, log_argv+2);
+	ti_len_sav = ti->len;
+	ti->len = sectors_per_dev;
+	log = dm_dirty_log_create(log_argv[0], ti, NULL, log_cnt, log_argv+2);
+	ti->len = ti_len_sav;
 	err = "Error creating dirty log";
 	if (!log)
 		goto err;
include/linux/dm-dirty-log.h +1 -2
index 641419f..7084503 100644
@@ -33,7 +33,6 @@ struct dm_dirty_log_type {
 	struct list_head list;
 
 	int (*ctr)(struct dm_dirty_log *log, struct dm_target *ti,
-		   sector_t log_size,
 		   unsigned argc, char **argv);
 	void (*dtr)(struct dm_dirty_log *log);
 
@@ -138,7 +137,7 @@ int dm_dirty_log_type_unregister(struct dm_dirty_log_type *type);
  * type->constructor/destructor() directly.
  */
 struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
-			struct dm_target *ti, sector_t log_size,
+			struct dm_target *ti,
 			int (*flush_callback_fn)(struct dm_target *ti),
 			unsigned argc, char **argv);
 void dm_dirty_log_destroy(struct dm_dirty_log *log);
author
#23 lore raw

Re: [dm-devel] [PATCH 21/24] dm-dirty-log: allow log size to be different from target size.

[Insert obligatory grumble about Reply-to headers inserted by
mail-list software. Adding linux-raid back to the cc list...]

Quoted 10 lines · On Wed, 02 Jun 2010 16:57:21 +0200 Heinz Mauelshagen <[email protected]> wrote:

Neil,

I had a first review through your patch series, which look mostly good to me.

I've got 2 points so far before I go for tests and further review:

o there's actually no need to change the dm-dirty-log interface in an incompatible way to allow for what's needed (see patch attached on top of your series) which we can't do in RHEL/SUSE/... anyway.

Yes I know. I saw that work-around in your original patch. However this patch set isn't aimed at RHEL or SLES, it is aimed upstream. And when we do things upstream we do them *right*.
If we then want to backport them to RHEL/SLES which require ABI compatibility, then little hacks like the one you show (i.e. lying about the size of the target when creating the log) may be perfectly appropriate.

Quoted 4 lines

Notwithstanding, we need a discussion on dm-devel to justify if we should change the API upstream in order to avoid such workaround as in my attached patched.

Certainly it is appropriate to discuss this API change - that is why I highlighted it in my introduction to the patch set. Do you have an opinion on it?

Quoted 2 lines

o any reason you limit the dm-dirty-log type to 'core' ?

That is only in the earlier part of the patch set. md/raid5 can work without a dirty log at all. When it does, it's behaviour is completely analogous to working with a 'core' dirty log. So to get the raid5 part working without needing to worry about the dirty-log stuff, I told md/raid5 not to use a dirty log, and pretended it was using a 'core' log.

Then in later patches after I had enabled md/bitmap to work with dm-log, I switched dm-raid45 over to use a real dm-log and removed the restriction to only using a core log.
So this is really just an artefact of the order in which I developed the code. I could go back and re-arrange it so the dm-log integration comes first, then I wouldn't need this intermediate stage which only supports 'core'.

Thanks,
NeilBrown

Quoted 189 lines

Heinz

Quoted 23 lines · NeilBrown

With RAID1, the dirty log covers the size of the target - the number of bits is the target size divided by the region size.

For RAID5 and similar the dirty log needs to cover the parity blocks, so it is best to base the dirty log size on the size of the component devices rather than the size of the array.

So when creating a dirty log, allow the log_size to be specified separately from the target, and in raid1, set it to the target length.

---
 drivers/md/dm-log-userspace-base.c |   11 +++++++----
 drivers/md/dm-log.c                |   18 +++++++++++-------
 drivers/md/dm-raid1.c              |    4 ++--
 include/linux/dm-dirty-log.h       |    3 ++-
 4 files changed, 22 insertions(+), 14 deletions(-)
drivers/md/dm-log-userspace-base.c +0 -0
index 1ed0094..935a49b 100644
@@ -94,7 +94,7 @@ retry:

<SNIP>

 drivers/md/dm-log.c          |   18 +++++++-----------
 drivers/md/dm-raid1.c        |    4 ++--
 drivers/md/dm-raid456.c      |    8 +++++---
 include/linux/dm-dirty-log.h |    3 +--
 4 files changed, 15 insertions(+), 18 deletions(-)
drivers/md/dm-log.c +7 -11
index a232c14..5a08be0 100644
@@ -146,7 +146,6 @@ EXPORT_SYMBOL(dm_dirty_log_type_unregister);
 
 struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
 			struct dm_target *ti,
-			sector_t log_size,
 			int (*flush_callback_fn)(struct dm_target *ti),
 			unsigned int argc, char **argv)
 {
@@ -165,7 +164,7 @@ struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
 
 	log->flush_callback_fn = flush_callback_fn;
 	log->type = type;
-	if (type->ctr(log, ti, log_size, argc, argv)) {
+	if (type->ctr(log, ti, argc, argv)) {
 		kfree(log);
 		put_type(type);
 		return NULL;
@@ -336,9 +335,9 @@ static int read_header(struct log_c *log)
 	return 0;
 }
 
-static int _check_region_size(sector_t log_size, uint32_t region_size)
+static int _check_region_size(struct dm_target *ti, uint32_t region_size)
 {
-	if (region_size < 2 || region_size > log_size)
+	if (region_size < 2 || region_size > ti->len)
 		return 0;
 
 	if (!is_power_of_2(region_size))
@@ -354,7 +353,6 @@ static int _check_region_size(sector_t log_size, uint32_t region_size)
  *--------------------------------------------------------------*/
 #define BYTE_SHIFT 3
 static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
-			      sector_t log_size,
 			      unsigned int argc, char **argv,
 			      struct dm_dev *dev)
 {
@@ -384,12 +382,12 @@ static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
 	}
 
 	if (sscanf(argv[0], "%u", &region_size) != 1 ||
-	    !_check_region_size(log_size, region_size)) {
+	    !_check_region_size(ti, region_size)) {
 		DMWARN("invalid region size %s", argv[0]);
 		return -EINVAL;
 	}
 
-	region_count = dm_sector_div_up(log_size, region_size);
+	region_count = dm_sector_div_up(ti->len, region_size);
 
 	lc = kmalloc(sizeof(*lc), GFP_KERNEL);
 	if (!lc) {
@@ -509,10 +507,9 @@ static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
 }
 
 static int core_ctr(struct dm_dirty_log *log, struct dm_target *ti,
-		    sector_t log_size,
 		    unsigned int argc, char **argv)
 {
-	return create_log_context(log, ti, log_size, argc, argv, NULL);
+	return create_log_context(log, ti, argc, argv, NULL);
 }
 
 static void destroy_log_context(struct log_c *lc)
@@ -536,7 +533,6 @@ static void core_dtr(struct dm_dirty_log *log)
  * argv contains log_device region_size followed optionally by [no]sync
  *--------------------------------------------------------------*/
 static int disk_ctr(struct dm_dirty_log *log, struct dm_target *ti,
-		    sector_t log_size,
 		    unsigned int argc, char **argv)
 {
 	int r;
@@ -551,7 +547,7 @@ static int disk_ctr(struct dm_dirty_log *log, struct dm_target *ti,
 	if (r)
 		return r;
 
-	r = create_log_context(log, ti, log_size, argc - 1, argv + 1, dev);
+	r = create_log_context(log, ti, argc - 1, argv + 1, dev);
 	if (r) {
 		dm_put_device(ti, dev);
 		return r;
drivers/md/dm-raid1.c +2 -2
index ea732fc..ddda531 100644
@@ -968,8 +968,8 @@ static struct dm_dirty_log *create_dirty_log(struct dm_target *ti,
 		return NULL;
 	}
 
-	dl = dm_dirty_log_create(argv[0], ti, ti->len, mirror_flush,
-				 param_count, argv + 2);
+	dl = dm_dirty_log_create(argv[0], ti, mirror_flush, param_count,
+				 argv + 2);
 	if (!dl) {
 		ti->error = "Error creating mirror dirty log";
 		return NULL;
drivers/md/dm-raid456.c +5 -3
index 3dcbc4a..33d2be8 100644
@@ -192,7 +192,7 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	int recovery = 1;
 	long raid_devs;
 	long rebuildA, rebuildB;
-	sector_t sectors_per_dev, chunks;
+	sector_t sectors_per_dev, chunks, ti_len_sav;
 	struct raid_set *rs = NULL;
 	int in_sync, i;
 	struct dm_dirty_log *log = NULL;
@@ -281,8 +281,10 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	if (sector_div(chunks, chunk_size))
 		goto err;
 
-	log = dm_dirty_log_create(log_argv[0], ti, sectors_per_dev,
-				  NULL, log_cnt, log_argv+2);
+	ti_len_sav = ti->len;
+	ti->len = sectors_per_dev;
+	log = dm_dirty_log_create(log_argv[0], ti, NULL, log_cnt, log_argv+2);
+	ti->len = ti_len_sav;
 	err = "Error creating dirty log";
 	if (!log)
 		goto err;
include/linux/dm-dirty-log.h +1 -3
index 641419f..7084503 100644
@@ -33,7 +33,6 @@ struct dm_dirty_log_type {
 	struct list_head list;
 
 	int (*ctr)(struct dm_dirty_log *log, struct dm_target *ti,
-		   sector_t log_size,
 		   unsigned argc, char **argv);
 	void (*dtr)(struct dm_dirty_log *log);
 
@@ -138,7 +137,7 @@ int dm_dirty_log_type_unregister(struct dm_dirty_log_type *type);
  * type->constructor/destructor() directly.
  */
 struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
-			struct dm_target *ti, sector_t log_size,
+			struct dm_target *ti,
 			int (*flush_callback_fn)(struct dm_target *ti),
 			unsigned argc, char **argv);
 void dm_dirty_log_destroy(struct dm_dirty_log *log);
 
 
--

dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

#24 lore raw

Re: [dm-devel] [PATCH 21/24] dm-dirty-log: allow log size to be different from target size.

Quoted 17 lines · Neil Brown

[Insert obligatory grumble about Reply-to headers inserted by
mail-list software. Adding linux-raid back to the cc list...]

Quoted 10 lines · On Wed, 02 Jun 2010 16:57:21 +0200 Heinz Mauelshagen <[email protected]> wrote:

Neil,

I had a first review through your patch series, which look mostly good to me.

I've got 2 points so far before I go for tests and further review:

o there's actually no need to change the dm-dirty-log interface in an incompatible way to allow for what's needed (see patch attached on top of your series) which we can't do in RHEL/SUSE/... anyway.

Quoted 6 lines

Yes I know. I saw that work-around in your original patch. However this patch set isn't aimed at RHEL or SLES, it is aimed upstream. And when we do things upstream we do them *right*.
If we then want to backport them to RHEL/SLES which require ABI compatibility, then little hacks like the one you show (i.e. lying about the size of the target when creating the log) may be perfectly appropriate.

Ok, we're in agreement then WRT ABI/API compatibility requirements in distribution main releases as opposed to flexibility in upstream.

Quoted 10 lines
Quoted 5 lines

Notwithstanding, we need a discussion on dm-devel to justify if we should change the API upstream in order to avoid such workaround as in my attached patched.

Certainly it is appropriate to discuss this API change - that is why I highlighted it in my introduction to the patch set. Do you have an opinion on it?

I'd like to avoid it altogether in order to avoid multiple code bases wherever possible.

Quoted 9 lines
Quoted 3 lines

o any reason you limit the dm-dirty-log type to 'core' ?

That is only in the earlier part of the patch set. md/raid5 can work without a dirty log at all. When it does, it's behaviour is completely analogous to working with a 'core' dirty log. So to get the raid5 part working without needing to worry about the dirty-log stuff, I told md/raid5 not to use a dirty log, and pretended it was using a 'core' log.

Yes, I meanwhile saw it. Overlooked it initially.

Quoted 8 lines

Then in later patches after I had enabled md/bitmap to work with dm-log, I switched dm-raid45 over to use a real dm-log and removed the restriction to only using a core log.
So this is really just an artefact of the order in which I developed the code. I could go back and re-arrange it so the dm-log integration comes first, then I wouldn't need this intermediate stage which only supports 'core'.

No need to; nicely illustrates the development history.

Heinz

Quoted 195 lines

Thanks,
NeilBrown

Quoted 189 lines

Heinz

Quoted 23 lines · NeilBrown

With RAID1, the dirty log covers the size of the target - the number of bits is the target size divided by the region size.

For RAID5 and similar the dirty log needs to cover the parity blocks, so it is best to base the dirty log size on the size of the component devices rather than the size of the array.

So when creating a dirty log, allow the log_size to be specified separately from the target, and in raid1, set it to the target length.

---
 drivers/md/dm-log-userspace-base.c |   11 +++++++----
 drivers/md/dm-log.c                |   18 +++++++++++-------
 drivers/md/dm-raid1.c              |    4 ++--
 include/linux/dm-dirty-log.h       |    3 ++-
 4 files changed, 22 insertions(+), 14 deletions(-)
drivers/md/dm-log-userspace-base.c +0 -0
index 1ed0094..935a49b 100644
@@ -94,7 +94,7 @@ retry:

<SNIP>

 drivers/md/dm-log.c          |   18 +++++++-----------
 drivers/md/dm-raid1.c        |    4 ++--
 drivers/md/dm-raid456.c      |    8 +++++---
 include/linux/dm-dirty-log.h |    3 +--
 4 files changed, 15 insertions(+), 18 deletions(-)
drivers/md/dm-log.c +7 -11
index a232c14..5a08be0 100644
@@ -146,7 +146,6 @@ EXPORT_SYMBOL(dm_dirty_log_type_unregister);
 
 struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
 			struct dm_target *ti,
-			sector_t log_size,
 			int (*flush_callback_fn)(struct dm_target *ti),
 			unsigned int argc, char **argv)
 {
@@ -165,7 +164,7 @@ struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
 
 	log->flush_callback_fn = flush_callback_fn;
 	log->type = type;
-	if (type->ctr(log, ti, log_size, argc, argv)) {
+	if (type->ctr(log, ti, argc, argv)) {
 		kfree(log);
 		put_type(type);
 		return NULL;
@@ -336,9 +335,9 @@ static int read_header(struct log_c *log)
 	return 0;
 }
 
-static int _check_region_size(sector_t log_size, uint32_t region_size)
+static int _check_region_size(struct dm_target *ti, uint32_t region_size)
 {
-	if (region_size < 2 || region_size > log_size)
+	if (region_size < 2 || region_size > ti->len)
 		return 0;
 
 	if (!is_power_of_2(region_size))
@@ -354,7 +353,6 @@ static int _check_region_size(sector_t log_size, uint32_t region_size)
  *--------------------------------------------------------------*/
 #define BYTE_SHIFT 3
 static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
-			      sector_t log_size,
 			      unsigned int argc, char **argv,
 			      struct dm_dev *dev)
 {
@@ -384,12 +382,12 @@ static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
 	}
 
 	if (sscanf(argv[0], "%u", &region_size) != 1 ||
-	    !_check_region_size(log_size, region_size)) {
+	    !_check_region_size(ti, region_size)) {
 		DMWARN("invalid region size %s", argv[0]);
 		return -EINVAL;
 	}
 
-	region_count = dm_sector_div_up(log_size, region_size);
+	region_count = dm_sector_div_up(ti->len, region_size);
 
 	lc = kmalloc(sizeof(*lc), GFP_KERNEL);
 	if (!lc) {
@@ -509,10 +507,9 @@ static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
 }
 
 static int core_ctr(struct dm_dirty_log *log, struct dm_target *ti,
-		    sector_t log_size,
 		    unsigned int argc, char **argv)
 {
-	return create_log_context(log, ti, log_size, argc, argv, NULL);
+	return create_log_context(log, ti, argc, argv, NULL);
 }
 
 static void destroy_log_context(struct log_c *lc)
@@ -536,7 +533,6 @@ static void core_dtr(struct dm_dirty_log *log)
  * argv contains log_device region_size followed optionally by [no]sync
  *--------------------------------------------------------------*/
 static int disk_ctr(struct dm_dirty_log *log, struct dm_target *ti,
-		    sector_t log_size,
 		    unsigned int argc, char **argv)
 {
 	int r;
@@ -551,7 +547,7 @@ static int disk_ctr(struct dm_dirty_log *log, struct dm_target *ti,
 	if (r)
 		return r;
 
-	r = create_log_context(log, ti, log_size, argc - 1, argv + 1, dev);
+	r = create_log_context(log, ti, argc - 1, argv + 1, dev);
 	if (r) {
 		dm_put_device(ti, dev);
 		return r;
drivers/md/dm-raid1.c +2 -2
index ea732fc..ddda531 100644
@@ -968,8 +968,8 @@ static struct dm_dirty_log *create_dirty_log(struct dm_target *ti,
 		return NULL;
 	}
 
-	dl = dm_dirty_log_create(argv[0], ti, ti->len, mirror_flush,
-				 param_count, argv + 2);
+	dl = dm_dirty_log_create(argv[0], ti, mirror_flush, param_count,
+				 argv + 2);
 	if (!dl) {
 		ti->error = "Error creating mirror dirty log";
 		return NULL;
drivers/md/dm-raid456.c +5 -3
index 3dcbc4a..33d2be8 100644
@@ -192,7 +192,7 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	int recovery = 1;
 	long raid_devs;
 	long rebuildA, rebuildB;
-	sector_t sectors_per_dev, chunks;
+	sector_t sectors_per_dev, chunks, ti_len_sav;
 	struct raid_set *rs = NULL;
 	int in_sync, i;
 	struct dm_dirty_log *log = NULL;
@@ -281,8 +281,10 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	if (sector_div(chunks, chunk_size))
 		goto err;
 
-	log = dm_dirty_log_create(log_argv[0], ti, sectors_per_dev,
-				  NULL, log_cnt, log_argv+2);
+	ti_len_sav = ti->len;
+	ti->len = sectors_per_dev;
+	log = dm_dirty_log_create(log_argv[0], ti, NULL, log_cnt, log_argv+2);
+	ti->len = ti_len_sav;
 	err = "Error creating dirty log";
 	if (!log)
 		goto err;
include/linux/dm-dirty-log.h +1 -3
index 641419f..7084503 100644
@@ -33,7 +33,6 @@ struct dm_dirty_log_type {
 	struct list_head list;
 
 	int (*ctr)(struct dm_dirty_log *log, struct dm_target *ti,
-		   sector_t log_size,
 		   unsigned argc, char **argv);
 	void (*dtr)(struct dm_dirty_log *log);
 
@@ -138,7 +137,7 @@ int dm_dirty_log_type_unregister(struct dm_dirty_log_type *type);
  * type->constructor/destructor() directly.
  */
 struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
-			struct dm_target *ti, sector_t log_size,
+			struct dm_target *ti,
 			int (*flush_callback_fn)(struct dm_target *ti),
 			unsigned argc, char **argv);
 void dm_dirty_log_destroy(struct dm_dirty_log *log);
 
 
--

dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

author
#25 lore raw

PATCH 22/24 md/bitmap: prepare for storing write-intent-bitmap via dm-dirty-log.

In reply to NeilBrown

This allows md/raid5 to fully work as a dm target.

Normally md uses a 'filemap' which contains a list of pages of bits each of which may be written separately.
dm-log uses and all-or-nothing approach to writing the log, so when using a dm-log, ->filemap is NULL and the flags normally stored in filemap_attr are stored in ->logattrs instead.

---
 drivers/md/bitmap.c |  128 +++++++++++++++++++++++++++++++++++----------------
 drivers/md/bitmap.h |    5 ++
 drivers/md/md.h     |    5 ++
 3 files changed, 99 insertions(+), 39 deletions(-)
drivers/md/bitmap.c +89 -39
index 29a3c86..9376526 100644
@@ -29,6 +29,7 @@
 #include "md.h"
 #include "bitmap.h"
 
+#include <linux/dm-dirty-log.h>
 /* debug macros */
 
 #define DEBUG 0
@@ -694,6 +695,8 @@ static inline unsigned long file_page_offset(struct bitmap *bitmap, unsigned lon
 static inline struct page *filemap_get_page(struct bitmap *bitmap,
 					unsigned long chunk)
 {
+	if (bitmap->filemap == NULL)
+		return NULL;
 	if (file_page_index(bitmap, chunk) >= bitmap->file_pages)
 		return NULL;
 	return bitmap->filemap[file_page_index(bitmap, chunk)
@@ -793,19 +796,28 @@ enum bitmap_page_attr {
 static inline void set_page_attr(struct bitmap *bitmap, struct page *page,
 				enum bitmap_page_attr attr)
 {
-	__set_bit((page->index<<2) + attr, bitmap->filemap_attr);
+	if (page)
+		__set_bit((page->index<<2) + attr, bitmap->filemap_attr);
+	else
+		__set_bit(attr, &bitmap->logattrs);
 }
 
 static inline void clear_page_attr(struct bitmap *bitmap, struct page *page,
 				enum bitmap_page_attr attr)
 {
-	__clear_bit((page->index<<2) + attr, bitmap->filemap_attr);
+	if (page)
+		__clear_bit((page->index<<2) + attr, bitmap->filemap_attr);
+	else
+		__clear_bit(attr, &bitmap->logattrs);
 }
 
 static inline unsigned long test_page_attr(struct bitmap *bitmap, struct page *page,
 					   enum bitmap_page_attr attr)
 {
-	return test_bit((page->index<<2) + attr, bitmap->filemap_attr);
+	if (page)
+		return test_bit((page->index<<2) + attr, bitmap->filemap_attr);
+	else
+		return test_bit(attr, &bitmap->logattrs);
 }
 
 /*
@@ -818,27 +830,30 @@ static inline unsigned long test_page_attr(struct bitmap *bitmap, struct page *p
 static void bitmap_file_set_bit(struct bitmap *bitmap, sector_t block)
 {
 	unsigned long bit;
-	struct page *page;
+	struct page *page = NULL;
 	void *kaddr;
 	unsigned long chunk = block >> CHUNK_BLOCK_SHIFT(bitmap);
 
-	if (!bitmap->filemap)
-		return;
-
-	page = filemap_get_page(bitmap, chunk);
-	if (!page)
-		return;
-	bit = file_page_offset(bitmap, chunk);
+	if (!bitmap->filemap) {
+		struct dm_dirty_log *log = bitmap->mddev->bitmap_info.log;
+		if (log)
+			log->type->mark_region(log, chunk);
+	} else {
 
-	/* set the bit */
-	kaddr = kmap_atomic(page, KM_USER0);
-	if (bitmap->flags & BITMAP_HOSTENDIAN)
-		set_bit(bit, kaddr);
-	else
-		ext2_set_bit(bit, kaddr);
-	kunmap_atomic(kaddr, KM_USER0);
-	PRINTK("set file bit %lu page %lu\n", bit, page->index);
+		page = filemap_get_page(bitmap, chunk);
+		if (!page)
+			return;
+		bit = file_page_offset(bitmap, chunk);
 
+		/* set the bit */
+		kaddr = kmap_atomic(page, KM_USER0);
+		if (bitmap->flags & BITMAP_HOSTENDIAN)
+			set_bit(bit, kaddr);
+		else
+			ext2_set_bit(bit, kaddr);
+		kunmap_atomic(kaddr, KM_USER0);
+		PRINTK("set file bit %lu page %lu\n", bit, page->index);
+	}
 	/* record page number so it gets flushed to disk when unplug occurs */
 	set_page_attr(bitmap, page, BITMAP_PAGE_DIRTY);
 }
@@ -855,6 +870,16 @@ void bitmap_unplug(struct bitmap *bitmap)
 
 	if (!bitmap)
 		return;
+	if (!bitmap->filemap) {
+		/* Must be using a dirty_log */
+		struct dm_dirty_log *log = bitmap->mddev->bitmap_info.log;
+		dirty = test_and_clear_bit(BITMAP_PAGE_DIRTY, &bitmap->logattrs);
+		need_write = test_and_clear_bit(BITMAP_PAGE_NEEDWRITE, &bitmap->logattrs);
+		if (dirty || need_write)
+			if (log->type->flush(log))
+				bitmap->flags |= BITMAP_WRITE_ERROR;
+		goto out;
+	}
 
 	/* look at each page to see if there are any set bits that need to be
 	 * flushed out to disk */
@@ -883,6 +908,7 @@ void bitmap_unplug(struct bitmap *bitmap)
 		else
 			md_super_wait(bitmap->mddev);
 	}
+out:
 	if (bitmap->flags & BITMAP_WRITE_ERROR)
 		bitmap_file_kick(bitmap);
 }
@@ -925,11 +951,11 @@ static int bitmap_init_from_disk(struct bitmap *bitmap, sector_t start)
 		printk(KERN_INFO "%s: bitmap file is out of date, doing full "
 			"recovery\n", bmname(bitmap));
 
-	bytes = (chunks + 7) / 8;
+	bytes = DIV_ROUND_UP(bitmap->chunks, 8);
 	if (!bitmap->mddev->bitmap_info.external)
 		bytes += sizeof(bitmap_super_t);
 
-	num_pages = (bytes + PAGE_SIZE - 1) / PAGE_SIZE;
+	num_pages = DIV_ROUND_UP(bytes, PAGE_SIZE);
 
 	if (file && i_size_read(file->f_mapping->host) < bytes) {
 		printk(KERN_INFO "%s: bitmap file too short %lu < %lu\n",
@@ -1090,6 +1116,7 @@ void bitmap_daemon_work(mddev_t *mddev)
 	struct page *page = NULL, *lastpage = NULL;
 	int blocks;
 	void *paddr;
+	struct dm_dirty_log *log = mddev->bitmap_info.log;
 
 	/* Use a mutex to guard daemon_work against
 	 * bitmap_destroy.
@@ -1114,11 +1141,12 @@ void bitmap_daemon_work(mddev_t *mddev)
 	spin_lock_irqsave(&bitmap->lock, flags);
 	for (j = 0; j < bitmap->chunks; j++) {
 		bitmap_counter_t *bmc;
-		if (!bitmap->filemap)
-			/* error or shutdown */
-			break;
-
-		page = filemap_get_page(bitmap, j);
+		if (!bitmap->filemap) {
+			if (!log)
+				/* error or shutdown */
+				break;
+		} else
+			page = filemap_get_page(bitmap, j);
 
 		if (page != lastpage) {
 			/* skip this page unless it's marked as needing cleaning */
@@ -1187,14 +1215,17 @@ void bitmap_daemon_work(mddev_t *mddev)
 						  -1);
 
 				/* clear the bit */
-				paddr = kmap_atomic(page, KM_USER0);
-				if (bitmap->flags & BITMAP_HOSTENDIAN)
-					clear_bit(file_page_offset(bitmap, j),
-						  paddr);
-				else
-					ext2_clear_bit(file_page_offset(bitmap, j),
-						       paddr);
-				kunmap_atomic(paddr, KM_USER0);
+				if (page) {
+					paddr = kmap_atomic(page, KM_USER0);
+					if (bitmap->flags & BITMAP_HOSTENDIAN)
+						clear_bit(file_page_offset(bitmap, j),
+							  paddr);
+					else
+						ext2_clear_bit(file_page_offset(bitmap, j),
+							       paddr);
+					kunmap_atomic(paddr, KM_USER0);
+				} else
+					log->type->clear_region(log, j);
 			}
 		} else
 			j |= PAGE_COUNTER_MASK;
@@ -1202,12 +1233,16 @@ void bitmap_daemon_work(mddev_t *mddev)
 	spin_unlock_irqrestore(&bitmap->lock, flags);
 
 	/* now sync the final page */
-	if (lastpage != NULL) {
+	if (lastpage != NULL || log != NULL) {
 		spin_lock_irqsave(&bitmap->lock, flags);
 		if (test_page_attr(bitmap, lastpage, BITMAP_PAGE_NEEDWRITE)) {
 			clear_page_attr(bitmap, lastpage, BITMAP_PAGE_NEEDWRITE);
 			spin_unlock_irqrestore(&bitmap->lock, flags);
-			write_page(bitmap, lastpage, 0);
+			if (lastpage)
+				write_page(bitmap, lastpage, 0);
+			else
+				if (log->type->flush(log))
+					bitmap->flags |= BITMAP_WRITE_ERROR;
 		} else {
 			set_page_attr(bitmap, lastpage, BITMAP_PAGE_NEEDWRITE);
 			spin_unlock_irqrestore(&bitmap->lock, flags);
@@ -1372,7 +1407,9 @@ void bitmap_endwrite(struct bitmap *bitmap, sector_t offset, unsigned long secto
 		(*bmc)--;
 		if (*bmc <= 2)
 			set_page_attr(bitmap,
-				      filemap_get_page(bitmap, offset >> CHUNK_BLOCK_SHIFT(bitmap)),
+				      filemap_get_page(
+					      bitmap,
+					      offset >> CHUNK_BLOCK_SHIFT(bitmap)),
 				      BITMAP_PAGE_CLEAN);
 
 		spin_unlock_irqrestore(&bitmap->lock, flags);
@@ -1649,10 +1686,13 @@ int bitmap_create(mddev_t *mddev)
 
 	BUILD_BUG_ON(sizeof(bitmap_super_t) != 256);
 
-	if (!file && !mddev->bitmap_info.offset) /* bitmap disabled, nothing to do */
+	if (!file
+	    && !mddev->bitmap_info.offset
+	    && !mddev->bitmap_info.log) /* bitmap disabled, nothing to do */
 		return 0;
 
 	BUG_ON(file && mddev->bitmap_info.offset);
+	BUG_ON(mddev->bitmap_info.offset && mddev->bitmap_info.log);
 
 	bitmap = kzalloc(sizeof(*bitmap), GFP_KERNEL);
 	if (!bitmap)
@@ -1730,7 +1770,17 @@ int bitmap_create(mddev_t *mddev)
 	    || bitmap->events_cleared == mddev->events)
 		/* no need to keep dirty bits to optimise a re-add of a missing device */
 		start = mddev->recovery_cp;
-	err = bitmap_init_from_disk(bitmap, start);
+	if (mddev->bitmap_info.log) {
+		unsigned long i;
+		struct dm_dirty_log *log = mddev->bitmap_info.log;
+		for (i = 0; i < bitmap->chunks; i++)
+			if (!log->type->in_sync(log, i, 1))
+				bitmap_set_memory_bits(bitmap,
+						       (sector_t)i << CHUNK_BLOCK_SHIFT(bitmap),
+						       1);
+		err = 0;
+	} else
+		err = bitmap_init_from_disk(bitmap, start);
 
 	if (err)
 		goto error;
drivers/md/bitmap.h +5 -0
index 3797dea..a7a1113 100644
@@ -222,6 +222,10 @@ struct bitmap {
 	unsigned long file_pages; /* number of pages in the file */
 	int last_page_size; /* bytes in the last page */
 
+	unsigned long logattrs; /* used when filemap_attr doesn't exist
+				 * because we are working with a dirty_log
+				 */
+
 	unsigned long flags;
 
 	int allclean;
@@ -243,6 +247,7 @@ struct bitmap {
 	wait_queue_head_t behind_wait;
 
 	struct sysfs_dirent *sysfs_can_clear;
+
 };
 
 /* the bitmap API */
drivers/md/md.h +5 -0
index a97e4b0..e97466f 100644
@@ -314,6 +314,11 @@ struct mddev_s
 							 * hot-adding a bitmap.  It should
 							 * eventually be settable by sysfs.
 							 */
+		/* When md is serving under dm, it might use a
+		 * dirty_log to store the bits.
+		 */
+		struct dm_dirty_log *log;
+
 		struct mutex		mutex;
 		unsigned long		chunksize;
 		unsigned long		daemon_sleep; /* how many seconds between updates? */
author
#26 lore raw

PATCH 23/24 md/bitmap: separate out loading a bitmap from initialising the structures.

In reply to NeilBrown

dm makes this distinction between ->ctr and ->resume, so we need to too.

Also get the new bitmap_load to clear out the bitmap first, as this is most consistent with the dm suspend/resume approach

---
 drivers/md/bitmap.c |   69 +++++++++++++++++++++++++++++++++++----------------
 drivers/md/bitmap.h |    1 +
 drivers/md/md.c     |   13 ++++++++--
 3 files changed, 60 insertions(+), 23 deletions(-)
drivers/md/bitmap.c +48 -21
index 9376526..1ba1e12 100644
@@ -1681,7 +1681,6 @@ int bitmap_create(mddev_t *mddev)
 	unsigned long pages;
 	struct file *file = mddev->bitmap_info.file;
 	int err;
-	sector_t start;
 	struct sysfs_dirent *bm = NULL;
 
 	BUILD_BUG_ON(sizeof(bitmap_super_t) != 256);
@@ -1763,13 +1762,40 @@ int bitmap_create(mddev_t *mddev)
 	if (!bitmap->bp)
 		goto error;
 
-	/* now that we have some pages available, initialize the in-memory
-	 * bitmap from the on-disk bitmap */
-	start = 0;
-	if (mddev->degraded == 0
-	    || bitmap->events_cleared == mddev->events)
-		/* no need to keep dirty bits to optimise a re-add of a missing device */
-		start = mddev->recovery_cp;
+	printk(KERN_INFO "created bitmap (%lu pages) for device %s\n",
+		pages, bmname(bitmap));
+
+	mddev->bitmap = bitmap;
+
+
+	return (bitmap->flags & BITMAP_WRITE_ERROR) ? -EIO : 0;
+
+ error:
+	bitmap_free(bitmap);
+	return err;
+}
+
+int bitmap_load(mddev_t *mddev)
+{
+	int err = 0;
+	sector_t sector = 0;
+	struct bitmap *bitmap = mddev->bitmap;
+
+	if (!bitmap)
+		goto out;
+
+	/* Clear out old bitmap info first:  Either there is none, or we
+	 * are resuming after someone else has possibly changed things,
+	 * so we should forget old cached info.
+	 * All chunks should be clean, but some might need_sync.
+	 */
+	while (sector < mddev->resync_max_sectors) {
+		int blocks;
+		bitmap_start_sync(bitmap, sector, &blocks, 0);
+		sector += blocks;
+	}
+	bitmap_close_sync(bitmap);
+
 	if (mddev->bitmap_info.log) {
 		unsigned long i;
 		struct dm_dirty_log *log = mddev->bitmap_info.log;
@@ -1778,29 +1804,30 @@ int bitmap_create(mddev_t *mddev)
 				bitmap_set_memory_bits(bitmap,
 						       (sector_t)i << CHUNK_BLOCK_SHIFT(bitmap),
 						       1);
-		err = 0;
-	} else
-		err = bitmap_init_from_disk(bitmap, start);
+	} else {
+		sector_t start = 0;
+		if (mddev->degraded == 0
+		    || bitmap->events_cleared == mddev->events)
+			/* no need to keep dirty bits to optimise a
+			 * re-add of a missing device */
+			start = mddev->recovery_cp;
 
+		err = bitmap_init_from_disk(bitmap, start);
+	}
 	if (err)
-		goto error;
-
-	printk(KERN_INFO "created bitmap (%lu pages) for device %s\n",
-		pages, bmname(bitmap));
-
-	mddev->bitmap = bitmap;
+		goto out;
 
 	mddev->thread->timeout = mddev->bitmap_info.daemon_sleep;
 	md_wakeup_thread(mddev->thread);
 
 	bitmap_update_sb(bitmap);
 
-	return (bitmap->flags & BITMAP_WRITE_ERROR) ? -EIO : 0;
-
- error:
-	bitmap_free(bitmap);
+	if (bitmap->flags & BITMAP_WRITE_ERROR)
+		err = -EIO;
+out:
 	return err;
 }
+EXPORT_SYMBOL_GPL(bitmap_load);
 
 static ssize_t
 location_show(mddev_t *mddev, char *page)
drivers/md/bitmap.h +1 -0
index a7a1113..e872a7b 100644
@@ -254,6 +254,7 @@ struct bitmap {
 
 /* these are used only by md/bitmap */
 int  bitmap_create(mddev_t *mddev);
+int bitmap_load(mddev_t *mddev);
 void bitmap_flush(mddev_t *mddev);
 void bitmap_destroy(mddev_t *mddev);
 
drivers/md/md.c +11 -2
index f24efd2..e0a9bf8 100644
@@ -4568,7 +4568,11 @@ static int do_md_run(mddev_t *mddev)
 	err = md_run(mddev);
 	if (err)
 		goto out;
-
+	err = bitmap_load(mddev);
+	if (err) {
+		bitmap_destroy(mddev);
+		goto out;
+	}
 	set_capacity(mddev->gendisk, mddev->array_sectors);
 	revalidate_disk(mddev->gendisk);
 	kobject_uevent(&disk_to_dev(mddev->gendisk)->kobj, KOBJ_CHANGE);
@@ -5356,8 +5360,11 @@ static int set_bitmap_file(mddev_t *mddev, int fd)
 	err = 0;
 	if (mddev->pers) {
 		mddev->pers->quiesce(mddev, 1);
-		if (fd >= 0)
+		if (fd >= 0) {
 			err = bitmap_create(mddev);
+			if (!err)
+				err = bitmap_load(mddev);
+		}
 		if (fd < 0 || err) {
 			bitmap_destroy(mddev);
 			fd = -1; /* make sure to put the file */
@@ -5606,6 +5613,8 @@ static int update_array_info(mddev_t *mddev, mdu_array_info_t *info)
 				mddev->bitmap_info.default_offset;
 			mddev->pers->quiesce(mddev, 1);
 			rv = bitmap_create(mddev);
+			if (!rv)
+				rv = bitmap_load(mddev);
 			if (rv)
 				bitmap_destroy(mddev);
 			mddev->pers->quiesce(mddev, 0);
author
#27 lore raw

PATCH 24/24 dm-raid456: switch to use dm_dirty_log for tracking dirty regions.

In reply to NeilBrown

Rather than faking a 'core' dirty log, we can now use the dm_dirty_log mechanism directly and thus potentially benefit from a permanent dirty log.

---
 drivers/md/dm-raid456.c |   46 +++++++++++++++++++++++++++++++++++++---------
 drivers/md/md.h         |    2 +-
 2 files changed, 38 insertions(+), 10 deletions(-)
drivers/md/dm-raid456.c +37 -9
index 3fda954..3dcbc4a 100644
@@ -7,6 +7,8 @@
 #include "md.h"
 #include "raid5.h"
 #include "dm.h"
+#include "bitmap.h"
+#include <linux/dm-dirty-log.h>
 
 struct raid_dev {
 	struct dm_dev *dev;
@@ -183,7 +185,8 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
 {
 	char *err = NULL;
 	int errnum = -EINVAL;
-	unsigned long cnt;
+	unsigned long cnt, log_cnt;
+	char **log_argv;
 	struct raid_type *rt;
 	unsigned long chunk_size;
 	int recovery = 1;
@@ -192,16 +195,18 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	sector_t sectors_per_dev, chunks;
 	struct raid_set *rs = NULL;
 	int in_sync, i;
+	struct dm_dirty_log *log = NULL;
 
-	/* log type - core XXX [no]sync */
+	/* log type - type arg-count args */
 	err = "Cannot parse log type";
 	if (argc < 2 ||
-	    strcmp(argv[0], "core") != 0 ||
 	    strict_strtoul(argv[1], 10, &cnt) < 0 ||
 	    cnt + 2 > argc)
 		goto err;
-	if (cnt >= 2 && strcmp(argv[3], "sync") == 0)
-		recovery = 0;
+
+	log_cnt = cnt;
+	log_argv = argv;
+
 	argc -= cnt+2;
 	argv += cnt+2;
 
@@ -276,6 +281,11 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	if (sector_div(chunks, chunk_size))
 		goto err;
 
+	log = dm_dirty_log_create(log_argv[0], ti, sectors_per_dev,
+				  NULL, log_cnt, log_argv+2);
+	err = "Error creating dirty log";
+	if (!log)
+		goto err;
 
 	/* Now the devices: three words each */
 	rs = context_alloc(rt, chunk_size, recovery,
@@ -318,6 +328,11 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	ti->split_io = rs->md.chunk_sectors;
 	ti->private = rs;
 
+	rs->md.bitmap_info.log = log;
+	rs->md.bitmap_info.daemon_sleep = 10 * HZ;
+	rs->md.bitmap_info.chunksize = log->type->get_region_size(log) * 512;
+	rs->md.bitmap_info.external = 1;
+
 	mutex_lock(&rs->md.reconfig_mutex);
 	err = "Fail to run raid array";
 	errnum = md_run(&rs->md);
@@ -332,6 +347,8 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
 
 	return 0;
 err:
+	if (log)
+		dm_dirty_log_destroy(log);
 	if (rs)
 		context_free(rs);
 	ti->error = err;
@@ -343,6 +360,7 @@ static void raid_dtr(struct dm_target *ti)
 	struct raid_set *rs = ti->private;
 
 	list_del_init(&rs->callbacks.list);
+	dm_dirty_log_destroy(rs->md.bitmap_info.log);
 	md_stop(&rs->md);
 	context_free(rs);
 }
@@ -362,6 +380,7 @@ static int raid_status(struct dm_target *ti, status_type_t type,
 {
 	struct raid_set *rs = ti->private;
 	struct raid5_private_data *conf = rs->md.private;
+	struct dm_dirty_log *log = conf->mddev->bitmap_info.log;
 	int sz = 0;
 	int rbcnt;
 	int i;
@@ -394,14 +413,14 @@ static int raid_status(struct dm_target *ti, status_type_t type,
 		DMEMIT("%llu/%llu ",
 		       (unsigned long long) sync,
 		       (unsigned long long) rs->md.resync_max_sectors);
-		DMEMIT("1 core");
+
+		sz += log->type->status(log, type, result + sz, maxlen - sz);
 
 		break;
 	case STATUSTYPE_TABLE:
 		/* The string you would use to construct this array */
-		/* Pretend to use a core log with a region size of 1 sector */
-		DMEMIT("core 2 %u %ssync ", 1,
-		       rs->md.recovery_cp == MaxSector ? "" : "no");
+		sz += log->type->status(log, type, result + sz, maxlen - sz);
+
 		DMEMIT("%s ", rs->raid_type->name);
 		DMEMIT("1 %u ", rs->md.chunk_sectors);
 
@@ -469,19 +488,28 @@ static void raid_io_hints(struct dm_target *ti,
 static void raid_presuspend(struct dm_target *ti)
 {
 	struct raid_set *rs = ti->private;
+	struct dm_dirty_log *log = rs->md.bitmap_info.log;
+
 	md_stop_writes(&rs->md);
+	log->type->presuspend(log);
 }
 
 static void raid_postsuspend(struct dm_target *ti)
 {
 	struct raid_set *rs = ti->private;
+	struct dm_dirty_log *log = rs->md.bitmap_info.log;
+
 	mddev_suspend(&rs->md);
+	log->type->postsuspend(log);
 }
 
 static void raid_resume(struct dm_target *ti)
 {
 	struct raid_set *rs = ti->private;
+	struct dm_dirty_log *log = rs->md.bitmap_info.log;
 
+	log->type->resume(log);
+	bitmap_load(&rs->md);
 	mddev_resume(&rs->md);
 }
 
drivers/md/md.h +1 -1
index e97466f..e53b355 100644
@@ -321,7 +321,7 @@ struct mddev_s
 
 		struct mutex		mutex;
 		unsigned long		chunksize;
-		unsigned long		daemon_sleep; /* how many seconds between updates? */
+		unsigned long		daemon_sleep; /* how many jiffies between updates? */
 		unsigned long		max_write_behind; /* write-behind mode */
 		int			external;
 	} bitmap_info;
#28 lore raw
In reply to NeilBrown

Neil,

for missing devices we (re)load the table with error mapped device(s) in their place rather than identifying them with a special char/string. Did you go for the later in order to avoid some MD hassle with error targets being accessed by the MD personalities? If not so, we can drop the special '-' char support to identify missing devices.

I'm thinking about adding a ctr wrapper to allow us to keep the "raid45" ctr interfaces semantics (ie. the arguments) and the new interface to "raid456" if you don't have objections. That would be implemented by 2 targets being registered implemented in one module.

Heinz

Quoted 93 lines · NeilBrown

Nearly two months ago I posted my first serious attempt and making md/raid5.c work as a dm target. I met with cautious approval I think, but as it didn't interact with dirty-logs yet it wasn't really read for prime time.

It has taken longer than I hoped, but here is version 2, now with dirty-log integration.
I have done a modest amount of testing, watching the bits in the log getting cleared and set just as you would expect, and watching the resync complete instantly when the dirty-log shows that all regions are clean.

There is not even a hint of cluster support yet, but that shouldn't be necessary for initial submission to mainline.

A significant difference to Heinz' dm-raid45 is that I only honour the table options that lvm actually sets. The extra ones don't really fit with md/raid5 and I don't think they need to be table options. If any are needed they might work OK as messages (???).

There are a number of changes to core-dm in here including:
- support for targets to be unplugged when the device is
- support for targets to report congestion beyond the congestion of component devices
- support for the dirty-log to cover and extent different from the size of the target (For raid5 it must be the size of the components).
I have tried to fit these to the general style of dm as best as I can.

There is certainly room for more testing and review, but I would like to see this entering -next soon with a view to seeing it merged in the next merge window.

Is this reasonable? Achievable?

Comments?

These patches can all be found on the "md-dm-raid45" branch of
git://neil.brown.name/md/
or at http://neil.brown.name/git?p=md;a=shortlog;h=refs/heads/md-dm-raid45

Thanks,
NeilBrown

---

NeilBrown (24):
md: reduce dependence on sysfs.
md/raid5: factor out code for changing size of stripe cache. md/raid5: ensure we create a unique name for kmem_cache when mddev has no gendisk
md: be more careful setting MD_CHANGE_CLEAN
md: split out md_rdev_init
md: export various start/stop interfaces
md/dm: create dm-raid456 module using md/raid5
dm-raid456: add support for raising events to userspace.
raid5: Don't set read-ahead when there is no queue
dm-raid456: add congestion checking.
md/raid5: add simple plugging infrastructure.
md/plug: optionally use plugger to unplug an array during resync/recovery.
dm-raid456: support unplug
dm-raid456: add support for setting IO hints.
dm-raid456: add suspend/resume method
dm-raid456: add message handler.
md/bitmap: white space clean up and similar.
md/bitmap: reduce dependence on sysfs.
md/bitmap: clean up plugging calls.
md/bitmap: optimise scanning of empty bitmaps.
dm-dirty-log: allow log size to be different from target size. md/bitmap: prepare for storing write-intent-bitmap via dm-dirty-log. md/bitmap: separate out loading a bitmap from initialising the structures.
dm-raid456: switch to use dm_dirty_log for tracking dirty regions.

 drivers/md/Kconfig                 |    8 +
 drivers/md/Makefile                |    1 
 drivers/md/bitmap.c                |  508 +++++++++++++++++---------------
 drivers/md/bitmap.h                |    6 
 drivers/md/dm-log-userspace-base.c |   11 -
 drivers/md/dm-log.c                |   18 +
 drivers/md/dm-raid1.c              |    4 
 drivers/md/dm-raid456.c            |  576 ++++++++++++++++++++++++++++++++++++
 drivers/md/dm-table.c              |   19 +
 drivers/md/md.c                    |  234 +++++++++------
 drivers/md/md.h                    |   51 +++
 drivers/md/raid5.c                 |  169 ++++++-----
 drivers/md/raid5.h                 |    8 -
 include/linux/device-mapper.h      |   13 +
 include/linux/dm-dirty-log.h       |    3 
 15 files changed, 1235 insertions(+), 394 deletions(-)
 create mode 100644 drivers/md/dm-raid456.c
author
#29 lore raw

Hi Heinz,

Quoted 8 lines · On Tue, 15 Jun 2010 15:23:26 +0200 Heinz Mauelshagen <[email protected]> wrote:

Neil,

for missing devices we (re)load the table with error mapped device(s) in their place rather than identifying them with a special char/string. Did you go for the later in order to avoid some MD hassle with error targets being accessed by the MD personalities? If not so, we can drop the special '-' char support to identify missing devices.

When raid5 determines that a device has failed and so it will not write to it again, it must be sure that the failure is record in the metadata before it proceeds with that decision not to even try writing to the failed device. Otherwise a crash/restart before the metadata was safely updated would result in corruption.

This means that it must be possible for user-space to explicitly tell the raid5 that a device is known to be faulty.
Doing that through the constructor seems the natural way to do it.

Quoted 5 lines

I'm thinking about adding a ctr wrapper to allow us to keep the "raid45" ctr interfaces semantics (ie. the arguments) and the new interface to "raid456" if you don't have objections. That would be implemented by 2 targets being registered implemented in one module.

I have no strong objections, though having two targets that do almost the same things seems rather ugly.

Is there existing published code that uses the extra arguments to raid45?

Thanks,
NeilBrown

Quoted 98 lines

Heinz

Quoted 93 lines · NeilBrown

Nearly two months ago I posted my first serious attempt and making md/raid5.c work as a dm target. I met with cautious approval I think, but as it didn't interact with dirty-logs yet it wasn't really read for prime time.

It has taken longer than I hoped, but here is version 2, now with dirty-log integration.
I have done a modest amount of testing, watching the bits in the log getting cleared and set just as you would expect, and watching the resync complete instantly when the dirty-log shows that all regions are clean.

There is not even a hint of cluster support yet, but that shouldn't be necessary for initial submission to mainline.

A significant difference to Heinz' dm-raid45 is that I only honour the table options that lvm actually sets. The extra ones don't really fit with md/raid5 and I don't think they need to be table options. If any are needed they might work OK as messages (???).

There are a number of changes to core-dm in here including:
- support for targets to be unplugged when the device is
- support for targets to report congestion beyond the congestion of component devices
- support for the dirty-log to cover and extent different from the size of the target (For raid5 it must be the size of the components).
I have tried to fit these to the general style of dm as best as I can.

There is certainly room for more testing and review, but I would like to see this entering -next soon with a view to seeing it merged in the next merge window.

Is this reasonable? Achievable?

Comments?

These patches can all be found on the "md-dm-raid45" branch of
git://neil.brown.name/md/
or at http://neil.brown.name/git?p=md;a=shortlog;h=refs/heads/md-dm-raid45

Thanks,
NeilBrown

---

NeilBrown (24):
md: reduce dependence on sysfs.
md/raid5: factor out code for changing size of stripe cache. md/raid5: ensure we create a unique name for kmem_cache when mddev has no gendisk
md: be more careful setting MD_CHANGE_CLEAN
md: split out md_rdev_init
md: export various start/stop interfaces
md/dm: create dm-raid456 module using md/raid5
dm-raid456: add support for raising events to userspace.
raid5: Don't set read-ahead when there is no queue
dm-raid456: add congestion checking.
md/raid5: add simple plugging infrastructure.
md/plug: optionally use plugger to unplug an array during resync/recovery.
dm-raid456: support unplug
dm-raid456: add support for setting IO hints.
dm-raid456: add suspend/resume method
dm-raid456: add message handler.
md/bitmap: white space clean up and similar.
md/bitmap: reduce dependence on sysfs.
md/bitmap: clean up plugging calls.
md/bitmap: optimise scanning of empty bitmaps.
dm-dirty-log: allow log size to be different from target size. md/bitmap: prepare for storing write-intent-bitmap via dm-dirty-log. md/bitmap: separate out loading a bitmap from initialising the structures.
dm-raid456: switch to use dm_dirty_log for tracking dirty regions.

 drivers/md/Kconfig                 |    8 +
 drivers/md/Makefile                |    1 
 drivers/md/bitmap.c                |  508 +++++++++++++++++---------------
 drivers/md/bitmap.h                |    6 
 drivers/md/dm-log-userspace-base.c |   11 -
 drivers/md/dm-log.c                |   18 +
 drivers/md/dm-raid1.c              |    4 
 drivers/md/dm-raid456.c            |  576 ++++++++++++++++++++++++++++++++++++
 drivers/md/dm-table.c              |   19 +
 drivers/md/md.c                    |  234 +++++++++------
 drivers/md/md.h                    |   51 +++
 drivers/md/raid5.c                 |  169 ++++++-----
 drivers/md/raid5.h                 |    8 -
 include/linux/device-mapper.h      |   13 +
 include/linux/dm-dirty-log.h       |    3 
 15 files changed, 1235 insertions(+), 394 deletions(-)
 create mode 100644 drivers/md/dm-raid456.c
#30 lore raw
Quoted 14 lines · Neil Brown

Hi Heinz,

Quoted 8 lines · On Tue, 15 Jun 2010 15:23:26 +0200 Heinz Mauelshagen <[email protected]> wrote:

Neil,

for missing devices we (re)load the table with error mapped device(s) in their place rather than identifying them with a special char/string. Did you go for the later in order to avoid some MD hassle with error targets being accessed by the MD personalities? If not so, we can drop the special '-' char support to identify missing devices.

Quoted 3 lines

When raid5 determines that a device has failed and so it will not write to it again, it must be sure that the failure is record in the metadata before it proceeds with that decision not to even try writing to the failed device.

Yes, that's the mandatory order to avoid data loss.

Quoted 6 lines

Otherwise a crash/restart before the metadata was safely updated would result in corruption.

This means that it must be possible for user-space to explicitly tell the raid5 that a device is known to be faulty.
Doing that through the constructor seems the natural way to do it.

If /dev/mapper/error with an error target mapping would be used instead, would that cause consistency troubles to the MD personality if accessing those rather than the NULL device solution with the '-' argument you have now?
If not so, we could drop the '-' support, which I'm aiming at.

Quoted 9 lines
Quoted 6 lines

I'm thinking about adding a ctr wrapper to allow us to keep the "raid45" ctr interfaces semantics (ie. the arguments) and the new interface to "raid456" if you don't have objections. That would be implemented by 2 targets being registered implemented in one module.

I have no strong objections, though having two targets that do almost the same things seems rather ugly.

Keep in mind it'll only be another target_type structs, another raid_ctr() function plus another (de)registration of the respective target. The ctr functions for raid45 and raid456 can share moset of the code anyway.

Quoted 2 lines

Is there existing published code that uses the extra arguments to raid45?

Yes, dmraid with a list of RAID5 supporting metadata format handlers.

Regards,
Heinz

Quoted 104 lines

Thanks,
NeilBrown

Quoted 98 lines

Heinz

Quoted 93 lines · NeilBrown

Nearly two months ago I posted my first serious attempt and making md/raid5.c work as a dm target. I met with cautious approval I think, but as it didn't interact with dirty-logs yet it wasn't really read for prime time.

It has taken longer than I hoped, but here is version 2, now with dirty-log integration.
I have done a modest amount of testing, watching the bits in the log getting cleared and set just as you would expect, and watching the resync complete instantly when the dirty-log shows that all regions are clean.

There is not even a hint of cluster support yet, but that shouldn't be necessary for initial submission to mainline.

A significant difference to Heinz' dm-raid45 is that I only honour the table options that lvm actually sets. The extra ones don't really fit with md/raid5 and I don't think they need to be table options. If any are needed they might work OK as messages (???).

There are a number of changes to core-dm in here including:
- support for targets to be unplugged when the device is
- support for targets to report congestion beyond the congestion of component devices
- support for the dirty-log to cover and extent different from the size of the target (For raid5 it must be the size of the components).
I have tried to fit these to the general style of dm as best as I can.

There is certainly room for more testing and review, but I would like to see this entering -next soon with a view to seeing it merged in the next merge window.

Is this reasonable? Achievable?

Comments?

These patches can all be found on the "md-dm-raid45" branch of
git://neil.brown.name/md/
or at http://neil.brown.name/git?p=md;a=shortlog;h=refs/heads/md-dm-raid45

Thanks,
NeilBrown

---

NeilBrown (24):
md: reduce dependence on sysfs.
md/raid5: factor out code for changing size of stripe cache. md/raid5: ensure we create a unique name for kmem_cache when mddev has no gendisk
md: be more careful setting MD_CHANGE_CLEAN
md: split out md_rdev_init
md: export various start/stop interfaces
md/dm: create dm-raid456 module using md/raid5
dm-raid456: add support for raising events to userspace.
raid5: Don't set read-ahead when there is no queue
dm-raid456: add congestion checking.
md/raid5: add simple plugging infrastructure.
md/plug: optionally use plugger to unplug an array during resync/recovery.
dm-raid456: support unplug
dm-raid456: add support for setting IO hints.
dm-raid456: add suspend/resume method
dm-raid456: add message handler.
md/bitmap: white space clean up and similar.
md/bitmap: reduce dependence on sysfs.
md/bitmap: clean up plugging calls.
md/bitmap: optimise scanning of empty bitmaps.
dm-dirty-log: allow log size to be different from target size. md/bitmap: prepare for storing write-intent-bitmap via dm-dirty-log. md/bitmap: separate out loading a bitmap from initialising the structures.
dm-raid456: switch to use dm_dirty_log for tracking dirty regions.

 drivers/md/Kconfig                 |    8 +
 drivers/md/Makefile                |    1 
 drivers/md/bitmap.c                |  508 +++++++++++++++++---------------
 drivers/md/bitmap.h                |    6 
 drivers/md/dm-log-userspace-base.c |   11 -
 drivers/md/dm-log.c                |   18 +
 drivers/md/dm-raid1.c              |    4 
 drivers/md/dm-raid456.c            |  576 ++++++++++++++++++++++++++++++++++++
 drivers/md/dm-table.c              |   19 +
 drivers/md/md.c                    |  234 +++++++++------
 drivers/md/md.h                    |   51 +++
 drivers/md/raid5.c                 |  169 ++++++-----
 drivers/md/raid5.h                 |    8 -
 include/linux/device-mapper.h      |   13 +
 include/linux/dm-dirty-log.h       |    3 
 15 files changed, 1235 insertions(+), 394 deletions(-)
 create mode 100644 drivers/md/dm-raid456.c
author
#31 lore raw
Quoted 28 lines · On Wed, 16 Jun 2010 13:26:27 +0200 Heinz Mauelshagen <[email protected]> wrote:
Quoted 14 lines · Neil Brown

Hi Heinz,

Quoted 8 lines · On Tue, 15 Jun 2010 15:23:26 +0200 Heinz Mauelshagen <[email protected]> wrote:

Neil,

for missing devices we (re)load the table with error mapped device(s) in their place rather than identifying them with a special char/string. Did you go for the later in order to avoid some MD hassle with error targets being accessed by the MD personalities? If not so, we can drop the special '-' char support to identify missing devices.

Quoted 3 lines

When raid5 determines that a device has failed and so it will not write to it again, it must be sure that the failure is record in the metadata before it proceeds with that decision not to even try writing to the failed device.

Yes, that's the mandatory order to avoid data loss.

Quoted 6 lines

Otherwise a crash/restart before the metadata was safely updated would result in corruption.

This means that it must be possible for user-space to explicitly tell the raid5 that a device is known to be faulty.
Doing that through the constructor seems the natural way to do it.

Quoted 6 lines

If /dev/mapper/error with an error target mapping would be used instead, would that cause consistency troubles to the MD personality if accessing those rather than the NULL device solution with the '-' argument you have now?
If not so, we could drop the '-' support, which I'm aiming at.

I would need to differentiate between /dev/mapper/error (where errors are expected) and any other device (where errors are not expected). How do you propose I do that?

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines

I'm thinking about adding a ctr wrapper to allow us to keep the "raid45" ctr interfaces semantics (ie. the arguments) and the new interface to "raid456" if you don't have objections. That would be implemented by 2 targets being registered implemented in one module.

Quoted 7 lines
Quoted 2 lines

I have no strong objections, though having two targets that do almost the same things seems rather ugly.

Keep in mind it'll only be another target_type structs, another raid_ctr() function plus another (de)registration of the respective target. The ctr functions for raid45 and raid456 can share moset of the code anyway.

Sure.

Quoted 5 lines
Quoted 3 lines

Is there existing published code that uses the extra arguments to raid45?

Yes, dmraid with a list of RAID5 supporting metadata format handlers.

I just had a look at the latest dmraid from CVS and the only place that I can find where a raid45 target is created is in lib/activate/activate.c in _dm_raid45_bol.
This only uses arguments that my code understands.
What am I missing?

Thanks,
NeilBrown

#32 lore raw
Quoted 38 lines · Neil Brown
Quoted 28 lines · On Wed, 16 Jun 2010 13:26:27 +0200 Heinz Mauelshagen <[email protected]> wrote:
Quoted 14 lines · Neil Brown

Hi Heinz,

Quoted 8 lines · On Tue, 15 Jun 2010 15:23:26 +0200 Heinz Mauelshagen <[email protected]> wrote:

Neil,

for missing devices we (re)load the table with error mapped device(s) in their place rather than identifying them with a special char/string. Did you go for the later in order to avoid some MD hassle with error targets being accessed by the MD personalities? If not so, we can drop the special '-' char support to identify missing devices.

Quoted 3 lines

When raid5 determines that a device has failed and so it will not write to it again, it must be sure that the failure is record in the metadata before it proceeds with that decision not to even try writing to the failed device.

Yes, that's the mandatory order to avoid data loss.

Quoted 6 lines

Otherwise a crash/restart before the metadata was safely updated would result in corruption.

This means that it must be possible for user-space to explicitly tell the raid5 that a device is known to be faulty.
Doing that through the constructor seems the natural way to do it.

Quoted 6 lines

If /dev/mapper/error with an error target mapping would be used instead, would that cause consistency troubles to the MD personality if accessing those rather than the NULL device solution with the '-' argument you have now?
If not so, we could drop the '-' support, which I'm aiming at.

Quoted 3 lines

I would need to differentiate between /dev/mapper/error (where errors are expected) and any other device (where errors are not expected). How do you propose I do that?

Is that differentiation mandatory for the MD personality at all?

You'll get immediate and durable errors from the error target and hence you'd instantly drop the leg.

Quoted 22 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines

I'm thinking about adding a ctr wrapper to allow us to keep the "raid45" ctr interfaces semantics (ie. the arguments) and the new interface to "raid456" if you don't have objections. That would be implemented by 2 targets being registered implemented in one module.

Quoted 7 lines
Quoted 2 lines

I have no strong objections, though having two targets that do almost the same things seems rather ugly.

Keep in mind it'll only be another target_type structs, another raid_ctr() function plus another (de)registration of the respective target. The ctr functions for raid45 and raid456 can share moset of the code anyway.

Sure.

Quoted 4 lines
Quoted 3 lines

Is there existing published code that uses the extra arguments to raid45?

Quoted 7 lines
Quoted 1 line

Yes, dmraid with a list of RAID5 supporting metadata format handlers.

I just had a look at the latest dmraid from CVS and the only place that I can find where a raid45 target is created is in lib/activate/activate.c in _dm_raid45_bol.
This only uses arguments that my code understands.
What am I missing?

I was arguing against breaking API compatibility.
dm-raid45.c is being shipped by distros.

Thanks,
Heinz

Quoted 2 lines

Thanks,
NeilBrown

author
#33 lore raw
Quoted 39 lines · On Thu, 17 Jun 2010 12:47:00 +0200 Heinz Mauelshagen <[email protected]> wrote:
Quoted 38 lines · Neil Brown
Quoted 34 lines · On Wed, 16 Jun 2010 13:26:27 +0200 Heinz Mauelshagen <[email protected]> wrote:
Quoted 14 lines · Neil Brown

Hi Heinz,

Quoted 8 lines · On Tue, 15 Jun 2010 15:23:26 +0200 Heinz Mauelshagen <[email protected]> wrote:

Neil,

for missing devices we (re)load the table with error mapped device(s) in their place rather than identifying them with a special char/string. Did you go for the later in order to avoid some MD hassle with error targets being accessed by the MD personalities? If not so, we can drop the special '-' char support to identify missing devices.

Quoted 3 lines

When raid5 determines that a device has failed and so it will not write to it again, it must be sure that the failure is record in the metadata before it proceeds with that decision not to even try writing to the failed device.

Yes, that's the mandatory order to avoid data loss.

Quoted 6 lines

Otherwise a crash/restart before the metadata was safely updated would result in corruption.

This means that it must be possible for user-space to explicitly tell the raid5 that a device is known to be faulty.
Doing that through the constructor seems the natural way to do it.

If /dev/mapper/error with an error target mapping would be used instead, would that cause consistency troubles to the MD personality if accessing those rather than the NULL device solution with the '-' argument you have now?
If not so, we could drop the '-' support, which I'm aiming at.

Quoted 5 lines
Quoted 3 lines

I would need to differentiate between /dev/mapper/error (where errors are expected) and any other device (where errors are not expected). How do you propose I do that?

Is that differentiation mandatory for the MD personality at all?

I thought we had already agreed that it was.

If the metadata records that a device is working, then raid5 will not expect an error and if it gets one it must wait for the metadata to be updated. If the metadata records that a device is not working, then raid5 will never bother even trying to write to that device.

So yes, and important distinction.

I imagine that the dmevent handler would handle a failure by updating the metadata, then dismantling the array and recreating it with the failed device missing. Given that in some cases it would want to do this anyway to introduce a spare, and as I think this is how dm-raid1 errors are handled, it seems the logical approach.
I note that your dm-raid45 code uses a message to "allow_writes". Using that could be racy unless you suspend the device before updating the metadata, and if you do that you may as well use the suspend/resume sequence as part of re-enabling writes. (???)

This is probably just a question of interface design, and we both naturally prefer the one we came up with first :-)

I guess it just feels clumsy for the raid5 driver to have to try to read and fail rather than simply being told up front not to bother trying. With md/raid5, a read failure will be followed by an attempt to write out the good data in case it was a simple media error. Having it do that every time you start a degraded array seems extra pointless....

Quoted 28 lines

You'll get immediate and durable errors from the error target and hence you'd instantly drop the leg.

Quoted 24 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines

I'm thinking about adding a ctr wrapper to allow us to keep the "raid45" ctr interfaces semantics (ie. the arguments) and the new interface to "raid456" if you don't have objections. That would be implemented by 2 targets being registered implemented in one module.

Quoted 7 lines
Quoted 2 lines

I have no strong objections, though having two targets that do almost the same things seems rather ugly.

Keep in mind it'll only be another target_type structs, another raid_ctr() function plus another (de)registration of the respective target. The ctr functions for raid45 and raid456 can share moset of the code anyway.

Sure.

Quoted 5 lines
Quoted 3 lines

Is there existing published code that uses the extra arguments to raid45?

Yes, dmraid with a list of RAID5 supporting metadata format handlers.

Quoted 9 lines
Quoted 5 lines

I just had a look at the latest dmraid from CVS and the only place that I can find where a raid45 target is created is in lib/activate/activate.c in _dm_raid45_bol.
This only uses arguments that my code understands.
What am I missing?

I was arguing against breaking API compatibility.
dm-raid45.c is being shipped by distros.

Shipped by distros which all have an 'upstream first' policy, and are fully aware of the possible consequences of not following it :-)

I'm not really that fussed about the API. I don't like seeing APIs that I think are badly designed go into Linux, but plenty of them do and one more isn't going to make much difference.

Approving an API is really a job for the dm maintainer. Is that Alasdair? Having stated my case I'm happy to accept whatever he prefers.

NeilBrown

Quoted 6 lines

Thanks,
Heinz

Quoted 2 lines

Thanks,
NeilBrown

#34 lore raw
Quoted 42 lines · Neil Brown
Quoted 39 lines · On Thu, 17 Jun 2010 12:47:00 +0200 Heinz Mauelshagen <[email protected]> wrote:
Quoted 38 lines · Neil Brown
Quoted 34 lines · On Wed, 16 Jun 2010 13:26:27 +0200 Heinz Mauelshagen <[email protected]> wrote:
Quoted 14 lines · Neil Brown

Hi Heinz,

Quoted 8 lines · On Tue, 15 Jun 2010 15:23:26 +0200 Heinz Mauelshagen <[email protected]> wrote:

Neil,

for missing devices we (re)load the table with error mapped device(s) in their place rather than identifying them with a special char/string. Did you go for the later in order to avoid some MD hassle with error targets being accessed by the MD personalities? If not so, we can drop the special '-' char support to identify missing devices.

Quoted 3 lines

When raid5 determines that a device has failed and so it will not write to it again, it must be sure that the failure is record in the metadata before it proceeds with that decision not to even try writing to the failed device.

Yes, that's the mandatory order to avoid data loss.

Quoted 6 lines

Otherwise a crash/restart before the metadata was safely updated would result in corruption.

This means that it must be possible for user-space to explicitly tell the raid5 that a device is known to be faulty.
Doing that through the constructor seems the natural way to do it.

If /dev/mapper/error with an error target mapping would be used instead, would that cause consistency troubles to the MD personality if accessing those rather than the NULL device solution with the '-' argument you have now?
If not so, we could drop the '-' support, which I'm aiming at.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

I would need to differentiate between /dev/mapper/error (where errors are expected) and any other device (where errors are not expected). How do you propose I do that?

Is that differentiation mandatory for the MD personality at all?

I thought we had already agreed that it was.

No ;)

The following is based on the assumption, that you're refering to MD metadata...

Quoted 5 lines

If the metadata records that a device is working, then raid5 will not expect an error and if it gets one it must wait for the metadata to be updated. If the metadata records that a device is not working, then raid5 will never bother even trying to write to that device.

So that will allow for a /dev/mapper/error device instead of the '-' because like you say, RAID5 is not going to write to it anyway.

And it shouldn't bother reading neither as long as the metadata tells it the drive is dead.

Quoted 6 lines

So yes, and important distinction.

I imagine that the dmevent handler would handle a failure by updating the metadata, then dismantling the array and recreating it with the failed device missing.

Yes, it updates the upspace metadata (ie. for lvm2/dmraid).

Quoted 4 lines

Given that in some cases it would want to do this anyway to introduce a spare, and as I think this is how dm-raid1 errors are handled, it seems the logical approach.
I note that your dm-raid45 code uses a message to "allow_writes".

Yes.

Quoted 3 lines

Using that could be racy unless you suspend the device before updating the metadata, and if you do that you may as well use the suspend/resume sequence as part of re-enabling writes. (???)

The message can be used optionally vs. suspend/resume pairs for performance reasons to allow for max reads to go through. Of course it needs to be called in the proper sequence (same applies to suspend/resume) and then it's not racy.

Quoted 3 lines

This is probably just a question of interface design, and we both naturally prefer the one we came up with first :-)

Well... ;-)

Quoted 3 lines

I guess it just feels clumsy for the raid5 driver to have to try to read and fail rather than simply being told up front not to bother trying.

If I understand you correctly, the state changes in the RAID5 personalities caused by a bad drive declared dead would lead to remembering the bad drive persistently and they (the personalities RAID4/5/6) wouldn't even bother accessing it, so it won't make a difference to put an error mapped device in place of the dead one.

Quoted 3 lines

With md/raid5, a read failure will be followed by an attempt to write out the good data in case it was a simple media error. Having it do that every time you start a degraded array seems extra pointless....

Hrm, getting confused here: so it'll still try reading even though the MD metadata reflects it to be dead?

/me trying to get twists sorted :)

Regards,
Heinz

Quoted 59 lines
Quoted 30 lines

You'll get immediate and durable errors from the error target and hence you'd instantly drop the leg.

Quoted 24 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines

I'm thinking about adding a ctr wrapper to allow us to keep the "raid45" ctr interfaces semantics (ie. the arguments) and the new interface to "raid456" if you don't have objections. That would be implemented by 2 targets being registered implemented in one module.

Quoted 7 lines
Quoted 2 lines

I have no strong objections, though having two targets that do almost the same things seems rather ugly.

Keep in mind it'll only be another target_type structs, another raid_ctr() function plus another (de)registration of the respective target. The ctr functions for raid45 and raid456 can share moset of the code anyway.

Sure.

Quoted 5 lines
Quoted 3 lines

Is there existing published code that uses the extra arguments to raid45?

Yes, dmraid with a list of RAID5 supporting metadata format handlers.

Quoted 9 lines
Quoted 5 lines

I just had a look at the latest dmraid from CVS and the only place that I can find where a raid45 target is created is in lib/activate/activate.c in _dm_raid45_bol.
This only uses arguments that my code understands.
What am I missing?

I was arguing against breaking API compatibility.
dm-raid45.c is being shipped by distros.

Shipped by distros which all have an 'upstream first' policy, and are fully aware of the possible consequences of not following it :-)

I'm not really that fussed about the API. I don't like seeing APIs that I think are badly designed go into Linux, but plenty of them do and one more isn't going to make much difference.

Approving an API is really a job for the dm maintainer. Is that Alasdair? Having stated my case I'm happy to accept whatever he prefers.

NeilBrown

Quoted 6 lines

Thanks,
Heinz

Quoted 2 lines

Thanks,
NeilBrown

author
#35 lore raw
Quoted 47 lines · On Fri, 18 Jun 2010 12:42:46 +0200 Heinz Mauelshagen <[email protected]> wrote:
Quoted 46 lines · Neil Brown
Quoted 43 lines · On Thu, 17 Jun 2010 12:47:00 +0200 Heinz Mauelshagen <[email protected]> wrote:
Quoted 41 lines · Neil Brown
Quoted 28 lines · On Wed, 16 Jun 2010 13:26:27 +0200 Heinz Mauelshagen <[email protected]> wrote:
Quoted 14 lines · Neil Brown

Hi Heinz,

Quoted 8 lines · On Tue, 15 Jun 2010 15:23:26 +0200 Heinz Mauelshagen <[email protected]> wrote:

Neil,

for missing devices we (re)load the table with error mapped device(s) in their place rather than identifying them with a special char/string. Did you go for the later in order to avoid some MD hassle with error targets being accessed by the MD personalities? If not so, we can drop the special '-' char support to identify missing devices.

Quoted 3 lines

When raid5 determines that a device has failed and so it will not write to it again, it must be sure that the failure is record in the metadata before it proceeds with that decision not to even try writing to the failed device.

Yes, that's the mandatory order to avoid data loss.

Quoted 6 lines

Otherwise a crash/restart before the metadata was safely updated would result in corruption.

This means that it must be possible for user-space to explicitly tell the raid5 that a device is known to be faulty.
Doing that through the constructor seems the natural way to do it.

Quoted 6 lines

If /dev/mapper/error with an error target mapping would be used instead, would that cause consistency troubles to the MD personality if accessing those rather than the NULL device solution with the '-' argument you have now?
If not so, we could drop the '-' support, which I'm aiming at.

I would need to differentiate between /dev/mapper/error (where errors are expected) and any other device (where errors are not expected). How do you propose I do that?

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

Is that differentiation mandatory for the MD personality at all?

I thought we had already agreed that it was.

No ;)

The following is based on the assumption, that you're refering to MD metadata...

Ahhh.... No, I'm not.
My dm-raid45 doesn't see any metadata directly.
Metadata much be handled entirely by user-space, whether lvm or dmraid or something else. It will pass the important information from the metadata to dm-raid45, largely via the target constructor line.

Quoted 9 lines
Quoted 6 lines

If the metadata records that a device is working, then raid5 will not expect an error and if it gets one it must wait for the metadata to be updated. If the metadata records that a device is not working, then raid5 will never bother even trying to write to that device.

So that will allow for a /dev/mapper/error device instead of the '-' because like you say, RAID5 is not going to write to it anyway.

But RAID5 doesn't *know* that it shouldn't write to it unless you tell it some how. That is what "-" means. It means "the metadata records this device has failed or missing, and so you should never try to read from or write to it".

Some how the RAID5 module needs to know that the device is failed and the this status is recorded in the metadata.
There seem to be two proposed ways for it to find this out.

1/ The target constructor line explicitly carries this information, e.g. by
giving "-" in place of a device name.
2/ RAID5 attempts IO, gets a failure, reports this to user-space, then
user-space sends the "allow_writes" message which implicitly says "all devices which you current think have failed, are recorded as such in the metadata".

Both can obviously work, and I can accept that there could be a place for "allow_writes", but I think that being able to give the information directly is a good thing.

Thanks,
NeilBrown

Quoted 117 lines

And it shouldn't bother reading neither as long as the metadata tells it the drive is dead.

Quoted 6 lines

So yes, and important distinction.

I imagine that the dmevent handler would handle a failure by updating the metadata, then dismantling the array and recreating it with the failed device missing.

Yes, it updates the upspace metadata (ie. for lvm2/dmraid).

Quoted 4 lines

Given that in some cases it would want to do this anyway to introduce a spare, and as I think this is how dm-raid1 errors are handled, it seems the logical approach.
I note that your dm-raid45 code uses a message to "allow_writes".

Yes.

Quoted 3 lines

Using that could be racy unless you suspend the device before updating the metadata, and if you do that you may as well use the suspend/resume sequence as part of re-enabling writes. (???)

The message can be used optionally vs. suspend/resume pairs for performance reasons to allow for max reads to go through. Of course it needs to be called in the proper sequence (same applies to suspend/resume) and then it's not racy.

Quoted 3 lines

This is probably just a question of interface design, and we both naturally prefer the one we came up with first :-)

Well... ;-)

Quoted 3 lines

I guess it just feels clumsy for the raid5 driver to have to try to read and fail rather than simply being told up front not to bother trying.

If I understand you correctly, the state changes in the RAID5 personalities caused by a bad drive declared dead would lead to remembering the bad drive persistently and they (the personalities RAID4/5/6) wouldn't even bother accessing it, so it won't make a difference to put an error mapped device in place of the dead one.

Quoted 3 lines

With md/raid5, a read failure will be followed by an attempt to write out the good data in case it was a simple media error. Having it do that every time you start a degraded array seems extra pointless....

Hrm, getting confused here: so it'll still try reading even though the MD metadata reflects it to be dead?

/me trying to get twists sorted :)

Regards,
Heinz

Quoted 59 lines
Quoted 30 lines

You'll get immediate and durable errors from the error target and hence you'd instantly drop the leg.

Quoted 24 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines

I'm thinking about adding a ctr wrapper to allow us to keep the "raid45" ctr interfaces semantics (ie. the arguments) and the new interface to "raid456" if you don't have objections. That would be implemented by 2 targets being registered implemented in one module.

Quoted 7 lines
Quoted 2 lines

I have no strong objections, though having two targets that do almost the same things seems rather ugly.

Keep in mind it'll only be another target_type structs, another raid_ctr() function plus another (de)registration of the respective target. The ctr functions for raid45 and raid456 can share moset of the code anyway.

Sure.

Quoted 5 lines
Quoted 3 lines

Is there existing published code that uses the extra arguments to raid45?

Yes, dmraid with a list of RAID5 supporting metadata format handlers.

Quoted 9 lines
Quoted 5 lines

I just had a look at the latest dmraid from CVS and the only place that I can find where a raid45 target is created is in lib/activate/activate.c in _dm_raid45_bol.
This only uses arguments that my code understands.
What am I missing?

I was arguing against breaking API compatibility.
dm-raid45.c is being shipped by distros.

Shipped by distros which all have an 'upstream first' policy, and are fully aware of the possible consequences of not following it :-)

I'm not really that fussed about the API. I don't like seeing APIs that I think are badly designed go into Linux, but plenty of them do and one more isn't going to make much difference.

Approving an API is really a job for the dm maintainer. Is that Alasdair? Having stated my case I'm happy to accept whatever he prefers.

NeilBrown

Quoted 6 lines

Thanks,
Heinz

Quoted 2 lines

Thanks,
NeilBrown