RE: [nfsv4] Last Call: <draft-ietf-nfsv4-scsi-layout-06.txt> (Parallel NFS (pNFS) SCSI Layout) to Proposed Standard

["Black, David" <[e-mail]>]

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:[e-mail]]
> Sent: Monday, July 18, 2016 12:35 AM
> To: Black, David
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> 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.