Skip to contentSkip to conversation

Diskdump Update

PATCH In review Takao Indoh 4 participants 21 messages Jun 11, 2004 – Jun 23, 2004

Subsystems (4)
SCSI Block layer Char and misc drivers AIC7XXX / AIC79XX SCSI driver
lore
Share & tools
lore

Takao Indoh

author
#0 lore raw

cover Diskdump Update

Hi,

I update the Diskdump patch for kernel 2.6.6. I fixed some codes according to comments from Christoph Hellwig.
http://marc.theaimsgroup.com/?l=linux-kernel&m=108566610109915&w=2

Source code can be downloaded from
http://sourceforge.net/projects/lkdump

Please feel free to comment.

Next week, I'll try the following and release new patch.

1) Change codes around add_timer()/del_timer()/etc.
2) Replace dev_t with block_device
3) Clean code more

Quoted 9 lines · Christoph Hellwig
Quoted 9 lines

+/******************************** Disk dump **************************** *******/

(quoted diff) +7 -0
+#if defined(CONFIG_DISKDUMP) || defined(CONFIG_DISKDUMP_MODULE)
+#undef  add_timer
+#define add_timer       diskdump_add_timer
+#undef  del_timer_sync
+#define del_timer_sync  diskdump_del_timer
+#undef  del_timer
+#define del_timer       diskdump_del_timer
Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+#undef  mod_timer
+#define mod_timer       diskdump_mod_timer
+
+#define tasklet_schedule        diskdump_tasklet_schedule
+#endif

Yikes. No way in hell we'll place code like this in drivers. This needs to be handled in common code.

Another approach is insering some codes into the core timer and tasklet routines.

For example,

static inline void add_timer(struct timer_list * timer)

{
	if(crashdump_mode())
		__diskdump_add_timer(timer);
	else
		__mod_timer(timer, timer->expires);
}

But I do not want to make common codes dirty...
Please let me know more good idea!

Best Regards,
Takao Indoh

author
#1 lore raw

PATCH 1/4 Diskdump Update

This is a patch for diskdump common layer.

arch/i386/kernel/nmi.c +1 -0
diff -Nur linux-2.6.6.org/arch/i386/kernel/nmi.c linux-2.6.6/arch/i386/kernel/nmi.c
@@ -517,3 +517,4 @@
 EXPORT_SYMBOL(release_lapic_nmi);
 EXPORT_SYMBOL(disable_timer_nmi_watchdog);
 EXPORT_SYMBOL(enable_timer_nmi_watchdog);
+EXPORT_SYMBOL(touch_nmi_watchdog);
arch/i386/kernel/reboot.c +2 -1
diff -Nur linux-2.6.6.org/arch/i386/kernel/reboot.c linux-2.6.6/arch/i386/kernel/reboot.c
@@ -252,7 +252,8 @@
 	 * Stop all CPUs and turn off local APICs and the IO-APIC, so
 	 * other OSs see a clean IRQ state.
 	 */
-	smp_send_stop();
+	if (!crashdump_mode())
+		smp_send_stop();
 #elif defined(CONFIG_X86_LOCAL_APIC)
 	if (cpu_has_apic) {
 		local_irq_disable();
arch/i386/kernel/traps.c +3 -1
diff -Nur linux-2.6.6.org/arch/i386/kernel/traps.c linux-2.6.6/arch/i386/kernel/traps.c
@@ -258,7 +258,8 @@
 	int nl = 0;
 
 	console_verbose();
-	spin_lock_irq(&die_lock);
+	if (!crashdump_mode())
+		spin_lock_irq(&die_lock);
 	bust_spinlocks(1);
 	handle_BUG(regs);
 	printk("%s: %04lx [#%d]\n", str, err & 0xffff, ++die_counter);

@@ -277,6 +278,7 @@
 	if (nl)
 		printk("\n");
 	show_registers(regs);
+	try_crashdump(regs);
 	bust_spinlocks(0);
 	spin_unlock_irq(&die_lock);
 	if (in_interrupt())
arch/i386/mm/init.c +2 -1
diff -Nur linux-2.6.6.org/arch/i386/mm/init.c linux-2.6.6/arch/i386/mm/init.c
@@ -168,7 +168,7 @@
 
 extern int is_available_memory(efi_memory_desc_t *);
 
-static inline int page_is_ram(unsigned long pagenr)
+int page_is_ram(unsigned long pagenr)
 {
 	int i;
 	unsigned long addr, end;

@@ -205,6 +205,7 @@
 	}
 	return 0;
 }
+EXPORT_SYMBOL(page_is_ram);
 
 #ifdef CONFIG_HIGHMEM
 pte_t *kmap_pte;
drivers/block/Kconfig +5 -0
diff -Nur linux-2.6.6.org/drivers/block/Kconfig linux-2.6.6/drivers/block/Kconfig
@@ -346,6 +346,11 @@
 	  your machine, or if you want to have a raid or loopback device
 	  bigger than 2TB.  Otherwise say N.
 
+config DISKDUMP
+	tristate "Disk dump support"
+	---help---
+	  Disk dump support.
+
 source "drivers/s390/block/Kconfig"
 
 endmenu
drivers/block/Makefile +1 -0
diff -Nur linux-2.6.6.org/drivers/block/Makefile linux-2.6.6/drivers/block/Makefile
@@ -43,3 +43,4 @@
 obj-$(CONFIG_VIODASD)		+= viodasd.o
 obj-$(CONFIG_BLK_DEV_CARMEL)	+= carmel.o
 
+obj-$(CONFIG_DISKDUMP)		+= diskdump.o diskdumplib.o
drivers/block/diskdump.c +967 -0

967 lines not on this page · raw message

drivers/block/diskdumplib.c +203 -0

203 lines not on this page · raw message

drivers/block/genhd.c +27 -0
diff -Nur linux-2.6.6.org/drivers/block/genhd.c linux-2.6.6/drivers/block/genhd.c
@@ -224,6 +224,8 @@
 	return  kobj ? to_disk(kobj) : NULL;
 }
 
+EXPORT_SYMBOL(get_gendisk);
+
 #ifdef CONFIG_PROC_FS
 /* iterator */
 static void *part_start(struct seq_file *part, loff_t *pos)

