Joe Eykholt, on 02/13/2010 01:30 AM wrote: >>> 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.
Agree. This can be done in future and shouldn't need any major changes in the driver or SCST core. On the rdy_to_xfer() time there's already alloced s/g list for the command, so the only thing will be needed is to ask HBA to use it. >> - 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. That's bad. I'll fix it. If you need additional accessors, just prepare a patch with them. The same is for ft_find_cmd(). Sometimes I miss in C a facility to mark fields in structures as private or protected. For performance reasons it isn't good to completely hide those structures inside SCST's internal headers, but that means that everybody, including me in the first time, can accidentally touch in the structures what shouldn't be touched. It's better for target drivers to use accessor functions to be more protected from possible future internal changes inside SCST core. >> - 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. As Bart correctly pointed out, SCST core does all the job to make sure all outstanding commands are correctly finished during sessions unregistration, so you only need to setup unreg_done_fn() callback and do what you need to finish session in it. >> 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? Yes, if ft_send_response() as well as ft_send_xfer_rdy() are not blocking, threads_num should be 0. Threads_num defines how many additional threads should be instantiated to workaround if those functions are blocking, hence minimize performance degradation caused by that. > 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. Yes, correct. It will be done either after some other command finished or after some time. > 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. I'm not sure what you mean. Basically, a target driver doesn't need to keep track of progress of any commands, it simply should on each event from SCST or hardware call the next function to switch to the next state of the processing state machine. On each state there is only one such function. The only area where there's a need to know commands processing states is errors recovery, when you inside driver need to figure out how to abort a command. For instance, it might be needed in on_hw_pending_cmd_timeout() callback. >> 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. Seems the best way for it should be something like: 1. Define a module parameter for the main (initiator) module, which would define if it starts acting as initiator from the very beginning just after load (default: yes). 2. Add a sysfs attribute "initiator_mode_enabled" for the main module, which would allow to enable it acting as initiator later after load. 3. Add a sysfs attribute "initiator_target_coexist" for the target module. If it's 0, the target module, when being enabled, would disable initiator mode. If it's 1, both initiator and target modes would coexist. >> 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. OK > 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. We can keep them as additional patches in fcst/patches for specific kernel versions as we currently do in trunk/scst/kernel. For instance, we have readahead-context patch for 2.6.29 and for 2.6.30. In 2.6.31 it was merged, so we don't have it anymore for 2.6.31+. Vlad _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
