Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754839AbbHEWAs (ORCPT ); Wed, 5 Aug 2015 18:00:48 -0400 Received: from mail-ig0-f174.google.com ([209.85.213.174]:36107 "EHLO mail-ig0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754440AbbHEWAq (ORCPT ); Wed, 5 Aug 2015 18:00:46 -0400 MIME-Version: 1.0 In-Reply-To: <20150805130836.16c42cd0a9fe6f4050cf0620@linux-foundation.org> References: <1438782403-29496-1-git-send-email-ddstreet@ieee.org> <1438782403-29496-2-git-send-email-ddstreet@ieee.org> <20150805130836.16c42cd0a9fe6f4050cf0620@linux-foundation.org> From: Dan Streetman Date: Wed, 5 Aug 2015 18:00:26 -0400 X-Google-Sender-Auth: dM22YT-7yBj01e9P4oLLsMwtTT8 Message-ID: Subject: Re: [PATCH 1/3] zpool: add zpool_has_pool() To: Andrew Morton Cc: Seth Jennings , Linux-MM , linux-kernel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2616 Lines: 69 On Wed, Aug 5, 2015 at 4:08 PM, Andrew Morton wrote: > On Wed, 5 Aug 2015 09:46:41 -0400 Dan Streetman 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/