Page MenuHomeFreeBSD

Make the iSCSI parameter negotiation more flexible.
ClosedPublic

Authored by np on Jul 22 2016, 1:33 AM.

Details

Summary

Decouple the send and receive limits on the amount of data in a single
iSCSI PDU. MaxRecvDataSegmentLength is declarative, not negotiated, and
is direction-specific so there is no reason for both ends to limit
themselves to the same min(initiator, target) value in both directions.

Allow iSCSI drivers to report their send, receive, first burst, and max
burst limits explicitly instead of using hardcoded values or trying to
derive all of them from the receive limit (which was the only limit
reported by the driver prior to this change).

Display the send and receive limits separately in the userspace iSCSI
utilities.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

np retitled this revision from to Make the iSCSI parameter negotiation more flexible..
np updated this object.
np edited the test plan for this revision. (Show Details)
np added reviewers: trasz, mav, jpaetzel, ken.
jpaetzel edited edge metadata.
This revision is now accepted and ready to land.Jul 22 2016, 6:35 PM

Thanks Josh. This touches a lot of files so I'll wait for at least 1 more review. (And I'm still not done testing all the changes).

np edited edge metadata.

Fix the send_dsl calculation in ctld.

This revision now requires review to proceed.Jul 22 2016, 11:22 PM
sys/dev/iscsi/icl.c
214 ↗(On Diff #18695)

Shouldn't this be a KASSERT? I mean, from what I understand it's the kernel code that returns those; it can be expected to provide valid ones.

sys/dev/iscsi/icl.h
133 ↗(On Diff #18695)

The prefixes are missing. Also, the code generally uses size_t for those. And some spares perhaps?

sys/dev/iscsi/iscsi_ioctl.h
85 ↗(On Diff #18695)

Again, why not size_t?

found a typo

usr.sbin/ctld/login.c
772 ↗(On Diff #18695)

s/iLLegal/illegal/

sys/dev/iscsi/icl.c
214 ↗(On Diff #18695)

I'm going to change cxgbei in the near future to report different limits based on the type of card in the system, and maybe even have user-tunable knobs (in cxgbei, not global) to have it report different limits.

sys/dev/iscsi/icl.h
133 ↗(On Diff #18695)

I'll add the prefix in the next update to this review.

The existing code isn't very consistent about using size_t for these parameters. For example, struct iscsi_session_state uses int and not size_t. Also, I was hoping to get this into 11 and the spare fields available were all ints, so ...

All these values are required by the RFC to be 0 or between 512 and 2^24 - 1. So it is guaranteed that they'll fit in an int. I'd actually like to change them to int all over to compact all the data structures. If that's ok I'd also like to rename foo_data_segment_length variables to foo_dsl during my sweep.

sys/dev/iscsi/iscsi_ioctl.h
85 ↗(On Diff #18695)

Takes twice the size on amd64. See reply to previous inline comment too.

usr.sbin/ctld/login.c
772 ↗(On Diff #18695)

Good catch, thanks. I'll fix it in the next iteration of the patch.

sys/dev/iscsi/icl.c
214 ↗(On Diff #18695)

Okay, but my point is: can't cxgbei, and other ICL modules, be trusted to not return invalid values to the stack?

sys/dev/iscsi/icl.h
133 ↗(On Diff #18695)

Ah, ok then - cleaning up the types is always welcome :-) Make sure you don't miss any checks. As for renaming - I'm not a huge fan of shortening stuff this way, to be honest.

np edited edge metadata.
  • Added prefix to members of icl_drv_limits.
  • Change all first_burst_length, max_burst_length, and *_data_segment_length to int. I've tried to change the structures used in ioctl in an MFC-friendly way. Extra code to cope with the layout changes will have to be written if this can actually be MFC'd to 11.

Build fixes caught by make tinderbox.

Another build fix for platforms that use gcc.

sys/dev/iscsi/icl.c
214 ↗(On Diff #18695)

If a driver makes these limits user-tunable within some bounds (I'm thinking of doing this eventually in cxgbei) then it makes sense to catch illegal values. The range checks could be in the driver(s) or in one central place for all. Putting them in one place is better imho.

trasz edited edge metadata.

Looks good, thanks for working on this :-)

This revision is now accepted and ready to land.Aug 23 2016, 9:42 AM
This revision was automatically updated to reflect the committed changes.
np marked an inline comment as done.