On 9/24/17 17:14, Christoph Hellwig wrote:
>> +
>> + struct request_queue *q;
>
> Do you really need the queue backpointer? At least as far as this
> patch is concerned we could just pass the queue on to
> deadline_enable_zones_wlock and be fine. And in general we should
> always passing the q, as we can trivial go from queue to deadline_data
> using queue->elevator->elevator_data.
This is for the sysfs zones_wlock store function which does not give the
queue. Instead of this backpointer, I can copy the queue node, number of
zones and zone model so that cdeadline_enable_zones_wlock() can be
called equally from init_queue context and from the sysfs zones_wlock
store context.
>> +static int deadline_zoned_init_queue(struct request_queue *q,
>> + struct deadline_data *dd)
>> +{
>> + if (!blk_queue_is_zoned(q) ||
>> + !blk_queue_nr_zones(q)) {
>
> Shouldn't !blk_queue_nr_zones(q) be enough? If not both conditionals
> could easily fit into the same line, and I'd be tempted to move them
> to the caller and call deadline_enable_zones_wlock straight from there.
OK. Will update.
>> @@ -341,6 +387,15 @@ static int dd_init_queue(struct request_queue *q, struct elevator_type *e)
>> spin_lock_init(&dd->lock);
>> INIT_LIST_HEAD(&dd->dispatch);
>>
>> + dd->q = q;
>> + spin_lock_init(&dd->zone_lock);
>> + ret = deadline_zoned_init_queue(q, dd);
>> + if (ret) {
>> + kfree(dd);
>> + kobject_put(&eq->kobj);
>> + return ret;
>> + }
>> +
>> q->elevator = eq;
>> return 0;
>
> This should probably grow goto based unwinding, e.g.
OK. Will update.
Thanks !
--
Damien Le Moal
Western Digital Research
