Subject: [nfsv4] draft-ietf-nfsv4-scsi-layout-01.txt review

From: Tom Haynes <[email-masked]>
Date: Mon, 10 Aug 2015 09:59:18 -0700

1) Section 1:

   See the pNFS portion of [RFC5661] for more details.

becomes:

   See Section 12 of [RFC5661] for more details.

2) Section 2.1:

   Clients must be able to perform I/O to
   the block extents without affecting additional areas of storage
   (especially important for writes);

Either use MUST or pick another word, i.e., avoid lower
case versions of the RFC2119 words.

3) Section 2.4.3:

   that any in- flight

becomes

   that any in-flight

4) Section 2.7

   In
   either case, a reasonable approach for the client is to wait several
   milliseconds and retry the request.  The client SHOULD track the
   number of retries, and if forward progress is not made, the client
   SHOULD send the READ or WRITE operation directly to the server.

LOL fwiw, the Linux client does a retry forever on NFS4ERR_TRYLATER. :-)

5) Section 3

As a whole, the document does a great job of highlighting the issues
with locking, layouts, and the client being responsible for ensuring
that it must enforce protection.

And the writing in this section eventually covers it all.

I'm torn between wanting a very blunt in your face paragraph about
the client's responsibility in the face of locks and layouts versus
leaving well enough alone. :-)

Finally, I thought we weren't supposed to treat layouts as locks? I
don't have a problem with this, especially the wording in
Section 2.4.5, I wonder if this may cause problems with existing
server implementations?

6) Welding of two documents together

I like not having the XDR built on top of 5663 XDR - it is much cleaner to read.

---

From: Christoph Hellwig <[email-masked]>
Date: Wed, 12 Aug 2015 07:22:21 +0200

Thanks Tom!

> LOL fwiw, the Linux client does a retry forever on NFS4ERR_TRYLATER. :-)

This is directly copied from RFC5663, and just details what RFC5661
says a little more.  I really don't like changing the language from
what I inherited from RFC5663, but then again all these errors
are generic pNFS error anyway.  What do you think about simply
removing the Section 2.7, "Transient and Permanent Errors" ?

> leaving well enough alone. :-)

What would you want to see?  I don't think anything in the spec
implies layouts are used as NFS-level locks, so locking handling
sit entirely in the generic NFS client and doesn't interact with
layouts at all, both in the spec and the implementation known to me.

> Section 2.4.5, I wonder if this may cause problems with existing

The wording comes from RFC5663 and I don't find it confusing.  Layouts
aren't locks, but because block I/O will not just cause potential
interleaving I/O from multiple writing clients we prevent it at
the layout level.  Note that this is different from locks which
prevent different applications from interfearing.

---

From: Tom Haynes <[email-masked]>
Date: Wed, 12 Aug 2015 08:15:14 -0700

Hi Christoph,

I mainly had nits with the draft - I’m not looking for wholesale changes. If
I didn’t suggest a change, I’m okay with no change taking place.

> On Aug 11, 2015, at 10:22 PM, Christoph Hellwig <[email-masked]> wrote:
> 
> Thanks Tom!
> 
> removing the Section 2.7, "Transient and Permanent Errors” ?

There is no problem keeping this section.

> layouts at all, both in the spec and the implementation known to me.

Again, I’m okay with no change here - it does say what I want it to say.

> prevent different applications from interfearing.

Twisting hairs - effectively both drafts state that block clients use layouts
to enforce the locking model.

Either way, I’m fine with the wording as well - it is a practical solution to
the problem.