[11/24] dm table: print error on preresume failure

Commit Message

Mike Snitzer Oct. 24, 2013, 6:30 p.m. UTC

If preresume fails it is worth logging an error given that a device is
left suspended due to the failure.

This change was motivated by local preresume error logging that was
added to the cache target.  Elevating this error logging to DM core
makes sense.

Signed-off-by: Mike Snitzer <[e-mail]>
Signed-off-by: Joe Thornber <[e-mail]>
---
 drivers/md/dm-table.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Alasdair G Kergon Oct. 25, 2013, 7:22 p.m. UTC | #1

On Thu, Oct 24, 2013 at 02:30:24PM -0400, Mike Snitzer wrote:
> If preresume fails it is worth logging an error given that a device is
> left suspended due to the failure.
 
Indeed.

> This change was motivated by local preresume error logging that was
> added to the cache target.  Elevating this error logging to DM core
> makes sense.
 
But here I disagree: I think it makes more sense not to elevate this
logging, but to leave the responsibility for it with the targets
themselves.

> +			DMERR("%s: %s: preresume failed, error = %d",
> +			      dm_device_name(t->md), ti->type->name, r);

Elevated, you're only getting a single number displayed.

If the targets retain the responsibility, then you should get a
more-detailed error message such as:

                DMERR("aborting resume - crypt key is not set.");

                        DMERR("Unable to resume snapshot source until "
                              "handover completes.");

                        DMERR("Unable to perform snapshot handover until "
                              "source is suspended.");

                        DMERR("could not resize cache metadata");
                        DMERR("could not load cache mappings");
                        DMERR("could not load origin discards");

I think we should retain the requirement that each target must log
a meaningful message whenever preresume fails.

Alasdair

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

Mike Snitzer Oct. 25, 2013, 7:58 p.m. UTC | #2

On Fri, Oct 25 2013 at  3:22pm -0400,
Alasdair G Kergon <[e-mail]> wrote:

> On Thu, Oct 24, 2013 at 02:30:24PM -0400, Mike Snitzer wrote:
> > If preresume fails it is worth logging an error given that a device is
> > left suspended due to the failure.
>  
> Indeed.
> 
> > This change was motivated by local preresume error logging that was
> > added to the cache target.  Elevating this error logging to DM core
> > makes sense.
>  
> But here I disagree: I think it makes more sense not to elevate this
> logging, but to leave the responsibility for it with the targets
> themselves.
> 
> > +			DMERR("%s: %s: preresume failed, error = %d",
> > +			      dm_device_name(t->md), ti->type->name, r);
> 
> Elevated, you're only getting a single number displayed.

Elevated was over-stated, I'll revise the header.  I was trying to say
that the target shouldn't need to worry about stating that the failure
happens to be in the preresume method on the target.

> If the targets retain the responsibility, then you should get a
> more-detailed error message such as:
> 
>                 DMERR("aborting resume - crypt key is not set.");
> 
>                         DMERR("Unable to resume snapshot source until "
>                               "handover completes.");
> 
>                         DMERR("Unable to perform snapshot handover until "
>                               "source is suspended.");
> 
>                         DMERR("could not resize cache metadata");
>                         DMERR("could not load cache mappings");
>                         DMERR("could not load origin discards");
> 
> I think we should retain the requirement that each target must log
> a meaningful message whenever preresume fails.

Yeah, I agree.  I just want DM core to make it clear that the target
failure resulted in preresume failing.

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

Patch

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 8f87835..9bd75dc 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1548,8 +1548,11 @@  int dm_table_resume_targets(struct dm_table *t)
 			continue;
 
 		r = ti->type->preresume(ti);
-		if (r)
+		if (r) {
+			DMERR("%s: %s: preresume failed, error = %d",
+			      dm_device_name(t->md), ti->type->name, r);
 			return r;
+		}
 	}
 
 	for (i = 0; i < t->num_targets; i++) {