Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1200769 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/3] zpool: add zpool_has_pool() Dan Streetman <ddstreet@ieee.org> - 2015-08-05 15:50 +0200
    Re: [PATCH 1/3] zpool: add zpool_has_pool() Andrew Morton <akpm@linux-foundation.org> - 2015-08-05 22:10 +0200
      Re: [PATCH 1/3] zpool: add zpool_has_pool() Andrew Morton <akpm@linux-foundation.org> - 2015-08-06 00:10 +0200
        Re: [PATCH 1/3] zpool: add zpool_has_pool() Seth Jennings <sjennings@variantweb.net> - 2015-08-07 00:00 +0200
          Re: [PATCH 1/3] zpool: add zpool_has_pool() Seth Jennings <sjennings@variantweb.net> - 2015-08-07 05:40 +0200
        Re: [PATCH 1/3] zpool: add zpool_has_pool() Dan Streetman <ddstreet@ieee.org> - 2015-08-14 22:10 +0200
      Re: [PATCH 1/3] zpool: add zpool_has_pool() Dan Streetman <ddstreet@ieee.org> - 2015-08-06 00:10 +0200
        [PATCH] zpool: clarification comment for zpool_has_pool Dan Streetman <ddstreet@ieee.org> - 2015-08-06 20:00 +0200

#1200769 — [PATCH 1/3] zpool: add zpool_has_pool()

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.

Signed-off-by: Dan Streetman <ddstreet@ieee.org>
---
 include/linux/zpool.h |  2 ++
 mm/zpool.c            | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/zpool.h b/include/linux/zpool.h
index c924a28..42f8ec9 100644
--- a/include/linux/zpool.h
+++ b/include/linux/zpool.h
@@ -36,6 +36,8 @@ enum zpool_mapmode {
 	ZPOOL_MM_DEFAULT = ZPOOL_MM_RW
 };
 
+bool zpool_has_pool(char *type);
+
 struct zpool *zpool_create_pool(char *type, char *name,
 			gfp_t gfp, const struct zpool_ops *ops);
 
