device: Fix basic async I/O error handlingdev-agk-tmp

8 files changed, 67 insertions, 41 deletions

diff --git a/lib/cache/lvmcache.c b/lib/cache/lvmcache.c
index 767829083..4022ea64c 100644
--- a/lib/cache/lvmcache.c
+++ b/lib/cache/lvmcache.c
@@ -1099,6 +1099,7 @@ next:
 }
 
 /* Track the number of outstanding label reads */
+/* FIXME Switch to struct and also track failed */
 static void _process_label_data(int failed, unsigned ioflags, void *context, const void *data)
 {
 	int *nr_labels_outstanding = context;
@@ -1160,6 +1161,7 @@ int lvmcache_label_scan(struct cmd_context *cmd)
 	_destroy_duplicate_device_list(&_found_duplicate_devs);
 
 	while ((dev = dev_iter_get(iter))) {
+		log_debug_io("Scanning device %s", dev_name(dev));
 		nr_labels_outstanding++;
 		if (!label_read_callback(dev, UINT64_C(0), AIO_SUPPORTED_CODE_PATH, _process_label_data, &nr_labels_outstanding))
 			nr_labels_outstanding--;
diff --git a/lib/config/config.c b/lib/config/config.c
index c69ac69fa..97c5db8a1 100644
--- a/lib/config/config.c
+++ b/lib/config/config.c
@@ -609,8 +609,11 @@ int config_file_read_fd(struct dm_pool *mem, struct dm_config_tree *cft, struct
 				goto_out;
 			_process_config_file_buffer(0, ioflags, pcfp, buf);
 			dm_free((void *)buf);
-		} else if (!dev_read_callback(dev, (uint64_t) offset, size, reason, ioflags, _process_config_file_buffer, pcfp))
-			goto_out;
+		} else {
+			dev_read_callback(dev, (uint64_t) offset, size, reason, ioflags, _process_config_file_buffer, pcfp);
+			if (config_file_read_fd_callback)
+				return 1;
+		}
 		r = pcfp->ret;
 	}
 
diff --git a/lib/device/dev-io.c b/lib/device/dev-io.c
index 460c874c1..31506e882 100644
--- a/lib/device/dev-io.c
+++ b/lib/device/dev-io.c
@@ -245,7 +245,8 @@ int dev_async_getevents(void)
 			_release_devbuf(devbuf);
 			if (dev_read_callback_fn)
 				dev_read_callback_fn(1, AIO_SUPPORTED_CODE_PATH, dev_read_callback_context, NULL);
-			r = 0;
+			else
+				r = 0;
 		}
 	}
 
@@ -1080,24 +1081,28 @@ static void _dev_inc_error_count(struct device *dev)
 }
 
 /*
- * Data is returned (read-only) at DEV_DEVBUF_DATA(dev, reason)
+ * Data is returned (read-only) at DEV_DEVBUF_DATA(dev, reason).
+ * If dev_read_callback_fn is supplied, we always return 1 and take
+ * responsibility for calling it exactly once.  This might happen before the
+ * function returns (if there's an error or the I/O is synchronous) or after.
+ * Any error is passed to that function, which must track it if required.
  */
-int dev_read_callback(struct device *dev, uint64_t offset, size_t len, dev_io_reason_t reason,
-		      unsigned ioflags, lvm_callback_fn_t dev_read_callback_fn, void *callback_context)
+static int _dev_read_callback(struct device *dev, uint64_t offset, size_t len, dev_io_reason_t reason,
+			      unsigned ioflags, lvm_callback_fn_t dev_read_callback_fn, void *callback_context)
 {
 	struct device_area where;
 	struct device_buffer *devbuf;
 	uint64_t buf_end;
 	int cached = 0;
-	int ret = 1;
+	int ret = 0;
 
 	if (!dev->open_count) {
 		log_error(INTERNAL_ERROR "Attempt to access device %s while closed.", dev_name(dev));
-		return 0;
+		goto out;
 	}
 
 	if (!_dev_is_valid(dev))
-		return 0;
+		goto_out;
 
 	/*
 	 * Can we satisfy this from data we stored last time we read?
@@ -1110,6 +1115,7 @@ int dev_read_callback(struct device *dev, uint64_t offset, size_t len, dev_io_re
 			devbuf->data_offset = offset - devbuf->where.start;
 			log_debug_io("Cached read for %" PRIu64 " bytes at %" PRIu64 " on %s (for %s)",
 				     (uint64_t) len, (uint64_t) offset, dev_name(dev), _reason_text(reason));
+			ret = 1;
 			goto out;
 		}
 	}
@@ -1126,16 +1132,26 @@ int dev_read_callback(struct device *dev, uint64_t offset, size_t len, dev_io_re
 
 out:
 	/* If we had an error or this was sync I/O, pass the result to any callback fn */
-	if ((!ret || !_aio_ctx || !aio_supported_code_path(ioflags) || cached) && dev_read_callback_fn)
+	if ((!ret || !_aio_ctx || !aio_supported_code_path(ioflags) || cached) && dev_read_callback_fn) {
 		dev_read_callback_fn(!ret, ioflags, callback_context, DEV_DEVBUF_DATA(dev, reason));
+		return 1;
+	}
 
 	return ret;
 }
 
