Problem setting host net params via iscsiuio on first try: bad error message

[The Lee-Man]

Hi Mike:

I have started working more with iscsiuio.

I have discovered what I consider a bug in an error message printed during normal operation of iscsiadm that makes it seem like something bad happened.

As you know, when performing discovery via the bnx2i transport and the iscsiuio daemon, the iscsiadm command tries to set up host parameters when a target is discovered. The iscsiadm command does this by sending a message to the iscsiuio daemon, who actually does the work in this case and then normally returns that information.

But it looks like the design of iscsiuio is that it does not even try to set up the NIC it is using very first time its called. Instead, it initializes the NIC only when the first attempt to use it is received, while it returns EAGAIN to the caller. The protocol evidently is that the caller knows to retry. In this case, the iscsiadm code retries several times no matter what error is returned. In fact, the iscsiadm code being used translates all errors received from this attempt to talk to iscsiuio into an "INTERNAL_ERROR", and the communication is retried anyway.

This translation is the only problem, IMHO, and resulted in these messages from iscsiadm, which are misleading (and poorly formatted):

 iscsiadm: Could not set host net params (err 29)

 iscsiadm: Connection to discovery portal 10.125.128.120 failed: internal error

 ...

which becomes, with my patches:

 iscsiadm: Could not set host net params (err 29)

 iscsiadm: Connection to discovery portal 192.168.30.1 failed: operation failed but retry may succeed

 ...

