Subject: [nfsv4] LC comments on draft-ietf-nfsv4-flex-files-06

From: Benny Halevy <[email-masked]>
Date: Tue, 25 Aug 2015 19:02:09 +0300

Hi Tom,

Overall, the document looks great.
Thanks much for your excellent work.

When reviewing this last version in details produced
the comments and issues below.  Nothing earth shattering, though
not all comments are entirely minor....

Thanks,

Benny

* Wordsmithing in Section 2.1. LAYOUTCOMMIT:
- Using "preceded" rather than "proceeded" better describes
the requirement for sending COMMIT to the DSs before LAYOUTCOMMIT
to the MDS.

- COMMIT to the DS is redundant if the DS replied with
stable_how=FILE_SYNC, yet this is not clearly spelled out
in the section.  How about the following? Change
   With a loosely coupled system, a LAYOUTCOMMIT to the metadata server MUST
   be proceeded with a COMMIT to the storage device.  It is the
   responsibility of the client to make sure the data file is stable
   before the metadata server begins to query the storage devices about
   the changes to the file.
To
   It is the responsibility of the client to make sure the data file is stable
   before the metadata server begins to query the storage devices about
   the changes to the file.
   With a loosely coupled system, if any WRITE to a storage device did not
   result with stable_how equal to FILE_SYNC, a LAYOUTCOMMIT to the metadata
   server MUST be preceded with a COMMIT to the storage device.

* 2.2.  Fencing Clients from the Data Server
- nit: s/it forced/it is forced/

* 2.2.1.  Implementation Notes for Synthetic uids/gids
- "An implementation might allow an administrator
   to restrict a range of such ids in the name servers"
   I suggest adding a definition for "name server" in the Definitions section.
   (ditto for further use of "name services")

* 2.3.  State and Locking Models
- "Standalone" and "clustered" are used here with respect to NFSv4.1+
  storage devices.  I suggest adding some text to clarify how these relate
  to loose vs tight coupling.

* 4.1.  ff_device_addr4
-  If ffdv_version equals 3
   then server MUST set ffdv_minorversion to 0 and the client MUST
   access the storage device using the NFSv3 protocol [RFC1813].

   I suggest also adding that the server MUST set ffdv_tightly_coupled
   to false in this case.

- ffdv_tightly_coupled COMMIT semantics:
   I.e., the writes MUST be committed by the
   client to stable storage via issuing WRITEs with stable_how ==
   FILE_SYNC or by issuing a COMMIT after WRITEs with stable_how !=
   FILE_SYNC (see Section 3.3.7 of [RFC1813]).

   The spec provides only two complimentary options, yet it is possible
   (and quite common) that the storage devices always supports stable storage
   regardless of what the client set for stable_how when issuing the WRITE
   call and in this cases the device will reply to the WRITE with "committed"
   = FILE_SYNC.  Note that the storage device MUST do so if the client set stable_how
   to FILE_SYNC in the request but it MAY do so in its own discretion.  The point is
   that what matters is that the data file data and attrs are stable so I'd prefer
   this requirement to be phrased along the following lines:

   I.e., the writes MUST be committed by the
   storage device to stable storage via issuing WRITEs resulting with stable_how ==
   FILE_SYNC or by issuing a COMMIT after WRITEs with stable_how !=
   FILE_SYNC (see Section 3.3.7 of [RFC1813]).

* 4.2.  Storage Device Multipathing
- "This array
   (data type multipath_list4) represents a list of storage device (each
   identified by a network address), with the possibility that some
   storage device will appear in the list multiple times."

  This is a bit confusing...
  a) (nit) "storage device" is mentioned here as singular and not plural
  b) The list does not necessarily represents multiple storage devices
  but rather multiple storage device network addresses that may or may not
  refer to multiple devices.

  Since all the conceptual devices addressed by the array need to recognize
  the filehandle representing the data file (ffda_netaddrs is decoupled from
  ffda_versions/ffds_fh_vers, right?) it is fair to consider them a
  single storage device service, that might or might not be monolithic, clustered,
  or distributed, yet from the client's point of view it's a single logical service.
  So my conclusion is to stick to talking about multiple network addresses to a
  single logical device and refrain from discussing its internal architecture in
  the general case and whether it might be comprised of several, separate devices.

