Re: [dm-devel] Make the mq policy an alias for smq

[Mike Snitzer]

On Thu, Feb 11 2016 at  5:41am -0500,
Joe Thornber  wrote:

> On Wed, Feb 10, 2016 at 09:13:05PM -0500, Mike Snitzer wrote:
> > Shouldn't get_policy()'s call to get_policy_once() resolve "mq" to be
> > "smq" if we just add this to the bottom of dm-cache-policy-smq?:
> > 
> > MODULE_ALIAS("dm-cache-mq");
> 
> Yes, you're right.  See my fixup patch.

Hmm, not seeing what you consider to be the latest fixed up version..
(nothing that sets: MODULE_ALIAS("dm-cache-mq") in smq.c)

But I've staged this in linux-next for 4.6, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next=8cbd0cb8cca67baa3e430fb6d636613fdc1cee1a

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] Make the mq policy an alias for smq

[Mike Snitzer]

On Wed, Feb 10 2016 at  5:35am -0500,
Joe Thornber  wrote:

> smq seems to be performing better than the old mq policy in all
> situations, as well as using a quarter of the memory.
> 
> This patch makes 'mq' and alias for 'smq' when choosing a cache
> policy.  The tunables that were present for the old mq are faked, and
> have no effect.  mq should be considered deprecated now.
> ---
>  drivers/md/Kconfig   |   11 +-
>  drivers/md/Makefile  |2 -
>  drivers/md/dm-cache-policy-mq.c  | 1472 
> --
>  drivers/md/dm-cache-policy-smq.c |   93 ++-
>  drivers/md/dm-cache-policy.c |6 +-
>  5 files changed, 97 insertions(+), 1487 deletions(-)
>  delete mode 100644 drivers/md/dm-cache-policy-mq.c
> 
> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
> index b871928..9ea570b 100644
> --- a/drivers/md/Kconfig
> +++ b/drivers/md/Kconfig
> @@ -295,14 +295,11 @@ config DM_CACHE
>   cleaned etc.  It supports writeback and writethrough modes.
>  
>  config DM_CACHE_MQ
> -   tristate "MQ Cache Policy (EXPERIMENTAL)"
> -   depends on DM_CACHE
> -   default y
> +   tristate "MQ Cache Policy (DEPRECATED)"
> +   depends on DM_CACHE_SMQ
> +   default n
> ---help---
> - A cache policy that uses a multiqueue ordered by recent hit
> - count to select which blocks should be promoted and demoted.
> - This is meant to be a general purpose policy.  It prioritises
> - reads over writes.
> + This policy is now an alias for the SMQ policy.
>  
>  config DM_CACHE_SMQ
> tristate "Stochastic MQ Cache Policy (EXPERIMENTAL)"
...
> diff --git a/drivers/md/dm-cache-policy-mq.c b/drivers/md/dm-cache-policy-mq.c
> deleted file mode 100644
> index dbdabdf..000
> --- a/drivers/md/dm-cache-policy-mq.c
> +++ /dev/null



> diff --git a/drivers/md/dm-cache-policy-smq.c 
> b/drivers/md/dm-cache-policy-smq.c
> index 1ffbeb1..8ad6b22 100644
> --- a/drivers/md/dm-cache-policy-smq.c
> +++ b/drivers/md/dm-cache-policy-smq.c
...

> @@ -1716,6 +1781,15 @@ static struct dm_cache_policy_type smq_policy_type = {
>   .create = smq_create
>  };
>  
> +static struct dm_cache_policy_type mq_policy_type = {
> + .name = "mq",
> + .version = {1, 4, 0},
> + .hint_size = 4,
> + .owner = THIS_MODULE,
> + .create = mq_create,
> + .real = _policy_type
> +};
> +
>  static struct dm_cache_policy_type default_policy_type = {
>   .name = "default",
>   .version = {1, 4, 0},
> @@ -1735,9 +1809,17 @@ static int __init smq_init(void)
>   return -ENOMEM;
>   }
>  
> + r = dm_cache_policy_register(_policy_type);
> + if (r) {
> + DMERR("register failed %d", r);
> + dm_cache_policy_unregister(_policy_type);
> + return -ENOMEM;
> + }
> +
>   r = dm_cache_policy_register(_policy_type);
>   if (r) {
>   DMERR("register failed (as default) %d", r);
> + dm_cache_policy_unregister(_policy_type);
>   dm_cache_policy_unregister(_policy_type);
>   return -ENOMEM;
>   }
> @@ -1748,6 +1830,7 @@ static int __init smq_init(void)
>  static void __exit smq_exit(void)
>  {
>   dm_cache_policy_unregister(_policy_type);
> + dm_cache_policy_unregister(_policy_type);
>   dm_cache_policy_unregister(_policy_type);
>  }
>  
> diff --git a/drivers/md/dm-cache-policy.c b/drivers/md/dm-cache-policy.c
> index c1a3cee..829f3af 100644
> --- a/drivers/md/dm-cache-policy.c
> +++ b/drivers/md/dm-cache-policy.c
> @@ -62,7 +62,11 @@ static struct dm_cache_policy_type *get_policy(const char 
> *name)
>   if (t)
>   return t;
>  
> - request_module("dm-cache-%s", name);
> + /* hack; mq is in the smq module */
> + if (!strcmp(name, "mq"))
> + request_module("dm-cache-smq");
> + else
> + request_module("dm-cache-%s", name);
>  
>   t = get_policy_once(name);
>   if (IS_ERR(t))
> -- 
> 2.5.0

I don't think we need this get_policy() hack.  We just want "mq" to be
an alias for "smq" no?  Same as "default".

The new call to dm_cache_policy_register(_policy_type) will add the
mq_policy_type to dm-cache-policy.c:register_list

Shouldn't get_policy()'s call to get_policy_once() resolve "mq" to be
"smq" if we just add this to the bottom of dm-cache-policy-smq?:

MODULE_ALIAS("dm-cache-mq");

I'm also missing why mq_policy_type's .real points to itself via
_policy_type -- shouldn't it be _policy_type (like "default"
does)?

Or is this some subtle compatability so that "mq" is always displayed
in cache status output?

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel