[PATCH REPOST 0/4] rbd: add warnings

[PATCH REPOST 0/4] rbd: add warnings

[Alex Elder]

This series adds a compact way of generating standardized
warning messages in rbd, and then adds some code to issue
warnings in a few spots.

					-Alex

[PATCH REPOST 1/4] rbd: define and use rbd_warn()
[PATCH REPOST 2/4] rbd: add warning messages for missing arguments
[PATCH REPOST 3/4] rbd: add a warning in bio_chain_clone_range()
[PATCH REPOST 4/4] rbd: add warnings to rbd_dev_probe_update_spec()

[PATCH REPOST 1/4] rbd: define and use rbd_warn()

[Alex Elder]

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 <elder@inktank.com>
---
 drivers/block/rbd.c |   43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index d97611e..635b81d 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -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);
 }
@@ -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. */

-- 
1.7.9.5

[PATCH REPOST 2/4] rbd: add warning messages for missing arguments

[Alex Elder]

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

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 635b81d..31da8c5 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -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 "-"
-- 
1.7.9.5

[PATCH REPOST 3/4] rbd: add a warning in bio_chain_clone_range()

[Alex Elder]

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 <elder@inktank.com>
---
 drivers/block/rbd.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 31da8c5..ce6c0cb 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -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)
-- 
1.7.9.5

[PATCH REPOST 4/4] rbd: add warnings to rbd_dev_probe_update_spec()

[Alex Elder]

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 <elder@inktank.com>
---
 drivers/block/rbd.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index ce6c0cb..530a121 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -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)
@@ -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;
 	}
-- 
1.7.9.5

Re: [PATCH REPOST 2/4] rbd: add warning messages for missing arguments

[Dan Mick]

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)

On 01/03/2013 11:11 AM, Alex Elder wrote:
> Tell the user (via dmesg) what was wrong with the arguments provided
> via /sys/bus/rbd/add.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 635b81d..31da8c5 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -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 "-"
>

Re: [PATCH REPOST 1/4] rbd: define and use rbd_warn()

[Dan Mick]

Nifty.

Reviewed-by: Dan Mick <dan.mick@inktank.com>

On 01/03/2013 11:11 AM, Alex Elder wrote:
> 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 <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   43 +++++++++++++++++++++++++++++++++----------
>   1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index d97611e..635b81d 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -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);
>   }
> @@ -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. */
>

Re: [PATCH REPOST 2/4] rbd: add warning messages for missing arguments

[Alex Elder]

On 01/03/2013 07:10 PM, Dan Mick wrote:
> 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)

Sure.  This comment really applies to the previous patch.  I'm
sure I thought of doing that and I'm not sure any more why I did
not.  Maybe worried about lines getting too long?  Or maybe
I bumped into varargs problems?  I don't know.

I'll rename the rbd_warn() function _rbd_warn(), and will add a new
first argument which is the function name.  Then I'll define a new
macro rbd_warn() that just calls _rbd_warn(), inserting __func__ as
a first argument.

					-Alex

> On 01/03/2013 11:11 AM, Alex Elder wrote:
>> Tell the user (via dmesg) what was wrong with the arguments provided
>> via /sys/bus/rbd/add.
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |   24 ++++++++++++++++--------
>>   1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 635b81d..31da8c5 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -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 "-"
>>

[PATCH, v2] rbd: define and use rbd_warn()

[Alex Elder]

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 <elder@inktank.com>
Reviewed-by: Dan Mick <dan.mick@inktank.com>
---
v2: Added function name to warning message, as suggested by Dan.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 414052d..86e60eb 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -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))) {				\
@@ -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);
 }
@@ -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. */

-- 
1.7.9.5

Re: [PATCH, v2] rbd: define and use rbd_warn()

[Josh Durgin]

On 01/04/2013 08:22 AM, Alex Elder wrote:
> 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 <elder@inktank.com>
> Reviewed-by: Dan Mick <dan.mick@inktank.com>
> ---

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

> v2: Added function name to warning message, as suggested by Dan.
>
>   drivers/block/rbd.c |   46 ++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 36 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 414052d..86e60eb 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -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))) {				\
> @@ -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);
>   }
> @@ -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. */
>

Re: [PATCH REPOST 3/4] rbd: add a warning in bio_chain_clone_range()

[Josh Durgin]

On 01/03/2013 11:12 AM, Alex Elder wrote:
> 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 <elder@inktank.com>
> ---

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

>   drivers/block/rbd.c |    4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 31da8c5..ce6c0cb 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -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)
>

Re: [PATCH REPOST 4/4] rbd: add warnings to rbd_dev_probe_update_spec()

[Josh Durgin]

On 01/03/2013 11:12 AM, Alex Elder wrote:
> 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.

The pool could be deleted without us knowing after
our first check.

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |    9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index ce6c0cb..530a121 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -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)
> @@ -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;
>   	}
>