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

From: Tom Haynes <[email-masked]>
Date: Thu, 10 Sep 2015 10:16:17 -0700

Hi Benny,

Thanks for the review, comments in line!

BTW - I’ve been sitting on the comments for a bit, waiting on free
time to respond. Unless noted, any response also indicates the
corresponding change has been made to the document.

Tom

> On Aug 25, 2015, at 9:02 AM, Benny Halevy <[email-masked]> wrote:
> 
> 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.

Ack

> 
> - 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.

I’m fine with that.

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

Ack

> 
> * 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”)

Ok, I might reword these out as name servers are not strictly required, but
I’ll define what I do use.

(Took it out and referenced LDAP ‘uid’).

> 
> * 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.

Okay, I actually do not like either of those terms to appear - “clustering”
does not imply tight coupling.

I went with:

   NFSv4.1+ storage devices that do not return the
   EXCHGID4_FLAG_USE_PNFS_DS flag set to EXCHANGE_ID are indicating that
   they are loosely coupled.  As such, they are treated the same way as
   NFSv4 storage devices.

   NFSv4.1+ storage devices that do identify themselves with the
   EXCHGID4_FLAG_USE_PNFS_DS flag set to EXCHANGE_ID are stongly
   coupled.  They will be using a back-end control protocol as described
   in [RFC5661] to implement a global stateid model as defined there.

> 
> * 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.

Yes

> 
> - 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]).

So I don’t think this flows correctly. The storage device is not issuing NFSv3 WRITE or
COMMIT calls.

Perhaps you could clarify?

> 
> * 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

ack

>  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.

I’m fine with the text the way it is - there is no claim this is the only way to do multipathing
and it provides some background.

> 
> - 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.

Again, I’m fine with the text the way it is.

> 
> * 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.
> 

I seem to recall Matt Benjamin wanting this?

Matt, was that the case?

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

ack

> * 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?

The first error is sufficient.

How about this?

     The client is responsible for updating all mirrored copies 
      of the layout segments that it is given in the layout.  A
      single failed update is suffcient to fail the entire operation. 
      I.e., 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.  If the client
      is updating the mirrors serially, then it SHOULD stop at the
      first error encountered and report that to the metadata server.
      If the client is updating the mirrors in parallel, then it
      SHOULD wait until all storage devices respond such that it
      can report all errors encountered during the update.

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

These are here for reference - note that they are stripped out when processing for the 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)

Yes, they have to - if the stateid were changed, then the deviceids might have also changed.
So the client would not know how to account for IO across a change.

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

Yes, it does.

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

Fixed

> 
> - "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).

Taken

>  Also, typo in "metdata”.

ack

> 
> * 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.

Still fine with the presentation of multipathing

> 
> * 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)

MAY

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

5661 does mention it explicitly, so, no problem repeating itself.

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

My guess is that the intent was to use CB_NOTIFY_DEVICEID to delete them.

Striking that part of the sentence.

> 
> * 15.  Security Considerations
> - "The control path contains all the new operations described by this
>   extension;"
> 
>  copy-paste from RFC5661?

No.

> 
> - "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.
> 
> 

It actually refers directly to loose coupling and not tight coupling. With tight
coupling, the stateid and real user credentials would control all access.

---

From: Benny Halevy <[email-masked]>
Date: Wed, 16 Sep 2015 09:56:28 +0300

> corresponding change has been made to the document.

Thanks.  Comments in line below...

Benny

> (Took it out and referenced LDAP ‘uid’).

Thanks.  This would work better.

>    EXCHGID4_FLAG_USE_PNFS_DS flag set to EXCHANGE_ID are stongly

nit: s/stongly/strongly/ or, alternatively, "tightly" as used elsewhere in the spec?

> Perhaps you could clarify?

The storage device replies to WRITE and COMMIT calls and it MAY promote
stable_how to FILE_SYNC in the reply, even if the client required only
UNSTABLE or DATA_SYNC.  In this case the client need not (SHOULD NOT) send
COMMIT to the storage device.

> Again, I’m fine with the text the way it is.

I'm not entirely comfortable with it since it adds a degree of freedom that
allows an implementation of a clustered storage device that is not tightly coupled
with the MDS and controlled with the back-end protocol. This bird looks like a duck
and quacks like a duck yet it is referred to as a flock of ducks :)
But without specifying the detailed semantics of how it MUST behave, e.g. with regards
to WRITEs, stable or not, COMMITs; may it support locking? Do locks survive
node restart or should the client reclaim them after failover? And so on.

>       single failed update is suffcient to fail the entire operation. 

s/suffcient/sufficient/

>       can report all errors encountered during the update.

Might be a good idea to add a clarification regarding timeouts:

    Note: The client need not wait forever for outstanding I/O requests
    and may declare them as failed after a reasonable timeout.
    Since the MDS is expected to resilver all mirrors following WRITE failure
    and since the outstanding WRITEs that were timed out by the client might
    race with WRITEs to be issued by the MDS; the MDS MUST fence off any client
    that might have outstanding WRITE requests to the failed stripe to prevent
    data corruption.

> These are here for reference - note that they are stripped out when processing for the XDR.

The spec is inconsistent with this regards.  We should either include the reference
in all respective sections or in none, no?

> Still fine with the presentation of multipathing

I'm confused so please help me out :)
For ff_layout4, ffds_fh_vers is described as follows:

   ffds_fh_vers is an array of filehandles of the data file matching to
   the available NFS versions on the given storage device.  There MUST
   be exactly as many elements in ffds_fh_vers as there are in
   ffda_versions.

Say one wants to use multipathing to access multiple read-only copies of files.
The length of ffda_netaddrs, in this case, relates to the number of devices in
this cluster but ffds_fh_vers is not so when selecting one of the available addresses,
how would the client select a corresponding filehandle to use on the selected
device?

> coupling, the stateid and real user credentials would control all access.

Duh, right. Though "corresponding credentials" is still a bit confusing.
How about the following text to clarify this point?

If access is allowed, the client uses the credentials that were returned
as part of the corresponding (READ or RW) layout to perform the I/O operations ...