On Fri, Aug 26 2005, Yani Ioannou wrote: > > Please make the interface accept number of seconds (as suggested by Jens) > > and remove this module parameter. This way interface will be more flexible > > and cleaner. I really don't see any advantage in doing "echo 1 > ..." > > instead > > of "echo x > ..." (Pavel, please explain). > > Either way is pretty easy enough to implement. Note though that I'd > expect the userspace app should thaw the device when danger is out of > the way (the timeout is mainly there to ensure that the queue isn't > frozen forever, and should probably be higher). Personally I don't > have too much of an opinion either way though... what's the consensus? > :).
Yes please, I don't understand why you would want a 0/1 interface instead, when the timer-seconds method gives you the exact same ability plus a way to control when to unfreeze... > > +static struct timer_list freeze_timer = > > + TIMER_INITIALIZER(freeze_expire, 0, 0); > > > > There needs to be a timer per device not a global one > > (it works for a current special case of T42 but sooner > > or later we will hit this problem). > > I was considering that, but I am confused as to whether each drive has > it's own queue or not? (I really am a newbie to this stuff...). If so > then yes there should be a per-device timer. Each drive has its own queue. > > queue handling should be done through block layer helpers > > (as described in Jens' email) - we will need them for libata too. > > Good point, I'll try to move as much as I can up to the block layer, > it helps when it comes to implementing freeze for libata as you point > out too. That includes things like the timer as well, reuse the queue plugging timer as I described in my initial posting on how to implement this. > > At this time attribute can still be in use (because refcounting is done > > on drive->gendev), you need to add "disk" class to ide-disk driver > > (drivers/scsi/st.c looks like a good example how to do it). > > I missed that completely, I'll look at changing it. > > > IMO this should also be handled by block layer > > which has all needed information, Jens? > > > > While at it: I think that sysfs support should be moved to block layer > > (queue > > attributes) and storage driver should only need to provide queue_freeze_fn > > and queue_thaw_fn functions (similarly to cache flush support). This should > > be done now not later because this stuff is exposed to the user-space. > > I was actually considering using a queue attribute originally, but in > my indecision I decided to go with Jen's suggestion. A queue attribute > does make sense in that the attribute primarily is there to freeze the > queue, but it would also be performing the head park. Would a queue > attribute be confusing because of that? I fully agree with Bart here. The only stuff that should be ide special is the actual command setup and completion check, since that is a hardware property. libata will get a few little helpers for that as well. The rest should be block layer implementation. > > * Sanity: don't accept a request that isn't a PM request > > * if we are currently power managed. This is very > > important as > > * blk_stop_queue() doesn't prevent the elv_next_request() > > @@ -1661,6 +1671,9 @@ int ide_do_drive_cmd (ide_drive_t *drive > > where = ELEVATOR_INSERT_FRONT; > > rq->flags |= REQ_PREEMPT; > > } > > + if (action == ide_next) > > + where = ELEVATOR_INSERT_FRONT; > > + > > __elv_add_request(drive->queue, rq, where, 0); > > ide_do_request(hwgroup, IDE_NO_IRQ); > > spin_unlock_irqrestore(&ide_lock, flags); > > > > Why is this needed? > > I think Jon discussed that in a previous thread, but basically > although ide_next is documented in the comment for ide_do_drive_cmd, > there isn't (as far as Jon or I could see) anything actually handling > it. This patch is carried over from Jon's work and adds the code to > handle ide_next by inserting the request at the front of the queue. As per my previous mail, I will ack that bit. > > Overall, very promising work! > > Thanks :-), most of it is Jon's work, and Jen's suggestions though. My name is Jens, not Jen :-) -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html