[PATCH 1/6] dm-delay: Use DM_MAPIO_XXX macros instead of 0/1 (resend)

[PATCH 1/6] dm-delay: Use DM_MAPIO_XXX macros instead of 0/1 (resend)

[Tomohiro Kusumi]

.map function of dm-delay returns return value of delay_bio(),
hence it's supposed to return either of DM_MAPIO_XXX macros.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 drivers/md/dm-delay.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-delay.c b/drivers/md/dm-delay.c
index b34f6e2..e10f69a 100644
--- a/drivers/md/dm-delay.c
+++ b/drivers/md/dm-delay.c
@@ -237,7 +237,7 @@ static int delay_bio(struct delay_c *dc, int delay, struct bio *bio)
 	unsigned long expires = 0;
 
 	if (!delay || !atomic_read(&dc->may_delay))
-		return 1;
+		return DM_MAPIO_REMAPPED;
 
 	delayed = dm_per_bio_data(bio, sizeof(struct dm_delay_info));
 
@@ -257,7 +257,7 @@ static int delay_bio(struct delay_c *dc, int delay, struct bio *bio)
 
 	queue_timeout(dc, expires);
 
-	return 0;
+	return DM_MAPIO_SUBMITTED;
 }
 
 static void delay_presuspend(struct dm_target *ti)
-- 
1.7.1

[PATCH 2/6] dm-delay: Update documentation for offsets (resend)

[Tomohiro Kusumi]

Only delay params are mentioned in delay.txt.
Mention offsets just like documents for linear and flakey do.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 Documentation/device-mapper/delay.txt |    1 +
 drivers/md/dm-delay.c                 |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Documentation/device-mapper/delay.txt b/Documentation/device-mapper/delay.txt
index 15adc55..a07b592 100644
--- a/Documentation/device-mapper/delay.txt
+++ b/Documentation/device-mapper/delay.txt
@@ -8,6 +8,7 @@ Parameters:
     <device> <offset> <delay> [<write_device> <write_offset> <write_delay>]
 
 With separate write parameters, the first set is only used for reads.
+Offsets are specified in sectors.
 Delays are specified in milliseconds.
 
 Example scripts
diff --git a/drivers/md/dm-delay.c b/drivers/md/dm-delay.c
index e10f69a..772b720 100644
--- a/drivers/md/dm-delay.c
+++ b/drivers/md/dm-delay.c
@@ -122,6 +122,7 @@ static void flush_expired_bios(struct work_struct *work)
  *    <device> <offset> <delay> [<write_device> <write_offset> <write_delay>]
  *
  * With separate write parameters, the first set is only used for reads.
+ * Offsets are specified in sectors.
  * Delays are specified in milliseconds.
  */
 static int delay_ctr(struct dm_target *ti, unsigned int argc, char **argv)
-- 
1.7.1

[PATCH 3/6] dm-delay: Capitalize error message consistently

[Tomohiro Kusumi]

Other ti->error's are all capitalized.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 drivers/md/dm-delay.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/md/dm-delay.c b/drivers/md/dm-delay.c
index 772b720..b4c356a 100644
--- a/drivers/md/dm-delay.c
+++ b/drivers/md/dm-delay.c
@@ -133,7 +133,7 @@ static int delay_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	int ret;
 
 	if (argc != 3 && argc != 6) {
-		ti->error = "requires exactly 3 or 6 arguments";
+		ti->error = "Requires exactly 3 or 6 arguments";
 		return -EINVAL;
 	}
 
-- 
1.7.1

[PATCH 4/6] dm-linear: Improve error message consistency

[Tomohiro Kusumi]