@@ -629,3 +631,28 @@
 }
 
 EXPORT_SYMBOL(invalidate_partition);
+
+/*
+ *  Dump  stuff.
+ */
+void (*diskdump_func) (struct pt_regs *regs) = NULL;
+EXPORT_SYMBOL_GPL(diskdump_func);
+
+int diskdump_register_hook(void (*dump_func) (struct pt_regs *))
+{
+	if (diskdump_func)
+		return -EEXIST;
+
+	diskdump_func = dump_func;
+
+	return 0;
+}
+
+EXPORT_SYMBOL_GPL(diskdump_register_hook);
+
+void diskdump_unregister_hook(void)
+{
+	diskdump_func = NULL;
+}
+
+EXPORT_SYMBOL_GPL(diskdump_unregister_hook);
drivers/char/sysrq.c +14 -1
diff -Nur linux-2.6.6.org/drivers/char/sysrq.c linux-2.6.6/drivers/char/sysrq.c
@@ -107,6 +107,19 @@
 	.action_msg	= "Resetting",
 };
 
+/* crash sysrq handler */
+static void sysrq_handle_crash(int key, struct pt_regs *pt_regs,
+			      struct tty_struct *tty) 
+{
+	*( (char *) 0) = 0;
+}
+
+static struct sysrq_key_op sysrq_crash_op = {
+	.handler	= sysrq_handle_crash,
+	.help_msg	= "Crash",
+	.action_msg	= "Crashing the kernel by request",
+};
+
 static void sysrq_handle_sync(int key, struct pt_regs *pt_regs,
 			      struct tty_struct *tty) 
 {

@@ -235,7 +248,7 @@
 		 it is handled specially on the sparc
 		 and will never arrive */
 /* b */	&sysrq_reboot_op,
-/* c */ NULL,
+/* c */ &sysrq_crash_op,
 /* d */	NULL,
 /* e */	&sysrq_term_op,
 /* f */	NULL,
include/asm-i386/diskdump.h +71 -0

71 lines not on this page · raw message

include/asm-i386/kmap_types.h +2 -1
diff -Nur linux-2.6.6.org/include/asm-i386/kmap_types.h linux-2.6.6/include/asm-i386/kmap_types.h
@@ -24,7 +24,8 @@
 D(11)	KM_IRQ1,
 D(12)	KM_SOFTIRQ0,
 D(13)	KM_SOFTIRQ1,
-D(14)	KM_TYPE_NR
+D(14)	KM_DISKDUMP,
+D(15)	KM_TYPE_NR
 };
 
 #undef D
include/linux/diskdump.h +154 -0

154 lines not on this page · raw message

include/linux/diskdumplib.h +67 -0

67 lines not on this page · raw message

include/linux/kernel.h +10 -0
diff -Nur linux-2.6.6.org/include/linux/kernel.h linux-2.6.6/include/linux/kernel.h
@@ -112,6 +112,10 @@
 extern int system_state;		/* See values below */
 extern int tainted;
 extern const char *print_tainted(void);
+extern void try_crashdump(struct pt_regs *);
+extern void (*diskdump_func) (struct pt_regs *regs);
+extern int diskdump_mode;
+#define crashdump_mode()       (diskdump_mode)
 
 /* Values used for system_state */
 #define SYSTEM_BOOTING 0

@@ -136,6 +140,12 @@
 #define pr_info(fmt,arg...) \
 	printk(KERN_INFO fmt,##arg)
 
+#define pr_err(fmt,arg...) \
+	printk(KERN_ERR fmt,##arg)
+
+#define pr_warn(fmt,arg...) \
+	printk(KERN_WARNING fmt,##arg)
+
 /*
  *      Display an IP address in readable format.
  */
kernel/panic.c +23 -0
diff -Nur linux-2.6.6.org/kernel/panic.c linux-2.6.6/kernel/panic.c
@@ -23,8 +23,10 @@
 int panic_timeout;
 int panic_on_oops;
 int tainted;
+int diskdump_mode = 0;
 
 EXPORT_SYMBOL(panic_timeout);
+EXPORT_SYMBOL_GPL(diskdump_mode);
 
 struct notifier_block *panic_notifier_list;
 

@@ -60,6 +62,8 @@
 	vsnprintf(buf, sizeof(buf), fmt, args);
 	va_end(args);
 	printk(KERN_EMERG "Kernel panic: %s\n",buf);
+	if (diskdump_func)
+		BUG();
 	if (in_interrupt())
 		printk(KERN_EMERG "In interrupt handler - not syncing\n");
 	else if (!current->pid)

@@ -134,3 +138,22 @@
 		snprintf(buf, sizeof(buf), "Not tainted");
 	return(buf);
 }
+
+/*
+ * Try crashdump. Diskdump is first, netdump is second.
+ * We clear diskdump_func before call of diskdump_func, so
+ * If double panic would occur in diskdump, netdump can handle
+ * it.
+ */
+void try_crashdump(struct pt_regs *regs)
+{
+	void (*func)(struct pt_regs *);
+
+	if (diskdump_func) {
+		func = diskdump_func;
+		diskdump_func = NULL;
+		func(regs);
+	}
+	if (panic_on_oops)
+		panic("Fatal exception");
+}
#2 lore raw

arch/i386/kernel/traps.c line 262

Quoted 11 lines

diff -Nur linux-2.6.6.org/arch/i386/kernel/traps.c linux-2.6.6/arch/i386/kernel/traps.c

arch/i386/kernel/traps.c +2 -1
@@ -258,7 +258,8 @@
 	int nl = 0;
 
 	console_verbose();
-	spin_lock_irq(&die_lock);
+	if (!crashdump_mode())
+		spin_lock_irq(&die_lock);

why is this??

arch/i386/kernel/traps.c

Quoted 6 lines
arch/i386/kernel/traps.c +6 -0
+#ifndef TRUE
+#define TRUE	1
+#endif
+#ifndef FALSE
+#define FALSE	0
+#endif

it's kernel convention to just use 0 and !0

arch/i386/kernel/traps.c

Quoted 4 lines
arch/i386/kernel/traps.c +4 -0
+MODULE_PARM(fallback_on_err, "i");
+MODULE_PARM(allow_risky_dumps, "i");
+MODULE_PARM(block_order, "i");
+MODULE_PARM(sample_rate, "i");

please exclusively use the 2.6 module parameter mechanism for new code.

arch/i386/kernel/traps.c

Quoted 1 line
arch/i386/kernel/traps.c +1 -0
+		page = mem_map + nr;

this is not safe for discontig mem and thus broken

arch/i386/kernel/traps.c

Quoted 4 lines
arch/i386/kernel/traps.c +4 -0
+	/*
+	 * Check the checksum of myself
+	 */
+	spin_trylock(&disk_dump_lock);

you need to handle the failure case of trylock of course

arch/i386/kernel/traps.c

Quoted 2 lines
arch/i386/kernel/traps.c +2 -0
+#ifdef CONFIG_PROC_FS
+static int proc_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long param)

ehhh this looks evil

drivers/block/genhd.c line 227

Quoted 8 lines

diff -Nur linux-2.6.6.org/drivers/block/genhd.c linux-2.6.6/drivers/block/genhd.c

drivers/block/genhd.c +1 -0
@@ -224,6 +224,8 @@
 	return  kobj ? to_disk(kobj) : NULL;
 }
 
