Subject: [nfsv4] draft-hellwig-nfsv4-scsi-layout-00.txt

From: Tom Haynes <[email-masked]>
Date: Mon, 9 Mar 2015 16:22:14 -0400

Hi Christoph,

1)
In looking at this draft, the first question that jumps out is why is this “Informational” and not “Proposed Standard”?

I.e.,

   This
   document extends the pNFS Block/Volume Layout [RFC5663] with a closer
   integration into the the SCSI Architecture Model ([SAM-4]) to provide
   a generic fencing method and more scalable device discovery.

So extends means it should be the same level of document. And this is a new Layout Type
which you want to be a standard.

2) The year should be 2015 here:
      /// /*
      ///  * This code was derived from draft-hellwig-nfsv4-scsi-layout
      ///  * Please reproduce this note if possible.
      ///  */
      /// /*
      ///  * Copyright (c) 2010 IETF Trust and the persons identified
      ///  * as the document authors.  All rights reserved.
      ///  *

3) For this, what is supposed to occur if it is referenced?

A PNFS_BLOCK_VOLUME_SIMPLE element MUST
   NOT be referenced by a pnfs_scsi_deviceaddr4, but is preserved for
   XDR level compatibility.

4) And to be clear, you have had an external review of the SCSI usage? By David and Robert?

Thanks,
Tom

---

From: Christoph Hellwig <[email-masked]>
Date: Mon, 9 Mar 2015 21:30:20 +0100

> In looking at this draft, the first question that jumps out is why is this “Informational” and not “Proposed Standard”?

It should.  I just haven't figured out a way how to submit a draft
as proposed standard..

>       ///  * as the document authors.  All rights reserved.

Ok.

>    NOT be referenced by a pnfs_scsi_deviceaddr4, but is preserved for

The client should treat it like any other unallocated value.  It is
only mentioned here to explain that the numerial value is taken in the
bock layout xdr, which is extended by this rfc.

> 4) And to be clear, you have had an external review of the SCSI usage? By David and Robert?

Yes.

---

From: "Black, David" <[email-masked]>
Date: Tue, 10 Mar 2015 00:56:17 +0000

> > 4) And to be clear, you have had an external review of the SCSI usage? By

That was a while ago - I looked at the draft before Christoph submitted it.
Looking back at the email then, I think I wanted to see more details on the
use of SCSI Persistent Reservations.  I should go look at the posted version
with this and my previous comments to Christoph in mind.

Thanks,
--David

> -----Original Message-----
> From: nfsv4 [mailto:[email-masked]] On Behalf Of Christoph Hellwig
> Sent: Monday, March 09, 2015 4:30 PM
> To: Tom Haynes
> Cc: NFSv4
> Subject: Re: [nfsv4] draft-hellwig-nfsv4-scsi-layout-00.txt
> 
> > In looking at this draft, the first question that jumps out is why is this
> “Informational” and not “Proposed Standard”?
> 
> _______________________________________________
> nfsv4 mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/nfsv4

---

From: [email-masked] (J. Bruce Fields)
Date: Mon, 16 Mar 2015 20:30:48 -0400

> with this and my previous comments to Christoph in mind.

Did you get a chance to look at this yet?

The problems Christoph describes here really are annoying for any block
pnfs implementation, so I'm anxious to see them resolved.

I'm not a scsi expert and haven't tried to read the relevant specs, so
I'm assuming that this is indeed a sensible way to address and fence
devices, and that any scsi protocol features required are sufficiently
ubiquitous to be useful.

Other than that my comments are pretty minor.  For totally trivial
spelling and other stuff I've also appended a diff against
git://git.infradead.org/users/hch/scsi-layout.git.

Remove section 1.3: if somebody really needs these definitions, the
right place would be 5661 or 5663.

"The layout4 type is defined in RFC5662 as follows".  That's not exactly
true.  Might be clearer to say: "the layout4 type defined in [RFC5662]
is extended with a new value as follows:"  (Same comment applies to
http://tools.ietf.org/html/draft-bhalevy-nfsv4-flex-files-03.)

Looks like both sbv_lu_name and svb_designator are used to refer to the
same field.

Does sbv_lu_name<> have some maximum size?

"revoke extents from one client in order to transfer the extents to
another client".  This language is a cut-and-paste from 5563.  (And not
entirely accurate--there's othe reasons to recall a layout.)  If only
for attribution reason, make that an explicit quote.

But actually I would consider just cutting the first couple sentences
and say:

	[RFC 5663] suggests fencing clients using either LUN masking or
	cooperative clients....

"common notation of a client" doesn't sound right.  How about just
"common address for a client"?  Or "common name for a client"?

"To allow fencing individual systems, each system MUST use a unique
Persistent Reservation key."  I think that MUST should be lower case.
You're describing a requirement on the protocol not really a requirement
for any specific client or server behavior yet.

"After a successful GETDEVICEINFO operation the client MUST register the
registration key returned in sbv_pr_key with the storage device..." Must
it really?  Couldn't it change its mind and decide it doesn't want to
use that device or layout after all?  And in any case, if it skips the
registration, the IO will just fail, so I don't know that we really need
to make an explicit requirement here.  As a description of typical use
it's still helpful, so maybe word it something like: "Before performing
the first IO to a device returned from a GETDEVICEINFO operation the
client will register the registration key...".

