rbd: add warnings

Define a new function rbd_warn() that produces a boilerplate warning message, identifying in the resulting message the affected rbd device in the best way available. Use it in a few places that now use pr_warning().

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   43 +++++++++++++++++++++++++++++++++----------
  1 file changed, 33 insertions(+), 10 deletions(-)

drivers/block/rbd.c +29 -2

index d97611e..635b81d 100644
@@ -294,6 +294,33 @@ static struct device rbd_root_dev = {
  	.release =      rbd_root_dev_release,
  };
 
+static __printf(2, 3)
+void rbd_warn(struct rbd_device *rbd_dev, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	if (!rbd_dev)
+		printk(KERN_WARNING "%s: %pV\n", RBD_DRV_NAME, &vaf);
+	else if (rbd_dev->disk)
+		printk(KERN_WARNING "%s: %s: %pV\n",
+			RBD_DRV_NAME, rbd_dev->disk->disk_name, &vaf);
+	else if (rbd_dev->spec && rbd_dev->spec->image_name)
+		printk(KERN_WARNING "%s: image %s: %pV\n",
+			RBD_DRV_NAME, rbd_dev->spec->image_name, &vaf);
+	else if (rbd_dev->spec && rbd_dev->spec->image_id)
+		printk(KERN_WARNING "%s: id %s: %pV\n",
+			RBD_DRV_NAME, rbd_dev->spec->image_id, &vaf);
+	else	/* punt */
+		printk(KERN_WARNING "%s: rbd_dev %p: %pV\n",
+			RBD_DRV_NAME, rbd_dev, &vaf);
+	va_end(args);
+}
+
  #ifdef RBD_DEBUG
  #define rbd_assert(expr)						\
  		if (unlikely(!(expr))) {				\

@@ -1403,8 +1430,8 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
 u8 opcode, void *data)
  		(unsigned int) opcode);
  	rc = rbd_dev_refresh(rbd_dev, &hver);
  	if (rc)
-		pr_warning(RBD_DRV_NAME "%d got notification but failed to "
-			   " update snaps: %d\n", rbd_dev->major, rc);
+		rbd_warn(rbd_dev, "got notification but failed to "
+			   " update snaps: %d\n", rc);
 
  	rbd_req_sync_notify_ack(rbd_dev, hver, notify_id);

  }

drivers/block/rbd.c +4 -8

@@ -1767,15 +1794,13 @@ rbd_dev_v1_header_read(struct rbd_device
 *rbd_dev, u64 *version)
  			goto out_err;
  		if (WARN_ON((size_t) ret < size)) {
  			ret = -ENXIO;
-			pr_warning("short header read for image %s"
-					" (want %zd got %d)\n",
-				rbd_dev->spec->image_name, size, ret);
+			rbd_warn(rbd_dev, "short header read (want %zd got %d)",
+				size, ret);
  			goto out_err;
  		}
  		if (!rbd_dev_ondisk_valid(ondisk)) {
  			ret = -ENXIO;
-			pr_warning("invalid header for image %s\n",
-				rbd_dev->spec->image_name);
+			rbd_warn(rbd_dev, "invalid header");
  			goto out_err;
  		}

@@ -2630,9 +2655,7 @@ static int rbd_dev_probe_update_spec(struct
 rbd_device *rbd_dev)
  	if (name)
  		rbd_dev->spec->image_name = (char *) name;
  	else
-		pr_warning(RBD_DRV_NAME "%d "
-			"unable to get image name for image id %s\n",
-			rbd_dev->major, rbd_dev->spec->image_id);
+		rbd_warn(rbd_dev, "unable to get image name");
 
  	/* Look up the snapshot name. */

Define a new function rbd_warn() that produces a boilerplate warning message, identifying in the resulting message the affected rbd device in the best way available. Use it in a few places that now use pr_warning().

Signed-off-by Alex Elder <[email protected]>

Reviewed-by Dan Mick <[email protected]>

---

v2: Added function name to warning message, as suggested by Dan.

  drivers/block/rbd.c |   46 ++++++++++++++++++++++++++++++++++++----------
  1 file changed, 36 insertions(+), 10 deletions(-)

drivers/block/rbd.c +30 -0

index 414052d..86e60eb 100644
@@ -294,6 +294,36 @@ static struct device rbd_root_dev = {
  	.release =      rbd_root_dev_release,
  };
 
