Re: [PATCH 27/36] scsi_dh_alua: Use workqueue for RTPG

[Hannes Reinecke <hare@suse.de>]

On 10/02/2015 01:34 AM, Bart Van Assche wrote:
> On 09/29/2015 03:48 AM, Hannes Reinecke wrote:
>> +static void alua_rtpg_work(struct work_struct *work)
>> +{
>> +    struct alua_port_group *pg =
>> +        container_of(work, struct alua_port_group, rtpg_work.work);
>> +    struct scsi_device *sdev;
>> +    LIST_HEAD(qdata_list);
>> +    int err = SCSI_DH_OK;
>> +    struct alua_queue_data *qdata, *tmp;
>> +    unsigned long flags;
>> +
>> +    spin_lock_irqsave(&pg->lock, flags);
>> +    sdev = pg->rtpg_sdev;
>> +    if (!sdev) {
>> +        WARN_ON(pg->flags & ALUA_PG_RUN_RTPG ||
>> +            pg->flags & ALUA_PG_RUN_STPG);
>> +        spin_unlock_irqrestore(&pg->lock, flags);
>> +        return;
>> +    }
>> +    pg->flags |= ALUA_PG_RUNNING;
>> +    if (pg->flags & ALUA_PG_RUN_RTPG) {
>> +        spin_unlock_irqrestore(&pg->lock, flags);
>> +        err = alua_rtpg(sdev, pg);
>> +        spin_lock_irqsave(&pg->lock, flags);
>> +        if (err == SCSI_DH_RETRY) {
>> +            pg->flags &= ~ALUA_PG_RUNNING;
>> +            spin_unlock_irqrestore(&pg->lock, flags);
>> +            queue_delayed_work(kaluad_wq, &pg->rtpg_work,
>> +                       pg->interval * HZ);
>> +            return;
>> +        }
>> +        pg->flags &= ~ALUA_PG_RUN_RTPG;
>> +        if (err != SCSI_DH_OK)
>> +            pg->flags &= ~ALUA_PG_RUN_STPG;
>> +    }
>> +    if (pg->flags & ALUA_PG_RUN_STPG) {
>> +        spin_unlock_irqrestore(&pg->lock, flags);
>> +        err = alua_stpg(sdev, pg);
>> +        spin_lock_irqsave(&pg->lock, flags);
>> +        pg->flags &= ~ALUA_PG_RUN_STPG;
>> +        if (err == SCSI_DH_RETRY) {
>> +            pg->flags |= ALUA_PG_RUN_RTPG;
>> +            pg->interval = 0;
>> +            pg->flags &= ~ALUA_PG_RUNNING;
>> +            spin_unlock_irqrestore(&pg->lock, flags);
>> +            queue_delayed_work(kaluad_wq, &pg->rtpg_work,
>> +                       pg->interval * HZ);
>> +            return;
>> +        }
>> +    }
>> +
>> +    list_splice_init(&pg->rtpg_list, &qdata_list);
>> +    pg->rtpg_sdev = NULL;
>> +    spin_unlock_irqrestore(&pg->lock, flags);
>> +
>> +    list_for_each_entry_safe(qdata, tmp, &qdata_list, entry) {
>> +        list_del(&qdata->entry);
>> +        if (qdata->callback_fn)
>> +            qdata->callback_fn(qdata->callback_data, err);
>> +        kfree(qdata);
>> +    }
>> +    spin_lock_irqsave(&pg->lock, flags);
>> +    pg->flags &= ~ALUA_PG_RUNNING;
>> +    spin_unlock_irqrestore(&pg->lock, flags);
>> +    scsi_device_put(sdev);
>> +    kref_put(&pg->kref, release_port_group);
>> +}
> 
> With this patch series applied kmemleak reports several leaks that
> were not reported without this patch. Is scsi_device_put() +
> kref_put() always called before this function returns without
> requeueing the work item ? Shouldn't the return value of
> queue_delayed_work() be checked ? The leaks reported by kmemleak are:
> 
Yes, you are right. I need to check queue_delayed_work() and issue
a scsi_device_put()/kref_put() if the item is already queued.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (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