diff --git a/mm/zpool.c b/mm/zpool.c
index 951db32..aafcf8f 100644
--- a/mm/zpool.c
+++ b/mm/zpool.c
@@ -100,6 +100,31 @@ static void zpool_put_driver(struct zpool_driver *driver)
 }
 
 /**
+ * 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;
+}
+EXPORT_SYMBOL(zpool_has_pool);
+
+/**
  * zpool_create_pool() - Create a new zpool
  * @type	The type of the zpool to create (e.g. zbud, zsmalloc)
  * @name	The name of the zpool (e.g. zram0, zswap)
-- 
2.1.0

--
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/

[toc] | [next] | [standalone]

#1201087

On Wed,  5 Aug 2015 09:46:41 -0400 Dan Streetman <ddstreet@ieee.org> 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.

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

--
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/

[toc] | [prev] | [next] | [standalone]

#1201221

On Wed, 5 Aug 2015 18:00:26 -0400 Dan Streetman <ddstreet@ieee.org> wrote:

> >
> > 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.

I like comments ;)

Seth, I'm planning on sitting on these patches until you've had a
chance to review them.

--
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/

[toc] | [prev] | [next] | [standalone]

#1202102

On Wed, Aug 05, 2015 at 03:06:59PM -0700, Andrew Morton wrote:
> On Wed, 5 Aug 2015 18:00:26 -0400 Dan Streetman <ddstreet@ieee.org> wrote:
> 
> > >
> > > 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.
> 
> I like comments ;)
> 
> Seth, I'm planning on sitting on these patches until you've had a
> chance to review them.

Thanks Andrew.  I'm reviewing now.  Patch 2/3 is pretty huge.  I've got
the gist of the changes now.  I'm also building and testing for myself
as this creates a lot more surface area for issues, alternating between
compressors and allocating new compression transforms on the fly.

I'm kinda with Sergey on this in that it adds yet another complexity to
an already complex feature.  This adds more locking, more RCU, more
refcounting.  It's becoming harder to review, test, and verify.

I should have results tomorrow.

Thanks,
Seth
--
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/

[toc] | [prev] | [next] | [standalone]

#1202270

On Thu, Aug 06, 2015 at 04:50:23PM -0500, Seth Jennings wrote:
> On Wed, Aug 05, 2015 at 03:06:59PM -0700, Andrew Morton wrote:
> > On Wed, 5 Aug 2015 18:00:26 -0400 Dan Streetman <ddstreet@ieee.org> wrote:
> > 
> > > >
> > > > 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.
> > 
> > I like comments ;)
> > 
> > Seth, I'm planning on sitting on these patches until you've had a
> > chance to review them.
> 
> Thanks Andrew.  I'm reviewing now.  Patch 2/3 is pretty huge.  I've got
> the gist of the changes now.  I'm also building and testing for myself
> as this creates a lot more surface area for issues, alternating between
> compressors and allocating new compression transforms on the fly.
> 
> I'm kinda with Sergey on this in that it adds yet another complexity to
> an already complex feature.  This adds more locking, more RCU, more
> refcounting.  It's becoming harder to review, test, and verify.
> 
> I should have results tomorrow.

So I gave it a test run turning all the knobs (compressor, enabled,
max_pool_percent, and zpool) like a crazy person and it was stable,
and all the adjustments had the expected result.

Dan, you might follow up with an update to Documentation/vm/zswap.txt
noting that these parameters are runtime adjustable now.

The growing complexity is a concern, but it is nice to have the
flexibility.  Thanks for the good work!

To patchset:

Acked-by: Seth Jennings <sjennings@variantweb.net>

> 
> Thanks,
> Seth
--
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/

[toc] | [prev] | [next] | [standalone]

#1207889

>>On Wed, Aug 5, 2015 at 6:06 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>>> On Wed, 5 Aug 2015 18:00:26 -0400 Dan Streetman <ddstreet@ieee.org> wrote:
>>>
>>>> >
>>>> > 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.
>>>
>>> I like comments ;)
>>>
>>> Seth, I'm planning on sitting on these patches until you've had a
>>> chance to review them.
>>>
>>>
>> Thanks Andrew.  I'm reviewing now.  Patch 2/3 is pretty huge.  I've got
>> the gist of the changes now.  I'm also building and testing for myself
>> as this creates a lot more surface area for issues, alternating between
>> compressors and allocating new compression transforms on the fly.
>>
>> I'm kinda with Sergey on this in that it adds yet another complexity to
>> an already complex feature.  This adds more locking, more RCU, more
>> refcounting.  It's becoming harder to review, test, and verify.
>>
>> I should have results tomorrow.
>
>So I gave it a test run turning all the knobs (compressor, enabled,
>max_pool_percent, and zpool) like a crazy person and it was stable,
>and all the adjustments had the expected result.
>
>Dan, you might follow up with an update to Documentation/vm/zswap.txt
>noting that these parameters are runtime adjustable now.
>
>The growing complexity is a concern, but it is nice to have the
>flexibility.  Thanks for the good work!
>
>To patchset:
>
>Acked-by: Seth Jennings <sjennings@variantweb.net>
>

Hi Seth!

FYI, for whatever reason I'm still not directly getting your emails :(
 I use gmail, if that helps...I don't know if there's a problem on
your end or mine...at least this time I knew to check the list archive
;-)

Thanks for reviewing!  I'll send a patch to update zswap.txt also.

Andrew, would you prefer an additional patch to update zswap.txt, or
should I roll up that patch and the other few correction patches and
resend this patch set?
--
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/

[toc] | [prev] | [next] | [standalone]

#1201231

On Wed, Aug 5, 2015 at 4:08 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed,  5 Aug 2015 09:46:41 -0400 Dan Streetman <ddstreet@ieee.org> 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/

[toc] | [prev] | [next] | [standalone]

#1201950 — [PATCH] zpool: clarification comment for zpool_has_pool

Add clarification in the documentation comment for zpool_has_pool() to
explain the caller should assume the requested driver is or is not
available, depending on return value.  If true is returned, the caller
should assume zpool_create_pool() will succeed, but still must be
prepared to handle failure.

Signed-off-by: Dan Streetman <ddstreet@ieee.org>
---
 mm/zpool.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/mm/zpool.c b/mm/zpool.c
index aafcf8f..d8cf7cd 100644
--- a/mm/zpool.c
+++ b/mm/zpool.c
@@ -103,7 +103,15 @@ static void zpool_put_driver(struct zpool_driver *driver)
  * 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.
+ * This checks if the @type pool driver is available.  This will try to load
+ * the requested module, if needed, but there is no guarantee the module will
+ * still be loaded and available immediately after calling.  If this returns
+ * true, the caller should assume the pool is available, but must be prepared
+ * to handle the @zpool_create_pool() returning failure.  However if this
+ * returns false, the caller should assume the requested pool type is not
+ * available; either the requested pool type module does not exist, or could
+ * not be loaded, and calling @zpool_create_pool() with the pool type will
+ * fail.
  *
  * Returns: true if @type pool is available, false if not
  */
-- 
2.1.0

--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web