But if I understand right the MUST for unregistering is important as
otherwise the registration just sits around until there's a conflict and
the server may have to unnecessarily delay conflicting IO, etc.

I assume this applies only if the client previously registered the key.
So maybe: "When a client stops using a device it previously registered,
it MUST..."  Also: "stops using" is a little vague.

"A client that detects I/O errors".... Is that any I/O error or just I/O
errors due to fencing?

"all outstanding layouts": why not just affected layouts?

"forget any open devices": again, what's scope of "any"?  What does
"forget" mean?  (Should it try to unregister?)

"by a future LAYOUTGET": should that be: by a future GETDEVICEINFO?  Or
by a future GETDEVICEINFO for this device?  Do we also need to instruct
the MDS to generate a new reservation key?

--b.

diff --git a/scsi_middle_introduction.xml b/scsi_middle_introduction.xml
index 09ff224ef731..3b8d5d376f66 100644
--- a/scsi_middle_introduction.xml
+++ b/scsi_middle_introduction.xml
@@ -24,7 +24,7 @@
       structures for the LAYOUTGET and LAYOUTCOMMIT operations.
 
       This document does not directly interact with <xref target='RFC6688' />,
-      although the mechanisms described in this document also archive the
+      although the mechanisms described in this document also achieve the
       goals of <xref target='RFC6688' />, and do so in a more robust fashion
       that does not depend on the cooperation of the systems involved. Thus,
       the mechanisms specified in <xref target='RFC6688' /> are not necessary
diff --git a/scsi_middle_layout.xml b/scsi_middle_layout.xml
index 8501f231762a..330d73160480 100644
--- a/scsi_middle_layout.xml
+++ b/scsi_middle_layout.xml
@@ -117,8 +117,7 @@
 	field does not need to be set separately.  Only certain combinations
 	of "sbv_code_set" and "sbv_designator_type" are valid, please refer to
 	<xref target="SPC3" /> for details, and note that ASCII may be used
-	as the code set for UTF-8 text that does not contain non-ASCII
-	characters.
+	as the code set for UTF-8 text that contains only ASCII characters.
 
 	Note that a Device Identification VPD page MAY contain multiple
 	descriptors with the same association, code set and designator type.
@@ -191,7 +190,7 @@
 
       <t>
         All rules for ordering and formation of a "pnfs_scsi_deviceaddr4"
-	structure are identical for those of a "pnfs_block_deviceaddr4"
+	structure are identical to those for a "pnfs_block_deviceaddr4"
 	structure in <xref target='RFC5663' />, except that the new
 	pnfs_scsi_base_volume_info4 PNFS_SCSI_VOLUME_BASE case is used in
 	place of the pnfs_block_simple_volume_info4 PNFS_BLOCK_VOLUME_SIMPLE
@@ -227,21 +226,21 @@
 	of type "Exclusive Access – All Registrants" on each SCSI logical unit
 	exported to pNFS clients the MDS prevents access from any client that
 	does not have an outstanding device device ID that gives the client
-	a reservation key to access to logical unit, and allows the MDS to
+	a reservation key to access the logical unit, and allows the MDS to
 	revoke access to the logic unit at any time.
       </t>
 
       <section anchor='ssc:fencing:keys'
 		title='PRs - Key Generation'>
       <t>
-	To allow fencing individual systems, each systems MUST use a unique
+	To allow fencing individual systems, each system MUST use a unique
 	Persistent Reservation key.  <xref target="SPC3" /> does not specify
 	a way to generate keys.  This document assigns the burden to generate
 	unique keys to the MDS, which MUST generate a key for itself before
-	exporting a volume, and one for each client that access a volume.
+	exporting a volume, and one for each client that accesses a volume.
 	The MDS MAY either generate a key for each client that accesses
-	logic units exported by the MDS, or to generate a key for each
-	[logical unit, client] combination.  In case of a single key per
+	logic units exported by the MDS, or generate a key for each
+	[logical unit, client] combination.  If using a single key per
 	client, the MDS needs to be aware of the per-client fencing
 	granularity.
       </t>
@@ -257,7 +256,7 @@
 	action of "REGISTER", followed by a "PERSISTENT RESERVE OUT" command,
 	with a service action of "RESERVE" and the type field set to 8h
 	(Exclusive Access – All Registrants).
-	To make sure all I_T nexus are registered, the MDS SHOULD set the
+	To make sure all I_T nexuses are registered, the MDS 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.
       </t>
@@ -290,16 +289,16 @@
 	  In case of a non-responding client the MDS MUST fence the client
 	  by issuing a "PERSISTENT RESERVE OUT" command with the service
 	  action set to "PREEMPT" or "PREEMPT AND ABORT", the reservation key
-	  field set to the server's reservation key, and the service action
+	  field set to the server's reservation key, the service action
 	  reservation key field set to the reservation key associated with
 	  the non-responding client, and the type field set to 8h (Exclusive
 	  Access – All Registrants).
         </t>
 
 	<t>
