Bug 170446

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.

Bug 170446 is a RHEL3 bug, and thus cannot be added to a RHEL4 blocker list.

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;
 	}
 

_

dump_smp_call_function() stuff looks good -- the diskdump stuff also looks
reasonable, although Nobuhiro is the man to verify it!

I think Kei's diskdump patch fixes the problem correctly.

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.

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,".

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.

I just posted the patch to rhkernel-list.  By the way, I got DEV ACK
on this the other day.

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).

I posted an updated patch to rhkernel-list and got one ACK from engineers.

I got one more ACK from engineers.  So, now I have two ACKs from engineers on 
the patch.

Keiichiro, as indicated by comment #19, your revised patch has already
been committed (in CVS) to RHEL3 U7.  So, this problem has been resolved.

Yes, I noticed that right after I posted comment #20 and #21:),
but thanks anyway!

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

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