Subject: [nfsv4] review of new section in draft-ietf-nfsv4-minorversion2-35

From: David Noveck <[email-masked]>
Date: Wed, 15 Apr 2015 05:37:37 -0400

Here's my contribution to the current minoverversion2 last call (for the
-35), with over a day to spare.

It looks like there will be a -36 at least, and presumably, yet another
last call.  In any case, I'd like to get these issues in the current
section 15.3 addressed as soon as possible

*Section 15.3:*

since this clones a byte/block range, perhaps the title should reflect this.

*Section 15.3.3:*

In this section, it isn't clear why "MUST fail" is used in some places and
"will fail" in others.

Since we may eventually decide to revise the first paragraph to be more
clear about how

cloning and copying relate, I'll limit myself for now, to a few low-level
issues.

   - Suggest that the comma after "e.g." be removed.
   - Suggest replacing "'copy on write" by "copy-on-write"

Within the second paragraph;

   - It's unclear whether named attributes are included as "regular files"
   - Suggest replacing "are not regular files" by "is not a regular file".

I don't see the point of the fourth paragraph.  We don't do this for other
operations that use the saved_fh.

There are some issues in the sixth paragraph that should be addressed:

   - The words "read" and "written" are used in a way that suggest the data
   is copied, while in other cases efforts are made to avoid saying that the
   data is copied.  You can say the data is cloned (and not copied) or that
   it's effectively copied by cloning or that it is copied, with the means by
   which copying occurs being immaterial. The text should pick one story and
   stick to it.
   - The handling of the case in which cl_size is zero should be clearer.

Here's my attempt to revise this using the "hey, we're cloning, not
copying" approach:

The cl_src_offset is the starting offset within the source file from which
the data to be cloned will be obtained and the cl_dst_offset is the
starting offset of the target region into which the cloned data will be
placed. An offset of 0 (zero) indicates the start of the respective file.
The number of bytes to be cloned is obtained from cl_count, except that a
cl_count of 0 (zero) indicates that the number of bytes to be cloned is the
count of bytes between cl_src_offset and the EOF of the source file. Both
cl_src_offset and cl_dst_offset must be aligned to the clone block size.
The number of bytes to be cloned must be a multiple of the clone block
size, except in the case in which cl_src_offset plus the number of bytes to
be cloned is equal to the source file size.

To be consistent with the above, the eighth paragraph could be written as
follows:

If the target area of the clone operation ends beyond the end of the
destination file, the offset at the end of the target area will determine
the new size of the destination file.  The contents of any block not part
of the target area will be the same as if the file size were extended by a
WRITE,

I think you need to add a paragraph like the following after the eight
paragraph:

If the area to be cloned is not a multiple of the clone block size and the
size of the destination file is past the end of the target area, the area
between the end of the target area and the nexrt multiple o the clone block
size wlll be zeroed.

The ninth paragraph, as written, is not consistent with my understanding of
atomicity.  Suggest the following replacement;

The CLONE operation is atomic in that other operations may not see see any
intermediate states between the state of the two files before the operation
and that after the operation.    READs of the destination file will never
see some blocks of the target area cloned without all of them being
cloned.  WRITEs of the source area will either have no effect on the data
of the target file or be fully reflected in the target area of the
destination file.

---

From: Christoph Hellwig <[email-masked]>
Date: Sat, 25 Apr 2015 15:22:27 +0200

> since this clones a byte/block range, perhaps the title should reflect this.

Ok.

> In this section, it isn't clear why "MUST fail" is used in some places and

This should always be MUST fail.  It's copy & paste from COPY andd needs
to fixed there as well.

>    - Suggest replacing "'copy on write" by "copy-on-write"

Fixed.

>    - It's unclear whether named attributes are included as "regular files"

This wording is used all over rfc5661, and I'd interpret it as
not including named attributes.  Better wording welcome.

>    - Suggest replacing "are not regular files" by "is not a regular file".

Fixed for CLONE and COPY.

> operations that use the saved_fh.

It comes straight from the COPY wording.  I'll remove it from both.

> There are some issues in the sixth paragraph that should be addressed:

Thanks for the various section rewrites, I've incorporated them.

---

From: David Noveck <[email-masked]>
Date: Sat, 25 Apr 2015 10:02:08 -0400

>>    - It's unclear whether named attributes are included as "regular
files"

Actually, I was wrong.  It is clear.   See below.

> not including named attributes.  Better wording welcome.

the current minor version 2 draft, and RFCs 5661 and 7530 define "is a
regular file" as including
named attributes.  So if CLONE (or COPY) wants to exclude them, it should
say "is of type
VREG".

If we don't make that change, we might want to say something to disallow
copying/cloning between
files of type VREG and named attributes.

---

From: Christoph Hellwig <[email-masked]>
Date: Sat, 25 Apr 2015 16:05:09 +0200

> files of type VREG and named attributes.

I'll need input from people actually implementing named attributes on
that.  I'm perfectly fine either way as any implementation I'd consider
sane wouldn't implement named attributes anyway..

---

From: Trond Myklebust <[email-masked]>
Date: Sat, 25 Apr 2015 10:17:11 -0400

> sane wouldn't implement named attributes anyway..

Named attributes are an implementation of subfiles, and so do contain
regular data. It seems unnecessary to exclude them without a good
reason to do so.

That said, I too am not a great fan of subfiles, so I'm not going to
strongly argue against anyone who claims otherwise.

Cheers
  Trond