Re: [PATCH] Reduce delays to improve iscsi boot performance

2018-04-16 Thread The Lee-Man 
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(, rec, sizeof(*rec)); 
>>   
>> -retry: 
>> -rc = iscsid_exec_req(, , 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(, , 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(, )) < 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

2018-04-16 Thread The Lee-Man 
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(, rec, sizeof(*rec)); 
>   
> -retry: 
> -rc = iscsid_exec_req(, , 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(, , 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(, )) < 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

2018-04-16 Thread Zhou Yun 
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(, , 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(, , 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(, )) < 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.