-	  After the MDS preempted a client, all client I/O to the logical unit
-	  will fail.  The client should at this point return any layout that
-	  refers to the device ID that poіnts to the logical unit.  Note that
+	  After the MDS preempts a client, all client I/O to the logical unit
+	  fails.  The client should at this point return any layout that
+	  refers to the device ID that points to the logical unit.  Note that
 	  the client can distinguish I/O errors due to fencing from other
 	  errors based on the "RESERVATION CONFLICT" status.  Refer to
 	  <xref target="SPC3" /> for details.

---

From: Christoph Hellwig <[email-masked]>
Date: Wed, 18 Mar 2015 12:08:29 +0100

> Does sbv_lu_name<> have some maximum size?

sbv_lu_name was renamed to svb_designator, but it looks like the XDR
didn't keep up with that change.

It does not have a theoretical limit, but the SCSI transfer length
into which it needs to fit has a practical limit that varies from setup to
setup.

> "common address for a client"?  Or "common name for a client"?

Both of them sound fine to me.

> for any specific client or server behavior yet.

The server could just hand out the same key to clients and the reservation
would just work, just the fencing would now affect multiple clients.  This
makes me assume it should be an uppercase MUST, but I'm happily overruled
by anyone with more experience than me.

> it MUST..."  Also: "stops using" is a little vague.

It's mostly about releasing the resources on the storage device, but
I agree with your wording choices.

> "A client that detects I/O errors".... Is that any I/O error or just I/O

Any I/O error.

> "all outstanding layouts": why not just affected layouts?

Which in general should be identical.  I'm pretty sure I copied both
above sentences from somewhere (probably 5663).

Yes.

---

From: "J. Bruce Fields" <[email-masked]>
Date: Wed, 18 Mar 2015 13:23:08 -0400

> by anyone with more experience than me.

I'm probably overthinking it but: who exactly (clients or servers) are
you asking to do something here, and what exactly are you asking them to
do?  The answer's two sentences down ("This document..."), and I like
the shouty MUSTs there.  Here you're just explaining the reason behind
the requirement.

But I don't feel that strongly about it.

--b.

---

From: Christoph Hellwig <[email-masked]>
Date: Fri, 20 Mar 2015 07:42:36 +0100

I've pushed out an update that should address your concerns.  A few
comments where I wrote it differently than you suggested:

> it MUST..."  Also: "stops using" is a little vague.

I can't really come up with anything better, as the device id rules
in RFC5661 are so vague.  Stop using could be a NOTIFY_DEVICE, but
most commonly it will just be an unmount.  But it could also just be
the client throwing it out of the device id cache if it reclaims unused
entries in it.

> "A client that detects I/O errors".... Is that any I/O error or just I/O

While this section is about fencing in general only the MDS has a chance
of dealing with I/O errors, so I think this wording should stay as is.

> "all outstanding layouts": why not just affected layouts?

all outstanding layouts for the device

> the MDS to generate a new reservation key?

I've rewritten this section entirely, please check if the new version
is easier to understand.

---

From: "J. Bruce Fields" <[email-masked]>
Date: Fri, 20 Mar 2015 10:22:47 -0400

> comments where I wrote it differently than you suggested:

Thanks!  I just noticed a few new typos, appended.

> On Mon, Mar 16, 2015 at 08:30:48PM -0400, J. Bruce Fields wrote:
...
> I've rewritten this section entirely, please check if the new version

Looks good to me.

--b.

diff --git a/scsi_middle_layout.xml b/scsi_middle_layout.xml
index 9fef85d..ce60a6a 100644
--- a/scsi_middle_layout.xml
+++ b/scsi_middle_layout.xml
@@ -73,7 +73,7 @@
  /// struct pnfs_scsi_base_volume_info4 {
  ///     pnfs_scsi_code_set             sbv_code_set;
  ///     pnfs_scsi_designator_type      sbv_designator_type;
- ///     opaque                         svb_designator&lt;&gt;;  
+ ///     opaque                         sbv_designator&lt;&gt;;
  ///     uint32_t                       sbv_pr_key;
  /// };
  ///
@@ -204,9 +204,9 @@
 
   <section anchor='ssc:fencing' title='Client Fencing'>
       <t>
-        <xref target="RFC5663" /> suggest to either use LUN masking or
+        <xref target="RFC5663" /> suggests using either LUN masking or
 	cooperative clients to implement client fencing.  The first
-	implementation requires th server and the storage device to have
+	implementation requires the server and the storage device to have
 	a common way to address a client, which is impossible when the NFS
 	and storage connection don't share a network, and requires a
 	non-standardized control protocol between the MDS and the storage
@@ -307,7 +307,7 @@
 	  commit through the MDS, return all outstanding layouts for the
 	  device, forget the device ID and unregister the reservation key.
 	  Future GETDEVICEINFO calls may refer to the storage device
-	  again, in which case a new registration will be perform.
+	  again, in which case a new registration will be performed.
         </t>
       </section>
     </section>