Subject: [nfsv4] DLB review of: draft-ietf-nfsv4-scsi-layout-02.txt

From: "Black, David" <[email-masked]>
Date: Tue, 22 Sep 2015 23:18:44 +0000

Christoph,

In Prague, I promised to review this draft, so here are some comments.
In general the draft is well-written, so this review focuses on the
SCSI aspects that are new to this draft.

One of the consequences of making this a self-contained draft is that
a section should be added to explain/summarize the differences between
the SCSI layout and the block-volume layout (which is independent of
SCSI).  The draft abstract and intro should contain some high-level
points of comparison/difference.

Also, somewhere early in the draft, it should be stated that SCSI
persistent reservations MUST be supported by all entities (clients,
servers, storage systems) that support pNFS SCSI layouts.

Section 2.3.1 - Nit - say that the Device identification VPD page is
VPD page 0x83 which is retrieved via the INQUIRY command with the EVPD
bit set to one.  This may seem obvious, but I would not assume that
every reader of this document will speak fluent SCSI ;-).

   2.  The "DESIGNATOR TYPE" must be set to one of three values
       explicitly listed in the "pnfs_scsi_designator_type"
       enumerations.

I'd expand this to explain what sorts of names the three types of
designators allow, capitalize must (-> MUST) and observe that this
requirement forbids a number of other types of SCSI names, including
those based on T10 vendor IDs (designator type 1).

   and note that ASCII may be used
   as the code set for UTF-8 text that contains only ASCII characters.

Important nit: "only ASCII characters" -> "only printable ASCII characters".
Every UTF-8 character is also an 8-bit "ASCII character" - don't go there.

   NFS clients thus MUST iterate the descriptors until a match for
   "sbv_code_set", "sbv_designator_type" and "sbv_designator" is found,
   or until the end of VPD page.

"iterate the descriptors until a match for ... is found, or until the end
of the VPD page." -> "check all the descriptors for a possible match to ... ."

Section 2.4.8.1 - how does the MDS pass reservation key values to clients?
The answer is sbv_pr_key, which should be mentioned here.

   If using a single key per client, the MDS needs to be
   aware of the per-client fencing granularity.

I think that's true no matter what, but at Logical Unit granularity in
all cases.  The quoted text seems to imply that a single fencing action
(PREEMPT AND ABORT) could affect multiple logical units, which I don't
believe is correct.

I'd also add text to suggest a single key per client is a simpler
approach rather than key per [client, LU] pair, in part because it
simplifies identifying which clients have reservations when looking
at reservation state on the storage systems.

Section 2.4.8.4

   In case of a non-responding client the MDS MUST fence the client by

This is subtle, but I don't think "MUST" is appropriate here because that
depends on MDS determination of "non-responding" which is implementation-
specific.  I'd suggest "MUST fence" -> "fences" in the current text.
Alternatively, it'd be ok to say that fencing MUST be performed by issuing
a "PERSISTENT RESERVE OUT" command ... - my concern is basing the "MUST"
on MDS determination that a client is non-responsive.

   Note that the client can
   distinguish I/O errors due to fencing from other errors based on the
   "RESERVATION CONFLICT" status.

For clarity: "status" -> "SCSI status (part of the SCSI sense data
returned for a command that encountered an I/O error)"

Section 2.4.8.5:

   A client that detects I/O errors on the storage devices MUST 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 performed.

Several nits in the first line:

OLD
   A client that detects I/O errors on the storage devices MUST commit
NEW
   A client that detects RESERVATION CONFLICT I/O errors on a storage
   device MUST commit all layouts that use the storage device
END

Also change last sentence to active voice:

OLD
   in which case a new registration will be performed.
NEW
   in which case the client will perform a new registration based
   on the key provided (via sbv_pr_key) at that time.
END

Section 2.8 should also discuss client interaction with volatile write
caches.  If the storage system contains a volatile cache (e.g., V_SUP
is set to one in the Extended Inquiry VPD page [page 0x86]), setting
the FUA bit to one in write commands is a very good idea.

Section 3 - Security considerations

   At one extreme, pNFS with SCSI
   layouts can be used with storage access protocols (e.g., parallel
   SCSI) that provide essentially no security functionality.  At the
   other extreme, pNFS may be used with storage protocols such as iSCSI
   that can provide significant security functionality.

References should be cited for both parallel SCSI and iSCSI.

Thanks,
--David

> -----Original Message-----
> From: nfsv4 [mailto:[email-masked]] On Behalf Of Christoph Hellwig
> Sent: Saturday, August 15, 2015 4:48 AM
> To: [email-masked]
> Cc: [email-masked]; [email-masked]
> Subject: Re: [nfsv4] I-D Action: draft-ietf-nfsv4-scsi-layout-02.txt
> 
> This contains the review feedback from Tom, and two minor XDR tweaks
> based on implementation experience.
> 
> I'd like to start a WG last call on this draft soon.
> 
> _______________________________________________
> nfsv4 mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/nfsv4

---

From: Spencer Shepler <[email-masked]>
Date: Thu, 24 Sep 2015 22:14:58 +0000

Christoph,