+EXPORT_SYMBOL(get_gendisk);

this is WRONG. VERY WRONG.

author
#3 lore raw

Hi,

Quoted 7 lines · Arjan van de Ven

arch/i386/kernel/traps.c line 262

Quoted 4 lines
arch/i386/kernel/traps.c +2 -1
 	console_verbose();
-	spin_lock_irq(&die_lock);
+	if (!crashdump_mode())
+		spin_lock_irq(&die_lock);

why is this??

It is for safety.
If diskdump dies during dumping, die() is called again.
In such a case, system falls into the deadlock by die_lock. User cannot't know what happened to diskdump.
By this correction, spin_lock_irq() is skipped and show_registers() shows stack trace. User may be able to know what happened even if dump fails.

Quoted 10 lines

drivers/block/diskdump.c line 60

Quoted 8 lines
drivers/block/diskdump.c +6 -0
+#ifndef TRUE
+#define TRUE	1
+#endif
+#ifndef FALSE
+#define FALSE	0
+#endif

it's kernel convention to just use 0 and !0

OK.

Quoted 7 lines

drivers/block/diskdump.c line 74

Quoted 5 lines
drivers/block/diskdump.c +4 -0
+MODULE_PARM(fallback_on_err, "i");
+MODULE_PARM(allow_risky_dumps, "i");
+MODULE_PARM(block_order, "i");
+MODULE_PARM(sample_rate, "i");

please exclusively use the 2.6 module parameter mechanism for new code.

I see. I'll change this code using __setup.

Quoted 3 lines

drivers/block/diskdump.c line 313

Quoted 1 line
drivers/block/diskdump.c +1 -0
+		page = mem_map + nr;

this is not safe for discontig mem and thus broken

Ok, I fix it.

- page = mem_map + nr;
+ page = pfn_to_page(nr);

I also need fix this.