72d94861 back in 2006 should have removed "dm-linear: "
from these three as well.

  # grep "ti->error =" drivers/md -rIh | grep "dm-"
		ti->error = "Cannot initialize dm-bufio";
		ti->error = "dm-linear: Cannot allocate linear context";
		ti->error = "dm-linear: Invalid device sector";
		ti->error = "dm-linear: Device lookup failed";
  # grep "Cannot allocate .* context\"" drivers/md -rI
  drivers/md/dm-raid1.c:		ti->error = "Cannot allocate mirror context";
  drivers/md/dm-crypt.c:		ti->error = "Cannot allocate encryption context";
  drivers/md/dm-raid.c:		ti->error = "Cannot allocate raid context";
  drivers/md/dm-linear.c:		ti->error = "dm-linear: Cannot allocate linear context";
  drivers/md/dm-switch.c:		ti->error = "Cannot allocate redirection context";
  # grep "Invalid device sector\"" drivers/md -rI
  drivers/md/dm-delay.c:		ti->error = "Invalid device sector";
  drivers/md/dm-crypt.c:		ti->error = "Invalid device sector";
  drivers/md/dm-flakey.c:		ti->error = "Invalid device sector";
  drivers/md/dm-linear.c:		ti->error = "dm-linear: Invalid device sector";
  # grep "Device lookup failed\"" drivers/md -rI
  drivers/md/dm-log-writes.c:		ti->error = "Device lookup failed";
  drivers/md/dm-delay.c:		ti->error = "Device lookup failed";
  drivers/md/dm-crypt.c:		ti->error = "Device lookup failed";
  drivers/md/dm-flakey.c:		ti->error = "Device lookup failed";
  drivers/md/dm-linear.c:		ti->error = "dm-linear: Device lookup failed";
  drivers/md/dm-switch.c:		ti->error = "Device lookup failed";

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 drivers/md/dm-linear.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c
index 436f5c9..60baa12 100644
--- a/drivers/md/dm-linear.c
+++ b/drivers/md/dm-linear.c
@@ -39,20 +39,20 @@ static int linear_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 
 	lc = kmalloc(sizeof(*lc), GFP_KERNEL);
 	if (lc == NULL) {
-		ti->error = "dm-linear: Cannot allocate linear context";
+		ti->error = "Cannot allocate linear context";
 		return -ENOMEM;
 	}
 
 	ret = -EINVAL;
 	if (sscanf(argv[1], "%llu%c", &tmp, &dummy) != 1) {
-		ti->error = "dm-linear: Invalid device sector";
+		ti->error = "Invalid device sector";
 		goto bad;
 	}
 	lc->start = tmp;
 
 	ret = dm_get_device(ti, argv[0], dm_table_get_mode(ti->table), &lc->dev);
 	if (ret) {
-		ti->error = "dm-linear: Device lookup failed";
+		ti->error = "Device lookup failed";
 		goto bad;
 	}
 
-- 
1.7.1

[PATCH 5/6] dm-switch: Remove unnecessary condition in conditional

[Tomohiro Kusumi]

It's always (sctx->nr_regions == nr_regions) considering
the assignment right before this conditional, and this
function is only used by .ctr.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 drivers/md/dm-switch.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/md/dm-switch.c b/drivers/md/dm-switch.c
index 50fca46..372ec34 100644
--- a/drivers/md/dm-switch.c
+++ b/drivers/md/dm-switch.c
@@ -100,7 +100,7 @@ static int alloc_region_table(struct dm_target *ti, unsigned nr_paths)
 		nr_regions++;
 
 	sctx->nr_regions = nr_regions;