The "fix" is simple: in the case where iscsiuio returns EAGAIN, let it through. (And fix the error message printing so we don't core dump, of course.)

I thought about a more aggressive fix, i.e. letting all return values from the communications attempt through, but I worried about fixing something that is not really broken and perhaps breaking something else. So, in the spirit of fixing just what is broken, I submit two patches:

0001-Supply-strings-for-newly-added-error-numbers.patch: This patch is needed by the second patch, though it stands alone. This patch fixes the problem that a couple of new error codes have been added to the open-iscsi package but the array of error strings used to convert those errors into printed messages has not kept pace. So two missing error messages are added.

0002-Allow-setting-host-params-to-return-EAGAIN-errors.patch - This is the simple patch that allows EAGAIN errors through when trying to talk to iscsiuio. If you want the more aggressive version please let me know.

[Mike Christie]

That is not how we do it. Just like in that other thread about the sysfs
scanning. We want to fix the root cause and also any issues that result
from it.

Please just fix this properly. It is really silly that the user would
have to run this command twice to actually do discovery.

[Lee Duncan]

Interesting that you would reference that other thread. I don't think there's any absolute root cause in either case, so it's a judgement call.

>
> Please just fix this properly. It is really silly that the user would
> have to run this command twice to actually do discovery.
>

Ouch. Okay, interesting approach. I guess I stand rebuffed.

"Fixing this" would really mean getting rid of iscsiuio and having BRCM devices use the network stack, wouldn't it? I'm not sure where you draw the line between cosmetic hacks and fixing it "properly".

Why do you think the user has to "run this command twice"? That is incorrect. The iscsiadm command retries the communication 10 times, I believe, or until iscsiuio replies. Do you consider that silly? Do you mean that the iscsiadm code should not retry?

Or do you mean the iscsiuio code should be redesigned? This old code uses the uIP stack to perform the functions of a network driver. I am hesitant to redesign this multi-threaded beast when I have only one BRCM card, and it mostly just works now.
--
The Lee-Man Rides Again

"...And maddest of all, to see life as it is and not as it should be!" — Cervantes

[Mike Christie]

In that other thread, I said I wanted to find out why we are
adding/removing sessions. That should not be happening and if you
figured that out, I was saying that I thought you would not hit the
sysfs scanning issue. Also I said in that other thread I found that bug
with the discoveryd daemon caching when it should not so that also
causes a bug that might be affecting your testing.

>>
>> Please just fix this properly. It is really silly that the user would
>> have to run this command twice to actually do discovery.
>>
>
> Ouch. Okay, interesting approach. I guess I stand rebuffed.
>
> "Fixing this" would really mean getting rid of iscsiuio and having BRCM devices use the network stack, wouldn't it? I'm not sure where you draw the line between cosmetic hacks and fixing it "properly".

Upstream already rejected the net stack approach so that is not an option.

>
> Why do you think the user has to "run this command twice"? That is incorrect. The iscsiadm command retries the communication 10 times, I believe, or until iscsiuio replies. Do you consider that silly? Do you mean that the iscsiadm code should not retry?
>

If the user/caller has retries off then they would have to retry. In the
command output you put in the mail, it was not clear if you allowed
retries or not.

iscsid and/or iscsiadm should not have to retry. It seems like a bogus
design that it does not work the first time. That is what should be fixed.

[Mike Christie]

Hi Lee,

I want to apologize for being a jerk above. The email is unprofessional.
I value your submissions and help on this project. I have no excuse, and
request that you please continue to help out and be part of this project

Mike

[The Lee-Man]

Mike: I apologize for taking it personally. No harm no foul. 

[Lee Duncan]

On Dec 12, 2014, at 12:08 PM, Mike Christie <mich...@cs.wisc.edu> wrote:
>
> In that other thread, I said I wanted to find out why we are
> adding/removing sessions. That should not be happening and if you
> figured that out, I was saying that I thought you would not hit the
> sysfs scanning issue. Also I said in that other thread I found that bug
> with the discoveryd daemon caching when it should not so that also
> causes a bug that might be affecting your testing.
>

You have valid points, of course, but let's discuss that issue on that thread.

>
>>>
>>> Please just fix this properly. It is really silly that the user would
>>> have to run this command twice to actually do discovery.
>>>
>>
>> Ouch. Okay, interesting approach. I guess I stand rebuffed.
>>
>> "Fixing this" would really mean getting rid of iscsiuio and having BRCM devices use the network stack, wouldn't it? I'm not sure where you draw the line between cosmetic hacks and fixing it "properly".
>
> Upstream already rejected the net stack approach so that is not an option.

I understood as much from Eddie. I shouldn't have suggested it, as I know it's not an option. I was trying (poorly) to make a point about how slippery the "right thing" slope can be. I withdraw the point.

>
>>
>> Why do you think the user has to "run this command twice"? That is incorrect. The iscsiadm command retries the communication 10 times, I believe, or until iscsiuio replies. Do you consider that silly? Do you mean that the iscsiadm code should not retry?
>>
>
> If the user/caller has retries off then they would have to retry. In the
> command output you put in the mail, it was not clear if you allowed
> retries or not.
>
> iscsid and/or iscsiadm should not have to retry. It seems like a bogus
> design that it does not work the first time. That is what should be fixed.

I am surprised to hear you say that. That seems a little like saying device drivers shouldn't have to retry I/O commands: they should just work the first time.

The code in open-iscsi that does this retry loop is in uip_broadcast() in iscsid_req.c:

...
+#define MAX_UIP_BROADCAST_READ_TRIES 3
+ for (count = 0; count < MAX_UIP_BROADCAST_READ_TRIES; count++) {
+ /* Wait for the response */
+ err = read(fd, &rsp, sizeof(rsp));
+ if (err == sizeof(rsp)) {
+ log_debug(3, "Broadcasted to uIP with length: %ld "
+ "cmd: 0x%x rsp: 0x%x\n", buf_len,
+ rsp.command, rsp.err);
+ err = 0;
+ break;
+ } else if ((err == -1) && (errno == EAGAIN)) {
+ usleep(250000);
+ continue;
+ } else {
+ log_error("Could not read response (%d/%d), daemon "
+ "died?", err, errno);
+ err = ISCSI_ERR;
+ break;
+ }
+ }
...

This seems to have been added last year when Eddie sent you a patch (see commit d571cdfc0b3c539310f43dc11d54d9308299efdc). That commit was quite extensive, introducing the EAGAIN error to iscsi processing.

And it looks like there are only 3 retries in this case, not 10 as I guessed before. And only in the case of EAGAIN.

If you look at iscsiuio (as I have now for a few days), it's clear that it was designed as a minimalist state machine. The choice of uIP for the network stack reenforces that impression. The design is all about assuming nothing comes from the operating system, and is often used in firmware.

Because of this, I hesitate to modify or replace this state machine, no matter how much fun or how pure such an exercise might be.

Instead, I believe the current design works just fine, given all of the design considerations (of which there are too many to list here). I therefore still think a simple tweak to this current design is all that is needed. The current design either works well enough or isn't used, since few bug reports seem to have surfaced.

I believe the best approach here is one of two:

- simplest: just allow the "EAGAIN" to percolate up so the error message is more correct, or
- better: change the code to not print an error message when EAGAIN is received and we still have retries left

I know you think there is a 3rd approach, but I see problems with "fixing" this, and here's why ...

The problem: iscsiuio needs to initialize an ethernet NIC/adapter before it can use it for bnx2i traffic (e.g. set up a VLAN, etc), but it does not know which adapter might be used until it gets a message from iscsid saying it has a new host connection. So there is no way for iscsiuio to prepare an adapter until it gets a request. And preparing an adapter takes a non-trivial amount of time. This is the same problems device drivers have with disc drives: the response will take too long to wait for.

One possible "fix" would be to have iscsiuio actually wait to respond to its first request until the adapter is prepared and UP, but that would be no better than having the caller retry: it still blocks success and makes the end user wait until it is complete.

Perhaps a cleaner fix would be to have iscsiuio accept the request to handle a new host by detaching and then replying later, asynchronously, when the NIC is set up and the host is registered. Even though this seems to me to be a cleaner design, the problem is that the poor end user that has typed "iscsiadm -m discovery -t st -I <bnx2i-interface> -t <some-target>" still has to wait! The are still going to have to wait, at least the first time, for the NIC to be set up. And discovery kind of has to be synchronous based on the current design.

The only way I can see that might fix this iscsiuio delay-on-first-use problem is to have iscsiuio aggressively configure any and all bnx2i interfaces it sees when it first starts up. Or perhaps it should have a configuration file that says "I'm going to use eth0, so get it ready"? I'm not sure this approach would be considered cleaner, though.
--
Lee-Man Duncan

"Beer is proof that God loves us and wants us to be happy." -- Ben Franklin

[Mike Christie]

On 12/12/2014 04:11 PM, Lee Duncan wrote:
> On Dec 12, 2014, at 12:08 PM, Mike Christie <mich...@cs.wisc.edu> wrote:
>>
>> In that other thread, I said I wanted to find out why we are
>> adding/removing sessions. That should not be happening and if you
>> figured that out, I was saying that I thought you would not hit the
>> sysfs scanning issue. Also I said in that other thread I found that bug
>> with the discoveryd daemon caching when it should not so that also
>> causes a bug that might be affecting your testing.
>>
>
> You have valid points, of course, but let's discuss that issue on that thread.
>
>>
>>>>
>>>> Please just fix this properly. It is really silly that the user would
>>>> have to run this command twice to actually do discovery.
>>>>
>>>
>>> Ouch. Okay, interesting approach. I guess I stand rebuffed.
>>>
>>> "Fixing this" would really mean getting rid of iscsiuio and having BRCM devices use the network stack, wouldn't it? I'm not sure where you draw the line between cosmetic hacks and fixing it "properly".
>>
>> Upstream already rejected the net stack approach so that is not an option.
>
> I understood as much from Eddie. I shouldn't have suggested it, as I know it's not an option. I was trying (poorly) to make a point about how slippery the "right thing" slope can be. I withdraw the point.
>
>>
>>>
>>> Why do you think the user has to "run this command twice"? That is incorrect. The iscsiadm command retries the communication 10 times, I believe, or until iscsiuio replies. Do you consider that silly? Do you mean that the iscsiadm code should not retry?
>>>
>>
>> If the user/caller has retries off then they would have to retry. In the
>> command output you put in the mail, it was not clear if you allowed
>> retries or not.
>>
>> iscsid and/or iscsiadm should not have to retry. It seems like a bogus
>> design that it does not work the first time. That is what should be fixed.
>
> I am surprised to hear you say that. That seems a little like saying device drivers shouldn't have to retry I/O commands: they should just work the first time.
>

That is not what I am saying at all. Above, I am saying that if some
code knows how to execute the operation and can, then I think it should
and should not rely on every caller to know what is right and drive retries.

We have retry code in iscsid. This is for cases like where the
driver/net init might race with iscsi init or for cases where the
network is down. To balance the needs of different callers, we want to
be able to allow iscsid to retry but we do not want to indefinitely
block iscsiadm calls that might be probing or during init time.

> The code in open-iscsi that does this retry loop is in uip_broadcast() in iscsid_req.c:
>
> ...
> +#define MAX_UIP_BROADCAST_READ_TRIES 3
> + for (count = 0; count < MAX_UIP_BROADCAST_READ_TRIES; count++) {
> + /* Wait for the response */
> + err = read(fd, &rsp, sizeof(rsp));
> + if (err == sizeof(rsp)) {
> + log_debug(3, "Broadcasted to uIP with length: %ld "
> + "cmd: 0x%x rsp: 0x%x\n", buf_len,
> + rsp.command, rsp.err);
> + err = 0;
> + break;
> + } else if ((err == -1) && (errno == EAGAIN)) {
> + usleep(250000);
> + continue;
> + } else {
> + log_error("Could not read response (%d/%d), daemon "
> + "died?", err, errno);
> + err = ISCSI_ERR;
> + break;
> + }
> + }
> ...
>
> This seems to have been added last year when Eddie sent you a patch (see commit d571cdfc0b3c539310f43dc11d54d9308299efdc). That commit was quite extensive, introducing the EAGAIN error to iscsi processing.
>

This is for when iscsiuio and iscsid init race. To try and detect that
race vs cases where iscsiio never comes we have limited retries there,
so we retry but do not block forever.

I think the code you are interested in is a little lower in that code
(the ISCSID_UIP_MGMT_IPC_DEVICE_UP chunk) in session_conn_uio_poll ->
iscsi_set_net_config. Are you saying below that the retries you have
been talking about are the same ones that for iscsid
session_conn_uio_poll handles? If so I misunderstood you the first time.
Your original program description made me think you were talking about a
different issue.

[Mike Christie]

Thanks. I merged and pushed these patches.

When you get some time, reply to that mail I sent offlist about how long
it is taking for the set host net params to succeed, so I can fix my
login/init patch as needed.

On 12/11/2014 01:53 PM, The Lee-Man wrote:

> --
> 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+...@googlegroups.com
> <mailto:open-iscsi+...@googlegroups.com>.
> To post to this group, send email to open-...@googlegroups.com
> <mailto:open-...@googlegroups.com>.
> Visit this group at http://groups.google.com/group/open-iscsi.
> For more options, visit https://groups.google.com/d/optout.

[Lee Duncan]

Hi Mike:

I am *finally* getting to this bug, previously buried under a mountain of more pressing matters.

> On Jan 12, 2015, at 12:50 PM, Mike Christie <mich...@cs.wisc.edu> wrote:
>
> Thanks. I merged and pushed these patches.
>
> When you get some time, reply to that mail I sent offlist about how long
> it is taking for the set host net params to succeed, so I can fix my
> login/init patch as needed.

I have looked at this problem again, and I will reply in a new email tomorrow. Fair warning. :)

