Subject: Re: [nfsv4] review of draft-ietf-nfsv4-scsi-layout-04

From: Christoph Hellwig <[email-masked]>
Date: Sat, 5 Dec 2015 12:45:47 +0100

Hi David,

thanks for the large amount of feedback!  I've incorporate it into the new
version I'm about to push out.

> *2.1: Background and Architecture*
> 
> I found the first few sentences of the first paragraph confusing.  Part of
> this is the use of the term "Logical Unit" for a scsi device, which we are
> stuck with.  Another part is the use of the term "abstraction" to describe
> a scsi device, i.e. a concrete piece of hardware.  Then, when possible LU
> realizations are listed, unfortunately, the techniques listed (i.e.
> concatenation, striping, RAID) are so like what pNFS provides that it is
> confusing when persistent reservations are mentioned that one realizes that
> this abstraction, is a SCSI device, which most people, for better or worse
> consider "physical".  After you read it several times, you decide it is
> accurate, but the meaning should be clearer the first time you read it.

I've replaced "storage abstraction" with "storage model" and removed the
list of potential implementation, which doesn't seem all that useful
and might confuse readers.

> In the last sentence of the first paragraph, the use of the term "Metadata
> server" should probably be avoided. This is the first use of the term
> "Metadata server" since the abstract and this may add confusion since
> storage of metadata is not involved here.

I'm not sure what to replace it with.  Do you prefer just "NFS server"?

> BTW, one could help the situation with regard to *2.5: Extents are *
> *permissions*, by ending the second paragraph just before the text "and
> writable extents".  That eliminates the problem cited above which seems OK
> as the restriction in question never had a clear justification. I think we
> should consider removing it (and allowing the DS to write a single physical
> block on its own, without using read-modify-write).

I agree.  The server can always recall layouts that it does see as
conflicting anyway.  This is something important for parity RAID, and
I see no reason to have the language specificly about block sizes and
mumbling about legacy file systems here.

> *2.4: Data Structures: Extents and Extent Lists*
> 
> I think there is a problem with the use of "logical" vs. "physical" in the
> second (non-code) paragraph:
> 
>    - It starts out with "logical regions of the file".  That's OK.
>    - Then you get to "physical locations on a volume".  The next sentence
>    it is a "location on the logical volume".  In the sentence after that, this
>    is a "logical offset"
>    - Eventually this is translated into an "offset" into an LU (i.e. a *logical
>    *unit). That offset is the thing that most people would consider
>    "physical", since it is sent over SCSI.
> 
> I think the best (i.e. most logical :-) thing to do is eliminate "physical"
> and "logical"  from this section.

I think the logical vs physical block are very confusing, as are the logical
volumes.  I've removed the logical and physical qualifiers and change some
other wordings in minor ways.

> Suggest rewriting the last paragraph of this section as follows:
> 
> According to [RFC5661], if the minimum requested size, loga_minlength,
> is zero, this is an indication to the metadata server that the client
> desires any layout at offset loga_offset or less that the metadata
> server has "readily available".  Given the lack of a clear definition
> of this phrase, in the context of the SCSI layout type, when
> loga_minlength is zero, the metadata server SHOULD:
> 
> 
>    - when processing requests for readable layouts, return all such,
> even if some extents are in the PNFS_SCSI_NONE_DATA state.
>       - when processing requests for writable layouts, return extents
> which can be returned in the PNFS_SCSI_READ_WRITE_DATA state.

I've done this.  I don't think loga_minlength == 0 is useful at all
in the block / SCSI context, but we'll probably need to keep some
defintion of it to adhere to the RFC5661 wording.

> *2.4.2: Layout Commits*
> 
> [...]
>
> I suggest deleting the last two sentences and replacing them with the following:
> 
> 
> Since the block in question is in state PNFS_SCSI_INVALID_DATA,byte
> rages not written should be filled with zeros.  This applies even if
> it appears that the area being written is beyond what the client
> believes to be the end of file

