Re: [PATCH 10/17] scsi_dh_alua: Use separate alua_port_group structure

Bart Van Assche
2015-05-07T12:34:39Z

On 05/04/15 14:42, Hannes Reinecke wrote:
> +static void release_port_group(struct kref *kref)
> +{
> + struct alua_port_group *pg;
> +
> + pg = container_of(kref, struct alua_port_group, kref);
> + printk(KERN_WARNING "alua: release port group %d\n", pg->group_id);

Does this really need to be a warning ?

> + spin_lock(&port_group_lock);
> + pg = kzalloc(sizeof(struct alua_port_group), GFP_ATOMIC);
> + if (!pg) {
> + sdev_printk(KERN_WARNING, sdev,
> + "%s: kzalloc port group failed\n",
> + ALUA_DH_NAME);
> + /* Temporary failure, bypass */
> + spin_unlock(&port_group_lock);
> + return SCSI_DH_DEV_TEMP_BUSY;
> + }
> + pg->group_id = group_id;
> + pg->buff = pg->inq;
> + pg->bufflen = ALUA_INQUIRY_SIZE;
> + pg->tpgs = h->tpgs;
> + pg->state = TPGS_STATE_OPTIMIZED;
> + kref_init(&pg->kref);
> + list_add(&pg->node, &port_group_list);
> + h->pg = pg;
> + spin_unlock(&port_group_lock);

Sorry but it's not clear to me why the kzalloc() statement happens under
port_group_lock. Please consider to perform only the list_add()
statement under port_group_lock, to perform all other assignments
without holding that lock and to change GFP_ATOMIC into GFP_KERNEL.

> - len = (h->buff[0] << 24) + (h->buff[1] << 16) +
> - (h->buff[2] << 8) + h->buff[3] + 4;
> + len = (pg->buff[0] << 24) + (pg->buff[1] << 16) +
> + (pg->buff[2] << 8) + pg->buff[3] + 4;

Has it been considered to use get_unaligned_be32() instead ?

Bart.