Subject: [nfsv4] various minor NFSv4.2 review comments

From: Christoph Hellwig <[email-masked]>
Date: Wed, 30 Apr 2014 13:14:41 +0200

=== data4.d_reserved ===

The data4 d_allocated field is never references in the spec.  It looks like
it was supposed to be the counterpart to di_allocated in data_info4 and
specify the allocation status.  Should we convert this to a

	space_info4	d_reserved

and document it, or simply drop it?

=== returning holes larger than asked for ===

15.11.3. says:

"If the client specifies an rpa_offset and rpa_count value that is entirely
contained within a hole of the file, then the di_offset and di_length returned
must be for the entire hole."

but this contradicts the wording in 15.11.4.:

"One delta is that when the owner has a locked byte range, the server MUST
return an array of rpr_contents with values inside that range."

I would suggest to rewrite the text in 15.11.3. into:

"If the client specifies an rpa_offset and rpa_count value that is entirely
 contained within a hole of the file, then the di_offset and di_length returned
 MAY be for the entire hole.  If the the owner has a locked byte range
 covering rpa_offset and rpa_count entirely the di_offset and di_length
 MUST NOT be extended outside the locked byte range."

and drop the the text in 15.11.4. entirely, as data returns aren't extended
and thus entirely under control of the client.  The MAY is also important
to not require servers to extent the hole if they don't have an easy way to do
it.

=== SEEK nitpicks ==

15.12.3.:

"From the given sa_offset, find the next data_content4 of type sa_what in the
file. For a hole, this must return the data_content4 in its entirety."

What does an entire hole mean here, especially if we have allocated
and non-allocated holes next to each other?  Why does the return of
the entire range no apply to data?

"For data, it must not return the actual data."

Given that the return value doesn't have a way to return the data this
statement seems superfluous.

=== OFFLOAD_* description text ===

All these talk about copy operations, but also apply to WRITE_SAME?

=== WRITE_SAME description ==

"15.13.  Operation 69: WRITE_SAME - WRITE a Block Multiple Times to a File"

It does not nessecarily write the exactly the same BLOCK, but slight
variations of it depending on the argument.  Maybe just use the ADB
terminology here as well?

---

From: Thomas Haynes <[email-masked]>
Date: Wed, 30 Apr 2014 10:51:54 -0700

> and document it, or simply drop it?

I think we drop it. You made a recent change that dropped this
sentence from draft-22:

   If the server elects to
   return the hole as data, then it can set the d_allocted to FALSE in
   the rpc_data to indicate it is a hole.

And the correct question is why would the server ever elect
to return the hole as data?

If the client wants it as data, it can just use READ.

> to not require servers to extent the hole if they don't have an easy way to do

I'll apply these changes.

> file. For a hole, this must return the data_content4 in its entirety."

must -> MAY

> the entire range no apply to data?

The intent is that if the server detects a hole, it returns the end of
the hole as well as the start.

If the server detects data, then finding the end would require I/O to
find the end!

Well, no, actually it would not. Either we can detect when a hole
will be found next or we can detect EOF.

So, I'd be willing to change that. I'll hold off on suggesting to what
for a bit.

The other question of interest is allocated next to non-allocated. This
wasn't an issue before we allowed byte-range space reservation. :-)

The obvious choices are:

1) SEEK stops when the space_info4 changes
2) SEEK reports the entire data_content4 but returns
SPACE_UNKNOWN4 when it consolidates different allocations.

> statement seems superfluous.

Sometimes you need a big stick. This can come out.

> All these talk about copy operations, but also apply to WRITE_SAME?

Yes

> terminology here as well?

Let me take a stab at it later...

---

From: Christoph Hellwig <[email-masked]>
Date: Wed, 30 Apr 2014 20:16:59 +0200

> to return the hole as data?

Because the NFS server might not get that information from the actual
filesystem.  Of course in that case it couldn't set d_allocated either..