+#define rbd_warn(rbd_dev, fmt, ...) \
+		_rbd_warn(__func__, (rbd_dev), (fmt), ##__VA_ARGS__)
+
+static __printf(3, 4) void
+_rbd_warn(const char *func, struct rbd_device *rbd_dev, const char
 *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	if (!rbd_dev)
+		printk(KERN_WARNING RBD_DRV_NAME ": %s: %pV\n", func, &vaf);
+	else if (rbd_dev->disk)
+		printk(KERN_WARNING RBD_DRV_NAME ": %s: %s: %pV\n",
+			func, rbd_dev->disk->disk_name, &vaf);
+	else if (rbd_dev->spec && rbd_dev->spec->image_name)
+		printk(KERN_WARNING RBD_DRV_NAME ": %s: image %s: %pV\n",
+			func, rbd_dev->spec->image_name, &vaf);
+	else if (rbd_dev->spec && rbd_dev->spec->image_id)
+		printk(KERN_WARNING RBD_DRV_NAME ": %s: id %s: %pV\n",
+			func, rbd_dev->spec->image_id, &vaf);
+	else	/* punt */
+		printk(KERN_WARNING RBD_DRV_NAME ": %s: rbd_dev %p: %pV\n",
+			func, rbd_dev, &vaf);
+	va_end(args);
+}
+
  #ifdef RBD_DEBUG
  #define rbd_assert(expr)						\

  		if (unlikely(!(expr))) {				\

drivers/block/rbd.c +2 -2

@@ -1407,8 +1437,8 @@ static void rbd_watch_cb(u64 ver, u64 notify_id,
 u8 opcode, void *data)
  		(unsigned int) opcode);
  	rc = rbd_dev_refresh(rbd_dev, &hver);
  	if (rc)
-		pr_warning(RBD_DRV_NAME "%d got notification but failed to "
-			   " update snaps: %d\n", rbd_dev->major, rc);
+		rbd_warn(rbd_dev, "got notification but failed to "
+			   " update snaps: %d", rc);
 
  	rbd_req_sync_notify_ack(rbd_dev, hver, notify_id);

  }

drivers/block/rbd.c +4 -8

@@ -1771,15 +1801,13 @@ rbd_dev_v1_header_read(struct rbd_device
 *rbd_dev, u64 *version)
  			goto out_err;
  		if (WARN_ON((size_t) ret < size)) {
  			ret = -ENXIO;
-			pr_warning("short header read for image %s"
-					" (want %zd got %d)\n",
-				rbd_dev->spec->image_name, size, ret);
+			rbd_warn(rbd_dev, "short header read (want %zd got %d)",
+				size, ret);
  			goto out_err;
  		}
  		if (!rbd_dev_ondisk_valid(ondisk)) {
  			ret = -ENXIO;
-			pr_warning("invalid header for image %s\n",
-				rbd_dev->spec->image_name);
+			rbd_warn(rbd_dev, "invalid header");
  			goto out_err;
  		}

@@ -2634,9 +2662,7 @@ static int rbd_dev_probe_update_spec(struct
 rbd_device *rbd_dev)
  	if (name)
  		rbd_dev->spec->image_name = (char *) name;
  	else
-		pr_warning(RBD_DRV_NAME "%d "
-			"unable to get image name for image id %s\n",
-			rbd_dev->major, rbd_dev->spec->image_id);
+		rbd_warn(rbd_dev, "unable to get image name");
 
  	/* Look up the snapshot name. */

Tell the user (via dmesg) what was wrong with the arguments provided via /sys/bus/rbd/add.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   24 ++++++++++++++++--------
  1 file changed, 16 insertions(+), 8 deletions(-)

drivers/block/rbd.c +16 -8

