Hi Hannes,
Could you take a look at those?
At least the first one causes noticeable memory decline over time
for some of our deployments.
Igor Druzhinin (2):
scsi: libfc: free response frame from GPN_ID
scsi: libfc: drop extra rport reference in fc_rport_create()
drivers/scsi/libfc/fc_disc.c | 2 ++
drivers/scsi/libfc/fc_rport.c | 4 +++-
2 files changed, 5 insertions(+), 1 deletion(-)
--
2.7.4
fc_disc_gpn_id_resp() should be the last function using it so free it
here to avoid memory leak.
Signed-off-by: Igor Druzhinin <[email protected]>
---
drivers/scsi/libfc/fc_disc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/scsi/libfc/fc_disc.c b/drivers/scsi/libfc/fc_disc.c
index 9c5f7c9..2b865c6 100644
--- a/drivers/scsi/libfc/fc_disc.c
+++ b/drivers/scsi/libfc/fc_disc.c
@@ -628,6 +628,8 @@ static void fc_disc_gpn_id_resp(struct fc_seq *sp, struct fc_frame *fp,
}
out:
kref_put(&rdata->kref, fc_rport_destroy);
+ if (!IS_ERR(fp))
+ fc_frame_free(fp);
}
/**
--
2.7.4
The callers of this function seem to assume the reference is not taken
in case rport already exists. This results in one extra reference taken
on each rport re-discovery that will eventually get to inability to
free rport structure on port removal.
Signed-off-by: Igor Druzhinin <[email protected]>
---
drivers/scsi/libfc/fc_rport.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index da6e97d..a43f9dd 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -133,8 +133,10 @@ struct fc_rport_priv *fc_rport_create(struct fc_lport *lport, u32 port_id)
lockdep_assert_held(&lport->disc.disc_mutex);
rdata = fc_rport_lookup(lport, port_id);
- if (rdata)
+ if (rdata) {
+ kref_put(&rdata->kref, fc_rport_destroy);
return rdata;
+ }
if (lport->rport_priv_size > 0)
rport_priv_size = lport->rport_priv_size;
--
2.7.4
Hannes,
> Could you take a look at those?
> At least the first one causes noticeable memory decline over time
> for some of our deployments.
>
> Igor Druzhinin (2):
> scsi: libfc: free response frame from GPN_ID
> scsi: libfc: drop extra rport reference in fc_rport_create()
>
> drivers/scsi/libfc/fc_disc.c | 2 ++
> drivers/scsi/libfc/fc_rport.c | 4 +++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
Please review!
--
Martin K. Petersen Oracle Linux Engineering
On 1/14/20 3:43 PM, Igor Druzhinin wrote:
> fc_disc_gpn_id_resp() should be the last function using it so free it
> here to avoid memory leak.
>
> Signed-off-by: Igor Druzhinin <[email protected]>
> ---
> drivers/scsi/libfc/fc_disc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/scsi/libfc/fc_disc.c b/drivers/scsi/libfc/fc_disc.c
> index 9c5f7c9..2b865c6 100644
> --- a/drivers/scsi/libfc/fc_disc.c
> +++ b/drivers/scsi/libfc/fc_disc.c
> @@ -628,6 +628,8 @@ static void fc_disc_gpn_id_resp(struct fc_seq *sp, struct fc_frame *fp,
> }
> out:
> kref_put(&rdata->kref, fc_rport_destroy);
> + if (!IS_ERR(fp))
> + fc_frame_free(fp);
> }
>
> /**
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
On 1/14/20 3:43 PM, Igor Druzhinin wrote:
> The callers of this function seem to assume the reference is not taken
> in case rport already exists. This results in one extra reference taken
> on each rport re-discovery that will eventually get to inability to
> free rport structure on port removal.
>
> Signed-off-by: Igor Druzhinin <[email protected]>
> ---
> drivers/scsi/libfc/fc_rport.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
> index da6e97d..a43f9dd 100644
> --- a/drivers/scsi/libfc/fc_rport.c
> +++ b/drivers/scsi/libfc/fc_rport.c
> @@ -133,8 +133,10 @@ struct fc_rport_priv *fc_rport_create(struct fc_lport *lport, u32 port_id)
> lockdep_assert_held(&lport->disc.disc_mutex);
>
> rdata = fc_rport_lookup(lport, port_id);
> - if (rdata)
> + if (rdata) {
> + kref_put(&rdata->kref, fc_rport_destroy);
> return rdata;
> + }
>
> if (lport->rport_priv_size > 0)
> rport_priv_size = lport->rport_priv_size;
>
NAK.
The caller _does_ assume that a reference is taken once
fc_rport_create() returns non-NULL.
And the caller is responsible to drop the reference once 'rdatat' isn't
used anymore.
Any other usage is an error, but should be fixed in the caller, not here.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
Igor,
> fc_disc_gpn_id_resp() should be the last function using it so free it
> here to avoid memory leak.
Applied to 5.6/scsi-fixes, thanks!
--
Martin K. Petersen Oracle Linux Engineering