> You received this message because you are subscribed to a topic in the Google Groups "open-iscsi" group.
> To unsubscribe from this topic, visit https://groups.google.com/d/topic/open-iscsi/PWbP_a2HR-g/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to open-iscsi+...@googlegroups.com.
> To post to this group, send email to open-...@googlegroups.com.

> Visit this group at http://groups.google.com/group/open-iscsi.
> For more options, visit https://groups.google.com/d/optout.

--
Lee Duncan

Love is a snowmobile racing across the tundra and then suddenly it flips over, pinning you underneath. At night, the ice weasels come. – Matt Groening

[Mike Christie]

On 03/09/2015 08:54 PM, Lee Duncan wrote:
> Hi Mike:
>
> I am *finally* getting to this bug, previously buried under a mountain of more pressing matters.
>
>> On Jan 12, 2015, at 12:50 PM, Mike Christie <mich...@cs.wisc.edu> wrote:
>>
>> Thanks. I merged and pushed these patches.
>>
>> When you get some time, reply to that mail I sent offlist about how long
>> it is taking for the set host net params to succeed, so I can fix my
>> login/init patch as needed.
>
> I have looked at this problem again, and I will reply in a new email tomorrow. Fair warning. :)

:)

Hey,

Attached is the patch that was modifying iscsid's code path which from
what I understand handled the same problem you were handling in the
iscsiadm's discovery code path.