+void dev_read_callback(struct device *dev, uint64_t offset, size_t len, dev_io_reason_t reason,
+		      unsigned ioflags, lvm_callback_fn_t dev_read_callback_fn, void *callback_context)
+{
+	/* Always returns 1 if callback fn is supplied */
+	if (!_dev_read_callback(dev, offset, len, reason, ioflags, dev_read_callback_fn, callback_context))
+		log_error(INTERNAL_ERROR "_dev_read_callback failed");
+}
+
 /* Returns pointer to read-only buffer. Caller does not free it.  */
 const char *dev_read(struct device *dev, uint64_t offset, size_t len, dev_io_reason_t reason)
 {
-	if (!dev_read_callback(dev, offset, len, reason, 0, NULL, NULL))
+	if (!_dev_read_callback(dev, offset, len, reason, 0, NULL, NULL))
 		return_NULL;
 
 	return DEV_DEVBUF_DATA(dev, reason);
@@ -1144,7 +1160,7 @@ const char *dev_read(struct device *dev, uint64_t offset, size_t len, dev_io_rea
 /* Read into supplied retbuf owned by the caller. */
 int dev_read_buf(struct device *dev, uint64_t offset, size_t len, dev_io_reason_t reason, void *retbuf)
 {
-	if (!dev_read_callback(dev, offset, len, reason, 0, NULL, NULL)) {
+	if (!_dev_read_callback(dev, offset, len, reason, 0, NULL, NULL)) {
 		log_error("Read from %s failed", dev_name(dev));
 		return 0;
 	}
diff --git a/lib/device/device.h b/lib/device/device.h
index 90611399f..ea71d00a4 100644
--- a/lib/device/device.h
+++ b/lib/device/device.h
@@ -184,9 +184,9 @@ const char *dev_read(struct device *dev, uint64_t offset, size_t len, dev_io_rea
 const char *dev_read_circular(struct device *dev, uint64_t offset, size_t len,
 			      uint64_t offset2, size_t len2, dev_io_reason_t reason);
 
-/* Passes the data to dev_read_callback_fn */
-int dev_read_callback(struct device *dev, uint64_t offset, size_t len, dev_io_reason_t reason,
-		      unsigned ioflags, lvm_callback_fn_t dev_read_callback_fn, void *callback_context);
+/* Passes the data (or error) to dev_read_callback_fn */
+void dev_read_callback(struct device *dev, uint64_t offset, size_t len, dev_io_reason_t reason,
+		       unsigned ioflags, lvm_callback_fn_t dev_read_callback_fn, void *callback_context);
 
 /* Read data and copy it into a supplied private buffer. */
 /* Only use for tiny reads or on unimportant code paths. */
diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index 7f63ad62a..e1603e737 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -385,8 +385,8 @@ static void _process_raw_mda_header(int failed, unsigned ioflags, void *context,
 bad:
 	prmp->ret = 0;
 out:
-	if (prmp->ret && prmp->mdah_callback_fn)
-		prmp->mdah_callback_fn(0, ioflags, prmp->mdah_callback_context, mdah);
+	if (prmp->mdah_callback_fn)
+		prmp->mdah_callback_fn(!prmp->ret, ioflags, prmp->mdah_callback_context, mdah);
 }
 
 static struct mda_header *_raw_read_mda_header(struct dm_pool *mem, struct device_area *dev_area, int primary_mda,
@@ -417,14 +417,15 @@ static struct mda_header *_raw_read_mda_header(struct dm_pool *mem, struct devic
 	prmp->mdah_callback_context = mdah_callback_context;
 	prmp->ret = 1;
 
-	if (!dev_read_callback(dev_area->dev, dev_area->start, MDA_HEADER_SIZE, MDA_HEADER_REASON(primary_mda),
-			       ioflags, _process_raw_mda_header, prmp))
-		stack;
+	dev_read_callback(dev_area->dev, dev_area->start, MDA_HEADER_SIZE, MDA_HEADER_REASON(primary_mda),
+			  ioflags, _process_raw_mda_header, prmp);
+	if (mdah_callback_fn)
+		return mdah;
 
 	if (!prmp->ret)
 		return_NULL;
-
-	return mdah;
+	else
+		return mdah;
 }
 
 struct mda_header *raw_read_mda_header(struct dm_pool *mem, struct device_area *dev_area, int primary_mda)
@@ -1404,8 +1405,7 @@ static void _vgname_from_mda_process(int failed, unsigned ioflags, void *context
 	}
 
 out:
-	if (vfmp->ret)
-		vfmp->update_vgsummary_fn(0, ioflags, vfmp->update_vgsummary_context, vfmp->vgsummary);
+	vfmp->update_vgsummary_fn(!vfmp->ret, ioflags, vfmp->update_vgsummary_context, vfmp->vgsummary);
 }
 
 static void _vgname_from_mda_validate(int failed, unsigned ioflags, void *context, const void *data)
@@ -1469,7 +1469,8 @@ static void _vgname_from_mda_validate(int failed, unsigned ioflags, void *contex
 	}
 
 out:
-	;
+	if (!vfmp->ret && vfmp->update_vgsummary_fn)
+		vfmp->update_vgsummary_fn(1, ioflags, vfmp->update_vgsummary_context, vfmp->vgsummary);
 }
 
 int vgname_from_mda(const struct format_type *fmt,
@@ -1517,11 +1518,12 @@ int vgname_from_mda(const struct format_type *fmt,
 
 	/* Do quick check for a vgname */
 	/* We cannot read the full metadata here because the name has to be validated before we use the size field */
-	if (!dev_read_callback(dev_area->dev, dev_area->start + rlocn->offset, NAME_LEN, MDA_CONTENT_REASON(primary_mda),
-			       ioflags, _vgname_from_mda_validate, vfmp))
-		return_0;
-
-	return vfmp->ret;
+	dev_read_callback(dev_area->dev, dev_area->start + rlocn->offset, NAME_LEN, MDA_CONTENT_REASON(primary_mda),
+			       ioflags, _vgname_from_mda_validate, vfmp);
+	if (update_vgsummary_fn)
+		return 1;
+	else
+		return vfmp->ret;
 }
 
 static int _scan_raw(const struct format_type *fmt, const char *vgname __attribute__((unused)))
diff --git a/lib/format_text/import.c b/lib/format_text/import.c
index 69af925bc..0138ddd8b 100644
--- a/lib/format_text/import.c
+++ b/lib/format_text/import.c
@@ -78,8 +78,8 @@ static void _import_vgsummary(int failed, unsigned ioflags, void *context, const
 out:
 	config_destroy(ivsp->cft);
 
-	if (ivsp->ret && ivsp->process_vgsummary_fn)
-		ivsp->process_vgsummary_fn(0, ioflags, ivsp->process_vgsummary_context, NULL);
+	if (ivsp->process_vgsummary_fn)
+		ivsp->process_vgsummary_fn(!ivsp->ret, ioflags, ivsp->process_vgsummary_context, NULL);
 }
 
 /*
diff --git a/lib/format_text/text_label.c b/lib/format_text/text_label.c
index d3d92f0b6..45136e9c4 100644
--- a/lib/format_text/text_label.c
+++ b/lib/format_text/text_label.c
@@ -344,6 +344,7 @@ static void _process_vgsummary(int failed, unsigned ioflags, void *context, cons
 
 	--pmp->umb->nr_outstanding_mdas;
 
+	/* FIXME Need to distinguish genuine errors here */
 	if (failed)
 		goto_out;
 
@@ -446,7 +447,10 @@ static int _update_mda(struct metadata_area *mda, void *baton)
 		return 1;
 	}
 
-	return pmp->ret;
+	if (umb->read_label_callback_fn)
+		return 1;
+	else
+		return pmp->ret;
 }
 
 static int _text_read(struct labeller *l, struct device *dev, void *buf, unsigned ioflags,
diff --git a/lib/label/label.c b/lib/label/label.c
index 0997b962c..ae3a2c181 100644
--- a/lib/label/label.c
+++ b/lib/label/label.c
@@ -155,8 +155,10 @@ out:
 	if (!dev_close(flp->dev))
 		stack;
 
-	if (flp->process_label_data_fn)
+	if (flp->process_label_data_fn) {
+		log_debug_io("Completed label reading for %s", dev_name(flp->dev));
 		flp->process_label_data_fn(!flp->ret, ioflags, flp->process_label_data_context, NULL);
+	}
 }
 
 static void _find_labeller(int failed, unsigned ioflags, void *context, const void *data)
@@ -324,8 +326,10 @@ static int _label_read(struct device *dev, uint64_t scan_sector, struct label **
 		log_debug_devs("Reading label from lvmcache for %s", dev_name(dev));
 		if (result)
 			*result = lvmcache_get_label(info);
-		if (process_label_data_fn)
+		if (process_label_data_fn) {
+			log_debug_io("Completed label reading for %s", dev_name(dev));
 			process_label_data_fn(0, ioflags, process_label_data_context, NULL);
+		}
 		return 1;
 	}
 
@@ -356,12 +360,7 @@ static int _label_read(struct device *dev, uint64_t scan_sector, struct label **
 		return 0;
 	}
 
-	if (!(dev_read_callback(dev, scan_sector << SECTOR_SHIFT, LABEL_SCAN_SIZE, DEV_IO_LABEL, ioflags, _find_labeller, flp))) {
-		log_debug_devs("%s: Failed to read label area", dev_name(dev));
-		_set_label_read_result(1, ioflags, flp, NULL);
-		return 0;
-	}
-
+	dev_read_callback(dev, scan_sector << SECTOR_SHIFT, LABEL_SCAN_SIZE, DEV_IO_LABEL, ioflags, _find_labeller, flp);
 	if (process_label_data_fn)
 		return 1;
 	else