On Fri, 27 Jun 2014 13:11:15 -0400 Dan Streetman <[email protected]> wrote:
> >> >> +struct zpool *zpool_create_pool(char *type, gfp_t flags,
> >> >> + struct zpool_ops *ops)
> >> >> +{
> >> >> + struct zpool_driver *driver;
> >> >> + struct zpool *zpool;
> >> >> +
> >> >> + pr_info("creating pool type %s\n", type);
> >> >> +
> >> >> + spin_lock(&drivers_lock);
> >> >> + driver = zpool_get_driver(type);
> >> >> + spin_unlock(&drivers_lock);
> >> >
> >> > Racy against unregister. Can be solved with a standard get/put
> >> > refcounting implementation. Or perhaps a big fat mutex.
> >
> > Was there a decision here?
>
> What I tried to do, with the final patch in the set, was use module
> usage counting combined with function documentation - in
> zpool_create_pool() the zpool_get_driver() does try_module_get()
> before releasing the spinlock, so if the driver *only* calls
> unregister from its module exit function, I think we should be good -
> once zpool_create_pool() gets the driver module, the driver won't
> enter its exit function and thus won't unregister; and if the driver
> module has started its exit function, try_module_get() will return
> failure and zpool_create_pool() will return failure.
>
> Now, if we remove the restriction that the driver module can only
> unregister from its module exit function, then we would need an
> additional refcount (we could use module_refcount() but the module may
> have refcounts unrelated to us) and unregister would need a return
> value, to indicate failure. I think the problem I had with that is,
> in the driver module's exit function it can't abort if unregister
> fails; but with the module refcounting, unregister shouldn't ever fail
> in the driver's exit function...
>
> So should I remove the unregister function doc asking to only call
> unregister from the module exit function, and add a separate refcount
> to the driver get/put functions? I don't think we need to use a kref,
> since we don't want to free the driver once kref == 0, we want to be
> able to check in the unregister function if there are any refs, so
> just an atomic_t should work. And we would still need to keep the
> module get/put, too, so it would be something like:
I'm not sure I understood all that. But I don't want to understand it
in this context! Readers should be able to gather all this from
looking at the code.
> spin_lock(&drivers_lock);
> ...
> bool got = try_module_get(driver->owner);
> if (got)
> atomic_inc(driver->refs);
> spin_unlock(&drivers_lock);
> return got ? driver : NULL;
>
> with the appropriate atomic_dec in zpool_put_driver(), and unregister
> would change to:
>
> int zpool_unregister_driver(struct zpool_driver *driver)
> {
> spin_lock(&drivers_lock);
> if (atomic_read(driver->refs) > 0) {
> spin_unlock(&drivers_lock);
> return -EBUSY;
> }
> list_del(&driver->list);
> spin_unlock(&drivers_lock);
> return 0;
> }
It sounds like that will work.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
