Re: [PATCH 1/4]Diskdump Update

[Takao Indoh <indou.takao@soft.fujitsu.com>]

Hi,

On Fri, 11 Jun 2004 13:50:45 +0200, Arjan van de Ven wrote:

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


>
>
>> +#ifndef TRUE
>> +#define TRUE	1
>> +#endif
>> +#ifndef FALSE
>> +#define FALSE	0
>> +#endif
>
>it's kernel convention to just use 0 and !0 

OK.


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



>> +		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++) {



>> +	/*
>> +	 * Check the checksum of myself
>> +	 */
>> +	spin_trylock(&disk_dump_lock);
>
>you need to handle the failure case of trylock of course

I see, thanks.



>> +#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?



>> 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
>> +++ linux-2.6.6/drivers/block/genhd.c	2004-06-09 19:17:46.000000000 +
0900
>> @@ -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