- The last section talks about special consideration for NFSv4.1+ devices.
  I'd leave it for drilling in to the non-monolithic cases. However, the example given:

   For example, the data could be read-only, and
   the data consist of exact replicas.

   Seems to be more elegantly solved by providing a (read only) mirrored layout
   rather than representing the mirrors in the device address - this is also apparent
   in Section 5.2.  Interactions Between Devices and Layouts.
   If we don't have a real use case we can simplify the spec and and allow multipathing
   only for clustered NFSv4.1+ DSs.

* 5.  Flexible File Layout Type
- Reference to the encapsulating NFSv4.1 structures was removed from section 4
  (GETDEVICEINFO). It might be a good idea to remove it here too (struct layout_content4
  and struct layout4).  This could be achieved a-la:

   The ff_layout4 data structure is returned by the server as the
   storage protocol specific opaque field loc_body in the
   layout_content4 structure by a successful LAYOUTGET operation.

- ffl_stripe_unit: is it really needed with mirroring only?  It was clearly needed
  when we supported RAID-0 striping (with the distinction of sparse vs. dense
  striping), but with mirroring I am not sure about its value.
  This also puts to the utility of Section 6 in question, with sparse striping and
  holes, the different data files are no longer mirrored replicas of each other.

* 8.  Mirroring
- nit: s/If a layout segments is being resilvered/If a layout segment is being resilvered/

* 8.2.  Writing to Mirrors
- The text refers to this specific case:
   If all but one copy
   is updated successfully and the last one provides an error, then the
   client needs to return the layout to the metadata server with an
   error indicating that the update failed to that storage device.

  How about the cases where more than one device failed?  How do they differ?
  What's expected from the client and mds in these cases?

* 9.  Flexible Files Layout Type Return
- ditto regarding verbatim copy of NFSv4.1 XDR.

* 9.2.1.  ff_io_latency4
- "Note that LAYOUTSTATS are cummulative, i.e., not reset each time the
   operation is sent."

  Do they reset when the layout stateid resets? (layout revoked or returned
  in full, either explicitly or implicitly with return-on-close)

  If so (and there are good reasons to define it so), this also covers
  the client restart case.

  Also, spelling mistake in "cummulative"?  should be "cumulative" I think...

- "If two RPC calls originating from the same NFS
   client are processed at the same time by the metdata server, then the
   call containing the larger values contains the most recent time
   series data."

  It is technically important to restrict that to the same file (and layout
  stateid.other if it resets the statistics).

  Also, typo in "metdata".

* 9.2.2.  ff_layout update4
- "In the case of multipathing, ffl_fhandle
   indicates which read-only copy was selected."

  As mentioned above, the layout filehandles are decoupled from multipathing
  and are related to multiple nfs versions for same device so this is a bit confusing.

* 10.  Flexible Files Layout Type LAYOUTERROR
  11.  Flexible Files Layout Type LAYOUTSTATS
- "it can use LAYOUT(ERROR|STATS)",
  it MAY? it SHOULD? either would probably be a better choice than "can"
  (I'd vote for SHOULD)

* 13.  Recalling Layouts
- How about the resilvering case?  Is it considered a "sharing conflict"?
  Maybe just spell it out explicitly for completeness...

* 14.  Client Fencing
- "the server MAY revoke client layouts and/
   or device address mappings"

  How would the server revoke device address mappings?

* 15.  Security Considerations
- "The control path contains all the new operations described by this
   extension;"

  copy-paste from RFC5661?

- "When the client acts on I/O operations on
   behalf of its local users, it MUST authenticate and authorize the
   user by issuing respective OPEN and ACCESS calls to the metadata
   server, similar to having NFSv4 data delegations.  If access is
   allowed, the client uses the corresponding (READ or RW) credentials
   to perform the I/O operations at the data file's storage devices."

  This seems to refer only to the tight coupling strategy, not to loose coupling.