-	if (sctx->nr_regions != nr_regions || sctx->nr_regions >= ULONG_MAX) {
+	if (sctx->nr_regions >= ULONG_MAX) {
 		ti->error = "Region table too large";
 		return -EINVAL;
 	}
-- 
1.7.1

Re: [PATCH 5/6] dm-switch: Remove unnecessary condition in conditional

[Mikulas Patocka]

On Wed, 28 Oct 2015, Tomohiro Kusumi wrote:

> It's always (sctx->nr_regions == nr_regions) considering
> the assignment right before this conditional, and this
> function is only used by .ctr.

The variable sctx->nr_regions has type unsigned long and the variable 
nr_regions has type sector_t.

Thus the variables may be different when overflow happens.

It could be changed to "if (nr_regions >= ULONG_MAX)".

Mikulas

> Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
> ---
>  drivers/md/dm-switch.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/md/dm-switch.c b/drivers/md/dm-switch.c
> index 50fca46..372ec34 100644
> --- a/drivers/md/dm-switch.c
> +++ b/drivers/md/dm-switch.c
> @@ -100,7 +100,7 @@ static int alloc_region_table(struct dm_target *ti, unsigned nr_paths)
>  		nr_regions++;
>  
>  	sctx->nr_regions = nr_regions;
> -	if (sctx->nr_regions != nr_regions || sctx->nr_regions >= ULONG_MAX) {
> +	if (sctx->nr_regions >= ULONG_MAX) {
>  		ti->error = "Region table too large";
>  		return -EINVAL;
>  	}
> -- 
> 1.7.1
> 

[PATCH 6/6] dm-stripe: Fix error message

[Tomohiro Kusumi]

The meaning of "not divisible by chunk size" has changed after
d793e684, so the error message here should probably be using
"Stripe length" since tmp_len is result of width/stripes, but
not the whole target device size.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 drivers/md/dm-stripe.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c
index 797ddb9..85cb09f 100644
--- a/drivers/md/dm-stripe.c
+++ b/drivers/md/dm-stripe.c
@@ -127,7 +127,7 @@ static int stripe_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 
 	tmp_len = width;
 	if (sector_div(tmp_len, chunk_size)) {
-		ti->error = "Target length not divisible by "
+		ti->error = "Stripe length not divisible by "
 		    "chunk size";
 		return -EINVAL;
 	}
-- 
1.7.1

Re: [PATCH 6/6] dm-stripe: Fix error message

[Mike Snitzer]

On Tue, Oct 27 2015 at  3:39pm -0400,
Tomohiro Kusumi <kusumi.tomohiro@gmail.com> wrote:

> The meaning of "not divisible by chunk size" has changed after
> d793e684, so the error message here should probably be using
> "Stripe length" since tmp_len is result of width/stripes, but
> not the whole target device size.
> 
> Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
> ---
>  drivers/md/dm-stripe.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c
> index 797ddb9..85cb09f 100644
> --- a/drivers/md/dm-stripe.c
> +++ b/drivers/md/dm-stripe.c
> @@ -127,7 +127,7 @@ static int stripe_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  
>  	tmp_len = width;
>  	if (sector_div(tmp_len, chunk_size)) {
> -		ti->error = "Target length not divisible by "
> +		ti->error = "Stripe length not divisible by "
>  		    "chunk size";
>  		return -EINVAL;
>  	}

There is no need for this change.  The user-facing error is more
meaningful.  The only input that is being checked here is the specified
target length.

Dropping this patch.

Re: [PATCH 6/6] dm-stripe: Fix error message

[Tomohiro Kusumi]

[-- Attachment #1.1: Type: text/plain, Size: 1636 bytes --]

hi, thanks for your reply.

I understand what you mean, but that's the reason.
I happened to use something like below via dmsetup

--table '0 100 striped 2 4 /dev/xxx 0 /dev/yyy 0'

and hit this message which is not obvious as long as it says "Target
length".
Users have no idea 50/4 was what was really done in kernel space.


2015-10-29 4:51 GMT+09:00 Mike Snitzer <snitzer@redhat.com>:

> On Tue, Oct 27 2015 at  3:39pm -0400,
> Tomohiro Kusumi <kusumi.tomohiro@gmail.com> wrote:
>
> > The meaning of "not divisible by chunk size" has changed after
> > d793e684, so the error message here should probably be using
> > "Stripe length" since tmp_len is result of width/stripes, but
> > not the whole target device size.
> >
> > Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
> > ---
> >  drivers/md/dm-stripe.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c
> > index 797ddb9..85cb09f 100644
> > --- a/drivers/md/dm-stripe.c
> > +++ b/drivers/md/dm-stripe.c
> > @@ -127,7 +127,7 @@ static int stripe_ctr(struct dm_target *ti, unsigned
> int argc, char **argv)
> >
> >       tmp_len = width;
> >       if (sector_div(tmp_len, chunk_size)) {
> > -             ti->error = "Target length not divisible by "
> > +             ti->error = "Stripe length not divisible by "
> >                   "chunk size";
> >               return -EINVAL;
> >       }
>
> There is no need for this change.  The user-facing error is more
> meaningful.  The only input that is being checked here is the specified
> target length.
>
> Dropping this patch.
>

[-- Attachment #1.2: Type: text/html, Size: 2467 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 6/6] dm-stripe: Fix error message

[Mike Snitzer]

On Wed, Oct 28 2015 at  4:40pm -0400,
Tomohiro Kusumi <kusumi.tomohiro@gmail.com> wrote:

> hi, thanks for your reply.
> 
> I understand what you mean, but that's the reason.
> I happened to use something like below via dmsetup
> 
> --table '0 100 striped 2 4 /dev/xxx 0 /dev/yyy 0'
> 
> and hit this message which is not obvious as long as it says "Target
> length".
> Users have no idea 50/4 was what was really done in kernel space.

You're allowing a very literal look at the code to cloud your perception
of the error and concluding that it is misleading/incorrect.

Just because the code does math in a certain order doesn't change the
fact that the error message is still correct (if/when it is reported).

Re: [PATCH 6/6] dm-stripe: Fix error message

[Tomohiro Kusumi]

[-- Attachment #1.1: Type: text/plain, Size: 888 bytes --]

i respect what you say.
anyhow thanks for applying the rest of the patches :)

2015-10-29 7:00 GMT+09:00 Mike Snitzer <snitzer@redhat.com>:

> On Wed, Oct 28 2015 at  4:40pm -0400,
> Tomohiro Kusumi <kusumi.tomohiro@gmail.com> wrote:
>
> > hi, thanks for your reply.
> >
> > I understand what you mean, but that's the reason.
> > I happened to use something like below via dmsetup
> >
> > --table '0 100 striped 2 4 /dev/xxx 0 /dev/yyy 0'
> >
> > and hit this message which is not obvious as long as it says "Target
> > length".
> > Users have no idea 50/4 was what was really done in kernel space.
>
> You're allowing a very literal look at the code to cloud your perception
> of the error and concluding that it is misleading/incorrect.
>
> Just because the code does math in a certain order doesn't change the
> fact that the error message is still correct (if/when it is reported).
>

[-- Attachment #1.2: Type: text/html, Size: 1396 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/6] dm-delay: Use DM_MAPIO_XXX macros instead of 0/1 (resend)

[Mikulas Patocka]

On Wed, 28 Oct 2015, Tomohiro Kusumi wrote:

> .map function of dm-delay returns return value of delay_bio(),
> hence it's supposed to return either of DM_MAPIO_XXX macros.
> 
> Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>

Acked-By: Mikulas Patocka <mpatocka@redhat.com>

> ---
>  drivers/md/dm-delay.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-delay.c b/drivers/md/dm-delay.c
> index b34f6e2..e10f69a 100644
> --- a/drivers/md/dm-delay.c
> +++ b/drivers/md/dm-delay.c
> @@ -237,7 +237,7 @@ static int delay_bio(struct delay_c *dc, int delay, struct bio *bio)
>  	unsigned long expires = 0;
>  
>  	if (!delay || !atomic_read(&dc->may_delay))
> -		return 1;
> +		return DM_MAPIO_REMAPPED;
>  
>  	delayed = dm_per_bio_data(bio, sizeof(struct dm_delay_info));
>  
> @@ -257,7 +257,7 @@ static int delay_bio(struct delay_c *dc, int delay, struct bio *bio)
>  
>  	queue_timeout(dc, expires);
>  
> -	return 0;
> +	return DM_MAPIO_SUBMITTED;
>  }
>  
>  static void delay_presuspend(struct dm_target *ti)
> -- 
> 1.7.1
>