Re: [PATCH 1/3] zpool: add zpool_has_pool()

[Dan Streetman]

On Wed, Aug 5, 2015 at 4:08 PM, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed,  5 Aug 2015 09:46:41 -0400 Dan Streetman <ddstreet@xxxxxxxx> wrote:
>
>> Add zpool_has_pool() function, indicating if the specified type of zpool
>> is available (i.e. zsmalloc or zbud).  This allows checking if a pool is
>> available, without actually trying to allocate it, similar to
>> crypto_has_alg().
>>
>> This is used by a following patch to zswap that enables the dynamic
>> runtime creation of zswap zpools.
>>
>> ...
>>
>>  /**
>> + * zpool_has_pool() - Check if the pool driver is available
>> + * @type     The type of the zpool to check (e.g. zbud, zsmalloc)
>> + *
>> + * This checks if the @type pool driver is available.
>> + *
>> + * Returns: true if @type pool is available, false if not
>> + */
>> +bool zpool_has_pool(char *type)
>> +{
>> +     struct zpool_driver *driver = zpool_get_driver(type);
>> +
>> +     if (!driver) {
>> +             request_module("zpool-%s", type);
>> +             driver = zpool_get_driver(type);
>> +     }
>> +
>> +     if (!driver)
>> +             return false;
>> +
>> +     zpool_put_driver(driver);
>> +     return true;
>> +}
>
> This looks racy: after that zpool_put_driver() has completed, an rmmod
> will invalidate zpool_has_pool()'s return value.

the true/false return value is only a snapshot of that moment in time;
zswap's use of this is only to validate that the user-provided zpool
name is valid; if this fails, zswap will just return failure to the
user (or if this happens at init-time, falls back to LZO).  If this
succeeds, zswap still must use zpool_create_pool() which will fail if
the requested module can't be loaded.

essentially zswap does:

if (!zpool_has_pool(zpool_type) || !crypto_has_comp(compressor_type))
  return -EINVAL;

that allows it to check that the requested zpool and compressor types
are valid, before actually creating anything.  The creation of the
zpool and compressor do have error handling if either of them fail.

>
> If there's some reason why this can't happen, can we please have a code
> comment which reveals that reason?

zpool_create_pool() should work if this returns true, unless as you
say the module is rmmod'ed *and* removed from the system - since
zpool_create_pool() will call request_module() just as this function
does.  I can add a comment explaining that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/