Bug 170446 - [RHEL3 U7] netdump hangs in processing of CPU stop after diskdump failed.
[RHEL3 U7] netdump hangs in processing of CPU stop after diskdump failed.
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 3
Classification: Red Hat
Component: kernel (Show other bugs)
3.0
All Linux
medium Severity medium
: ---
: ---
Assigned To: Keiichiro Tokunaga
Brian Brock
:
Depends On:
Blocks: 168424
  Show dependency treegraph
 
Reported: 2005-10-11 15:48 EDT by Issue Tracker
Modified: 2010-10-21 23:31 EDT (History)
6 users (show)

See Also:
Fixed In Version: RHSA-2006-0144
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-03-15 11:45:55 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Issue Tracker 2005-10-11 15:48:55 EDT
Escalated to Bugzilla from IssueTracker
Comment 7 Dave Anderson 2005-10-11 16:38:50 EDT
If I recall correctly the design below of the dump_smp_call_function()
was to handle a situation where two cpus oops'd at the same time:

(1) Both cpus would proceed to netconsole_netdump() and both would call
    dump_smp_call_function().  
(2) Only the first cpu would see the dumpdata.func as NULL, and would try to
    freeze the other cpus.  
(3) The second cpu would then come in, see dumpdata.func set non-NULL, realize
    that the first cpu had beaten it here, and instead just call the freeze
    function.
 
With the proposed patch, the second cpu would return, and there would be two
simultaneous netdump operations!  So I believe the patch would break that
"two-simultaneous-dump-attempts", functionality.  Do you agree with that?
   
        spin_lock(&dump_call_lock);
        /* if another cpu beat us, they win! */
        if (dumpdata.func) {
                spin_unlock(&dump_call_lock);
                func(info);
                for (;;);
                /* NOTREACHED */
        }

In any case, I'm not disputing the fact that when a diskdump operation fails,
that you are seeing a hang -- but I believe that you should fix it in a
different manner in order to preserve the original intent of the code above.
Comment 9 Ernie Petrides 2005-10-13 23:41:00 EDT
Bug 170446 is a RHEL3 bug, and thus cannot be added to a RHEL4 blocker list.
Comment 11 Keiichiro Tokunaga 2005-10-19 16:54:36 EDT
Here is the patch for RHEL3.  I have had it reviewed by Fujitsu
Japan team and tested it on x86, ia64, and x86_64 with the latest
kernel of RHEL3.  It has become a little bit bigger in size because
I have added some comments to describe what is expected there.

