Re: Re: [PATCH] Reduce delays to improve iscsi boot performance
Ulrich,
Thanks for your comments. Please see inline:
On Mon, Apr 23, 2018 at 1:55 AM, Ulrich Windl <
ulrich.wi...@rz.uni-regensburg.de> wrote:
> >>> The Lee-Man schrieb am 16.04.2018 um 23:57
> in
> Nachricht <59dd48da-66d2-44c0-a2ef-669e1897f...@googlegroups.com>:
> > On Monday, April 16, 2018 at 2:38:01 PM UTC-7, The Lee-Man wrote:
> >>
> >> On Friday, April 13, 2018 at 3:19:11 PM UTC-7, cathy.z...@oracle.com
> >> wrote:
> >>>
> >>> -retry:
> >>> -rc = iscsid_exec_req(&req, &rsp, 0, ISCSID_REQ_TIMEOUT);
> >>> /*
> >>> - * handle race where iscsid proc is starting up while we are
> >>> - * trying to connect.
> >>> + * Need to handle race where iscsid proc is starting up while
> we
> >>> are
> >>> + * trying to connect. Retry with exponential backoff, start
> from
> >>> 50 ms.
> >>> */
> >>> -if (rc == ISCSI_ERR_ISCSID_NOTCONN && retries < 30) {
> >>> -retries++;
> >>> -sleep(1);
> >>> -goto retry;
> >>> -} else if (rc)
> >>> -iscsi_err_print_msg(rc);
> >>> +for (msec = 50; msec <= 15000; msec <<= 1) {
> >>>
> >>
> >> Why are you using 50 -> 15000? Isn't the sequence then going to be:
> >> -> 50
> >> -> 100
> >> -> 200
> >> -> 400
> >> -> 800
> >> -> 1600 (too large)
> >>
> >> So why not either go to 800 or to 1600?
> >>
> >>
> > Doh! I am off by a factor of 10!
> >
> > -> 3200, 6400, 12800
>
> I'm late in the discussion, but I'd vote for three things:
> 1) Make the limits configurable (first delay, maximum wait duration)
>
Personally I am not a fan of introducing manual tunables, the existing
implementation does not allow configurable max time either. I would rather
not to change that.
> 2) Change the algorithm not to specify the last delay being used, but the
> total amount of waiting
>
Make the last delay time to be 1/2 of the expected total wait duration
already implies the total waiting time, right?
3) Log if waiting for some longer time
>
Again, this was not in the old implementation and I am reluctant to add it.
Thanks
- Cathy
>
> I had implemented something like that years ago in Perl. Maybe to make it
> clearer what I'm thinking of, here's a code snipplet:
>
>
> $self->Log(0, 'D', $logger, "Waiting up to $limit seconds for
> condition")
> if ($verbosity > 1 && !$result);
> no integer;
> for (my $wait = $min_wait; !$result && $limit > 0;
> $limit -= $wait, $wait *= 2) {
> $wait = $limit
> if ($wait > $limit);
> $self->Log(1, 'D', $logger,
>"Waiting $wait (of $limit remaining) seconds")
> if ($verbosity > 0 && !$result);
> # sleep $wait seconds to allow condition to become true
> select(undef, undef, undef, $wait);
> $result = $condition_r->($self);
> }
> $self->Log(1, 'D', $logger, "$condition_txt with $limit seconds
> remaining")
> if ($verbosity > 0 && $result);
>
> [...]
>
> Regards,
> Ulrich
>
>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to open-iscsi+unsubscr...@googlegroups.com.
> To post to this group, send email to open-iscsi@googlegroups.com.
> Visit this group at https://groups.google.com/group/open-iscsi.
> For more options, visit https://groups.google.com/d/optout.
>
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.
Antw: Re: [PATCH] Reduce delays to improve iscsi boot performance
>>> The Lee-Man schrieb am 16.04.2018 um 23:57 in
Nachricht <59dd48da-66d2-44c0-a2ef-669e1897f...@googlegroups.com>:
> On Monday, April 16, 2018 at 2:38:01 PM UTC-7, The Lee-Man wrote:
>>
>> On Friday, April 13, 2018 at 3:19:11 PM UTC-7, cathy.z...@oracle.com
>> wrote:
>>>
>>> From: Cathy Zhou
>>>
>>> iscsi boot performance can be improved by sleep() with finer
>>> grained interval and exponential backoff.
>>>
>>> Signed-off-by: Cathy Zhou
>>> ---
>>> usr/iscsistart.c | 35 ---
>>> 1 file changed, 24 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/usr/iscsistart.c b/usr/iscsistart.c
>>> index 67ac475..1e77e40 100644
>>> --- a/usr/iscsistart.c
>>> +++ b/usr/iscsistart.c
>>> @@ -27,6 +27,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> #include
>>> #include
>>> #include
>>> @@ -224,7 +225,8 @@ static int login_session(struct node_rec *rec)
>>> {
>>> iscsiadm_req_t req;
>>> iscsiadm_rsp_t rsp;
>>> -int rc, retries = 0;
>>> +int rc, msec, err;
>>> +struct timespec ts;
>>>
>>> rc = apply_params(rec);
>>> if (rc)
>>> @@ -237,18 +239,29 @@ static int login_session(struct node_rec *rec)
>>> req.command = MGMT_IPC_SESSION_LOGIN;
>>> memcpy(&req.u.session.rec, rec, sizeof(*rec));
>>>
>>> -retry:
>>> -rc = iscsid_exec_req(&req, &rsp, 0, ISCSID_REQ_TIMEOUT);
>>> /*
>>> - * handle race where iscsid proc is starting up while we are
>>> - * trying to connect.
>>> + * Need to handle race where iscsid proc is starting up while we
>>> are
>>> + * trying to connect. Retry with exponential backoff, start from
>>> 50 ms.
>>> */
>>> -if (rc == ISCSI_ERR_ISCSID_NOTCONN && retries < 30) {
>>> -retries++;
>>> -sleep(1);
>>> -goto retry;
>>> -} else if (rc)
>>> -iscsi_err_print_msg(rc);
>>> +for (msec = 50; msec <= 15000; msec <<= 1) {
>>>
>>
>> Why are you using 50 -> 15000? Isn't the sequence then going to be:
>> -> 50
>> -> 100
>> -> 200
>> -> 400
>> -> 800
>> -> 1600 (too large)
>>
>> So why not either go to 800 or to 1600?
>>
>>
> Doh! I am off by a factor of 10!
>
> -> 3200, 6400, 12800
I'm late in the discussion, but I'd vote for three things:
1) Make the limits configurable (first delay, maximum wait duration)
2) Change the algorithm not to specify the last delay being used, but the total
amount of waiting
3) Log if waiting for some longer time
I had implemented something like that years ago in Perl. Maybe to make it
clearer what I'm thinking of, here's a code snipplet:
$self->Log(0, 'D', $logger, "Waiting up to $limit seconds for condition")
if ($verbosity > 1 && !$result);
no integer;
for (my $wait = $min_wait; !$result && $limit > 0;
$limit -= $wait, $wait *= 2) {
$wait = $limit
if ($wait > $limit);
$self->Log(1, 'D', $logger,
"Waiting $wait (of $limit remaining) seconds")
if ($verbosity > 0 && !$result);
# sleep $wait seconds to allow condition to become true
select(undef, undef, undef, $wait);
$result = $condition_r->($self);
}
$self->Log(1, 'D', $logger, "$condition_txt with $limit seconds remaining")
if ($verbosity > 0 && $result);
[...]
Regards,
Ulrich
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] Reduce delays to improve iscsi boot performance
Leeman,
Thanks very much for your comments. See inline:
On Mon, Apr 16, 2018 at 2:38 PM, The Lee-Man
wrote:
...
>> -retry:
>> -rc = iscsid_exec_req(&req, &rsp, 0, ISCSID_REQ_TIMEOUT);
>> /*
>> - * handle race where iscsid proc is starting up while we are
>> - * trying to connect.
>> + * Need to handle race where iscsid proc is starting up while
we are
>> + * trying to connect. Retry with exponential backoff, start
from 50 ms.
>> */
>> -if (rc == ISCSI_ERR_ISCSID_NOTCONN && retries < 30) {
>> -retries++;
>> -sleep(1);
>> -goto retry;
>> -} else if (rc)
>> -iscsi_err_print_msg(rc);
>> +for (msec = 50; msec <= 15000; msec <<= 1) {
>
>
> Why are you using 50 -> 15000? Isn't the sequence then going to be:
> -> 50
> -> 100
> -> 200
> -> 400
> -> 800
> -> 1600 (too large)
>
> So why not either go to 800 or to 1600?
I saw your another email. Yes, with the old implementation, retry gives up
until 30 seconds, that is why we choose to stop at 30s/2.
>
>
>> +rc = iscsid_exec_req(&req, &rsp, 0, ISCSID_REQ_TIMEOUT);
>> +if (rc == 0) {
>> +return rc;
>> +} else if (rc == ISCSI_ERR_ISCSID_NOTCONN) {
>> +ts.tv_sec = msec / 1000;
>> +ts.tv_nsec = (msec % 1000) * 100L;
>> +
>> +/* On EINTR, retry nanosleep with remaining
time. */
>> +while ((err = nanosleep(&ts, &ts)) < 0 &&
>> + errno == EINTR);
>
>
> I'd feel better about this if you used one more level of parenthesis.
>
>
> Also, why are you supplying two timespec values? You don't care about
> the remainder, so pass in a NULL as the second argument?
As the comments explained: on EINTR, we will retry nanosleep with the
remaining time, that is why we care about the remainder. Hopefully this
also explains why we don't need one more level of parenthesis.
>>
>> +if (err < 0)
>> +break;
>> +} else {
>> +break;
>> +}
>> +}
>> +
>> +iscsi_err_print_msg(rc);
>> return rc;
>> }
>>
>
>
> Also, what kind of time improvement did you see with this?
>From what we can see, the interval between below two log messages has been
reduced from 1s to 0.05s.
systemd[689]: Executing: /usr/sbin/iscsistart -i ...
scsi host2: iSCSI Initiator over TCP/IP
Thanks
- Cathy
>>
>> --
>> 2.17.0
>>
> --
> You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to open-iscsi+unsubscr...@googlegroups.com.
> To post to this group, send email to open-iscsi@googlegroups.com.
> Visit this group at https://groups.google.com/group/open-iscsi.
> For more options, visit https://groups.google.com/d/optout.
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] Reduce delays to improve iscsi boot performance
On Monday, April 16, 2018 at 2:38:01 PM UTC-7, The Lee-Man wrote:
>
> On Friday, April 13, 2018 at 3:19:11 PM UTC-7, cathy.z...@oracle.com
> wrote:
>>
>> From: Cathy Zhou
>>
>> iscsi boot performance can be improved by sleep() with finer
>> grained interval and exponential backoff.
>>
>> Signed-off-by: Cathy Zhou
>> ---
>> usr/iscsistart.c | 35 ---
>> 1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/usr/iscsistart.c b/usr/iscsistart.c
>> index 67ac475..1e77e40 100644
>> --- a/usr/iscsistart.c
>> +++ b/usr/iscsistart.c
>> @@ -27,6 +27,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -224,7 +225,8 @@ static int login_session(struct node_rec *rec)
>> {
>> iscsiadm_req_t req;
>> iscsiadm_rsp_t rsp;
>> -int rc, retries = 0;
>> +int rc, msec, err;
>> +struct timespec ts;
>>
>> rc = apply_params(rec);
>> if (rc)
>> @@ -237,18 +239,29 @@ static int login_session(struct node_rec *rec)
>> req.command = MGMT_IPC_SESSION_LOGIN;
>> memcpy(&req.u.session.rec, rec, sizeof(*rec));
>>
>> -retry:
>> -rc = iscsid_exec_req(&req, &rsp, 0, ISCSID_REQ_TIMEOUT);
>> /*
>> - * handle race where iscsid proc is starting up while we are
>> - * trying to connect.
>> + * Need to handle race where iscsid proc is starting up while we
>> are
>> + * trying to connect. Retry with exponential backoff, start from
>> 50 ms.
>> */
>> -if (rc == ISCSI_ERR_ISCSID_NOTCONN && retries < 30) {
>> -retries++;
>> -sleep(1);
>> -goto retry;
>> -} else if (rc)
>> -iscsi_err_print_msg(rc);
>> +for (msec = 50; msec <= 15000; msec <<= 1) {
>>
>
> Why are you using 50 -> 15000? Isn't the sequence then going to be:
> -> 50
> -> 100
> -> 200
> -> 400
> -> 800
> -> 1600 (too large)
>
> So why not either go to 800 or to 1600?
>
>
Doh! I am off by a factor of 10!
-> 3200, 6400, 12800
Still, the point is that maybe the max should be the a power of 10 of the
min? (It's really just a nit)
> +rc = iscsid_exec_req(&req, &rsp, 0, ISCSID_REQ_TIMEOUT);
>> +if (rc == 0) {
>> +return rc;
>> +} else if (rc == ISCSI_ERR_ISCSID_NOTCONN) {
>> +ts.tv_sec = msec / 1000;
>> +ts.tv_nsec = (msec % 1000) * 100L;
>> +
>> +/* On EINTR, retry nanosleep with remaining
>> time. */
>> +while ((err = nanosleep(&ts, &ts)) < 0 &&
>> + errno == EINTR);
>>
>
> I'd feel better about this if you used one more level of parenthesis.
>
> Also, why are you supplying two timespec values? You don't care about
> the remainder, so pass in a NULL as the second argument?
>
>> +if (err < 0)
>> +break;
>> +} else {
>> +break;
>> +}
>> +}
>> +
>> +iscsi_err_print_msg(rc);
>> return rc;
>> }
>>
>>
>
> Also, what kind of time improvement did you see with this?
>
>> --
>> 2.17.0
>>
>>
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] Reduce delays to improve iscsi boot performance
On Friday, April 13, 2018 at 3:19:11 PM UTC-7, cathy.z...@oracle.com wrote:
>
> From: Cathy Zhou
>
> iscsi boot performance can be improved by sleep() with finer
> grained interval and exponential backoff.
>
> Signed-off-by: Cathy Zhou
> ---
> usr/iscsistart.c | 35 ---
> 1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/usr/iscsistart.c b/usr/iscsistart.c
> index 67ac475..1e77e40 100644
> --- a/usr/iscsistart.c
> +++ b/usr/iscsistart.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -224,7 +225,8 @@ static int login_session(struct node_rec *rec)
> {
> iscsiadm_req_t req;
> iscsiadm_rsp_t rsp;
> -int rc, retries = 0;
> +int rc, msec, err;
> +struct timespec ts;
>
> rc = apply_params(rec);
> if (rc)
> @@ -237,18 +239,29 @@ static int login_session(struct node_rec *rec)
> req.command = MGMT_IPC_SESSION_LOGIN;
> memcpy(&req.u.session.rec, rec, sizeof(*rec));
>
> -retry:
> -rc = iscsid_exec_req(&req, &rsp, 0, ISCSID_REQ_TIMEOUT);
> /*
> - * handle race where iscsid proc is starting up while we are
> - * trying to connect.
> + * Need to handle race where iscsid proc is starting up while we
> are
> + * trying to connect. Retry with exponential backoff, start from
> 50 ms.
> */
> -if (rc == ISCSI_ERR_ISCSID_NOTCONN && retries < 30) {
> -retries++;
> -sleep(1);
> -goto retry;
> -} else if (rc)
> -iscsi_err_print_msg(rc);
> +for (msec = 50; msec <= 15000; msec <<= 1) {
>
Why are you using 50 -> 15000? Isn't the sequence then going to be:
-> 50
-> 100
-> 200
-> 400
-> 800
-> 1600 (too large)
So why not either go to 800 or to 1600?
+rc = iscsid_exec_req(&req, &rsp, 0, ISCSID_REQ_TIMEOUT);
> +if (rc == 0) {
> +return rc;
> +} else if (rc == ISCSI_ERR_ISCSID_NOTCONN) {
> +ts.tv_sec = msec / 1000;
> +ts.tv_nsec = (msec % 1000) * 100L;
> +
> +/* On EINTR, retry nanosleep with remaining time.
> */
> +while ((err = nanosleep(&ts, &ts)) < 0 &&
> + errno == EINTR);
>
I'd feel better about this if you used one more level of parenthesis.
Also, why are you supplying two timespec values? You don't care about
the remainder, so pass in a NULL as the second argument?
> +if (err < 0)
> +break;
> +} else {
> +break;
> +}
> +}
> +
> +iscsi_err_print_msg(rc);
> return rc;
> }
>
>
Also, what kind of time improvement did you see with this?
> --
> 2.17.0
>
>
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.
[PATCH] Reduce delays to improve iscsi boot performance
From: Cathy Zhou
iscsi boot performance can be improved by sleep() with finer
grained interval and exponential backoff.
Signed-off-by: Cathy Zhou
---
usr/iscsistart.c | 35 ---
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/usr/iscsistart.c b/usr/iscsistart.c
index 67ac475..1e77e40 100644
--- a/usr/iscsistart.c
+++ b/usr/iscsistart.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -224,7 +225,8 @@ static int login_session(struct node_rec *rec)
{
iscsiadm_req_t req;
iscsiadm_rsp_t rsp;
- int rc, retries = 0;
+ int rc, msec, err;
+ struct timespec ts;
rc = apply_params(rec);
if (rc)
@@ -237,18 +239,29 @@ static int login_session(struct node_rec *rec)
req.command = MGMT_IPC_SESSION_LOGIN;
memcpy(&req.u.session.rec, rec, sizeof(*rec));
-retry:
- rc = iscsid_exec_req(&req, &rsp, 0, ISCSID_REQ_TIMEOUT);
/*
-* handle race where iscsid proc is starting up while we are
-* trying to connect.
+* Need to handle race where iscsid proc is starting up while we are
+* trying to connect. Retry with exponential backoff, start from 50 ms.
*/
- if (rc == ISCSI_ERR_ISCSID_NOTCONN && retries < 30) {
- retries++;
- sleep(1);
- goto retry;
- } else if (rc)
- iscsi_err_print_msg(rc);
+ for (msec = 50; msec <= 15000; msec <<= 1) {
+ rc = iscsid_exec_req(&req, &rsp, 0, ISCSID_REQ_TIMEOUT);
+ if (rc == 0) {
+ return rc;
+ } else if (rc == ISCSI_ERR_ISCSID_NOTCONN) {
+ ts.tv_sec = msec / 1000;
+ ts.tv_nsec = (msec % 1000) * 100L;
+
+ /* On EINTR, retry nanosleep with remaining time. */
+ while ((err = nanosleep(&ts, &ts)) < 0 &&
+ errno == EINTR);
+ if (err < 0)
+ break;
+ } else {
+ break;
+ }
+ }
+
+ iscsi_err_print_msg(rc);
return rc;
}
--
2.17.0
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.
