Vladislav Bolkhovitin wrote: > Joe Eykholt, on 02/12/2010 04:07 AM wrote: >> This patch adds a new target module for SCST and libfc >> that accepts FCP requests from libfc HBAs running Fibre Channel >> over Ethernet (FCoE) and passes them to SCST. I'm hoping that >> this can be included in the SCST tree eventually. > > It's very nice to see this driver!
Thanks! >> This is mostly to gather comments. It isn't at all well tested. >> It works with simple read/write tests with vdisk targets. >> It doesn't use any libfc offloads as of yet. Task management >> and error cases haven't been tested. >> >> It relies on a set of patches just submitted to open-fcoe.org >> to add hooks for target providers like this. >> >> It has only been used under the 2.6.33-rc4 kernel. >> >> The known issues are: >> * task management >> * could offload or allocate buffers to avoid copies > > Which copies do you mean? It probably isn't worth worrying about until we get further and start measuring performance. Generally on reads there's no data copy due to s/g lists. On write, we might also be able to avoid a copy by setting up an s/g list referring to the data in sk_buffs that have been received. Most HBAs will be able to offload data placement, though, where we point the HBA at the buffers before sending out the transfer-ready. >> * doesn't handle target timeouts. >> * completion handling not done (no tape support yet) >> * exchanges not held around for REC/SRR after response sent >> * SRR not tested >> * error handling could be better >> * need way of sending LOGO when disabling I/T nexus. >> * Asynchronous Event Notification (AEN) not handled. >> >> Management is similar to the qla2x00t target. I tested this >> with /sys only, but management uses the common SCST code, so >> /proc may work as well. > > /sys only is OK. /proc is going to die soon. > >> Signed-off-by: Joe Eykholt <[email protected]> > > After a brief review for the current state of development it looks very > good! I noticed only few issues: > > - ft_send_xfer_rdy() uses cmd->data_len, but should use > scst_cmd_get_bufflen(cmd) instead. Data_len is internal variable for > commands which need data buffer bigger than data to transfer. Example of > such commands is VERIFY commands, which, if BYTCHK set to zero, read > data from media, but not transfer them anywhere. OK, Thanks. I'll fix that. > - The driver in many cases directly accesses internal data of SCST > structures. This isn't an encouraged practice. I tried to use the interfaces I could find. Some of them I discovered after already accessing the fields directly. I'll go through it again. One area that does that is my debug code, ft_cmd_dump() and ft_cmd_tm_dump(). Would you like to put those (or something like them) in the main SCST code? Another field is scst_cmd->queue_type. I couldn't find an accessor for that and iscsi.c accesses it directly as well. > - When sessions are being closed I don't see the driver anywhere > refuses new coming commands or blocked them. Without it under high load > the closing can take too long. As soon as a session starts closing in ft_prlo(), it takes it out of the hash table, so further commands won't find the session and will be rejected. It doesn't do that cleanly. It should send a LOGO or PRLO, but for now just drops it. I'll work on that. Another issue there is that it calls scst_unregister_session() right away so there could be commands still using the session at that point. I have a note to do it later when I'm sure there are no more commands in progress. > Also, ft_scst_template.threads_num = 4 means that ft_send_response() is > blocking, i.e. doesn't return until all data are sent. Correct? I didn't determine how to set that. ft_send_response() isn't blocking. Does that mean I should set threads_num to 0? If send_response returns SCST_TGT_RES_QUEUE_FULL, I suppose it gets called later to send the rest of the data, right? I'll check to make sure that works. fcst keeps track of read/write progress in its own ft_cmd struct for now. Is there a standard way of doing that via scst_cmd? If not, that'd be nice. > Could you write a small README how to use this driver for people not > familiar with FCoE like me? I'd like to try it. Does this driver support > P2P mode? I'll write up a README. The driver relies on open-fcoe to do P2P mode, which does work with an old target, but I haven't tried it with the latest version. Basically, just set up an FCoE initiator (based on the latest fcoe-next.git plus the patches I submitted to open-fcoe.org on Feb. 11 (plus one fix described in my follow-up mail)) and load fcst. Then just configure scst like you would a qla2x00t target. The HBAs appear under /sys/kernel/scst_tgt/targets/fcst. If the remote initiator logs in before the target is configured and enabled for that initiator, it may need to be reset to "see" the target and probe the LUNs. I suppose there's a way to do this nicer ... by resetting the local port, perhaps, or re-registering with the name server. The remote initiator's scsi_transport_fc is not informed of the role change. Just like for other HBAs, I'd like to have a way of configuring whether target mode co-exists with initiator mode, or whether to make the port target-only. The libfc hooks would need to allow for that, too. > Would you like I commit it somewhere in trunk/fcoe? Or trunk/fcst? Then > I can grant you commit rights to it. We need the driver can be compiled > inside SCST SVN tree as a module. Don't worry how to compile it inside > kernel. We have script generate-kernel-patch, which takes care of it. I think trunk/fcst is a good place. Let me fix these issues and get it a little more complete before we check it in. What would we do about the dependency on header file changes in my open-fcoe patches? We could include the patch set in the SCST source with fcst for now. > Thanks, > Vlad You're very welcome! Cheers, Joe _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
