Re: [PATCHv4 3/6] mm/zpool: implement common zpool api to zbud/zsmalloc

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 27 Jun 2014 13:11:15 -0400 Dan Streetman <ddstreet@ieee.org> 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.