[PATCH] dm-delay: Add a message to change delay

[PATCH] dm-delay: Add a message to change delay

[Andy Grover]

This enables runtime modification of the read and write delay values.

Make sure if the delay time is reduced to flush currently-delayed
bios first, to maintain ordering.

Signed-off-by: Andy Grover <agrover@redhat.com>
---
 Documentation/device-mapper/delay.txt |  8 +++++++
 drivers/md/dm-delay.c                 | 42 +++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/Documentation/device-mapper/delay.txt b/Documentation/device-mapper/delay.txt
index 15adc55..9e80751 100644
--- a/Documentation/device-mapper/delay.txt
+++ b/Documentation/device-mapper/delay.txt
@@ -10,6 +10,14 @@ Parameters:
 With separate write parameters, the first set is only used for reads.
 Delays are specified in milliseconds.
 
+Message Interface
+-----------------
+The delay target will accept a message of the following format:
+
+set_delay <read_delay> [<write_delay>]
+
+'man dmsetup' for more information on the message interface.
+
 Example scripts
 ===============
 [[
diff --git a/drivers/md/dm-delay.c b/drivers/md/dm-delay.c
index 57b6a19..04c2ab0 100644
--- a/drivers/md/dm-delay.c
+++ b/drivers/md/dm-delay.c
@@ -290,6 +290,47 @@ static int delay_map(struct dm_target *ti, struct bio *bio)
 	return delay_bio(dc, dc->read_delay, bio);
 }
 
+/* Message interface
+ *	set_delay <read_delay> [<write_delay>]
+*/
+static int delay_message(struct dm_target *ti, unsigned argc, char **argv)
+{
+	struct delay_c *dc = ti->private;
+	unsigned read_delay = dc->read_delay;
+	unsigned write_delay = dc->write_delay;
+	char dummy;
+
+	if (argc < 2 || argc > 3)
+		goto error;
+
+	if (strcasecmp(argv[0], "set_delay"))
+		goto error;
+
+	if (sscanf(argv[1], "%u%c", &read_delay, &dummy) != 1) {
+		ti->error = "Invalid read delay";
+		goto error;
+	}
+
+	if (argc == 3) {
+		if (sscanf(argv[2], "%u%c", &write_delay, &dummy) != 1) {
+			ti->error = "Invalid write delay";
+			goto error;
+		}
+	}
+
+	if (read_delay < dc->read_delay
+	    || write_delay < dc->write_delay)
+		flush_bios(flush_delayed_bios(dc, 1));
+
+	dc->read_delay = read_delay;
+	dc->write_delay = write_delay;
+	return 0;
+
+error:
+	DMWARN("unrecognised message received.");
+	return -EINVAL;
+}
+
 static void delay_status(struct dm_target *ti, status_type_t type,
 			 unsigned status_flags, char *result, unsigned maxlen)
 {
@@ -339,6 +380,7 @@ static struct target_type delay_target = {
 	.map	     = delay_map,
 	.presuspend  = delay_presuspend,
 	.resume	     = delay_resume,
+	.message     = delay_message,
 	.status	     = delay_status,
 	.iterate_devices = delay_iterate_devices,
 };
-- 
2.4.3

Re: [PATCH] dm-delay: Add a message to change delay

[Vivek Goyal]

On Mon, Aug 31, 2015 at 02:24:36PM -0700, Andy Grover wrote:
> This enables runtime modification of the read and write delay values.
> 
> Make sure if the delay time is reduced to flush currently-delayed
> bios first, to maintain ordering.
> 
> Signed-off-by: Andy Grover <agrover@redhat.com>
> ---
>  Documentation/device-mapper/delay.txt |  8 +++++++
>  drivers/md/dm-delay.c                 | 42 +++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/Documentation/device-mapper/delay.txt b/Documentation/device-mapper/delay.txt
> index 15adc55..9e80751 100644
> --- a/Documentation/device-mapper/delay.txt
> +++ b/Documentation/device-mapper/delay.txt
> @@ -10,6 +10,14 @@ Parameters:
>  With separate write parameters, the first set is only used for reads.
>  Delays are specified in milliseconds.
>  
> +Message Interface
> +-----------------
> +The delay target will accept a message of the following format:
> +
> +set_delay <read_delay> [<write_delay>]
> +

Hi Andy,

So if I want to change only write_delay and keep read_delay same, how do
I do that. Do I have to keep track of existing delay values in user space
and pass same value in read_delay to achieve this.

Thanks
Vivek

> +'man dmsetup' for more information on the message interface.
> +
>  Example scripts
>  ===============
>  [[
> diff --git a/drivers/md/dm-delay.c b/drivers/md/dm-delay.c
> index 57b6a19..04c2ab0 100644
> --- a/drivers/md/dm-delay.c
> +++ b/drivers/md/dm-delay.c
> @@ -290,6 +290,47 @@ static int delay_map(struct dm_target *ti, struct bio *bio)
>  	return delay_bio(dc, dc->read_delay, bio);
>  }
>  
> +/* Message interface
> + *	set_delay <read_delay> [<write_delay>]
> +*/
> +static int delay_message(struct dm_target *ti, unsigned argc, char **argv)
> +{
> +	struct delay_c *dc = ti->private;
> +	unsigned read_delay = dc->read_delay;
> +	unsigned write_delay = dc->write_delay;
> +	char dummy;
> +
> +	if (argc < 2 || argc > 3)
> +		goto error;
> +
> +	if (strcasecmp(argv[0], "set_delay"))
> +		goto error;
> +
> +	if (sscanf(argv[1], "%u%c", &read_delay, &dummy) != 1) {
> +		ti->error = "Invalid read delay";
> +		goto error;
> +	}
> +
> +	if (argc == 3) {
> +		if (sscanf(argv[2], "%u%c", &write_delay, &dummy) != 1) {
> +			ti->error = "Invalid write delay";
> +			goto error;
> +		}
> +	}
> +
> +	if (read_delay < dc->read_delay
> +	    || write_delay < dc->write_delay)
> +		flush_bios(flush_delayed_bios(dc, 1));
> +
> +	dc->read_delay = read_delay;
> +	dc->write_delay = write_delay;
> +	return 0;
> +
> +error:
> +	DMWARN("unrecognised message received.");
> +	return -EINVAL;
> +}
> +
>  static void delay_status(struct dm_target *ti, status_type_t type,
>  			 unsigned status_flags, char *result, unsigned maxlen)
>  {
> @@ -339,6 +380,7 @@ static struct target_type delay_target = {
>  	.map	     = delay_map,
>  	.presuspend  = delay_presuspend,
>  	.resume	     = delay_resume,
> +	.message     = delay_message,
>  	.status	     = delay_status,
>  	.iterate_devices = delay_iterate_devices,
>  };
> -- 
> 2.4.3
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH] dm-delay: Add a message to change delay

[Andy Grover]

On 08/31/2015 07:05 PM, Vivek Goyal wrote:
> On Mon, Aug 31, 2015 at 02:24:36PM -0700, Andy Grover wrote:
>> This enables runtime modification of the read and write delay values.
>>
>> Make sure if the delay time is reduced to flush currently-delayed
>> bios first, to maintain ordering.
>>
>> Signed-off-by: Andy Grover <agrover@redhat.com>
>> ---
>>   Documentation/device-mapper/delay.txt |  8 +++++++
>>   drivers/md/dm-delay.c                 | 42 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 50 insertions(+)
>>
>> diff --git a/Documentation/device-mapper/delay.txt b/Documentation/device-mapper/delay.txt
>> index 15adc55..9e80751 100644
>> --- a/Documentation/device-mapper/delay.txt
>> +++ b/Documentation/device-mapper/delay.txt
>> @@ -10,6 +10,14 @@ Parameters:
>>   With separate write parameters, the first set is only used for reads.
>>   Delays are specified in milliseconds.
>>
>> +Message Interface
>> +-----------------
>> +The delay target will accept a message of the following format:
>> +
>> +set_delay <read_delay> [<write_delay>]
>> +
>
> Hi Andy,
>
> So if I want to change only write_delay and keep read_delay same, how do
> I do that. Do I have to keep track of existing delay values in user space
> and pass same value in read_delay to achieve this.
>
> Thanks
> Vivek

Hi Vivek,

Yes I suppose userspace would either need to remember read_delay so as 
to not change it while setting write_delay, or I guess it could read the 
existing values by getting table status before sending the message. Is 
this reasonable, or do you think it would be better to, say, have 
separate messages for setting the two values, or some other message style?

Thanks -- Regards -- Andy

Re: [PATCH] dm-delay: Add a message to change delay

[Zdenek Kabelac]

Dne 31.8.2015 v 23:24 Andy Grover napsal(a):
> This enables runtime modification of the read and write delay values.
>
> Make sure if the delay time is reduced to flush currently-delayed
> bios first, to maintain ordering.
>


Hi


Flush with suspend is an optional feature.

You could  'dmsetup suspend --noflush'  so IMHO there is no need for
message support - you just load new table line
and go through suspend resume (resume should be enough)

Messages are normally used for something unrelated to table line,
for table line args - you should go with standard table line load
(so dmsetup table always shows correct value)

Regards

Zdenek



> Signed-off-by: Andy Grover <agrover@redhat.com>
> ---
>   Documentation/device-mapper/delay.txt |  8 +++++++
>   drivers/md/dm-delay.c                 | 42 +++++++++++++++++++++++++++++++++++
>   2 files changed, 50 insertions(+)
>
> diff --git a/Documentation/device-mapper/delay.txt b/Documentation/device-mapper/delay.txt
> index 15adc55..9e80751 100644
> --- a/Documentation/device-mapper/delay.txt
> +++ b/Documentation/device-mapper/delay.txt
> @@ -10,6 +10,14 @@ Parameters:
>   With separate write parameters, the first set is only used for reads.
>   Delays are specified in milliseconds.
>
> +Message Interface
> +-----------------
> +The delay target will accept a message of the following format:
> +
> +set_delay <read_delay> [<write_delay>]
> +
> +'man dmsetup' for more information on the message interface.
> +
>   Example scripts
>   ===============
>   [[
> diff --git a/drivers/md/dm-delay.c b/drivers/md/dm-delay.c
> index 57b6a19..04c2ab0 100644
> --- a/drivers/md/dm-delay.c
> +++ b/drivers/md/dm-delay.c
> @@ -290,6 +290,47 @@ static int delay_map(struct dm_target *ti, struct bio *bio)
>   	return delay_bio(dc, dc->read_delay, bio);
>   }
>
> +/* Message interface
> + *	set_delay <read_delay> [<write_delay>]
> +*/
> +static int delay_message(struct dm_target *ti, unsigned argc, char **argv)
> +{
> +	struct delay_c *dc = ti->private;
> +	unsigned read_delay = dc->read_delay;
> +	unsigned write_delay = dc->write_delay;
> +	char dummy;
> +
> +	if (argc < 2 || argc > 3)
> +		goto error;
> +
> +	if (strcasecmp(argv[0], "set_delay"))
> +		goto error;
> +
> +	if (sscanf(argv[1], "%u%c", &read_delay, &dummy) != 1) {
> +		ti->error = "Invalid read delay";
> +		goto error;
> +	}
> +
> +	if (argc == 3) {
> +		if (sscanf(argv[2], "%u%c", &write_delay, &dummy) != 1) {
> +			ti->error = "Invalid write delay";
> +			goto error;
> +		}
> +	}
> +
> +	if (read_delay < dc->read_delay
> +	    || write_delay < dc->write_delay)
> +		flush_bios(flush_delayed_bios(dc, 1));
> +
> +	dc->read_delay = read_delay;
> +	dc->write_delay = write_delay;
> +	return 0;
> +
> +error:
> +	DMWARN("unrecognised message received.");
> +	return -EINVAL;
> +}
> +
>   static void delay_status(struct dm_target *ti, status_type_t type,
>   			 unsigned status_flags, char *result, unsigned maxlen)
>   {
> @@ -339,6 +380,7 @@ static struct target_type delay_target = {
>   	.map	     = delay_map,
>   	.presuspend  = delay_presuspend,
>   	.resume	     = delay_resume,
> +	.message     = delay_message,
>   	.status	     = delay_status,
>   	.iterate_devices = delay_iterate_devices,
>   };
>

Re: [PATCH] dm-delay: Add a message to change delay

[Vivek Goyal]

On Mon, Aug 31, 2015 at 10:02:33PM -0700, Andy Grover wrote:
> On 08/31/2015 07:05 PM, Vivek Goyal wrote:
> >On Mon, Aug 31, 2015 at 02:24:36PM -0700, Andy Grover wrote:
> >>This enables runtime modification of the read and write delay values.
> >>
> >>Make sure if the delay time is reduced to flush currently-delayed
> >>bios first, to maintain ordering.
> >>
> >>Signed-off-by: Andy Grover <agrover@redhat.com>
> >>---
> >>  Documentation/device-mapper/delay.txt |  8 +++++++
> >>  drivers/md/dm-delay.c                 | 42 +++++++++++++++++++++++++++++++++++
> >>  2 files changed, 50 insertions(+)
> >>
> >>diff --git a/Documentation/device-mapper/delay.txt b/Documentation/device-mapper/delay.txt
> >>index 15adc55..9e80751 100644
> >>--- a/Documentation/device-mapper/delay.txt
> >>+++ b/Documentation/device-mapper/delay.txt
> >>@@ -10,6 +10,14 @@ Parameters:
> >>  With separate write parameters, the first set is only used for reads.
> >>  Delays are specified in milliseconds.
> >>
> >>+Message Interface
> >>+-----------------
> >>+The delay target will accept a message of the following format:
> >>+
> >>+set_delay <read_delay> [<write_delay>]
> >>+
> >
> >Hi Andy,
> >
> >So if I want to change only write_delay and keep read_delay same, how do
> >I do that. Do I have to keep track of existing delay values in user space
> >and pass same value in read_delay to achieve this.
> >
> >Thanks
> >Vivek
> 
> Hi Vivek,
> 
> Yes I suppose userspace would either need to remember read_delay so as to
> not change it while setting write_delay, or I guess it could read the
> existing values by getting table status before sending the message. Is this
> reasonable, or do you think it would be better to, say, have separate
> messages for setting the two values, or some other message style?

Ideally I think we should have those --key=value type of arguments which
we don't have yet. So that option is not feasible I guess.

If latest values are readable from status, then I think single message
sounds reasoanble to me.

Thanks
Vivek

> 
> Thanks -- Regards -- Andy

Re: dm-delay: Add a message to change delay

[Mike Snitzer]

On Tue, Sep 01 2015 at  8:14am -0400,
Vivek Goyal <vgoyal@redhat.com> wrote:

> On Mon, Aug 31, 2015 at 10:02:33PM -0700, Andy Grover wrote:
> > On 08/31/2015 07:05 PM, Vivek Goyal wrote:
> > >On Mon, Aug 31, 2015 at 02:24:36PM -0700, Andy Grover wrote:
> > >>This enables runtime modification of the read and write delay values.
> > >>
> > >>Make sure if the delay time is reduced to flush currently-delayed
> > >>bios first, to maintain ordering.
> > >>
> > >>Signed-off-by: Andy Grover <agrover@redhat.com>
> > >>---
> > >>  Documentation/device-mapper/delay.txt |  8 +++++++
> > >>  drivers/md/dm-delay.c                 | 42 +++++++++++++++++++++++++++++++++++
> > >>  2 files changed, 50 insertions(+)
> > >>
> > >>diff --git a/Documentation/device-mapper/delay.txt b/Documentation/device-mapper/delay.txt
> > >>index 15adc55..9e80751 100644
> > >>--- a/Documentation/device-mapper/delay.txt
> > >>+++ b/Documentation/device-mapper/delay.txt
> > >>@@ -10,6 +10,14 @@ Parameters:
> > >>  With separate write parameters, the first set is only used for reads.
> > >>  Delays are specified in milliseconds.
> > >>
> > >>+Message Interface
> > >>+-----------------
> > >>+The delay target will accept a message of the following format:
> > >>+
> > >>+set_delay <read_delay> [<write_delay>]
> > >>+
> > >
> > >Hi Andy,
> > >
> > >So if I want to change only write_delay and keep read_delay same, how do
> > >I do that. Do I have to keep track of existing delay values in user space
> > >and pass same value in read_delay to achieve this.
> > >
> > >Thanks
> > >Vivek
> > 
> > Hi Vivek,
> > 
> > Yes I suppose userspace would either need to remember read_delay so as to
> > not change it while setting write_delay, or I guess it could read the
> > existing values by getting table status before sending the message. Is this
> > reasonable, or do you think it would be better to, say, have separate
> > messages for setting the two values, or some other message style?
> 
> Ideally I think we should have those --key=value type of arguments which
> we don't have yet. So that option is not feasible I guess.
> 
> If latest values are readable from status, then I think single message
> sounds reasoanble to me.

As Zdenek already effectively said: there is no need for this patch.
You don't need to use a message when a table reload would suffice to
change the parameter that is already passed on the table ctr.

Any layer that would be trained to send a message can just as easily be
trained to reload the table to achieve the same result.

Re: dm-delay: Add a message to change delay

[Andy Grover]

On 09/01/2015 06:09 AM, Mike Snitzer wrote:
> On Tue, Sep 01 2015 at  8:14am -0400,
> Vivek Goyal <vgoyal@redhat.com> wrote:
>
>> On Mon, Aug 31, 2015 at 10:02:33PM -0700, Andy Grover wrote:
>>> On 08/31/2015 07:05 PM, Vivek Goyal wrote:
>>>> On Mon, Aug 31, 2015 at 02:24:36PM -0700, Andy Grover wrote:
>>>>> This enables runtime modification of the read and write delay values.
>>>>>
>>>>> Make sure if the delay time is reduced to flush currently-delayed
>>>>> bios first, to maintain ordering.
>>>>>
>>>>> Signed-off-by: Andy Grover <agrover@redhat.com>
>>>>> ---
>>>>>   Documentation/device-mapper/delay.txt |  8 +++++++
>>>>>   drivers/md/dm-delay.c                 | 42 +++++++++++++++++++++++++++++++++++
>>>>>   2 files changed, 50 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/device-mapper/delay.txt b/Documentation/device-mapper/delay.txt
>>>>> index 15adc55..9e80751 100644
>>>>> --- a/Documentation/device-mapper/delay.txt
>>>>> +++ b/Documentation/device-mapper/delay.txt
>>>>> @@ -10,6 +10,14 @@ Parameters:
>>>>>   With separate write parameters, the first set is only used for reads.
>>>>>   Delays are specified in milliseconds.
>>>>>
>>>>> +Message Interface
>>>>> +-----------------
>>>>> +The delay target will accept a message of the following format:
>>>>> +
>>>>> +set_delay <read_delay> [<write_delay>]
>>>>> +
>>>>
>>>> Hi Andy,
>>>>
>>>> So if I want to change only write_delay and keep read_delay same, how do
>>>> I do that. Do I have to keep track of existing delay values in user space
>>>> and pass same value in read_delay to achieve this.
>>>>
>>>> Thanks
>>>> Vivek
>>>
>>> Hi Vivek,
>>>
>>> Yes I suppose userspace would either need to remember read_delay so as to
>>> not change it while setting write_delay, or I guess it could read the
>>> existing values by getting table status before sending the message. Is this
>>> reasonable, or do you think it would be better to, say, have separate
>>> messages for setting the two values, or some other message style?
>>
>> Ideally I think we should have those --key=value type of arguments which
>> we don't have yet. So that option is not feasible I guess.
>>
>> If latest values are readable from status, then I think single message
>> sounds reasoanble to me.
>
> As Zdenek already effectively said: there is no need for this patch.
> You don't need to use a message when a table reload would suffice to
> change the parameter that is already passed on the table ctr.
>
> Any layer that would be trained to send a message can just as easily be
> trained to reload the table to achieve the same result.

Ah I see. Thanks for the explanations. Noflush suspend & table reload is 
better for this.

Regards -- Andy