On Thu, Dec 9, 2010 at 6:06 AM, Joe Eykholt <jeykholt@xxxxxxxxx> wrote:
>
>
> On 12/8/10 5:27 AM, Hillf Danton wrote:
>> Stats info is added, in the define of local port, to monitor the usage
>> of remote ports.
>>
>> Then it is simple to answer how many rport data leaked in the life
>> cycle of lport.
>>
>> This work is motivated to capture mm leak of rports of
>> FC_FID_DIR_SERV, and thanks for dropping a reply to this message if
>> captured.
>
> This sounds like code that could be done locally and then
> tossed out once the leak is found. ÂAlso, there is already a list
> of remote ports, I think, so the count is redundant info.
It is fine to read your comments, Joe, in winter evening.
Though the list is defined, rport of FC_FID_DIR_SERV is not added
onto it, at least when created, which is hard to trace then.
And FC_FID_DIR_SERV is the major concern of this work.
If captured, fix should have been delivered.
>
>> Signed-off-by: Hillf Danton <dhillf@xxxxxxxxx>
>> ---
>>
>> --- a/include/scsi/libfc.h  Â2010-11-01 19:54:12.000000000 +0800
>> +++ b/include/scsi/libfc.h  Â2010-12-08 21:05:46.000000000 +0800
>> @@ -803,6 +803,9 @@ struct fc_lport {
>>    void              *scsi_priv;
>>    struct fc_disc         disc;
>>
>> + Â Â /* stats info of the usage of rports */
>> +   unsigned long          nr_rdata;
>> +
>> Â Â Â /* Virtual port information */
>>    struct list_head        vports;
>>    struct fc_vport        Â*vport;
>> --- a/drivers/scsi/libfc/fc_rport.c  2010-11-01 19:54:12.000000000 +0800
>> +++ b/drivers/scsi/libfc/fc_rport.c  2010-12-08 21:16:40.000000000 +0800
>> @@ -128,7 +128,7 @@ static struct fc_rport_priv *fc_rport_cr
>> Â Â Â rdata = kzalloc(sizeof(*rdata) + lport->rport_priv_size, GFP_KERNEL);
>> Â Â Â if (!rdata)
>> Â Â Â Â Â Â Â return NULL;
>> -
>> + Â Â lport->nr_rdata++;
>> Â Â Â rdata->ids.node_name = -1;
>> Â Â Â rdata->ids.port_name = -1;
>> Â Â Â rdata->ids.port_id = port_id;
>> @@ -159,6 +159,7 @@ static void fc_rport_free_rcu(struct rcu
>> Â Â Â struct fc_rport_priv *rdata;
>>
>> Â Â Â rdata = container_of(rcu, struct fc_rport_priv, rcu);
>> + Â Â rdata->local_port->nr_rdata--;
>
> What about locking? ÂI don't think you should do this in the rcu
Locking here looks unnecessary, since lport could not goto Mars.
If could, lot of orphans have to stay on Earth with no more care.
> callback because the local port may no longer exist. Instead, I
> would do it in fc_rport_destroy.
>
>> Â Â Â kfree(rdata);
>> Â}
>>
>> @@ -1880,7 +1881,7 @@ int fc_rport_init(struct fc_lport *lport
>>
>> Â Â Â if (!lport->tt.rport_destroy)
>> Â Â Â Â Â Â Â lport->tt.rport_destroy = fc_rport_destroy;
>> -
>> + Â Â lport->nr_rdata = 0;
>
> Everything in the lport starts as zero, so no need to insert this explicitly.
When initing lport, clarity could be redundant.
>
>> Â Â Â return 0;
>> Â}
>> ÂEXPORT_SYMBOL(fc_rport_init);
>> --- a/drivers/scsi/libfc/fc_fcp.c   2010-11-01 19:54:12.000000000 +0800
>> +++ b/drivers/scsi/libfc/fc_fcp.c   2010-12-08 21:19:20.000000000 +0800
>> @@ -2184,6 +2184,11 @@ void fc_fcp_destroy(struct fc_lport *lpo
>> Â{
>> Â Â Â struct fc_fcp_internal *si = fc_get_scsi_internal(lport);
>>
>> + Â Â if (lport->nr_rdata != 0)
>> + Â Â Â Â Â Â printk(KERN_WARNING "libfc: %lu Leaked rports when "
>> + Â Â Â Â Â Â Â Â Â Â "destroying local port (%6.6x)\n",
>> + Â Â Â Â Â Â Â Â Â Â lport->nr_rdata, lport->port_id);
>> +
>
> The equivalent of this check should go into fc_rport.c, not here.
When destroying lport, it is the right place to report leakage.
Cheers
Hillf
>
>> Â Â Â if (!list_empty(&si->scsi_pkt_queue))
>> Â Â Â Â Â Â Â printk(KERN_ERR "libfc: Leaked SCSI packets when destroying "
>> Â Â Â Â Â Â Â Â Â Â Â"port (%6.6x)\n", lport->port_id);
>> _______________________________________________
>> devel mailing list
>> devel@xxxxxxxxxxxxx
>> http://www.open-fcoe.org/mailman/listinfo/devel
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
