On 12/30/2015 02:19 PM, Christoph Hellwig wrote:
This looks good in general, but a couple nitpicks below:
+static uint optimize_stpg;
+module_param(optimize_stpg, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(optimize_stpg, "Allow use of a non-optimized path, rather than sending a STPG, when implicit TPGS is supported (0=No,1=Yes). Default is 0.");
why is this moved around in this patch? It doesn't seem related to the
rest of it, and isn't documented in the changelog either.
Yeah, can move it to a separate patch.
{
- struct alua_port_group *pg = NULL;
+ struct alua_port_group *pg;
looks like this should be folded into the patch that introduces the
unessecary NULL assignment.
Ok.
list_for_each_entry(pg, &port_group_list, node) {
if (pg->group_id != group_id)
@@ -214,18 +240,26 @@ struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
pg->group_id = group_id;
pg->tpgs = tpgs;
pg->state = TPGS_STATE_OPTIMIZED;
+ if (optimize_stpg)
+ pg->flags |= ALUA_OPTIMIZE_STPG;
why is this moved earlier here? Doing it from the beginning seems
useful to me, but I'd expect it in a separate patch with a proper
description.
Ok.
kref_init(&pg->kref);
+ INIT_DELAYED_WORK(&pg->rtpg_work, alua_rtpg_work);
+ INIT_LIST_HEAD(&pg->rtpg_list);
+ INIT_LIST_HEAD(&pg->node);
+ spin_lock_init(&pg->lock);
/* Re-check list again to catch concurrent updates */
spin_lock(&port_group_lock);
tmp_pg = alua_lookup_pg(id_str, id_size, group_id);
if (tmp_pg) {
spin_unlock(&port_group_lock);
- kfree(pg);
- return tmp_pg;
+ kref_put(&pg->kref, release_port_group);
+ pg = tmp_pg;
+ tmp_pg = NULL;
The only thing release_port_group does in addition to the kfree
is a list_del on pg->entry. But given that we never added
the pg to a list it doesn't need to be deleted, and it can't
have another reference. Why this change?
Mainly for symmetry; with this patch we're always calling kref_put() to
delete the port_group structure.
While we're at it, there are 6 calls to
'kref_put(&pg->kref, release_port_group)' in addition to this one,
so it might make sense to add a helper for it to the patch introducing
struct alua_port_group.
Ok.
* Extract the relative target port and the target port group
* descriptor from the list of identificators.
*
- * Returns 0 or SCSI_DH_ error code on failure.
+ * Returns the target port group id or -1 on failure
That's not how I interpret the code below, it seems to always return
SCSI_DH_* values.
+ struct alua_port_group *pg = NULL, *old_pg = NULL;
+ bool pg_found = false;
+ pg = alua_get_pg(sdev, group_id, tpgs, id_str, id_size);
+ if (!pg)
return SCSI_DH_NOMEM;
+ /* Check for existing port_group references */
+ spin_lock(&h->pg_lock);
+ if (h->pg) {
+ old_pg = pg;
+ /* port_group has changed. Update to new port group */
+ if (h->pg != pg) {
+ old_pg = h->pg;
+ rcu_assign_pointer(h->pg, pg);
+ h->pg->expiry = 0;
why do we set expiry to 0 here, but not if we didn't have a pg
yet? This could use a comment not just here but in all the places
that set it to zero.
+ pg_found = true;
pg_found should be pg_updated, right?
+ if (pg_found)
+ synchronize_rcu();
+ if (old_pg) {
+ if (old_pg->rtpg_sdev)
+ flush_delayed_work(&old_pg->rtpg_work);
+ kref_put(&old_pg->kref, release_port_group);
+ }
This code looks odd. I can't see why we need a synchronize_rcu here.
The only thing we should need is a kfree_rcu for the final free in
release_port_group. I also don't quite understand the flush_delayed_work.
As far as I can tell we only need it if rtpg_sdev is the sdev passed
in, so it we probably should check for that.
rtpg_sdev is set whenever the port_group structure needs or is scheduled
for alua_rtpg_work(), ie whenever some action needs to be taken.
As the sdev might be a different sdev than that one we've been called
from (a port_group might have several sdevs pointing to it), the
existence of an sdev signals that we need to flush the workqueue item.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@xxxxxxx +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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
