Hi Pavel,
thanks a lot for your first review. See comments below.
Pavel Machek <[email protected]> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