index 635b81d..31da8c5 100644
@@ -3244,8 +3244,10 @@ static int rbd_add_parse_args(const char *buf,
  	/* The first four tokens are required */
 
  	len = next_token(&buf);
-	if (!len)
-		return -EINVAL;	/* Missing monitor address(es) */
+	if (!len) {
+		rbd_warn(NULL, "no monitor address(es) provided");
+		return -EINVAL;
+	}
  	mon_addrs = buf;
  	mon_addrs_size = len + 1;
  	buf += len;

@@ -3254,8 +3256,10 @@ static int rbd_add_parse_args(const char *buf,
  	options = dup_token(&buf, NULL);
  	if (!options)
  		return -ENOMEM;
-	if (!*options)
-		goto out_err;	/* Missing options */
+	if (!*options) {
+		rbd_warn(NULL, "no options provided");
+		goto out_err;
+	}
 
  	spec = rbd_spec_alloc();
  	if (!spec)

@@ -3264,14 +3268,18 @@ static int rbd_add_parse_args(const char *buf,
  	spec->pool_name = dup_token(&buf, NULL);
  	if (!spec->pool_name)
  		goto out_mem;
-	if (!*spec->pool_name)
-		goto out_err;	/* Missing pool name */
+	if (!*spec->pool_name) {
+		rbd_warn(NULL, "no pool name provided");
+		goto out_err;
+	}
 
  	spec->image_name = dup_token(&buf, NULL);
  	if (!spec->image_name)
  		goto out_mem;
-	if (!*spec->image_name)
-		goto out_err;	/* Missing image name */
+	if (!*spec->image_name) {
+		rbd_warn(NULL, "no image name provided");
+		goto out_err;
+	}
 
  	/*
  	 * Snapshot name is optional; default is to use "-"

Do you want to include in the message some kind of indication which operation/function is involved? (this is definitely better, but even better might be to add "rbd add" or "rbd_add_parse_args" to the msgs)

Quoted 62 lines

Tell the user (via dmesg) what was wrong with the arguments provided via /sys/bus/rbd/add.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |   24 ++++++++++++++++--------
  1 file changed, 16 insertions(+), 8 deletions(-)

drivers/block/rbd.c +16 -8

index 635b81d..31da8c5 100644
@@ -3244,8 +3244,10 @@ static int rbd_add_parse_args(const char *buf,
      /* The first four tokens are required */
 
      len = next_token(&buf);
-    if (!len)
-        return -EINVAL;    /* Missing monitor address(es) */
+    if (!len) {
+        rbd_warn(NULL, "no monitor address(es) provided");
+        return -EINVAL;
+    }
      mon_addrs = buf;
      mon_addrs_size = len + 1;
      buf += len;

@@ -3254,8 +3256,10 @@ static int rbd_add_parse_args(const char *buf,
      options = dup_token(&buf, NULL);
      if (!options)
          return -ENOMEM;
-    if (!*options)
-        goto out_err;    /* Missing options */
+    if (!*options) {
+        rbd_warn(NULL, "no options provided");
+        goto out_err;
+    }
 
      spec = rbd_spec_alloc();
      if (!spec)

@@ -3264,14 +3268,18 @@ static int rbd_add_parse_args(const char *buf,
      spec->pool_name = dup_token(&buf, NULL);
      if (!spec->pool_name)
          goto out_mem;
-    if (!*spec->pool_name)
-        goto out_err;    /* Missing pool name */
+    if (!*spec->pool_name) {
+        rbd_warn(NULL, "no pool name provided");
+        goto out_err;
+    }
 
      spec->image_name = dup_token(&buf, NULL);
      if (!spec->image_name)
          goto out_mem;
-    if (!*spec->image_name)
-        goto out_err;    /* Missing image name */
+    if (!*spec->image_name) {
+        rbd_warn(NULL, "no image name provided");
+        goto out_err;
+    }
 
      /*
       * Snapshot name is optional; default is to use "-"

Add a warning in bio_chain_clone_range() to help a user determine what exactly might have led to a failure. There is only one; please say something if you disagree with the following reasoning.

There are three places this can return abnormally:
- Initially, if there is nothing to clone. It turns out that right now this cannot happen anyway. The test is in place because the code below it doesn't work if those conditions don't hold. As such they could be assertions but since I can return a null to indicate an error I just do that instead. I have not added a warning here because it won't happen.
- While processing bio's, if none remain but there are supposed to be more bytes to clone. Here I have added a warning.
- If bio_clone_range() returns a null pointer. That function will have already produced a warning (at least the first time, via WARN_ON_ONCE()) to distinguish the cause of the error. The only exception is memory exhaustion, and I'd rather not pepper the code with warnings in all those spots. So no warning is added in that place.

Signed-off-by Alex Elder <[email protected]>

---

  drivers/block/rbd.c |    4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

drivers/block/rbd.c +3 -1

index 31da8c5..ce6c0cb 100644
@@ -993,8 +993,10 @@ static struct bio *bio_chain_clone_range(struct bio
 **bio_src,
  		unsigned int bi_size;
  		struct bio *bio;
 
-		if (!bi)
+		if (!bi) {
+			rbd_warn(NULL, "bio_chain exhausted with %u left", len);
  			goto out_err;	/* EINVAL; ran out of bio's */
+		}
  		bi_size = min_t(unsigned int, bi->bi_size - off, len);
  		bio = bio_clone_range(bi, off, bi_size, gfpmask);

if (!bio)

Josh suggested adding warnings to this function to help users diagnose problems.

Other than memory allocatino errors, there are two places where errors can be returned. Both represent problems that should have been caught earlier, and as such might well have been handled with BUG_ON() calls. But if either ever did manage to happen, it will be reported.

Signed-off-by Alex Elder <[email protected]>

---
  drivers/block/rbd.c |    9 +++++++--
  1 file changed, 7 insertions(+), 2 deletions(-)

drivers/block/rbd.c +5 -2

index ce6c0cb..530a121 100644
@@ -2644,8 +2644,11 @@ static int rbd_dev_probe_update_spec(struct
 rbd_device *rbd_dev)
 
  	osdc = &rbd_dev->rbd_client->client->osdc;
  	name = ceph_pg_pool_name_by_id(osdc->osdmap, rbd_dev->spec->pool_id);
-	if (!name)
-		return -EIO;	/* pool id too large (>= 2^31) */
+	if (!name) {
+		rbd_warn(rbd_dev, "there is no pool with id %llu",
+			rbd_dev->spec->pool_id);	/* Really a BUG() */
+		return -EIO;
+	}
 
  	rbd_dev->spec->pool_name = kstrdup(name, GFP_KERNEL);

if (!rbd_dev->spec->pool_name)

drivers/block/rbd.c +2 -0

@@ -2663,6 +2666,8 @@ static int rbd_dev_probe_update_spec(struct
 rbd_device *rbd_dev)
 
  	name = rbd_snap_name(rbd_dev, rbd_dev->spec->snap_id);
  	if (!name) {
+		rbd_warn(rbd_dev, "no snapshot with id %llu",
+			rbd_dev->spec->snap_id);	/* Really a BUG() */
  		ret = -EIO;
  		goto out_err;

  	}