Re: [PATCH V4 10/16] block: mq-deadline: Add zoned block device data
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(>lock);
>> INIT_LIST_HEAD(>dispatch);
>>
>> +dd->q = q;
>> +spin_lock_init(>zone_lock);
>> +ret = deadline_zoned_init_queue(q, dd);
>> +if (ret) {
>> +kfree(dd);
>> +kobject_put(>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
Re: [PATCH V4 10/16] block: mq-deadline: Add zoned block device data
> +
> + 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.
> +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.
> @@ -341,6 +387,15 @@ static int dd_init_queue(struct request_queue *q, struct
> elevator_type *e)
> spin_lock_init(>lock);
> INIT_LIST_HEAD(>dispatch);
>
> + dd->q = q;
> + spin_lock_init(>zone_lock);
> + ret = deadline_zoned_init_queue(q, dd);
> + if (ret) {
> + kfree(dd);
> + kobject_put(>kobj);
> + return ret;
> + }
> +
> q->elevator = eq;
> return 0;
This should probably grow goto based unwinding, e.g.
int ret = -ENOMEM;
...
dd = kzalloc_node(sizeof(*dd), GFP_KERNEL, q->node);
if (!dd)
goto out_put_object;
...
if (blk_queue_nr_zones(q))) {
ret = deadline_enable_zones_wlock(...);
if (ret)
goto out_free_dd;
}
q->elevator = eq;
return 0;
out_free_dd:
kfree(dd);
out_put_object
kobject_put(>kobj);
return ret;
