Hi Christoph, This generally looks good, two quick comments: - I'd prefer that the comparison to RFC 5663 (existing pNFS Block/Volume layout) remain. - Yes, making 4k alignment a MUST would be a fine thing to do now. Thanks, --David > -----Original Message----- > From: Christoph Hellwig [mailto:hch@xxxxxx] > Sent: Monday, July 18, 2016 12:35 AM > To: Black, David > Cc: ietf@xxxxxxxx; draft-ietf-nfsv4-scsi-layout@xxxxxxxx; > spencerdawkins.ietf@xxxxxxxxx; nfsv4@xxxxxxxx; nfsv4-chairs@xxxxxxxx > Subject: Re: [nfsv4] Last Call: <draft-ietf-nfsv4-scsi-layout-06.txt> (Parallel NFS > (pNFS) SCSI Layout) to Proposed Standard > > Hi David, > > thanks for the updates. Let's talk about the detail tomorrow in Berlin, > but unless Spencer disagrees I'll prepare a new draft once the meeting > is over. > > On Sun, Jul 10, 2016 at 06:51:40PM +0000, Black, David wrote: > > -- Introduction, last paragraph. > > > > Add a sentence saying that there are no other significant differences from RFC > 5663 > > (previous sentences indicate use of SCSI for fencing and LAYOUTCOMMIT > improvements), > > e.g., the volume topology (Section 2.3.2) and data structures that describe > extents > > (Section 2.3.3.) are common with RFC 5663. Those two examples seem > important. > > Just scrapping the sentence might be best. > > > > > -- Section 2.1, 1st paragraph > > " and the SCSI initiators used for the pNFS Metadata Server and clients > MUST > > support SCSI persistent reservations." > > Add a citation of [SPC4] to support that MUST requirement. > > Ok. > > > > -- Section 2.1, 2nd paragraph: > > > > Clients MUST be able to perform I/O to > > the block extents without affecting additional areas of storage > > (especially important for writes); therefore, extents MUST be aligned > > to 512-byte boundaries. > > > > That assumes a 512 byte logical block size, which is generally ok for now, but 4k > is coming. > > At a minimum, "extents MUST be aligned to logical block size boundaries of the > logical > > units, e.g., 512 bytes." OTOH, would it be reasonable to just make 4k alignment > a MUST > > now, as there will be storage systems that need 4k alignment and 4k alignment > generally > > works better than 512-byte alignment with existing systems? > > I think the right thing is to simply scrap the 512 byte example and just > require extents to be aligned to at least the logical block size. > > All implementations of the block and scsi layout known to me align to 4k or > larger, but I see no fundamental reason to forbid 512 byte alignment. > > > > > -- Section 2.3.1 > > > > It is similar to the "Identification > > Descriptor Target Descriptor" specified in [SPC4], but limits the > > allowed values to those that uniquely identify a LU. > > > > I suggest just deleting this sentence, as that is now called an "Identification > CSCD Descriptor" - > > if this sentence is retained, the use of those descriptors in EXTENDED COPY > would be > > important to mention - that seems like a diversion. > > Ok. > > > 2. The "DESIGNATOR TYPE" MUST be set to one of four values > > > > T10 is now allowing UUIDs to also be used in the working draft of SPC-5, and > those would be > > appropriate here. Nonetheless, I suggest no change until SPC-5 is completed at > T10, as this > > draft is (properly, IMHO) based on SPC-4. > > Yes, I would love to be able to support UUIDs, but I see no way to allow > for that until SPC-5 has been completed. > > > -- Section 2.4.10.3 > > > > To make sure all I_T nexuses are registered, > > the client SHOULD set the "All Target Ports" (ALL_TG_PT) bit when > > registering the key, or otherwise ensure the registration is > > performed for each initiator port. > > > > It looks like initiator and target registration scopes were conflated here and > > need to be separated. Suggested text: > > > > To make sure all I_T nexuses are registered, > > the client SHOULD set the "All Target Ports" (ALL_TG_PT) bit when > > registering the key, or otherwise ensure the registration is > > performed for each target port, and MUST perform registration > > for each initiator port. > > Thanks, this will need an update. > > > > > -- References > > > > Current version of SAM is SAM-5, consider updating SAM-4 reference to SAM- > 5. > > Ok.