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-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]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |