Re: [PATCH 6/9] scsi_dh: add generic SPC-3 alua handler

[Hannes Reinecke <hare@suse.de>]

Mike Christie wrote:
> Hannes Reinecke wrote:
>> +
>> +static struct request *get_alua_req(struct scsi_device *sdev,
>> +                    void *buffer, unsigned buflen, int rw)
>> +{
>> +    struct request *rq;
>> +    struct request_queue *q = sdev->request_queue;
>> +
>> +    rq = blk_get_request(q, rw, GFP_KERNEL);
>> +
>> +    if (!rq) {
>> +        sdev_printk(KERN_INFO, sdev,
>> +                "%s: blk_get_request failed\n", __FUNCTION__);
>> +        return NULL;
>> +    }
>> +
>> +    if (buflen && blk_rq_map_kern(q, rq, buffer, buflen, GFP_KERNEL)) {
>> +        blk_put_request(rq);
>> +        sdev_printk(KERN_INFO, sdev,
>> +                "%s: blk_rq_map_kern failed\n", __FUNCTION__);
>> +        return NULL;
>> +    }
>> +
>> +    rq->cmd_type = REQ_TYPE_BLOCK_PC;
>> +    rq->cmd_flags |= REQ_FAILFAST | REQ_NOMERGE;
>> +    rq->retries = ALUA_FAILOVER_RETRIES;
>> +    rq->timeout = ALUA_FAILOVER_TIMEOUT;
>> +
>> +    return rq;
>> +}
> 
> 
> It looks like this can be called from alua_activate, and we cannot use 
> GFP_KERNEL in the same IO path something could get written to.
> 
Is something like GFP_ATOMIC more appropriate?

[ .. ]
>> +/*
>> + * alua_std_inquiry - Evaluate standard INQUIRY command
>> + * @sdev: device to be checked
>> + *
>> + * Just extract the TPGS setting to find out if ALUA
>> + * is supported.
>> + */
>> +static int alua_std_inquiry(struct scsi_device *sdev, struct 
>> alua_dh_data *h)
>> +{
>> +    int err;
>> +
>> +    err = submit_std_inquiry(sdev, h);
>> +
> 
> You could remove the space so it looks like the other code.
> 
> 
Sure.

[ .. ]
>> +/*
>> + * alua_bus_attach - Attach device handler
>> + * @sdev: device to be attached to
>> + */
>> +static int alua_bus_attach(struct scsi_device *sdev)
>> +{
>> +    struct scsi_dh_data *scsi_dh_data;
>> +    struct alua_dh_data *h;
>> +    unsigned long flags;
>> +    int err = SCSI_DH_OK;
>> +
>> +    scsi_dh_data = kzalloc(sizeof(struct scsi_device_handler *)
>> +                   + sizeof(*h) , GFP_KERNEL);
>> +    if (!scsi_dh_data) {
>> +        sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
>> +                ALUA_DH_NAME);
>> +        return -ENOMEM;
>> +    }
>> +
>> +    scsi_dh_data->scsi_dh = &alua_dh;
>> +    h = (struct alua_dh_data *) scsi_dh_data->buf;
>> +    h->tpgs = TPGS_MODE_UNINITIALIZED;
>> +    h->state = TPGS_STATE_OPTIMIZED;
>> +    h->group_id = -1;
>> +    h->rel_port = -1;
>> +    h->buff = h->inq;
>> +    h->bufflen = ALUA_INQUIRY_SIZE;
>> +
>> +    err = alua_initialize(sdev, h);
>> +    if (err != SCSI_DH_OK)
>> +        goto failed;
>> +
>> +    spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
>> +    sdev->scsi_dh_data = scsi_dh_data;
>> +    spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
>> +
>> +    try_module_get(THIS_MODULE);
> 
> 
> Do we need to handle the case where this fails?

I cannot see how this _could_ fail, as this function is provided by
the module, so the very existence of the function depends on the
module. But maybe it's better to check, even so.

I'll send an updated patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html