Yes, this is especially important for hole filling.

> *2.4.3: Layout Returns*
> 
> Suggest rewriting the first paragraph as follows, since it isn't
> organized very well and moving the second paragraph into a new
> section, since the material is only tangentially related.
> 
> A LAYOUTRETURN operation represents an explicit release of resources
> by the client, This may be done in response to a CB_LAYOUTRECALL or
> before any recall, in order to avoid a future C_LAYOUTRECALL. When the
> LAYOUTRETURN operation specifies a LAYOUTRETURN4_FILE_return type,
> then the layoutreturn_file4 data structure specifies the region of the
> file layout that is no longer needed by the client. The client may
> return disjoint regions of the file by using multiple LAYOUTRETURN
> operations within a single COMPOUND operation.
> 
> 
> The LAYOUTRETURN operation is done without any SCSI layout specific
> data.    The opaque "lrf_body" field of the "layoutreturn_file4" data
> structure MUST have length zero.

Thanks.  I've done this and also removed "The client may
return disjoint regions of the file by using multiple LAYOUTRETURN
operations within a single COMPOUND operation." as that's obvious
from RFC5661 and has no business here.

> *2.4.3a: Layout Revocation*.
>
> [...]
> 
> Here is my proposed new text:

Done, thanks a lot.

> *Overview of Issues Related to Section 2.4.5.*
> 
> If this were an rfc5663bis, then it would be necessary to address this
> 
> A further issue is that draft-ietf-nfsv4-scsi-layout-04 makes the
> situation worse by talking about clients using read-modify-write to do
> non-block-aligned updates which would justify some sort of MDS
> concurrency control, but:

I think the Section 2.4.5 is conflating two issues.  One is the concurrent
write issue which as you said applies to base NFS already, and must be
solved by file locking or delegations.  The second issue is the concurrent
access to disjoint byte ranges inside the same block on the storage
devices. The latter is what I think RFC5633 tries to address, and which
I tried to explain a bit more in this document, but apparently failed.

I've edited the secion so that it stops mentioning the first case, and
better describes the second case, which I think is essential to handle‥

>    - I don't see the value of pNFS scsi clients doing such
> read-modify-write cycles, especially when they can do IO that is not
> block-aligned via the MDS.  I always thought that was the intention of
> RFC5663 but I don't know what people actually implemented.  In any
> case, given that such operations are very infrequent, I don't think it
> is right to suggest that pNFS clients should be doing such things.
> The fact that it would require the MDS to effect a concurrency control
> policy strengthens the argument that this should be something that
> should be done via the MDS,

No, they are very common.  Application do non-aligned I/O all the time,
and the Operating Systems do the rmw cycles in the file cache  (buffer
cache, page cache, etc) for the.  Not handling this case would dramatically
limit the usefulness of this protocol.

> *2.4.7: Layout Hints*
> 
> What's there is incomplete in that:
> 
>    - It doesn't cover what is reported by supported_attrs.  probably should
>    say the server must not support this on an fs only supporting pnfs using
>    the scsi layout.
>    - Should mention that layout hints are primarily specified, not by
>    SETATTR, but in the attributes specified with OPEN.

Ok.  I've come up with some wording that should address these two items.

> *2.4.8: Client Fencing*
> 
> In a number of case, this text refers to transferring resources to another
> client.  as not all revocations fit this scenario, the text needs to be
> clarified that revocation (and fencing) can occur for other reasons.

Thanks, I've fixed this up by my removing a lot of weird text and just
adding a small blurb that we'll fence after failed CB_LAYOUTRECALL
requests.

> 
> *2.7: Transient and Permanent Errors*
> 
> 
> This sectio needs to be reworked in light of the fact that the MDS is
> going to enforcing NFSv4 semantics, and that conflicts with different
> NFSv4 locks may have different retry stratgeies.
> 
> 
> Here's a possible replacement:

Thanks.  I've added this after some minor copy editing.