LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Elias Oltmanns <eo@nebensachen.de>
To: Pavel Machek <pavel@ucw.cz>
Cc: Jens Axboe <jens.axboe@oracle.com>,
	Christoph Schmid <chris@schlagmichtod.de>,
	linux-kernel@vger.kernel.org
Subject: Re: is there any Hard-disk shock-protection for 2.6.18 and above?
Date: Fri, 01 Dec 2006 15:19:55 +0100	[thread overview]
Message-ID: <87k61bpuk4.fsf@denkblock.local> (raw)
In-Reply-To: <20061130171910.GD1860@elf.ucw.cz> (Pavel Machek's message of "Thu\, 30 Nov 2006 18\:19\:10 +0100")

Hi Pavel,

thanks a lot for your first review. See comments below.

Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
[...]
>> Here is a short description of what the patch does in its current
>> shape:
>> 
>> 1. Adds functions to ide-disk.c and scsi_lib.c that issue an idle
>>    immediate with head unload or a standby immediate command as
>>    appropriate and stop the queue on command completion.
>
> Can we get short Documentation/ patch?

Sure. Would Documentation/block/disk-protection.txt be an appropriate
location?

[...]
>> +module_param_named(protect_method, libata_protect_method, int, 0444);
>> +MODULE_PARM_DESC(protect_method, "hdaps disk protection method  (0=autodetect, 1=unload, 2=standby)");
>
> Should this be configurable by module parameter? Why not tell each
> unload what to do?

As I understand, ATA specs expect drives to indicate whether they
support the head unload feature of the idle immediate command or not.
Unfortunately, a whole lot of them doesn't, well, mine doesn't anyway.
Since I know that my drive does actually support head unloading, I'd
like to tell the module so in order to prevent it from falling back to
standby immediate. Applications that issue disk parking requests
should not be bothered with this issue, in my opinion.

>
> Is /sys interface right thing to do?

Probably, you're right here. Since this feature is actually drive
specific, it should not really be set globally as a libata or ide-disk
parameter but specifically for each drive connected. Perhaps we should
add another attribute to /sys/block/*/queue or enhance the scope of
/sys/block/*/queue/protect?

[...]
>> +	else if (libata_protect_method == 2) {
>> +		unload = 0;	
>> +		printk(KERN_DEBUG "ata_scsi_issue_protect_fn(): standby method requested, overriding drive capability check..\n");
>> +	}
>> +	else if (ata_id_has_unload(dev->id)) {
>> +		unload = 1;
>> +		printk(KERN_DEBUG "ata_scsi_issue_protect_fn(): unload support reported by drive..\n");
>> +	}
>> +	else {
>> +		unload = 0;
>> +		printk(KERN_DEBUG "ata_scsi_issue_protect_fn(): unload support NOT reported by drive!..\n");
>> +	}
>
> Can we consolidate the strings somehow?

Actually, I'd like to move this to the initialisation sequence of the
drive. I still have to figure out how to do this properly.

[...]
>> +	/*
>> +	 * Auto-unfreeze state
>> +	 */
>> +	struct timer_list	unfreeze_timer;
>> +	int			max_unfreeze;	/* At most this many seconds */
>> +	struct work_struct	unfreeze_work;
>> +
>>  	struct backing_dev_info	backing_dev_info;
>>
>
> Should we have kernel doing auto-unfreeze? Perhaps we can just mlock()
> the daemon?
> 								Pavel

I'd strongly second Shem's comments on this. Besides, anybody with
root privileges can issue diks park requests, not just hdapsd. Please
note that this is not a hard timeout as userspace can always refresh
the timer before it has actually expired. On the other hand I would
not want to rely on user space to unfreeze my drives.

Regards

Elias

  parent reply	other threads:[~2006-12-01 14:20 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <7ibks-1fg-15@gated-at.bofh.it>
     [not found] ` <7kpjn-7th-23@gated-at.bofh.it>
     [not found]   ` <7kDFF-8rd-29@gated-at.bofh.it>
2006-11-27 18:31     ` Elias Oltmanns
2006-11-30 17:19       ` Pavel Machek
2006-11-30 17:51         ` Shem Multinymous
2006-12-01 14:19         ` Elias Oltmanns [this message]
2006-12-02 11:57           ` Pavel Machek
2006-12-10  1:02             ` Elias Oltmanns
2006-12-10  1:16               ` Elias Oltmanns
2006-12-11  8:26                 ` Jens Axboe
2007-02-04 20:41               ` Pavel Machek
2007-02-08 10:04                 ` Elias Oltmanns
2006-11-30 18:43       ` Jens Axboe
2006-11-17 12:47 Christoph Schmid
2006-11-21 20:51 ` Pavel Machek
2006-11-23 18:26   ` Shem Multinymous
2006-11-30 17:11     ` Pavel Machek
2006-11-30 17:47       ` Shem Multinymous
2006-11-24  7:21   ` Jens Axboe
2006-11-26 23:14     ` Jon Escombe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87k61bpuk4.fsf@denkblock.local \
    --to=eo@nebensachen.de \
    --cc=chris@schlagmichtod.de \
    --cc=jens.axboe@oracle.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pavel@ucw.cz \
    --subject='Re: is there any Hard-disk shock-protection for 2.6.18 and above?' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).