[Stgt-devel] [PATCH] iscsi fix xmit param

[FUJITA Tomonori]

From: Pete Wyckoff <pw at osc.edu>
Subject: Re: [Stgt-devel] [PATCH] iscsi fix xmit param
Date: Sat, 18 Aug 2007 11:59:06 -0400

> tomof at acm.org wrote on Sun, 19 Aug 2007 00:14 +0900:
> > From: Pete Wyckoff <pw at osc.edu>
> > Subject: Re: [Stgt-devel] [PATCH] iscsi fix xmit param
> > Date: Sat, 18 Aug 2007 10:52:00 -0400
> > 
> > > tomof at acm.org wrote on Sat, 18 Aug 2007 14:10 +0900:
> > > > From: Pete Wyckoff <pw at osc.edu>
> > > > Subject: Re: [Stgt-devel] [PATCH] iscsi fix xmit param
> > > > Date: Wed, 15 Aug 2007 11:15:59 -0500
> > > >
> > > > > True.  There are a few cases.
> > > > > 
> > > > > 1.  ini sends mrdsl > target mxdsl.  Apply minimum, use target max.
> > > > > 2.  ini sends mrdsl < target mxdsl.  Apply minimum, use ini max.
> > > > > 3.  ini does not mention mrdsl.  Target should set its mxdsl to the
> > > > >     standard default (8192), not use its default max.
> > > > > 
> > > > > This last case was broken before my earlier patch "iscsi param
> > > > > cleanup".  Now it is fixed but I changed the target default to 8k.
> > > > > I want the target to agree to use a larger value if the initiator is
> > > > > willing, but still get case #3 correct if the initiator does not
> > > > > specify.
> > > > 
> > > > Really?
> > > > 
> > > > I think that if ini does not mention mrdsl and target doesn't use the
> > > > standard default mxdsl value (a target admin sets non default value),
> > > > text_check_param() sets xmdsl to the default value.
> > > 
> > > It really was broken.  But this is an odd case as open-iscsi at
> > > least always sends mrdsl, even if it is set to use the default.
> > > 
> > > If you roll back to 92d767a711ed82791b171ce7c9b722263076c519 or so,
> > > just before "iscsi param cleanup" you'll see:
> > > 
> > > iscsi/param.c:   mxdsl = 262144
> > > iscsi/target.c:  mxdsl = 8192
> > > 
> > > and this code in iscsi/iscsid.c:
> > > 
> > > 	if (p[i].state == KEY_STATE_START && p[i].val != session_keys[i].def) {
> > > 		if (conn->state == STATE_LOGIN) {
> > > 			if (i == ISCSI_PARAM_MAX_XMIT_DLENGTH) {
> > > 				if (p[i].val > session_keys[i].def)
> > > 					p[i].val = session_keys[i].def;
> > > 				p[i].state = KEY_STATE_DONE;
> > > 				continue;
> > > 			}
> > > 			memset(buf, 0, sizeof(buf));
> > > 			param_val_to_str(session_keys, i, p[i].val,
> > > 					 buf);
> > > 			text_key_add(conn, session_keys[i].name, buf);
> > > 			p[i].state = KEY_STATE_REQUEST;
> > > 		}
> > > 		cnt++;
> > > 	}
> > > 
> > > (p[] is conn->session_param, which is initialized from target params
> > > then modified by negotiation.)
> > > 
> > > If target admin does not change target mxdsl, and initiator does not
> > > send mrdsl, this code will see 8192 != 262144 and use 8192.  Fine,
> > > that's the default.
> > > 
> > > If target admin changes target mxdsl to 16384 using tgtadm, and
> > > initiator does not send mrdsl, this code will see 16384 != 262144
> > > and use 16384 without negotiation.  Bad, should use the default.
> > 
> > session_keys[i].def doesn't change (it is always the iSCSI default
> > value).
> 
> Understood, I wasn't assuming that.  In fact, I toyed with adding
> lots of "const" to enforce this, but it causes a big diff for not a
> lot of benefit.

Yeah, it's too ugly and complicated. Please no cleanups or
improvement. :) I'm happy as long as it works.