- for (nr = 0; nr < max_mapnr; nr++) {
+ for (nr = 0; nr < max_pfn; nr++) {

Quoted 6 lines

drivers/block/diskdump.c line 402

Quoted 4 lines
drivers/block/diskdump.c +4 -0
+	/*
+	 * Check the checksum of myself
+	 */
+	spin_trylock(&disk_dump_lock);

you need to handle the failure case of trylock of course

I see, thanks.

Quoted 6 lines
Quoted 3 lines
(quoted diff) +2 -0
+#ifdef CONFIG_PROC_FS
+static int proc_ioctl(struct inode *inode, struct file *file, unsigned

int cmd, unsigned long param)

ehhh this looks evil

Sorry, but would you please explain?

Quoted 3 lines
Quoted 3 lines

diff -Nur linux-2.6.6.org/drivers/block/genhd.c linux-2.6.6/drivers/block /genhd.c
--- linux-2.6.6.org/drivers/block/genhd.c 2004-05-10 11:32:29.000000000

+>> 0900

Quoted 1 line
Quoted 1 line

+++ linux-2.6.6/drivers/block/genhd.c 2004-06-09 19:17:46.000000000 +

0900

Quoted 8 lines

drivers/block/genhd.c line 227

Quoted 5 lines
drivers/block/genhd.c +1 -0
@@ -224,6 +224,8 @@
 	return  kobj ? to_disk(kobj) : NULL;
 }
 
+EXPORT_SYMBOL(get_gendisk);

this is WRONG. VERY WRONG.

I used get_gendisk() to get gendisk from inode.

	gd = get_gendisk(inode->i_rdev, &part);

But, I can get gendiskd by i_bdev->bd_disk, so this export is not needed. I remove it.

Best Regards,
Takao Indoh

#4 lore raw

Review

Quoted 4 lines

Ok, I fix it.

(quoted diff) +1 -1
-		page = mem_map + nr;
+		page = pfn_to_page(nr);

That's correct now ...

Review

Quoted 4 lines

I also need fix this.

(quoted diff) +1 -1
-	for (nr = 0; nr < max_mapnr; nr++) {
+	for (nr = 0; nr < max_pfn; nr++) {

... but that's not (at least AFAICS from this snippet). You need to iterate over pgdats, and then over the lmem_map inside each pgdat.

M.

author
#5 lore raw

Hi,

Quoted 7 lines · Martin J. Bligh

Review

Quoted 4 lines

Ok, I fix it.

(quoted diff) +1 -1
-		page = mem_map + nr;
+		page = pfn_to_page(nr);

That's correct now ...

Quoted 7 lines

Review

Quoted 4 lines

I also need fix this.

(quoted diff) +1 -1
-	for (nr = 0; nr < max_mapnr; nr++) {
+	for (nr = 0; nr < max_pfn; nr++) {

... but that's not (at least AFAICS from this snippet). You need to iterate over pgdats, and then over the lmem_map inside each pgdat.

How about this?

for_each_pgdat(pgdat) {
	for (i = 0; i < pgdat->node_spanned_pages; ++i) {
		page = pgdat->node_mem_map + i;
		...
        }
}
#6 lore raw
Quoted 15 lines
Quoted 7 lines

Review

Quoted 4 lines

Ok, I fix it.

(quoted diff) +1 -1
-		page = mem_map + nr;
+		page = pfn_to_page(nr);

That's correct now ...

Quoted 7 lines

Review

Quoted 4 lines

I also need fix this.

(quoted diff) +1 -1
-	for (nr = 0; nr < max_mapnr; nr++) {
+	for (nr = 0; nr < max_pfn; nr++) {

... but that's not (at least AFAICS from this snippet). You need to iterate over pgdats, and then over the lmem_map inside each pgdat.

Quoted 8 lines

How about this?

for_each_pgdat(pgdat) {
	for (i = 0; i < pgdat->node_spanned_pages; ++i) {
		page = pgdat->node_mem_map + i;
		...
        }
}

Yes, looks OK. I started writing out what it'd take to use the actual pfn_to_page macros, but it just makes a mess ... what you have is fine.

M.

author
#7 lore raw
In reply to Arjan van de Ven

Hi,

Now I am fixing diskdump according to comments by you and Christoph.

Quoted 6 lines · Arjan van de Ven
Quoted 3 lines
(quoted diff) +2 -0
+#ifdef CONFIG_PROC_FS
+static int proc_ioctl(struct inode *inode, struct file *file, unsigned

int cmd, unsigned long param)

ehhh this looks evil

Do you mean I should use not ioctl but the following style?

echo "add /dev/hda1" > /proc/diskdump
echo "delete /dev/hda1" > /proc/diskdump

Best Regards,
Takao Indoh

#8 lore raw
Quoted 13 lines · Takao Indoh

Hi,

Now I am fixing diskdump according to comments by you and Christoph.

Quoted 6 lines · Arjan van de Ven
Quoted 3 lines
(quoted diff) +2 -0
+#ifdef CONFIG_PROC_FS
+static int proc_ioctl(struct inode *inode, struct file *file, unsigned

int cmd, unsigned long param)

ehhh this looks evil

Quoted 4 lines

Do you mean I should use not ioctl but the following style?

echo "add /dev/hda1" > /proc/diskdump
echo "delete /dev/hda1" > /proc/diskdump

well no since /dev/hda is pointless; major/minor pairs maybe. But why in /proc???? it sounds like a sysfs job to me, where you probably want to represent a dump relationship with a symlink, and use "rm" to remove an entry..

author
#9 lore raw
Quoted 11 lines · Arjan van de Ven
Quoted 11 lines · Arjan van de Ven
Quoted 7 lines
Quoted 4 lines
(quoted diff) +2 -0
+#ifdef CONFIG_PROC_FS
+static int proc_ioctl(struct inode *inode, struct file *file, unsigned

int cmd, unsigned long param)

ehhh this looks evil

Do you mean I should use not ioctl but the following style?

Quoted 7 lines
Quoted 2 lines

echo "add /dev/hda1" > /proc/diskdump
echo "delete /dev/hda1" > /proc/diskdump

well no since /dev/hda is pointless; major/minor pairs maybe. But why in /proc???? it sounds like a sysfs job to me, where you probably want to represent a dump relationship with a symlink, and use "rm" to remove an entry..

Plural devices(partitions) can be registered to the diskdump as dump device, so it seems difficult to represent a dump relationship with a symlink.

Possible idea is adding new attribute to somewhere of sysfs entry, for example /sys/block/sda/sda1/dumpdev.
When new device is added, echo 1 to this entry.

# echo 1 > /sys/block/sda/sda1/dumpdev

Its store function is as follows.

static ssize_t store(struct kobject * kobj, struct attribute * attr,
const char * buf, size_t count)

{
        ssize_t ret;
        struct gendisk *disk = 
        	container_of(p->kobj.parent, struct gendisk, kobj);

        return dump_register_device(disk);
}

The problem is how to get scsi_device from gendisk.

It seems that the method using sysfs is more complex than current method using proc...
It may be better to use simply module parameter like netdump.

Best Regards,
Takao Indoh

#10 lore raw
In reply to Takao Indoh
Quoted 9 lines · Takao Indoh
Quoted 4 lines
Quoted 1 line

int cmd, unsigned long param)

ehhh this looks evil

Do you mean I should use not ioctl but the following style?

echo "add /dev/hda1" > /proc/diskdump
echo "delete /dev/hda1" > /proc/diskdump

the right interface would probably be a dump attribute for the scsi_device where you can echo enable or disable (or 1/0) to. The also nicely solves then need to find an associated scsi_device for a given block_dev.

author
#11 lore raw

PATCH 2/4 Diskdump Update

In reply to Takao Indoh

This is a patch for scsi common layer.

drivers/scsi/Kconfig +6 -0
diff -Nur linux-2.6.6.org/drivers/scsi/Kconfig linux-2.6.6/drivers/scsi/Kconfig
@@ -55,6 +55,12 @@
 	  In this case, do not compile the driver for your SCSI host adapter
 	  (below) as a module either.
 
+config SCSI_DUMP
+	tristate "SCSI dump support"
+	depends on DISKDUMP && SCSI
+	help
+	   SCSI dump support
+
 config CHR_DEV_ST
 	tristate "SCSI tape support"
 	depends on SCSI
drivers/scsi/Makefile +2 -0
diff -Nur linux-2.6.6.org/drivers/scsi/Makefile linux-2.6.6/drivers/scsi/Makefile
@@ -133,6 +133,8 @@
 obj-$(CONFIG_BLK_DEV_SR)	+= sr_mod.o
 obj-$(CONFIG_CHR_DEV_SG)	+= sg.o
 
+obj-$(CONFIG_SCSI_DUMP)		+= scsi_dump.o
+
 scsi_mod-y			+= scsi.o hosts.o scsi_ioctl.o constants.o \
 				   scsicam.o scsi_error.o scsi_lib.o \
 				   scsi_scan.o scsi_syms.o scsi_sysfs.o \
drivers/scsi/scsi.c +3 -0
diff -Nur linux-2.6.6.org/drivers/scsi/scsi.c linux-2.6.6/drivers/scsi/scsi.c
@@ -691,6 +691,9 @@
 {
 	unsigned long flags;
 
+	if (crashdump_mode())
+		return;
+
 	/*
 	 * We don't have to worry about this one timing out any more.
 	 * If we are unable to remove the timer, then the command
drivers/scsi/scsi_dump.c +574 -0

574 lines not on this page · raw message

drivers/scsi/scsi_error.c +3 -0
diff -Nur linux-2.6.6.org/drivers/scsi/scsi_error.c linux-2.6.6/drivers/scsi/scsi_error.c
@@ -402,6 +402,9 @@
  **/
 static void scsi_eh_done(struct scsi_cmnd *scmd)
 {
+	if (crashdump_mode())
+		return;
+
 	/*
 	 * if the timeout handler is already running, then just set the
 	 * flag which says we finished late, and return.  we have no
drivers/scsi/scsi_syms.c +3 -0
diff -Nur linux-2.6.6.org/drivers/scsi/scsi_syms.c linux-2.6.6/drivers/scsi/scsi_syms.c
@@ -25,6 +25,7 @@
 #include "scsi.h"
 
 #include "scsi_logging.h"
+#include "scsi_priv.h"
 
 
 /*

@@ -107,3 +108,5 @@
  */
 EXPORT_SYMBOL(scsi_add_timer);
 EXPORT_SYMBOL(scsi_delete_timer);
+
+EXPORT_SYMBOL(scsi_decide_disposition);
drivers/scsi/sd.c +15 -0
diff -Nur linux-2.6.6.org/drivers/scsi/sd.c linux-2.6.6/drivers/scsi/sd.c
@@ -192,6 +192,21 @@
 	up(&sd_ref_sem);
 }
 
+#if defined(CONFIG_DISKDUMP) || defined(CONFIG_DISKDUMP_MODULE)
+struct scsi_device *sd_find_scsi_device(dev_t dev)
+{
+	struct gendisk *disk;
+	int part;
+	disk = get_gendisk(dev, &part);
+	if(disk && disk->private_data)
+		return scsi_disk(disk)->device;
+	else
+		return NULL;
+}
+
+EXPORT_SYMBOL(sd_find_scsi_device);
+#endif
+
 /**
  *	sd_init_command - build a scsi (read or write) command from
  *	information in the request structure.
include/scsi/scsi_dump.h +38 -0
diff -Nur linux-2.6.6.org/include/scsi/scsi_dump.h linux-2.6.6/include/scsi/scsi_dump.h
@@ -0,0 +1,38 @@
+#ifndef _SCSI_DUMP_H
+#define _SCSI_DUMP_H
+
+/*
+ *  linux/include/scsi/scsi_dump.h
+ *
+ *  Copyright (C) 2004  FUJITSU LIMITED
+ *  Written by Nobuhiro Tachino ([email protected])
+ *
+ */
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+struct scsi_dump_ops {
+	int (*sanity_check)(struct scsi_device *);
+	int (*quiesce)(struct scsi_device *);
+	int (*shutdown)(struct scsi_device *);
+	void (*poll)(struct scsi_device *);
+};
+
+/* sd.c function */
+extern struct scsi_device *sd_find_scsi_device(dev_t);
+
+#endif /* _SCSI_DUMP_H */
include/scsi/scsi_host.h +5 -0
diff -Nur linux-2.6.6.org/include/scsi/scsi_host.h linux-2.6.6/include/scsi/scsi_host.h
@@ -345,6 +345,11 @@
 	 * module_init/module_exit.
 	 */
 	struct list_head legacy_hosts;
+
+	/*
+	 * operations for dump
+	 */
+	struct scsi_dump_ops *dump_ops;
 };
 
 /*
#12 lore raw

my old comments for this are still valid, please add the actual dumping methods directly to scsi_host_template instead of a pointer to another method vector, please make it not a module of it's own but part of the scsi code, please don't use "scsi.h" in new code, and the find gendisk by dev_t code in sd.c is still no good.

author
#13 lore raw
Quoted 3 lines · Christoph Hellwig

my old comments for this are still valid, please add the actual dumping methods directly to scsi_host_template instead of a pointer to another method vector,

I have already done in the latest patch.

drivers/scsi/aic7xxx/aic7xxx_osm.c +8 -0
diff -Nur linux-2.6.6.org/drivers/scsi/aic7xxx/aic7xxx_osm.c linux-2.6.6/drivers/scsi/aic7xxx/aic7xxx_osm.c
@@ -774,6 +774,10 @@
 static int	   ahc_linux_bus_reset(Scsi_Cmnd *);
 static int	   ahc_linux_dev_reset(Scsi_Cmnd *);
 static int	   ahc_linux_abort(Scsi_Cmnd *);
+#if defined(CONFIG_SCSI_DUMP) || defined(CONFIG_SCSI_DUMP_MODULE)
+static int	   ahc_linux_sanity_check(struct scsi_device *);
+static void	   ahc_linux_poll(struct scsi_device *);
+#endif
 
 /*
  * Calculate a safe value for AHC_NSEG (as expressed through ahc_linux_nseg).

@@ -1313,6 +1317,10 @@
 	.slave_alloc		= ahc_linux_slave_alloc,
 	.slave_configure	= ahc_linux_slave_configure,
 	.slave_destroy		= ahc_linux_slave_destroy,
+#if defined(CONFIG_SCSI_DUMP) || defined(CONFIG_SCSI_DUMP_MODULE)
+	.dump_sanity_check	= ahc_linux_sanity_check,
+	.dump_poll		= ahc_linux_poll,
+#endif
 };
 
 /**************************** Tasklet Handler *********************************/
Quoted 2 lines

please make it not a module of it's own but part of the scsi code,

Do you mean scsi_dump module should be merged with sd_mod.o or scsi_mod.o?

Quoted 1 line

please don't use "scsi.h" in new code,

Oh, I missed it. Thanks.

Quoted 2 lines

and the find gendisk
by dev_t code in sd.c is still no good.

In the latest source, sd_find_scsi_device, which I added, does not use dev_t. Here is the latest version.

struct scsi_device *sd_find_scsi_device(struct block_device *bdev)

{
	struct gendisk *disk = bdev->bd_disk;

	/* Check whether dev is scsi or not */
	if (!disk || (disk->fops != &sd_fops))
		return NULL;

	if(disk->private_data)
		return scsi_disk(disk)->device;
	else
		return NULL;
}

I'll try sysfs attribute instead of this.

Quoted 4 lines · Christoph Hellwig
Quoted 3 lines

Not the kind of interface we want exported. IMHO you shouldn't find device by dev_t but add a dumpdevice sysfs attribute to the scsi_device where you can echo 1 to to make it a possible dump device.

Best Regards,
Takao Indoh

#14 lore raw
Quoted 7 lines · Takao Indoh
Quoted 3 lines · Christoph Hellwig

my old comments for this are still valid, please add the actual dumping methods directly to scsi_host_template instead of a pointer to another method vector,

I have already done in the latest patch.

Okay. Looks like I looked at an older patch, sorry.

drivers/scsi/aic7xxx/aic7xxx_osm.c line 780

Quoted 12 lines

diff -Nur linux-2.6.6.org/drivers/scsi/aic7xxx/aic7xxx_osm.c linux-2.6.6/drivers/scsi/aic7xxx/aic7xxx_osm.c

drivers/scsi/aic7xxx/aic7xxx_osm.c +4 -0
@@ -774,6 +774,10 @@
 static int	   ahc_linux_bus_reset(Scsi_Cmnd *);
 static int	   ahc_linux_dev_reset(Scsi_Cmnd *);
 static int	   ahc_linux_abort(Scsi_Cmnd *);
+#if defined(CONFIG_SCSI_DUMP) || defined(CONFIG_SCSI_DUMP_MODULE)
+static int	   ahc_linux_sanity_check(struct scsi_device *);
+static void	   ahc_linux_poll(struct scsi_device *);
+#endif

I'd say implement these unconditionally, it's not _that_ much code..

Quoted 4 lines
Quoted 2 lines

please make it not a module of it's own but part of the scsi code,

Do you mean scsi_dump module should be merged with sd_mod.o or scsi_mod.o?

scsi_mod.o.

Quoted 1 line

I'll try sysfs attribute instead of this.

Okay, thanks. Or some other way to find by host/channel/target/lun using scsi_device_lookup.

author
#15 lore raw
Quoted 6 lines · Christoph Hellwig
Quoted 4 lines
Quoted 2 lines

please make it not a module of it's own but part of the scsi code,

Do you mean scsi_dump module should be merged with sd_mod.o or scsi_mod.o?

scsi_mod.o.

It is difficult because disk_dump and scsi_dump try to check checksum of itself using check_crc_module so as to confirm whether module is compromised or not.

drivers/scsi/scsi_dump.c:
static int
scsi_dump_sanity_check(struct disk_dump_device *dump_device)

{
        struct scsi_device *sdev = dump_device->device;
        struct Scsi_Host *host = sdev->host;
        int adapter_sanity = 0;
        int sanity = 0;

        if (!check_crc_module()) {
                Err("checksum error. scsi dump module may be compromised.
");
                return -EINVAL;
        }

Therefore, scsi_dump need to be always compiled as independent module. If scsi_dump is merged with scsi_mod, scsi_mod is not able to be compiled statically.

Best Regards,
Takao Indoh

#16 lore raw
Quoted 9 lines · Takao Indoh
Quoted 6 lines · Christoph Hellwig
Quoted 4 lines
Quoted 2 lines

please make it not a module of it's own but part of the scsi code,

Do you mean scsi_dump module should be merged with sd_mod.o or scsi_mod.o?

scsi_mod.o.

Quoted 8 lines

It is difficult because disk_dump and scsi_dump try to check checksum of itself using check_crc_module so as to confirm whether module is compromised or not.

Therefore, scsi_dump need to be always compiled as independent module. If scsi_dump is merged with scsi_mod, scsi_mod is not able to be compiled statically.

So check if it's a module and otherwise not.

author
#17 lore raw
Quoted 16 lines · Christoph Hellwig
Quoted 13 lines · Takao Indoh
Quoted 7 lines · Christoph Hellwig
Quoted 5 lines
Quoted 2 lines

please make it not a module of it's own but part of the scsi code,

Do you mean scsi_dump module should be merged with sd_mod.o or scsi_mod.o?

scsi_mod.o.

It is difficult because disk_dump and scsi_dump try to check checksum of itself using check_crc_module so as to confirm whether module is compromised or not.

Quoted 5 lines
Quoted 3 lines

Therefore, scsi_dump need to be always compiled as independent module. If scsi_dump is merged with scsi_mod, scsi_mod is not able to be compiled statically.

So check if it's a module and otherwise not.

It means checksum is not used when scsi_mod is compiled statically. I think it is very important to check the checksum before dump starts.

The most serious problem of dumping to the disk is that dumping may overwrite parts of filesystem and destroy user data. This problem happens when data structures or codes which is used during dumping are broken.

Diskdump solves this problem by the following methods.
a) Using module checksum, check whether codes (and data) are broken.
b) Confirm whether the target device is really dump device, by checking that target device is rightly formatted.

Therefore, checking the checksum is necessary before dumping. scsi_dump always needs to be compiled as module. So I said it is difficult to merge scsi_dump with scsi_mod.

Best Regards,
Takao Indoh

author
#18 lore raw

PATCH 3/4 Diskdump Update

In reply to Takao Indoh

This is a patch for aic7xxx driver.

drivers/scsi/aic7xxx/aic7xxx_osm.c +43 -0
diff -Nur linux-2.6.6.org/drivers/scsi/aic7xxx/aic7xxx_osm.c linux-2.6.6/drivers/scsi/aic7xxx/aic7xxx_osm.c
@@ -774,6 +774,8 @@
 static int	   ahc_linux_bus_reset(Scsi_Cmnd *);
 static int	   ahc_linux_dev_reset(Scsi_Cmnd *);
 static int	   ahc_linux_abort(Scsi_Cmnd *);
+static int	   ahc_linux_sanity_check(struct scsi_device *);
+static void	   ahc_linux_poll(struct scsi_device *);
 
 /*
  * Calculate a safe value for AHC_NSEG (as expressed through ahc_linux_nseg).

@@ -1294,6 +1296,11 @@
 	return SUCCESS;
 }
 
+static struct scsi_dump_ops ahc_dump_ops = {
+	.sanity_check   = ahc_linux_sanity_check,
+	.poll           = ahc_linux_poll,
+};
+
 Scsi_Host_Template aic7xxx_driver_template = {
 	.module			= THIS_MODULE,
 	.name			= "aic7xxx",

@@ -1313,6 +1320,7 @@
 	.slave_alloc		= ahc_linux_slave_alloc,
 	.slave_configure	= ahc_linux_slave_configure,
 	.slave_destroy		= ahc_linux_slave_destroy,
+	.dump_ops		= &ahc_dump_ops,
 };
 
 /**************************** Tasklet Handler *********************************/

@@ -3863,6 +3871,41 @@
 	return IRQ_RETVAL(ours);
 }
 
+static int
+ahc_linux_sanity_check(struct scsi_device *device)
+{
+	struct ahc_softc *ahc;
+	struct ahc_linux_device *dev;
+
+	ahc = *(struct ahc_softc **)device->host->hostdata;
+	dev = ahc_linux_get_device(ahc, device->channel,
+				   device->id, device->lun,
+					   /*alloc*/FALSE);
+	if (dev == NULL)
+		return -ENXIO;
+	if (ahc->platform_data->qfrozen || dev->qfrozen)
+		return -EBUSY;
+	if (spin_is_locked(&ahc->platform_data->spin_lock))
+		return -EBUSY;
+	return 0;
+}
+
+static void
+ahc_linux_poll(struct scsi_device *device)
+{
+	struct ahc_softc *ahc;
+	struct ahc_linux_device *dev;
+
+	ahc = *(struct ahc_softc **)device->host->hostdata;
+	ahc_intr(ahc);
+	while ((dev = ahc_linux_next_device_to_run(ahc)) != NULL) {
+		TAILQ_REMOVE(&ahc->platform_data->device_runq, dev, links);
+		dev->flags &= ~AHC_DEV_ON_RUN_LIST;
+		ahc_linux_check_device_queue(ahc, dev);
+	}
+	ahc_linux_run_complete_queue(ahc);
+}
+
 void
 ahc_platform_flushwork(struct ahc_softc *ahc)
 {
drivers/scsi/aic7xxx/aic7xxx_osm.h +19 -0
diff -Nur linux-2.6.6.org/drivers/scsi/aic7xxx/aic7xxx_osm.h linux-2.6.6/drivers/scsi/aic7xxx/aic7xxx_osm.h
@@ -67,6 +67,7 @@
 #include <linux/smp_lock.h>
 #include <linux/version.h>
 #include <linux/module.h>
+#include <linux/diskdumplib.h>
 #include <asm/byteorder.h>
 #include <asm/io.h>
 

@@ -82,6 +83,8 @@
 #include <linux/malloc.h>
 #endif
 
+#include <scsi/scsi_dump.h>
+
 /* Core SCSI definitions */
 #define AIC_LIB_PREFIX ahc
 #include "scsi.h"

@@ -253,6 +256,22 @@
  */
 #define ahc_dmamap_sync(ahc, dma_tag, dmamap, offset, len, op)
 
+
+/******************************** Disk dump ***********************************/
+#if defined(CONFIG_DISKDUMP) || defined(CONFIG_DISKDUMP_MODULE)
+#undef  add_timer
+#define add_timer       diskdump_add_timer
+#undef  del_timer_sync
+#define del_timer_sync  diskdump_del_timer
+#undef  del_timer
+#define del_timer       diskdump_del_timer
+#undef  mod_timer
+#define mod_timer       diskdump_mod_timer
+
+#define tasklet_schedule        diskdump_tasklet_schedule
+#endif
+
+
 /************************** Timer DataStructures ******************************/
 typedef struct timer_list ahc_timer_t;
author
#19 lore raw

PATCH 4/4 Diskdump Update

In reply to Takao Indoh

This is a patch for aic79xx driver.

drivers/scsi/aic7xxx/aic79xx_osm.c +49 -0
diff -Nur linux-2.6.6.org/drivers/scsi/aic7xxx/aic79xx_osm.c linux-2.6.6/drivers/scsi/aic7xxx/aic79xx_osm.c
@@ -786,6 +786,10 @@
 static int	   ahd_linux_bus_reset(Scsi_Cmnd *);
 static int	   ahd_linux_dev_reset(Scsi_Cmnd *);
 static int	   ahd_linux_abort(Scsi_Cmnd *);
+#if defined(CONFIG_SCSI_DUMP) || defined(CONFIG_SCSI_DUMP_MODULE)
+static int	   ahd_linux_sanity_check(struct scsi_device *);
+static void	   ahd_linux_poll(struct scsi_device *);
+#endif
 
 /*
  * Calculate a safe value for AHD_NSEG (as expressed through ahd_linux_nseg).

@@ -1665,6 +1669,13 @@
 	return (SUCCESS);
 }
 
+#if defined(CONFIG_SCSI_DUMP) || defined(CONFIG_SCSI_DUMP_MODULE)
+static struct scsi_dump_ops ahd_dump_ops = {
+	.sanity_check	= ahd_linux_sanity_check,
+	.poll		= ahd_linux_poll,
+};
+#endif	/* CONFIG_SCSI_DUMP || CONFIG_SCSI_DUMP_MODULE */
+
 Scsi_Host_Template aic79xx_driver_template = {
 	.module			= THIS_MODULE,
 	.name			= "aic79xx",

@@ -1684,6 +1695,9 @@
 	.slave_alloc		= ahd_linux_slave_alloc,
 	.slave_configure	= ahd_linux_slave_configure,
 	.slave_destroy		= ahd_linux_slave_destroy,
+#if defined(CONFIG_SCSI_DUMP) || defined(CONFIG_SCSI_DUMP_MODULE)
+	.dump_ops		= &ahd_dump_ops,
+#endif	/* CONFIG_SCSI_DUMP || CONFIG_SCSI_DUMP_MODULE */
 };
 
 /**************************** Tasklet Handler *********************************/

@@ -4190,6 +4204,41 @@
 	return IRQ_RETVAL(ours);
 }
 
+#if defined(CONFIG_SCSI_DUMP) || defined(CONFIG_SCSI_DUMP_MODULE)
+static int
+ahd_linux_sanity_check(struct scsi_device *device)
+{
+	struct ahd_softc *ahd;
+	struct ahd_linux_device *dev;
+
+	ahd = *(struct ahd_softc **)device->host->hostdata;
+	dev = ahd_linux_get_device(ahd, device->channel,
+				   device->id, device->lun,
+				   /*alloc*/FALSE);
+
+	if(dev == NULL)
+		return -ENXIO;
+	if(ahd->platform_data->qfrozen || dev->qfrozen)
+		return -EBUSY;
+	if(spin_is_locked(&ahd->platform_data->spin_lock))
+		return -EBUSY;
+	return 0;
+}
+
+static void
+ahd_linux_poll(struct scsi_device *device)
+{
+	struct ahd_softc *ahd;
+	int ours;
+
+	ahd = *(struct ahd_softc **)device->host->hostdata;
+	ours = ahd_intr(ahd);
+	if(ahd_linux_next_device_to_run(ahd) != NULL)
+		ahd_schedule_runq(ahd);
+	ahd_linux_run_complete_queue(ahd);
+}
+#endif	/* CONFIG_SCSI_DUMP || CONFIG_SCSI_DUMP_MODULE */
+
 void
 ahd_platform_flushwork(struct ahd_softc *ahd)
 {
drivers/scsi/aic7xxx/aic79xx_osm.h +21 -0
diff -Nur linux-2.6.6.org/drivers/scsi/aic7xxx/aic79xx_osm.h linux-2.6.6/drivers/scsi/aic7xxx/aic79xx_osm.h
@@ -50,6 +50,9 @@
 #include <linux/smp_lock.h>
 #include <linux/version.h>
 #include <linux/module.h>
+#if defined(CONFIG_SCSI_DUMP) || defined(CONFIG_SCSI_DUMP_MODULE)
+#include <linux/diskdumplib.h>
+#endif
 #include <asm/byteorder.h>
 #include <asm/io.h>
 

@@ -65,6 +68,10 @@
 #include <linux/malloc.h>
 #endif
 
+#if defined(CONFIG_SCSI_DUMP) || defined(CONFIG_SCSI_DUMP_MODULE)
+#include <scsi/scsi_dump.h>
+#endif
+
 /* Core SCSI definitions */
 #define AIC_LIB_PREFIX ahd
 #include "scsi.h"

@@ -96,6 +103,20 @@
 /* No debugging code. */
 #endif
 
+/********************************** Disk Dump *********************************/
+#if defined(CONFIG_SCSI_DUMP) || defined(CONFIG_SCSI_DUMP_MODULE)
+#undef	add_timer
+#define	add_timer		diskdump_add_timer
+#undef	del_timer_sync
+#define	del_timer_sync		diskdump_del_timer
+#undef	del_timer
+#define	del_timer		diskdump_del_timer
+#undef	mod_timer
+#define	mod_timer		diskdump_mod_timer
+
+#define	tasklet_schedule	diskdump_tasklet_schedule
+#endif
+
 /********************************** Misc Macros *******************************/
 #define	roundup(x, y)   ((((x)+((y)-1))/(y))*(y))
 #define	powerof2(x)	((((x)-1)&(x))==0)
author
#20 lore raw
In reply to Takao Indoh
Quoted 33 lines · Takao Indoh
Quoted 9 lines · Christoph Hellwig
Quoted 9 lines

+/******************************** Disk dump **************************** *******/

(quoted diff) +7 -0
+#if defined(CONFIG_DISKDUMP) || defined(CONFIG_DISKDUMP_MODULE)
+#undef  add_timer
+#define add_timer       diskdump_add_timer
+#undef  del_timer_sync
+#define del_timer_sync  diskdump_del_timer
+#undef  del_timer
+#define del_timer       diskdump_del_timer
Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+#undef  mod_timer
+#define mod_timer       diskdump_mod_timer
+
+#define tasklet_schedule        diskdump_tasklet_schedule
+#endif

Yikes. No way in hell we'll place code like this in drivers. This needs to be handled in common code.

Another approach is insering some codes into the core timer and tasklet routines.

For example,

static inline void add_timer(struct timer_list * timer)

{
	if(crashdump_mode())
		__diskdump_add_timer(timer);
	else
		__mod_timer(timer, timer->expires);
}
Quoted 2 lines

But I do not want to make common codes dirty...
Please let me know more good idea!

I forgot to explain what is problem. At first, I explain how diskdump writes the system memory to the disk.

disk_dump() in drivers/block/diskdump.c is main routine of diskdump. It is called from die()/panic().

First, disk_dump() silences system as follows.

	local_save_flags(flags);
	local_irq_disable();
	smp_call_function(freeze_cpu, NULL, 1, 0);

Diskdump disables interrupt and stops other cpus.
Next, after preparing for dump, diskdump starts dumping. Diskdump calls driver's handler via scis_dump module. Driver's handler writes data to the disk with polling mode.

What is a problem? Scsi driver uses timer/tasklet to complete I/O. But, diskdump disables interrupt, so timer/taslket doesn't work!

The current diskdump uses the following macros to solve this problem.

#define add_timer       diskdump_add_timer
#define del_timer_sync  diskdump_del_timer
#define del_timer       diskdump_del_timer
#define mod_timer       diskdump_mod_timer

#define tasklet_schedule diskdump_tasklet_schedule

ex.
static inline void diskdump_add_timer(struct timer_list *timer)

{
	if (crashdump_mode())
		_diskdump_add_timer(timer);
	else
		add_timer(timer);
}

It's very easy way. If diskdump is working, timer is registered with diskdump. Otherwise timer is registered with normal kernel timer. The timer registered with diskdump is called by diskdump itself during dumping.

This way is easy but ugly. Another way is add new codes into the core timer and tasklet routines as I wrote in the previous mail. I wondered whether it is possible to insert general-purpose hooks into the timer/ taslket routine.

Please let me know if there is another good idea!

Best Regards,
Takao Indoh