| Message ID | 154949781342.10620.11880180011933935219.stgit@noble.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lustre: Assorted cleanups for obdclass | expand |
On Feb 6, 2019, at 17:03, NeilBrown <neilb@suse.com> wrote: > > If there are two parallel attempts to register the > same class name, it could get registered twice. > So re-check after allocation to make sure the name is > still unique. The patch itself seems reasonable, but I don't see how this scenario could ever happen? class_register_type() is only called once per device type from the module init code, and I don't think it is possible to insert the same module twice? Cheers, Andreas --- Andreas Dilger Principal Lustre Architect Whamcloud
On Fri, Feb 08 2019, Andreas Dilger wrote: > On Feb 6, 2019, at 17:03, NeilBrown <neilb@suse.com> wrote: >> >> If there are two parallel attempts to register the >> same class name, it could get registered twice. >> So re-check after allocation to make sure the name is >> still unique. > > The patch itself seems reasonable, but I don't see how this > scenario could ever happen? class_register_type() is only > called once per device type from the module init code, and > I don't think it is possible to insert the same module twice? I agree. I don't think it can happen. By the same logic, the test if (class_search_type(name)) { CDEBUG(D_IOCTL, "Type %s already registered\n", name); return -EEXIST; } at the start of class_register_type() is pointless, as it is not possible for the same name to be registered twice. But I think it is still good to have this test, and to fail-safe. To me the test as it is "looks" prone to races, so I wanted to fix it. When I'm analysining obscure bugs, it helps a lot to have code that is "obviously correct" without having the understand (and double check) various assumptions about usage which are necessary for it to be correct. So this patch isn't needed for correctness. It is needed (I think) to make the code understandable and maintainable. Thanks, NeilBrown > > Cheers, Andreas > --- > Andreas Dilger > Principal Lustre Architect > Whamcloud
> If there are two parallel attempts to register the > same class name, it could get registered twice. > So re-check after allocation to make sure the name is > still unique. > > Signed-off-by: NeilBrown <neilb@suse.com> > --- > drivers/staging/lustre/lustre/obdclass/genops.c | 29 +++++++++++++++-------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c > index 382eaf519a79..a174f538dd0d 100644 > --- a/drivers/staging/lustre/lustre/obdclass/genops.c > +++ b/drivers/staging/lustre/lustre/obdclass/genops.c > @@ -86,17 +86,23 @@ static void obd_device_free(struct obd_device *obd) > kmem_cache_free(obd_device_cachep, obd); > } > > -static struct obd_type *class_search_type(const char *name) > +static struct obd_type *__class_search_type(const char *name) > { > struct obd_type *type; > > - spin_lock(&obd_types_lock); > list_for_each_entry(type, &obd_types, typ_chain) { This change is fine but we really don't need typ_chain anymore. We have a list of obd_types already due to the power of kobjects!!!! We can do an static struct obd_type *class_search_type(const char *name) { struct kobject *kobj; struct obd_type *type; kobj = kset_find_obj(lustre_kset, name); return "some_mapping from kobj to type" Hmm. This kobj is not embedded in struct obd_types. Might need more surgery to make that work. > - if (strcmp(type->typ_name, name) == 0) { > - spin_unlock(&obd_types_lock); > + if (strcmp(type->typ_name, name) == 0) > return type; > - } > } > + return NULL; > +} > + > +static struct obd_type *class_search_type(const char *name) > +{ > + struct obd_type *type; > + > + spin_lock(&obd_types_lock); > + type = __class_search_type(name); > spin_unlock(&obd_types_lock); > return NULL; > } > @@ -214,19 +220,22 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops, > if (ldt) { > type->typ_lu = ldt; > rc = lu_device_type_init(ldt); > - if (rc != 0) { > - kobject_put(type->typ_kobj); > + if (rc != 0) > goto failed; > - } > } > > + INIT_LIST_HEAD(&type->typ_chain); > spin_lock(&obd_types_lock); > - list_add(&type->typ_chain, &obd_types); > + if (__class_search_type(name) == NULL) > + list_add(&type->typ_chain, &obd_types); > spin_unlock(&obd_types_lock); > > - return 0; > + if (!list_empty(&type->typ_chain)) > + return 0; > > failed: > + if (!IS_ERR_OR_NULL(type->typ_kobj)) > + kobject_put(type->typ_kobj); > kfree(type->typ_name); > kfree(type->typ_md_ops); > kfree(type->typ_dt_ops); > > >
On Tue, Feb 12 2019, James Simmons wrote: >> If there are two parallel attempts to register the >> same class name, it could get registered twice. >> So re-check after allocation to make sure the name is >> still unique. >> >> Signed-off-by: NeilBrown <neilb@suse.com> >> --- >> drivers/staging/lustre/lustre/obdclass/genops.c | 29 +++++++++++++++-------- >> 1 file changed, 19 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c >> index 382eaf519a79..a174f538dd0d 100644 >> --- a/drivers/staging/lustre/lustre/obdclass/genops.c >> +++ b/drivers/staging/lustre/lustre/obdclass/genops.c >> @@ -86,17 +86,23 @@ static void obd_device_free(struct obd_device *obd) >> kmem_cache_free(obd_device_cachep, obd); >> } >> >> -static struct obd_type *class_search_type(const char *name) >> +static struct obd_type *__class_search_type(const char *name) >> { >> struct obd_type *type; >> >> - spin_lock(&obd_types_lock); >> list_for_each_entry(type, &obd_types, typ_chain) { > > This change is fine but we really don't need typ_chain anymore. > We have a list of obd_types already due to the power of kobjects!!!! > > We can do an > > static struct obd_type *class_search_type(const char *name) > { > struct kobject *kobj; > struct obd_type *type; > > kobj = kset_find_obj(lustre_kset, name); > > return "some_mapping from kobj to type" > > Hmm. This kobj is not embedded in struct obd_types. Might need more > surgery to make that work. > Well.... that was a fun rabbit-hole to fall down. I now have 8 patches where before I had two - definitely a better result. typ_chain and several other fields are gone. I'll post them in my next series which will probably go out tomorrow. Thanks, NeilBrown
diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c index 382eaf519a79..a174f538dd0d 100644 --- a/drivers/staging/lustre/lustre/obdclass/genops.c +++ b/drivers/staging/lustre/lustre/obdclass/genops.c @@ -86,17 +86,23 @@ static void obd_device_free(struct obd_device *obd) kmem_cache_free(obd_device_cachep, obd); } -static struct obd_type *class_search_type(const char *name) +static struct obd_type *__class_search_type(const char *name) { struct obd_type *type; - spin_lock(&obd_types_lock); list_for_each_entry(type, &obd_types, typ_chain) { - if (strcmp(type->typ_name, name) == 0) { - spin_unlock(&obd_types_lock); + if (strcmp(type->typ_name, name) == 0) return type; - } } + return NULL; +} + +static struct obd_type *class_search_type(const char *name) +{ + struct obd_type *type; + + spin_lock(&obd_types_lock); + type = __class_search_type(name); spin_unlock(&obd_types_lock); return NULL; } @@ -214,19 +220,22 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops, if (ldt) { type->typ_lu = ldt; rc = lu_device_type_init(ldt); - if (rc != 0) { - kobject_put(type->typ_kobj); + if (rc != 0) goto failed; - } } + INIT_LIST_HEAD(&type->typ_chain); spin_lock(&obd_types_lock); - list_add(&type->typ_chain, &obd_types); + if (__class_search_type(name) == NULL) + list_add(&type->typ_chain, &obd_types); spin_unlock(&obd_types_lock); - return 0; + if (!list_empty(&type->typ_chain)) + return 0; failed: + if (!IS_ERR_OR_NULL(type->typ_kobj)) + kobject_put(type->typ_kobj); kfree(type->typ_name); kfree(type->typ_md_ops); kfree(type->typ_dt_ops);
If there are two parallel attempts to register the same class name, it could get registered twice. So re-check after allocation to make sure the name is still unique. Signed-off-by: NeilBrown <neilb@suse.com> --- drivers/staging/lustre/lustre/obdclass/genops.c | 29 +++++++++++++++-------- 1 file changed, 19 insertions(+), 10 deletions(-)