Re: [Scst-devel] scst pr patch

[Alexey O. <ale...@op...>]

Vladislav Bolkhovitin wrote:

Thank you for review code.
> Hello Alexey,
>
> In future, please discuss with me if you don't understand anything, 
> not try to make quietly something, which can be incorrect.
>
> In iSCSI each I_T nexus identified not by initiator name only, but by 
> "iSCSI Initiator Name + ',i,'+ ISID" tuple. So, all your code trying 
> to get initiator name from TransportID is incorrect, hence should be 
> removed, and FORMAT CODE 01b must be supported.
This is true. I agree that code must handle both formats. But how what 
to do with different session id values that saved in file or in case of 
ression reconnect. For each connection from same initiator we got 
different sid.
>
> So, why what I suggested is insufficient? The following is a slightly 
> improved version of it:
>
> 1. Only single callback get_initiator_port_transport_id() should be 
> added in scst_tgt_template. This callback will return TransportID of 
> the I_T Nexus, i.e. with FORMAT CODE 01b for iSCSI. (BTW, cmd has 
> direct pointer to the target template cmd->tgtt.)
If we try to make PR code indifferent to transport id, we need somehow 
handle it (parse/save/load). I see no reason have opaque structure for 
different transport id's and bunch of code to handle them in PR code. 
For current scst realization initiator name is enough to identify 
initiator - why to add another structure without hiding operation with it.
>
> 2. Add in struct scst_dev_registrant new field uint8_t *transport_id 
> and use it to keep TransportID of this I_T Nexus, returned by 
> get_initiator_port_transport_id().
In current realization of PR we need to tie tuple - tgt_dev and 
registered initiator, so we can do time to time allocation/free of 
initiator port transport id structure or preallocate it in tgt_dev and 
in registrant.
>
> 3. Then the common PRs code should work with TransportIDs in binary 
> form without any attempt to look inside, except (4).
>
> 4. Since all TransportIDs for all transports with FORMAT CODE 0 have 
> the same format, the common PRs code can, if necessary, convert from 
> FORMAT CODE 1 to FORMAT CODE 0 to satisfy requirements of PERSISTENT 
> RESERVE OUT ("If a iSCSI TransportID with the format code set to 00b 
> appears in a PERSISTENT RESERVE OUT parameter list [...], all 
> initiator ports known to the device server with an iSCSI node name 
> matching the one in the TransportID shall be registered"). Then you 
> can compare also in a binary form the converted TransportID with one 
> PERSISTENT RESERVE OUT received.
>
> 5. The "initiator" pointer of struct scst_dev_registrant will be used
> only for logging and errors reporting purposes.
I try to minimize number of structures and try to reuse initiator port 
transport id. If you think this is not good decision I return initiator 
name for this purpose.
>
> Also:
>
> 1. You forgot to delete pr_local from struct scst_session.
>
> 2. I think, all scst_pr_*() functions called from 
> scst_persistent_reserve_out_local() and 
> scst_persistent_reserve_in_local() should have void return type and in 
> case of error set status/sense of cmd.
>
> 3. I think we can better handle write errors in 
> scst_pr_sync_device_file(). I'd suggest:
>
>  - Add in the beginning of /var/cache/scst/dev_name new field
>
> uint64_t signature.
>
> It will validate the file. If the file is valid, it should contain 
> "0xBBEEEEAAEEBBDD77" (quite random constant).
>
>  - In scst_pr_sync_device_file() start from copying the current 
> content of /var/cache/scst/dev_name to /var/cache/scst/dev_name.1.
>
>  - Do the writing in /var/cache/scst/dev_name. Write 0 in the 
> signature field.
>
>  - On success, write the correct signature in 
> /var/cache/scst/dev_name. On failure, restore previous state of PRs 
> from /var/cache/scst/dev_name.1 and return error to the initiator.
>
> In scst_pr_load_device_file() called from scst_pr_init_tgt_dev() at 
> first check if the signature field is valid. If it isn't, then try the 
> backup file /var/cache/scst/dev_name.1.
>
> I later will add necessary write barriers to convert it to a fully 
> journaling updates. This way by very small effort we will be protected 
> from events like power failure while the file is being written.
>
> 4. Still PRs capabilities not set and, hence, reported by 
> scst_pr_report_caps() correctly. Also, pr_generation field is still 
> unsigned int and other comments from 11/18 ignored. Have you missed them?
>
> Please find my minor changes in the attachment.
>
> Thanks,
> Vlad
>
> Alexey Obitotskiy, on 12/11/2009 12:52 PM wrote:
>> Hello,
>>
>> In attachment patch with PR realization. Patch is against 1383 revision.
>>
>> Alexey
>
Also I want to tell that PR code have no integration with previously 
realized SPC-2 RESERVE/RELEASE commands. Do you have some 
suggestions/comments for this ?

Alexey