Subject: [nfsv4] [PATCH] Provide a new operation to notify the MDS of client events via the layout

From: Thomas Haynes <[email-masked]>
Date: Mon, 12 May 2014 17:12:09 -0700

From: Tom Haynes <[email-masked]>

We talked about a LAYOUT_STATS operation to allow the
client to notify the MDS about statistics for IO
with the DS. That did not fly very well.

Instead, I'd like to present a per-Layout Type blob
based on the layoutreturn/layoutcommit.

My current Flex File blob would be something like this:

struct io_info4 {
        uint32_t        ii_count;
        uint64_t        ii_bytes;
};

struct io_stats4 {
        io_info4        is_read;
        io_info4        is_write;
};

enum ff_layout_content {
        FF_WARNING      = 1,
        FF_STATS        = 2
};

struct ff_layout_warning {
        deviceid4       fflw_deviceid;
        bool            fflw_iswrite;
        pnfs_ff_errno   fflw_errno;
};

union ff_layout_update (ff_layout_content fflu_content) {
case FF_WARNING:
        io_stats4       fflu_warning;
case FF_STATS:
        io_stats4       fflu_stats;
default:
        void;
};

A question I've been asked is whether we want to
have part of this embedded within the layoutevent
because it is generic enough?

Signed-off-by: Tom Haynes <[email-masked]>
---
 Makefile                         |  3 ++
 dotx.d/Makefile                  |  2 ++
 dotx.d/layoutevent_args.x        |  8 ++++++
 dotx.d/layoutevent_res.x         |  3 ++
 dotx.d/spit_types.sh             |  3 ++
 nfsv42_middle_errors.xml         | 31 +++++++++++++++++++++
 nfsv42_middle_new_errors.xml     | 31 +++++++++++++++++++++
 nfsv42_middle_op_layoutevent.xml | 59 ++++++++++++++++++++++++++++++++++++++++
 nfsv42_middle_op_mandlist.xml    |  1 +
 9 files changed, 141 insertions(+)
 create mode 100644 dotx.d/layoutevent_args.x
 create mode 100644 dotx.d/layoutevent_res.x
 create mode 100644 nfsv42_middle_op_layoutevent.xml

diff --git a/Makefile b/Makefile
index f4dae79..2ee74ab 100755
--- a/Makefile
+++ b/Makefile
@@ -357,6 +357,8 @@ AUTOGEN =	\
 		autogen/layoutget_res.xml \
 		autogen/layoutreturn_args.xml \
 		autogen/layoutreturn_res.xml \
+		autogen/layoutevent_args.xml \
+		autogen/layoutevent_res.xml \
 		autogen/link_args.xml \
 		autogen/link_res.xml \
 		autogen/lock_args.xml \
@@ -449,6 +451,7 @@ IDXMLSRC_BASE = \
 	${DOC_PREFIX}_middle_op_exchange_id.xml \
 	${DOC_PREFIX}_middle_op_deallocate.xml \
 	${DOC_PREFIX}_middle_op_io_advise.xml \
+	${DOC_PREFIX}_middle_op_layoutevent.xml \
 	${DOC_PREFIX}_middle_op_layoutreturn.xml \
 	${DOC_PREFIX}_middle_op_offload_abort.xml \
 	${DOC_PREFIX}_middle_op_offload_revoke.xml \
diff --git a/dotx.d/Makefile b/dotx.d/Makefile
index 7d15fb1..026d933 100755
--- a/dotx.d/Makefile
+++ b/dotx.d/Makefile
@@ -125,6 +125,8 @@ FORE_OPS_AS_IS2 = \
 		type_io_advise.x \
 		io_advise_args.x \
 		io_advise_res.x \
+		layoutevent_args.x \
+		layoutevent_res.x \
 		read_plus_args.x \
 		read_plus_content.x \
 		read_plus_res_pre.x \
diff --git a/dotx.d/layoutevent_args.x b/dotx.d/layoutevent_args.x
new file mode 100644
index 0000000..d87fd6f
--- /dev/null
+++ b/dotx.d/layoutevent_args.x
@@ -0,0 +1,8 @@
+
+struct LAYOUTEVENT4args {
+        /* CURRENT_FH: file */
+        offset4                 lea_offset;
+        length4                 lea_length;
+        stateid4                lea_stateid;
+        layoutupdate4           lea_layoutupdate;
+};
diff --git a/dotx.d/layoutevent_res.x b/dotx.d/layoutevent_res.x
new file mode 100644
index 0000000..0c86471
--- /dev/null
+++ b/dotx.d/layoutevent_res.x
@@ -0,0 +1,3 @@
+struct LAYOUTEVENT4res {
+	nfsstat4	ler_status;
+};
diff --git a/dotx.d/spit_types.sh b/dotx.d/spit_types.sh
index 76ac094..b22a534 100755
--- a/dotx.d/spit_types.sh
+++ b/dotx.d/spit_types.sh
@@ -1004,6 +1004,7 @@ enum nfs_opnum4 {
  OP_READ_PLUS		= 67,
  OP_SEEK		= 68,
  OP_WRITE_SAME		= 69,
+ OP_LAYOUTEVENT	= 70,
  OP_ILLEGAL		= 10044
 };
 EOF
@@ -1126,6 +1127,7 @@ union nfs_argop4 switch (nfs_opnum4 argop) {
  case OP_READ_PLUS:	READ_PLUS4args opread_plus;
  case OP_SEEK:		SEEK4args opseek;
  case OP_WRITE_SAME:	WRITE_SAME4args opwrite_same;
+ case OP_LAYOUTEVENT:	LAYOUTEVENT4args oplayoutevent;
 
  /* Operations not new to NFSv4.1 */
  case OP_ILLEGAL:	void;
@@ -1258,6 +1260,7 @@ union nfs_resop4 switch (nfs_opnum4 resop) {
  case OP_READ_PLUS:	READ_PLUS4res opread_plus;
  case OP_SEEK:		SEEK4res opseek;
  case OP_WRITE_SAME:	WRITE_SAME4res opwrite_same;
+ case OP_LAYOUTEVENT:	LAYOUTEVENT4res oplayoutevent;
 
  /* Operations not new to NFSv4.1 */
  case OP_ILLEGAL:	ILLEGAL4res opillegal;
diff --git a/nfsv42_middle_errors.xml b/nfsv42_middle_errors.xml
index 0bb25e3..578a42b 100644
--- a/nfsv42_middle_errors.xml
+++ b/nfsv42_middle_errors.xml
@@ -1955,6 +1955,37 @@
         NFS4ERR_WRONG_TYPE
       </c>
 
+      <c>LAYOUTEVENT</c>
+      <c>
+        NFS4ERR_ADMIN_REVOKED,
+        NFS4ERR_BADXDR,
+        NFS4ERR_BAD_STATEID,
+        NFS4ERR_DEADSESSION,
+        NFS4ERR_DELAY,
+        NFS4ERR_DELEG_REVOKED,
+        NFS4ERR_EXPIRED,
+        NFS4ERR_FHEXPIRED,
+        NFS4ERR_GRACE,
+        NFS4ERR_INVAL,
+        NFS4ERR_ISDIR,
+        NFS4ERR_MOVED,
+        NFS4ERR_NOFILEHANDLE,
+        NFS4ERR_NOTSUPP,
+        NFS4ERR_NO_GRACE,
+        NFS4ERR_OLD_STATEID,
+        NFS4ERR_OP_NOT_IN_SESSION,
+        NFS4ERR_REP_TOO_BIG,
+        NFS4ERR_REP_TOO_BIG_TO_CACHE,
+        NFS4ERR_REQ_TOO_BIG,
+        NFS4ERR_RETRY_UNCACHED_REP,
+        NFS4ERR_SERVERFAULT,
+        NFS4ERR_STALE,
+        NFS4ERR_TOO_MANY_OPS,
+        NFS4ERR_UNKNOWN_LAYOUTTYPE,
+        NFS4ERR_WRONG_CRED,
+        NFS4ERR_WRONG_TYPE
+      </c>
+
       <c>LINK</c>
       <c>
         NFS4ERR_ACCESS,
diff --git a/nfsv42_middle_new_errors.xml b/nfsv42_middle_new_errors.xml
index 325fc68..34f210e 100644
--- a/nfsv42_middle_new_errors.xml
+++ b/nfsv42_middle_new_errors.xml
@@ -269,6 +269,37 @@
         NFS4ERR_WRONG_TYPE
       </c>
 
+      <c>LAYOUTEVENT</c>
+      <c>
+        NFS4ERR_ADMIN_REVOKED,
+        NFS4ERR_BADXDR,
+        NFS4ERR_BAD_STATEID,
+        NFS4ERR_DEADSESSION,
+        NFS4ERR_DELAY,
+        NFS4ERR_DELEG_REVOKED,
+        NFS4ERR_EXPIRED,
+        NFS4ERR_FHEXPIRED,
+        NFS4ERR_GRACE,
+        NFS4ERR_INVAL,
+        NFS4ERR_ISDIR,
+        NFS4ERR_MOVED,
+        NFS4ERR_NOFILEHANDLE,
+        NFS4ERR_NOTSUPP,
+        NFS4ERR_NO_GRACE,
+        NFS4ERR_OLD_STATEID,
+        NFS4ERR_OP_NOT_IN_SESSION,
+        NFS4ERR_REP_TOO_BIG,
+        NFS4ERR_REP_TOO_BIG_TO_CACHE,
+        NFS4ERR_REQ_TOO_BIG,
+        NFS4ERR_RETRY_UNCACHED_REP,
+        NFS4ERR_SERVERFAULT,
+        NFS4ERR_STALE,
+        NFS4ERR_TOO_MANY_OPS,
+        NFS4ERR_UNKNOWN_LAYOUTTYPE,
+        NFS4ERR_WRONG_CRED,
+        NFS4ERR_WRONG_TYPE
+      </c>
+
       <c>OFFLOAD_ABORT</c>
       <c>
         NFS4ERR_ADMIN_REVOKED,
diff --git a/nfsv42_middle_op_layoutevent.xml b/nfsv42_middle_op_layoutevent.xml
new file mode 100644
index 0000000..331d705
--- /dev/null
+++ b/nfsv42_middle_op_layoutevent.xml
@@ -0,0 +1,59 @@
+<!-- Copyright (C) The IETF Trust (2014) -->
+<!-- Copyright (C) The Internet Society (2014) -->
+
+<section anchor='op:layout_event' title='Operation 70: LAYOUTEVENT - Update the Status of the Layout'>
+  <section toc='exclude' title="ARGUMENT">
+    <?rfc include='autogen/type_layoutupdate4.xml'?>
+    <?rfc include='autogen/layoutevent_args.xml'?>
+  </section>
+
+  <section toc='exclude' title="RESULT">
+    <?rfc include='autogen/layoutevent_res.xml'?>
+  </section>
+
+  <section toc='exclude' title="DESCRIPTION">
+    <t>
+      The client can use LAYOUTEVENT to inform the metadata server
+      about its interaction with the layout represented by the
+      current filehandle, client ID (derived from the session ID
+      in the preceding SEQUENCE operation), byte-range (lea_offset
+      + lea_length), and lea_stateid. The information the client
+      is presenting is specific to the Layout Type and presented
+      in the lea_layoutupdate field. Each Layout Type MUST define
+      the contents of lea_layoutupdate in their respective
+      specifications. If a Layout Type specification is already
+      published and it does not define the payload, then it MUST
+      return NFS4ERR_NOTSUPP for this new operation.
+    </t>
+
+    <t>
+      While LAYOUTRETURN can be used to provide error information
+      to the metadata server (see <xref target='sec:pLeh:LAYOUTRETURN' />),
+      such updates are limited to when the client is normally done
+      processing the file. If the Layout Type allows for multiple
+      paths as targets for the I/O operations (e.g., the File Layout
+      Type's nflda_multipath_ds_list in Section 13.5 of <xref
+      target='RFC5661' />), then the client can inform the metadata
+      server of a non-responsive storage device which impacts
+      performance but does not prevent I/O.
+    </t>
+
+    <t>
+      LAYOUTEVENT can be combined with IO_ADVISE (see
+      <xref target='op:io_advise' />) to augment the decision making
+      process of how the metadata server handles a file.  I.e.,
+      IO_ADVISE lets the server know that a byte range has a certain
+      characteristic, but not necessarily the range of that
+      characteristic.
+    </t>
+
+    <t>
+      Both LAYOUTRETURN and LAYOUTCOMMIT are not effective in
+      providing this type of update in a timely fashion. For
+      LAYOUTRETURN, the client has to give up the layout when it
+      is actively processing the file. LAYOUTCOMMIT is tightly
+      coupled with WRITEs, the semantics of what the metadata server
+      needs to do upon receiving one is already complex.
+    </t>
+  </section>
+</section>
diff --git a/nfsv42_middle_op_mandlist.xml b/nfsv42_middle_op_mandlist.xml
index e78a671..0f0f77c 100644
--- a/nfsv42_middle_op_mandlist.xml
+++ b/nfsv42_middle_op_mandlist.xml
@@ -144,6 +144,7 @@
       <c>LAYOUTCOMMIT         </c> <c> OPT </c> <c> pNFS (REQ)   </c>
       <c>LAYOUTGET            </c> <c> OPT </c> <c> pNFS (REQ)   </c>
       <c>LAYOUTRETURN         </c> <c> OPT </c> <c> pNFS (REQ)   </c>
+      <c>LAYOUTEVENT          </c> <c> OPT </c> <c> pNFS (OPT)   </c>
       <c>LINK                 </c> <c> OPT </c> <c>              </c>
       <c>LOCK                 </c> <c> REQ </c> <c>              </c>
       <c>LOCKT                </c> <c> REQ </c> <c>              </c>
-- 
1.8.5.2 (Apple Git-48)

---

From: Benny Halevy <[email-masked]>
Date: Tue, 13 May 2014 12:39:25 +0300

comments inline below...

> struct ff_layout_warning {

In the flex files draft we had a filehandle and a byte range
as part of struct pnfs_ff_ioerr.

The motivation for the filehandle is to support striping over multiple component-files
that are stored on the same DS.

The motivation for the byte range is to support fine grain scrubbing and fixing of RAID.

Is the byte range below in LAYOUTEVENT4args is supposed to be equivalent to
this one?

Knowing the mapping between the user-file address space onto
the component-files, given the <deviceid, fhandle> the server can compute
the component-file byte range but this might be hairy if the user-file range
is striped over multiple component-files.

Also, from the implementation perspective, the i/o errors are encountered at
the lower layer that does component-level i/o, and the errors are accumulated
and summarized. The proposed design would make it harder on the client to reverse
map the actual i/o byte range onto the user-file address space.

What range would you choose if the i/o error happened on a component file
holding RAID-5 or RAID-6 parity data?

Another change is that pnfs_ff_layoutreturn contained an array of
pflr_ioerr_report and pflr_iostats_report.  Is the intention here
to send multiple LAYOUTEVENTs if necessary?

> union ff_layout_update (ff_layout_content fflu_content) {

Did you mean ff_layout_warning rather than io_stats4?

> because it is generic enough?

That sounds reasonable.
Note that we may want to recommend that the client sends a fflu_warning
with fflw_iswrite == true followed by a respective LAYOUTRETURN or (final) CLOSE/DELEGRETURN
in the same COMPOUND, otherwise servers may want to recall the layout when receiving such an
error and that is saved if the layout is returned voluntarily.

That was the intention of integrating pflr_ioerr_report with pnfs_ff_layoutreturn.

Thanks!

Benny

---

From: Christoph Hellwig <[email-masked]>
Date: Wed, 14 May 2014 08:39:52 +0200

> is striped over multiple component-files.

I think specifying the ranges and letting the MDS map that to the interal
geometry of the device is the way to go, devices are built way to different
in the different layouts [1]

> to send multiple LAYOUTEVENTs if necessary?

I think an array would make sense here at least for the errors.  How would
you use an array for the stats?

> That was the intention of integrating pflr_ioerr_report with pnfs_ff_layoutreturn.

Seems like instead having an error case here it might be better to just
always move the error reporting into LAYOUTRETURN.  4.2 already modifies
LAYOUTRETURN to support error reporting, so how about we get it right so
it can do so at the right detail level?

That would also solve my other major gripe with the proposal: right now it
multiplexes two vert different operations into a single one using the fancy
new discriminated union.  It should really be two operations, but merging
the error reporting into layoutreturn sounds even better.

[1] actually the way devices are built up is mostly the same in all existing
layouts, they just use gratiously different data structures for it.  If it had
defined the "device" to be the actual component to talk to (file handle, block
volume, OSD) the geometry could have been kept in the common code and everyones
life would be simpler.  Too late..

---

From: Tom Haynes <[email-masked]>
Date: Wed, 14 May 2014 11:55:05 -0500

> the error reporting into layoutreturn sounds even better.

There are two proposals here:

1) Allow the client to send events to the MDS without returning the layout.

2) How the Flex File layout will work.

The second is just an example - the multiplexing is not set in stone.

So can we focus on the proposal as defined in the PATCH and not the
comments?

---

From: "Matt W. Benjamin" <[email-masked]>
Date: Wed, 14 May 2014 16:41:48 -0400 (EDT)

Hi,

One of the aspects of the original proposal #1 I found most useful, especially
after further in-person discussion with Tom and Trond, is my sense that
"allowing a client to send events to the MDS without returning the layout" is
a flex-files specialization of a more general operation, possibly the same
one as LAYOUTCOMMIT, but if that is viewed as too semantically loaded,
then a weaker "LAYOUTEVENT" (Tom's name) operation.

My design intuition is that the different, and new, layouts are instances of
derivation from a common type, in the OO sense.  Each operation uses typed
opaque data to specialize the general/abstract version of the operation.

(As Trond and I also agreed, one way to think about the problem of people
inventing layouts as a mechanism for injecting ill-defined behavior into the
protocol, is to reason about whether all the proposed operations of a new
layout type are well matched to the set of operations provided by the general
LAYOUT form (something that looks something like a transaction)--not just by
how much the intended application of the proposed layout matches up with the
exact purpose of an existing layout.  That's another way of saying, it's
valuable that we can apply the layout concept to metadata as well as data,
as in Eisler's metastripe idea, and potentially others, depending on the quality
of the fit of the whole.)

Thanks,

Matt

----- "Tom Haynes" <[email-masked]> wrote:

> _______________________________________________
> nfsv4 mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/nfsv4

-- 
Matt Benjamin
The Linux Box
206 South Fifth Ave. Suite 150
Ann Arbor, MI  48104

http://linuxbox.com

tel.  734-761-4689 
fax.  734-769-8938 
cel.  734-216-5309

---

From: Christoph Hellwig <[email-masked]>
Date: Thu, 15 May 2014 07:57:03 +0200

> So can we focus on the proposal as defined in the PATCH and not the

I think my comments could be summarized as:

 - both I/O stats and I/O error communication seem like a generic enought
   problem that I think they should be handled as much as possible in the
   NFS spec and not in a specific layout driver.
 
 - I/O stats and error communication are different enought that we should
   not try to fit them into an overly generic "event" operation.

and last but not least:

 - given that 4.2 already adds commication of I/O errors to LAYOUTRETURN
   it's time to think about that particular problem very hard:

    - should I/O errors be associcated with a a LAYOUTRETURN or not?
    - If yes we should make it fully functional so that we don't need another
      operation to communicate I/O errors
    - If not we should remove the changes to LAYOUTRETURN in 4.2 and rely
      on the new operation for it.

---

From: Tom Haynes <[email-masked]>
Date: Thu, 15 May 2014 11:00:55 -0500

>      on the new operation for it.

Thanks for the great writeup.

I went with a generic operation in order to support things other than errors or
stats. 

In your two new operations model, are you envisioning:

A) Specific fields

B) Specific fields + a blob

C) Just a blob (I.e., much like the current event)

---

From: Christoph Hellwig <[email-masked]>
Date: Thu, 15 May 2014 18:21:50 +0200

> I went with a generic operation in order to support things other than errors or

I think an overly generic back channel to the layout is a bad idea.  If
the operation can't get enough consensus to be added to the main protocol
it probably should go back to the drawing board.

> C) Just a blob (I.e., much like the current event)---end quoted text---

B-ish: Specific fields + an optional blob.

---

From: Trond Myklebust <[email-masked]>
Date: Thu, 15 May 2014 17:58:47 -0400

>       on the new operation for it.

Hi Christoph,

As long as the LAYOUT_EVENT/STATUS/... can't exit with an error, then
I'm fine with removing the functionality from the LAYOUTRETURN. That
allows us to combine a LAYOUT_EVENT and LAYOUTRETURN in a single
COMPOUND, with the guarantee that the LAYOUTRETURN will succeed.

Cheers
  Trond

---

From: Tom Haynes <[email-masked]>
Date: Thu, 15 May 2014 17:46:57 -0500

> On May 15, 2014, at 4:58 PM, Trond Myklebust <[email-masked]> wrote:
> 

Agreed, they can error out due to stateid, etc, but those are all things that will cause the corresponding LAYOUTRETURN to puke.

---

From: Tom Haynes <[email-masked]>
Date: Thu, 15 May 2014 18:16:21 -0500

> you use an array for the stats?

Coming back to this (in the context of splitting the ops):

   struct layoutupdate4 {
           layouttype4             lou_type;
           opaque                  lou_body<>;
   };

   struct io_info4 {
           uint32_t        ii_count;
           uint64_t        ii_bytes;
   };

struct layoutstats_device4 {
        deviceid4               lsd_deviceid;
        offset4                 lsd_offset;
        length4                 lsd_length;
        io_info4                lsd_read;
        io_info4                lsd_write;
        layoutupdate4           lsd_layoutupdate;
};

struct LAYOUTSTATS4args {
        /* CURRENT_FH: file */
        stateid4                lsa_stateid;
        layoutstats_device4     lsa_devices<>;
};

---

From: Trond Myklebust <[email-masked]>
Date: Thu, 15 May 2014 21:28:53 -0400

>> you use an array for the stats?

Under what circumstance do we expect the client to want to delay
reporting errors until it has accumulated more than one?

Cheers
  Trond

---

From: Christoph Hellwig <[email-masked]>
Date: Fri, 16 May 2014 10:33:46 +0200

> reporting errors until it has accumulated more than one?

If we do client side striping different devices [1] might return different
errors at more or less the same time.  Not sure that's worth caring up.

[1] with that I mean actual storage devices.  I dread the person how came
up with the name device for what's a collecting of devices in the NFSv4.1
spec..