You and I spoke earlier this week about last-call schedule for this I-D.  Please work to integrate David's comments and when the I-D is ready with those, we will start WG last call.

Spencer

-----Original Message-----
From: nfsv4 [mailto:[email-masked]] On Behalf Of Black, David
Sent: Tuesday, September 22, 2015 4:19 PM
To: Christoph Hellwig <[email-masked]>
Cc: [email-masked]
Subject: [nfsv4] DLB review of: draft-ietf-nfsv4-scsi-layout-02.txt

Christoph,

In Prague, I promised to review this draft, so here are some comments.
In general the draft is well-written, so this review focuses on the SCSI aspects that are new to this draft.

One of the consequences of making this a self-contained draft is that a section should be added to explain/summarize the differences between the SCSI layout and the block-volume layout (which is independent of SCSI).  The draft abstract and intro should contain some high-level points of comparison/difference.

Also, somewhere early in the draft, it should be stated that SCSI persistent reservations MUST be supported by all entities (clients, servers, storage systems) that support pNFS SCSI layouts.

Section 2.3.1 - Nit - say that the Device identification VPD page is VPD page 0x83 which is retrieved via the INQUIRY command with the EVPD bit set to one.  This may seem obvious, but I would not assume that every reader of this document will speak fluent SCSI ;-).

I'd expand this to explain what sorts of names the three types of designators allow, capitalize must (-> MUST) and observe that this requirement forbids a number of other types of SCSI names, including those based on T10 vendor IDs (designator type 1).

"iterate the descriptors until a match for ... is found, or until the end of the VPD page." -> "check all the descriptors for a possible match to ... ."

I think that's true no matter what, but at Logical Unit granularity in all cases.  The quoted text seems to imply that a single fencing action (PREEMPT AND ABORT) could affect multiple logical units, which I don't believe is correct.

I'd also add text to suggest a single key per client is a simpler approach rather than key per [client, LU] pair, in part because it simplifies identifying which clients have reservations when looking at reservation state on the storage systems.

Section 2.4.8.4

This is subtle, but I don't think "MUST" is appropriate here because that depends on MDS determination of "non-responding" which is implementation- specific.  I'd suggest "MUST fence" -> "fences" in the current text.
Alternatively, it'd be ok to say that fencing MUST be performed by issuing a "PERSISTENT RESERVE OUT" command ... - my concern is basing the "MUST"
on MDS determination that a client is non-responsive.

   Note that the client can

For clarity: "status" -> "SCSI status (part of the SCSI sense data returned for a command that encountered an I/O error)"

Section 2.4.8.5:

OLD
   A client that detects I/O errors on the storage devices MUST commit NEW
   A client that detects RESERVATION CONFLICT I/O errors on a storage
   device MUST commit all layouts that use the storage device END

OLD

Section 2.8 should also discuss client interaction with volatile write caches.  If the storage system contains a volatile cache (e.g., V_SUP is set to one in the Extended Inquiry VPD page [page 0x86]), setting the FUA bit to one in write commands is a very good idea.

Thanks,
--David

> https://na01.safelinks.protection.outlook.com/?url=https%3a%2f%2fwww.i
> etf.org%2fmailman%2flistinfo%2fnfsv4&data=01%7c01%7csshepler%40microso
> ft.com%7cf9159012c64b4139e9f908d2c3a43398%7c72f988bf86f141af91ab2d7cd0
> 11db47%7c1&sdata=Ljiuu9iTM71xgumoXs296qq9%2buv4OsU0SP%2fdc2rlmo0%3d

_______________________________________________
nfsv4 mailing list
[email-masked]
https://na01.safelinks.protection.outlook.com/?url=https%3a%2f%2fwww.ietf.org%2fmailman%2flistinfo%2fnfsv4&data=01%7c01%7csshepler%40microsoft.com%7cf9159012c64b4139e9f908d2c3a43398%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=Ljiuu9iTM71xgumoXs296qq9%2buv4OsU0SP%2fdc2rlmo0%3d

---

From: Christoph Hellwig <[email-masked]>
Date: Fri, 25 Sep 2015 00:27:57 +0200

Thanks Spencer, I plan to have a version that addressed David's comment
ready early next week.

I also received a lot of good feedback from David Noveck, which I would
feel more comfortable incorporating if they had a public review cycle on
the WG mainling list.

Dave, can you repost those to the nfsv4 list?

---

From: David Noveck <[email-masked]>
Date: Fri, 25 Sep 2015 15:23:19 -0400

I'll post those in a bit but my understanding was that you do not intend,
for the most part, to follow up on my comments.

I want to make it clear that these comments do not represent any objection
on my part to publishing the document in its current form.

I still believe that a lot of the document is confusing (at least to me),
in the way that RFC5663 was.  However, while I think we should
someday address those issues, I think that it would be a shame if we let
those concerns get in the way of publishing this document
in the near future, given that it is an improvement on RFC5663.

---

From: Christoph Hellwig <[email-masked]>
Date: Tue, 29 Sep 2015 09:26:30 +0200

Hi David,

> on my part to publishing the document in its current form.

thanks for posting these comments to the mailinglist.  I actually
think all of your comments are extremly valuable and worth following
up to.  For some of them I just don't think this RFC is the right venue.