Merge branch 'master' into 2018-05-11-fork-libdm

1 files changed, 123 insertions, 82 deletions

diff --git a/lib/label/label.c b/lib/label/label.c
index ba34c7d31..baa81370f 100644
--- a/lib/label/label.c
+++ b/lib/label/label.c
@@ -251,9 +251,11 @@ static bool _in_bcache(struct device *dev)
 
 static struct labeller *_find_lvm_header(struct device *dev,
 				   char *scan_buf,
+				   uint32_t scan_buf_sectors,
 				   char *label_buf,
 				   uint64_t *label_sector,
-				   uint64_t scan_sector)
+				   uint64_t block_sector,
+				   uint64_t start_sector)
 {
 	struct labeller_i *li;
 	struct labeller *labeller_ret = NULL;
@@ -266,25 +268,34 @@ static struct labeller *_find_lvm_header(struct device *dev,
 	 * and copy it into label_buf.
 	 */
 
-	for (sector = 0; sector < LABEL_SCAN_SECTORS;
+	for (sector = start_sector; sector < start_sector + LABEL_SCAN_SECTORS;
 	     sector += LABEL_SIZE >> SECTOR_SHIFT) {
+
+		/*
+		 * The scan_buf passed in is a bcache block, which is
+		 * BCACHE_BLOCK_SIZE_IN_SECTORS large.  So if start_sector is
+		 * one of the last couple sectors in that buffer, we need to
+		 * break early.
+		 */
+		if (sector >= scan_buf_sectors)
+			break;
+
 		lh = (struct label_header *) (scan_buf + (sector << SECTOR_SHIFT));
 
 		if (!strncmp((char *)lh->id, LABEL_ID, sizeof(lh->id))) {
 			if (found) {
 				log_error("Ignoring additional label on %s at sector %llu",
-					  dev_name(dev), (unsigned long long)(sector + scan_sector));
+					  dev_name(dev), (unsigned long long)(block_sector + sector));
 			}
-			if (xlate64(lh->sector_xl) != sector + scan_sector) {
-				log_very_verbose("%s: Label for sector %llu found at sector %llu - ignoring.",
-						 dev_name(dev),
-						 (unsigned long long)xlate64(lh->sector_xl),
-						 (unsigned long long)(sector + scan_sector));
+			if (xlate64(lh->sector_xl) != sector) {
+				log_warn("%s: Label for sector %llu found at sector %llu - ignoring.",
+					 dev_name(dev),
+					 (unsigned long long)xlate64(lh->sector_xl),
+					 (unsigned long long)(block_sector + sector));
 				continue;
 			}
-			if (calc_crc(INITIAL_CRC, (uint8_t *)&lh->offset_xl, LABEL_SIZE -
-				     ((uint8_t *) &lh->offset_xl - (uint8_t *) lh)) !=
-			    xlate32(lh->crc_xl)) {
+			if (calc_crc(INITIAL_CRC, (uint8_t *)&lh->offset_xl,
+				     LABEL_SIZE - ((uint8_t *) &lh->offset_xl - (uint8_t *) lh)) != xlate32(lh->crc_xl)) {
 				log_very_verbose("Label checksum incorrect on %s - ignoring", dev_name(dev));
 				continue;
 			}
@@ -293,14 +304,14 @@ static struct labeller *_find_lvm_header(struct device *dev,
 		}
 
 		dm_list_iterate_items(li, &_labellers) {
-			if (li->l->ops->can_handle(li->l, (char *) lh, sector + scan_sector)) {
+			if (li->l->ops->can_handle(li->l, (char *) lh, block_sector + sector)) {
 				log_very_verbose("%s: %s label detected at sector %llu", 
 						 dev_name(dev), li->name,
-						 (unsigned long long)(sector + scan_sector));
+						 (unsigned long long)(block_sector + sector));
 				if (found) {
 					log_error("Ignoring additional label on %s at sector %llu",
 						  dev_name(dev),
-						  (unsigned long long)(sector + scan_sector));
+						  (unsigned long long)(block_sector + sector));
 					continue;
 				}
 
@@ -309,7 +320,7 @@ static struct labeller *_find_lvm_header(struct device *dev,
 
 				memcpy(label_buf, lh, LABEL_SIZE);
 				if (label_sector)
-					*label_sector = sector + scan_sector;
+					*label_sector = block_sector + sector;
 				break;
 			}
 		}
@@ -329,7 +340,9 @@ static struct labeller *_find_lvm_header(struct device *dev,
  * are performed in the processing functions to get that data.
  */
 static int _process_block(struct cmd_context *cmd, struct dev_filter *f,
-			  struct device *dev, struct block *bb, int *is_lvm_device)
+			  struct device *dev, struct block *bb,
+			  uint64_t block_sector, uint64_t start_sector,
+			  int *is_lvm_device)
 {
 	char label_buf[LABEL_SIZE] __attribute__((aligned(8)));
 	struct label *label = NULL;
@@ -345,7 +358,7 @@ static int _process_block(struct cmd_context *cmd, struct dev_filter *f,
 	 * data had been read (here).  They set this flag to indicate that the
 	 * filters should be retested now that data from the device is ready.
 	 */
-	if (cmd && (dev->flags & DEV_FILTER_AFTER_SCAN)) {
+	if (f && (dev->flags & DEV_FILTER_AFTER_SCAN)) {
 		dev->flags &= ~DEV_FILTER_AFTER_SCAN;
 
 		log_debug_devs("Scan filtering %s", dev_name(dev));
@@ -374,7 +387,7 @@ static int _process_block(struct cmd_context *cmd, struct dev_filter *f,
 	 * FIXME: we don't need to copy one sector from bb->data into label_buf,
 	 * we can just point label_buf at one sector in ld->buf.
 	 */
-	if (!(labeller = _find_lvm_header(dev, bb->data, label_buf, &sector, 0))) {
+	if (!(labeller = _find_lvm_header(dev, bb->data, BCACHE_BLOCK_SIZE_IN_SECTORS, label_buf, &sector, block_sector, start_sector))) {
 
 		/*
 		 * Non-PVs exit here
@@ -567,7 +580,7 @@ static int _scan_list(struct cmd_context *cmd, struct dev_filter *f,
 		} else {
 			log_debug_devs("Processing data from device %s fd %d block %p", dev_name(devl->dev), devl->dev->bcache_fd, bb);
 
-			ret = _process_block(cmd, f, devl->dev, bb, &is_lvm_device);
+			ret = _process_block(cmd, f, devl->dev, bb, 0, 0, &is_lvm_device);
 
 			if (!ret && is_lvm_device) {
 				log_debug_devs("Scan failed to process %s", dev_name(devl->dev));
@@ -610,32 +623,46 @@ static int _scan_list(struct cmd_context *cmd, struct dev_filter *f,
 	return 1;
 }
 
+/*
+ * How many blocks to set up in bcache?  Is 1024 a good max?
+ *
+ * Currently, we tell bcache to set up N blocks where N
+ * is the number of devices that are going to be scanned.
+ * Reasons why this number may not be be a good choice:
+ *
+ * - there may be a lot of non-lvm devices, which
+ *   would make this number larger than necessary
+ *
+ * - each lvm device may use more than one cache
+ *   block if the metadata is large enough or it
+ *   uses more than one metadata area, which
+ *   would make this number smaller than it
+ *   should be for the best performance.
+ *
+ * This is even more tricky to estimate when lvmetad
+ * is used, because it's hard to predict how many
+ * devs might need to be scanned when using lvmetad.
+ * This currently just sets up bcache with MIN blocks.
+ */
+
 #define MIN_BCACHE_BLOCKS 32
+#define MAX_BCACHE_BLOCKS 1024
 
 static int _setup_bcache(int cache_blocks)
 {
 	struct io_engine *ioe;
 
-	/* No devices can happen, just create bcache with any small number. */
 	if (cache_blocks < MIN_BCACHE_BLOCKS)
 		cache_blocks = MIN_BCACHE_BLOCKS;
 
-	/*
-	 * 100 is arbitrary, it's the max number of concurrent aio's
-	 * possible, i.e, the number of devices that can be read at
-	 * once.  Should this be configurable?
-	 */
+	if (cache_blocks > MAX_BCACHE_BLOCKS)
+		cache_blocks = MAX_BCACHE_BLOCKS;
+
 	if (!(ioe = create_async_io_engine())) {
 		log_error("Failed to create bcache io engine.");
 		return 0;
 	}
 
-	/*
-	 * Configure one cache block for each device on the system.
-	 * We won't generally need to cache that many because some
-	 * of the devs will not be lvm devices, and we don't need
-	 * an entry for those.  We might want to change this.
-	 */
 	if (!(scan_bcache = bcache_create(BCACHE_BLOCK_SIZE_IN_SECTORS, cache_blocks, ioe))) {
 		log_error("Failed to create bcache with %d cache blocks.", cache_blocks);
 		return 0;
@@ -653,7 +680,7 @@ int label_scan(struct cmd_context *cmd)
 {
 	struct dm_list all_devs;
 	struct dev_iter *iter;
-	struct device_list *devl;
+	struct device_list *devl, *devl2;
 	struct device *dev;
 
 	log_debug_devs("Finding devices to scan");
@@ -695,16 +722,17 @@ int label_scan(struct cmd_context *cmd)
 	log_debug_devs("Found %d devices to scan", dm_list_size(&all_devs));
 
 	if (!scan_bcache) {
-		/*
-		 * FIXME: there should probably be some max number of
-		 * cache blocks we use when setting up bcache.
-		 */
 		if (!_setup_bcache(dm_list_size(&all_devs)))
 			return 0;
 	}
 
 	_scan_list(cmd, cmd->full_filter, &all_devs, NULL);
 
+	dm_list_iterate_items_safe(devl, devl2, &all_devs) {
+		dm_list_del(&devl->list);
+		dm_free(devl);
+	}
+
 	return 1;
 }
 
@@ -834,9 +862,7 @@ void label_scan_destroy(struct cmd_context *cmd)
  * device, this is not a commonly used function.
  */
 
-/* FIXME: remove unused_sector arg */
-
-int label_read(struct device *dev, struct label **labelp, uint64_t unused_sector)
+int label_read(struct device *dev)
 {
 	struct dm_list one_dev;
 	struct device_list *devl;
@@ -856,17 +882,7 @@ int label_read(struct device *dev, struct label **labelp, uint64_t unused_sector
 
 	_scan_list(NULL, NULL, &one_dev, &failed);
 
-	/*
-	 * FIXME: this ugliness of returning a pointer to the label is
-	 * temporary until the callers can be updated to not use this.
-	 */
-	if (labelp) {
-		struct lvmcache_info *info;
-
-		info = lvmcache_info_from_pvid(dev->pvid, dev, 1);
-		if (info)
-			*labelp = lvmcache_get_label(info);
-	}
+	dm_free(devl);
 
 	if (failed)
 		return 0;
@@ -875,25 +891,66 @@ int label_read(struct device *dev, struct label **labelp, uint64_t unused_sector
 
 /*
  * Read a label from a specfic, non-zero sector.  This is used in only
- * one place: pvck -> pv_analyze.
+ * one place: pvck/pv_analyze.
  */
 
-int label_read_sector(struct device *dev, struct label **labelp, uint64_t scan_sector)
+int label_read_sector(struct device *dev, uint64_t read_sector)
 {
-	if (scan_sector) {
-		/* TODO: not yet implemented */
-		/* When is this done?  When does it make sense?  Is it actually possible? */
-		return 0;
+	struct block *bb = NULL;
+	uint64_t block_num;
+	uint64_t block_sector;
+	uint64_t start_sector;
+	int is_lvm_device = 0;
+	int result;
+	int ret;
+
+	block_num = read_sector / BCACHE_BLOCK_SIZE_IN_SECTORS;
+	block_sector = block_num * BCACHE_BLOCK_SIZE_IN_SECTORS;
+	start_sector = read_sector % BCACHE_BLOCK_SIZE_IN_SECTORS;
+
+	label_scan_open(dev);
+
+	bcache_prefetch(scan_bcache, dev->bcache_fd, block_num);
+
+	if (!bcache_get(scan_bcache, dev->bcache_fd, block_num, 0, &bb)) {
+		log_error("Scan failed to read %s at %llu",
+			  dev_name(dev), (unsigned long long)block_num);
+		ret = 0;
+		goto out;
 	}
 
-	return label_read(dev, labelp, 0);
+	/*
+	 * TODO: check if scan_sector is larger than the bcache block size.
+	 * If it is, we need to fetch a later block from bcache.
+	 */
+
+	result = _process_block(NULL, NULL, dev, bb, block_sector, start_sector, &is_lvm_device);
+
+	if (!result && is_lvm_device) {
+		log_error("Scan failed to process %s", dev_name(dev));
+		ret = 0;
+		goto out;
+	}
+
+	if (!result || !is_lvm_device) {
+		log_error("Could not find LVM label on %s", dev_name(dev));
+		ret = 0;
+		goto out;
+	}
+
+	ret = 1;
+out:
+	if (bb)
+		bcache_put(bb);
+	return ret;
 }
 
 /*
  * This is only needed when commands are using lvmetad, in which case they
  * don't do an initial label_scan, but may later need to rescan certain devs
  * from disk and call this function.  FIXME: is there some better number to
- * choose here?
+ * choose here?  How should we predict the number of devices that might need
+ * scanning when using lvmetad?
  */
 
 int label_scan_setup_bcache(void)
@@ -922,18 +979,10 @@ int label_scan_open(struct device *dev)
 
 bool dev_read_bytes(struct device *dev, uint64_t start, size_t len, void *data)
 {
-	int ret;
-
 	if (!scan_bcache) {
-		if (!dev_open_readonly(dev))
-			return false;
-
-		ret = dev_read(dev, start, len, 0, data);
-
-		if (!dev_close(dev))
-			stack;
-
-		return ret ? true : false;
+		/* Should not happen */
+		log_error("dev_read bcache not set up %s", dev_name(dev));
+		return false;
 	}
 
 	if (dev->bcache_fd <= 0) {
@@ -956,21 +1005,13 @@ bool dev_read_bytes(struct device *dev, uint64_t start, size_t len, void *data)
 
 bool dev_write_bytes(struct device *dev, uint64_t start, size_t len, void *data)
 {
-	int ret;
-
 	if (test_mode())
 		return true;
 
 	if (!scan_bcache) {
-		if (!dev_open(dev))
-			return false;
-
-		ret = dev_write(dev, start, len, 0, data);
-
-		if (!dev_close(dev))
-			stack;
-
-		return ret ? true : false;
+		/* Should not happen */
+		log_error("dev_write bcache not set up %s", dev_name(dev));
+		return false;
 	}
 
 	if (dev->bcache_fd <= 0) {
@@ -1003,7 +1044,7 @@ bool dev_write_zeros(struct device *dev, uint64_t start, size_t len)
 		return true;
 
 	if (!scan_bcache) {
-		log_error("dev_write_zeros %s bcache not set up", dev_name(dev));
+		log_error("dev_write_zeros bcache not set up %s", dev_name(dev));
 		return false;
 	}
 
@@ -1037,7 +1078,7 @@ bool dev_set_bytes(struct device *dev, uint64_t start, size_t len, uint8_t val)
 		return true;
 
 	if (!scan_bcache) {
-		log_error("dev_set_bytes %s bcache not set up", dev_name(dev));
+		log_error("dev_set_bytes bcache not set up %s", dev_name(dev));
 		return false;
 	}