The patch is removing the iscsiuio specific calls in iscsid and using
the generic initial login/connect retry code. It is currently retrying
for up to login_timeout seconds instead of the hard coded 5 seconds.

[Mike Christie]

On 03/10/2015 11:52 AM, wei, xiaoyan wrote:
> Hi Mike,
>
> I sent out an email regarding iscsiadm's discovery problem (attached).
> Looks like Lee is referring the same problem and the patch will fix it. Could you please confirm?
>

My patch only cleans up the code that handles the problem when doing
normal session login. I cleaned it up when fixing another bug in that
code, and was only bygging Lee about the issue because I was looking for
more info on how to hit it so I could test my patch. Lee - someone at RH
ended up having a system btw.

Ideally, we would want to have these type of issues fixed in one place.
Due to how iscsiadm does not use iscsid though, I do not think that is
possible though.


> Thanks!

> --
> 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+...@googlegroups.com.

[wei, xiaoyan]

Hi,

 

Open-iscsi README specified a bug in iscsiadm discovery:

“Be aware that iscsiadm will use the default route to do discovery. It will

not use the iface specified. So if you are using a offload card, you will need a separate network connection to the target for discovery purposes.

*This will be fixed in the next version of open-iscsi*”

 

Is there already a fix for this?  When the fix is planned to be released?

 

Thanks.

[The Lee-Man]

Hi Mike:

I verified this patch works. Feel free to add a "Tested-by:".

By "works", I mean that it eliminated the retry that I was seeing when

connecting the first time to the iscsiuio daemon.

[The Lee-Man]

Oops. I almost forgot. The patch did not apply cleanly because of a recent
update you did. Please make sure I modified it correctly (attached).