In addition, I found another bugs that cause the similar phenomenon
(netdump doesn't start after diskdump fails).  This happens only
on RHEL3.  The patch also fixes the issue.

Here is the details of the issue.  disk_dump() is like below.

static void disk_dump(struct pt_regs *regs, void *platform_arg)
{
        ......
        int ret = -EIO;  /* #1 */
        ......
        /*
         * Stop ongoing I/O with polling driver and make the shift to I/O mode
         * for dump
         */
        Dbg("do quiesce");
        if (dump_device->ops.quiesce)
                if ((ret = dump_device->ops.quiesce(dump_device)) < 0) { /* #2 
*/
                        Err("quiesce failed. error %d", ret);
                        goto done;
                }

        if (SECTOR_BLOCK(dump_part->nr_sects) < header_blocks + bitmap_blocks) 
{  /* #4 */
                Warn("dump partition is too small. Aborted");
                goto done;  /* #5 */
        }
        /* Check dump partition */
        printk("check dump partition...\n");
        if (!check_dump_partition(dump_part, total_blocks)) {  /* #6 */
                Err("check partition failed.");
                goto done;  /* #7 */
        }
        ......
done:
        ......
       /*
         * If diskdump failed and fallback_on_err is set,
         * We just return and leave panic to netdump.
         */
        if (fallback_on_err && ret != 0)  /* #8 */
                return;
        ......
}

Suppose that the dump device's driver has a function for
ops.quiesce.  The variable 'ret' is initialized to -EIO
at #1.  If ops.quiesce returns success at #2, 'ret' is set
to a value of 0.  Then if the condition of if-statement at
#4 or #6 is true, the program control jumps to 'done' without
changing the value of 'ret' at #5 or #7.  In that case,
the condition of if-statement at #8 would never be true.
Consequently, netdump doesn't start after diskdump fails
and diskdump just reboots the system in stead in that case.

If there is no problem with it, I would go on to the next step.

---

 linux-2.4.21-org-kei/arch/i386/kernel/smp.c   |   21 ++++++++++++++++++---
 linux-2.4.21-org-kei/arch/ia64/kernel/smp.c   |   21 ++++++++++++++++++---
 linux-2.4.21-org-kei/arch/x86_64/kernel/smp.c |   21 ++++++++++++++++++---
 linux-2.4.21-org-kei/drivers/block/diskdump.c |    2 ++
 4 files changed, 56 insertions(+), 9 deletions(-)

diff -puN arch/i386/kernel/smp.c~linux-2.4.21-fix-dump_smp_call_function 
arch/i386/kernel/smp.c
--- linux-2.4.21-org/arch/i386/kernel/smp.c~linux-2.4.21-fix-
dump_smp_call_function	2005-10-18 15:22:42.000000000 -0400
+++ linux-2.4.21-org-kei/arch/i386/kernel/smp.c	2005-10-18 15:22:42.000000000 -
0400
@@ -532,16 +532,31 @@ void dump_smp_call_function (void (*func
 {
 	static struct call_data_struct dumpdata;
 	int waitcount;
+	static int dumping_cpu = -1;
 
 	spin_lock(&dump_call_lock);
-	/* if another cpu beat us, they win! */
+	/*
+	 * The cpu that reaches here first will do dumping.  Only the dumping
+	 * cpu skips the if-statement below ONLY ONCE.  The other cpus freeze
+	 * themselves here.
+	 */
 	if (dumpdata.func) {
 		spin_unlock(&dump_call_lock);
-		func(info);
-		for (;;);
+		/*
+		 * The dumping cpu reaches here in case that the netdump starts
+		 * after the diskdump fails.  In the case, the dumping cpu
+		 * needs to return to continue the netdump.  In other cases,
+		 * freezes itself by calling func().
+		 */
+		if (dumping_cpu == smp_processor_id())
+			return;
+		else
+			func(info);
 		/* NOTREACHED */
 	}
 
+	dumping_cpu = smp_processor_id();
+
 	/* freeze call_lock or wait for on-going IPIs to settle down */
 	waitcount = 0;
 	while (!spin_trylock(&call_lock)) {
diff -puN arch/ia64/kernel/smp.c~linux-2.4.21-fix-dump_smp_call_function 
arch/ia64/kernel/smp.c
--- linux-2.4.21-org/arch/ia64/kernel/smp.c~linux-2.4.21-fix-
dump_smp_call_function	2005-10-18 15:22:42.000000000 -0400
+++ linux-2.4.21-org-kei/arch/ia64/kernel/smp.c	2005-10-18 15:22:42.000000000 -
0400
@@ -263,16 +263,31 @@ void dump_smp_call_function (void (*func
 {
 	static struct call_data_struct dumpdata;
 	int waitcount;
+	static int dumping_cpu = -1;
 
 	spin_lock(&dump_call_lock);
-	/* if another cpu beat us, they win! */
+	/*
+	 * The cpu that reaches here first will do dumping.  Only the dumping
+	 * cpu skips the if-statement below ONLY ONCE.  The other cpus freeze
+	 * themselves here.
+	 */
 	if (dumpdata.func) {
 		spin_unlock(&dump_call_lock);
-		func(info);
-		for (;;);
+		/*
+		 * The dumping cpu reaches here in case that the netdump starts
+		 * after the diskdump fails.  In the case, the dumping cpu
+		 * needs to return to continue the netdump.  In other cases,
+		 * freezes itself by calling func().
+		 */
+		if (dumping_cpu == smp_processor_id())
+			return;
+		else
+			func(info);
 		/* NOTREACHED */
 	}
 
+	dumping_cpu = smp_processor_id();
+
 	/* freeze call_lock or wait for on-going IPIs to settle down */
 	waitcount = 0;
 	while (!spin_trylock(&call_lock)) {
diff -puN arch/x86_64/kernel/smp.c~linux-2.4.21-fix-dump_smp_call_function 
arch/x86_64/kernel/smp.c
--- linux-2.4.21-org/arch/x86_64/kernel/smp.c~linux-2.4.21-fix-
dump_smp_call_function	2005-10-18 15:22:42.000000000 -0400
+++ linux-2.4.21-org-kei/arch/x86_64/kernel/smp.c	2005-10-18 
15:22:42.000000000 -0400
@@ -351,16 +351,31 @@ void dump_smp_call_function (void (*func
 {
 	static struct call_data_struct dumpdata;
 	int waitcount;
+	static int dumping_cpu = -1;
 
 	spin_lock(&dump_call_lock);
-	/* if another cpu beat us, they win! */
+	/*
+	 * The cpu that reaches here first will do dumping.  Only the dumping
+	 * cpu skips the if-statement below ONLY ONCE.  The other cpus freeze
+	 * themselves here.
+	 */
 	if (dumpdata.func) {
 		spin_unlock(&dump_call_lock);
-		func(info);
-		for (;;);
+		/*
+		 * The dumping cpu reaches here in case that the netdump starts
+		 * after the diskdump fails.  In the case, the dumping cpu
+		 * needs to return to continue the netdump.  In other cases,
+		 * freezes itself by calling func().
+		 */
+		if (dumping_cpu == smp_processor_id())
+			return;
+		else
+			func(info);
 		/* NOTREACHED */
 	}
 
+	dumping_cpu = smp_processor_id();
+
 	/* freeze call_lock or wait for on-going IPIs to settle down */
 	waitcount = 0;
 	while (!spin_trylock(&call_lock)) {
diff -puN drivers/block/diskdump.c~linux-2.4.21-fix-dump_smp_call_function 
drivers/block/diskdump.c
--- linux-2.4.21-org/drivers/block/diskdump.c~linux-2.4.21-fix-
dump_smp_call_function	2005-10-18 15:23:02.000000000 -0400
+++ linux-2.4.21-org-kei/drivers/block/diskdump.c	2005-10-18 
15:29:50.000000000 -0400
@@ -487,6 +487,7 @@ static void disk_dump(struct pt_regs *re
 
 	if (SECTOR_BLOCK(dump_part->nr_sects) < header_blocks + bitmap_blocks) 
{
 		Warn("dump partition is too small. Aborted");
+		ret = -EIO;
 		goto done;
 	}
 
@@ -494,6 +495,7 @@ static void disk_dump(struct pt_regs *re
 	printk("check dump partition...\n");
 	if (!check_dump_partition(dump_part, total_blocks)) {
 		Err("check partition failed.");
+		ret = -EIO;
 		goto done;
 	}
 

_
Comment 12 Dave Anderson 2005-10-19 17:02:27 EDT
dump_smp_call_function() stuff looks good -- the diskdump stuff also looks
reasonable, although Nobuhiro is the man to verify it!
Comment 13 Nobuhiro Tachino 2005-10-19 17:08:26 EDT
I think Kei's diskdump patch fixes the problem correctly.
Comment 15 Keiichiro Tokunaga 2005-10-20 12:03:04 EDT
Here is an additional information regarding the testing.  I have made
the patch for the latest kernel of RHEL4, created a binary kernel rpm
with the patch, and installed it to the OS.

On the kernel, I enabled diskdump and netdump services and made the
system panic.  I made diskdump fail intentionally in the event of the
panic and checked if netdump started properly.

I did the testing above on x86, ia64, and x86_64 that support diskdump.
Comment 16 Keiichiro Tokunaga 2005-10-20 12:13:12 EDT
There is a typo due to cut-and-paste in the last comments.

"I have made the patch for the latest kernel of RHEL4," should be "I have made
the patch for the latest kernel of RHEL3,".
Comment 17 Keiichiro Tokunaga 2005-10-25 14:36:49 EDT
I have changed the Summary properly to prevent any confusion.

  old: [RHEL3 U5] netdump...
  new: [RHEL3 U7] netdump...

The side effect of this change is that there is the slight
difference between the Summary of BZ and a related IT now.
I hope that's ok.
Comment 18 Keiichiro Tokunaga 2005-10-25 17:12:31 EDT
I just posted the patch to rhkernel-list.  By the way, I got DEV ACK
on this the other day.
Comment 19 Ernie Petrides 2005-10-26 22:24:56 EDT
A fix for this problem has just been committed to the RHEL3 U7
patch pool this evening (in kernel version 2.4.21-37.7.EL).
Comment 20 Keiichiro Tokunaga 2005-10-27 10:23:00 EDT
I posted an updated patch to rhkernel-list and got one ACK from engineers.
Comment 21 Keiichiro Tokunaga 2005-10-27 10:31:34 EDT
I got one more ACK from engineers.  So, now I have two ACKs from engineers on 
the patch.
Comment 22 Ernie Petrides 2005-10-27 17:30:20 EDT
Keiichiro, as indicated by comment #19, your revised patch has already
been committed (in CVS) to RHEL3 U7.  So, this problem has been resolved.
Comment 23 Keiichiro Tokunaga 2005-10-27 17:35:11 EDT
Yes, I noticed that right after I posted comment #20 and #21:),
but thanks anyway!
Comment 28 Akira Imamura 2006-02-21 11:24:32 EST
Since we have done testing, we report the results here.

1. Result of the testing
 Overall result: PASS
 - Regressions: None
 - Unfixed bugs: None

2. Hardware that we used for the testing
 - i386
 - ia64
 - x86_64

3. Version of packages that we used for the testing
 - kernel-2.4.21-40.EL
 - diskdumputils-1.2.8-1

4. Additional Comments
 None
Comment 30 Red Hat Bugzilla 2006-03-15 11:45:56 EST
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on the solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHSA-2006-0144.html

Note You need to log in before you can comment on or make changes to this bug.