[PATCH] dm cache: require io_mode cache feature selection to be mutually exclusive

[PATCH] dm cache: require io_mode cache feature selection to be mutually exclusive

[John Pittman]

Due to commit 629d0a8a1a10 ("dm cache metadata: add "metadata2" feature"),
when creating a dm cache device, more than one io_mode can be selected.
As the io_mode selections are incompatible with one another, we should 
force them to be selected exclusively.  Add simple ctr to check for 
more than one io_mode selection.

Signed-off-by: John Pittman <jpittman@redhat.com>
---
 drivers/md/dm-cache-target.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
index ce14a3d..45e3bed 100644
--- a/drivers/md/dm-cache-target.c
+++ b/drivers/md/dm-cache-target.c
@@ -2250,7 +2250,7 @@ static int parse_features(struct cache_args *ca, struct dm_arg_set *as,
 		{0, 2, "Invalid number of cache feature arguments"},
 	};
 
-	int r;
+	int r, mode_ctr = 0;
 	unsigned argc;
 	const char *arg;
 	struct cache_features *cf = &ca->features;
@@ -2264,14 +2264,20 @@ static int parse_features(struct cache_args *ca, struct dm_arg_set *as,
 	while (argc--) {
 		arg = dm_shift_arg(as);
 
-		if (!strcasecmp(arg, "writeback"))
+		if (!strcasecmp(arg, "writeback")) {
 			cf->io_mode = CM_IO_WRITEBACK;
+			mode_ctr++;
+		}
 
-		else if (!strcasecmp(arg, "writethrough"))
+		else if (!strcasecmp(arg, "writethrough")) {
 			cf->io_mode = CM_IO_WRITETHROUGH;
+			mode_ctr++;
+		}
 
-		else if (!strcasecmp(arg, "passthrough"))
+		else if (!strcasecmp(arg, "passthrough")) {
 			cf->io_mode = CM_IO_PASSTHROUGH;
+			mode_ctr++;
+		}
 
 		else if (!strcasecmp(arg, "metadata2"))
 			cf->metadata_version = 2;
@@ -2282,6 +2288,11 @@ static int parse_features(struct cache_args *ca, struct dm_arg_set *as,
 		}
 	}
 
+	if (mode_ctr > 1) {
+		*error = "Invalid number of cache io_mode features requested";
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
2.7.5

Re: dm cache: require io_mode cache feature selection to be mutually exclusive

[Mike Snitzer]

On Thu, Jun 21 2018 at  5:35pm -0400,
John Pittman <jpittman@redhat.com> wrote:

> Due to commit 629d0a8a1a10 ("dm cache metadata: add "metadata2" feature"),
> when creating a dm cache device, more than one io_mode can be selected.
> As the io_mode selections are incompatible with one another, we should 
> force them to be selected exclusively.  Add simple ctr to check for 
> more than one io_mode selection.

I'm missing why you're associating your fix with commit 629d0a8a1a10.

Can you explain what you did to hit a problem that this patch is fixing?
Did you list conflicting modes on the ctr and the last one in the table
line won?

Thanks,
Mike

Re: dm cache: require io_mode cache feature selection to be mutually exclusive

[John Pittman]

Hi Mike, thanks for replying and sorry for the lack of clarity on that
point.  In commit 629d0a8a1a10 the metadata2 feature was added. In
that same commit, to accommodate the new feature, the max feature args
was incremented to 2.  However, as it's written now, there is nothing
to keep those 2 args from both being io_mode feature args.  Prior to
that, the max feature args was only 1 and the user would just be
forced to pick from writeback, writethrough, or passthrough; the
io_mode args.

 static int parse_features(struct cache_args *ca, struct dm_arg_set *as,
                          char **error)
 {
        static struct dm_arg _args[] = {
-               {0, 1, "Invalid number of cache feature arguments"},
+               {0, 2, "Invalid number of cache feature arguments"},

Explaining how I hit the issue, in support we have scripts that
automatically pull information from the crash environment.  While
developing one of these scripts, I was duplicating the  raid_status
code and (iirc) I had to set a max feature arg myself.  I realized
that just setting to 2 would not work because specifying more than one
io_mode was possible.

When specified, they are all printed with the table. I assumed the
first one was applied, but I'm not sure and possibly/probably
incorrect.

I did open a medium sev RH bz1579002, if you'd like to see the output
from my system at the time.  No customer case associated, just thought
this may be a small oversight that could be fixed with a simple
counter.

On Wed, Jun 27, 2018 at 11:27 AM, Mike Snitzer <snitzer@redhat.com> wrote:
> On Thu, Jun 21 2018 at  5:35pm -0400,
> John Pittman <jpittman@redhat.com> wrote:
>
>> Due to commit 629d0a8a1a10 ("dm cache metadata: add "metadata2" feature"),
>> when creating a dm cache device, more than one io_mode can be selected.
>> As the io_mode selections are incompatible with one another, we should
>> force them to be selected exclusively.  Add simple ctr to check for
>> more than one io_mode selection.
>
> I'm missing why you're associating your fix with commit 629d0a8a1a10.
>
> Can you explain what you did to hit a problem that this patch is fixing?
> Did you list conflicting modes on the ctr and the last one in the table
> line won?
>
> Thanks,
> Mike