LU-12585 obdfilter: Use actual I/O bytes in stats

Currently the obdfilter stats note the number of bytes
requested by the client rather than the number of bytes
actually read or written.  This is particularly confusing
for reads because clients can request more data than
exists and some applications do this normally.

This results in statistics that can be off by almost any
amount from the actual number of bytes read.  This patch
moves the stats to be collected just before commit, which
allows the true number of bytes to be recorded but does not
include the commit time in the time stats.  (Since commit
time is not part of the operation latency as experienced by
the client.)

Signed-off-by: Patrick Farrell <pfarrell@whamcloud.com>
Change-Id: I81fe9a6afdad5b48e8421f4aa72b8ef10a0eee93
Reviewed-on: https://review.whamcloud.com/46075
Tested-by: jenkins <devops@whamcloud.com>
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Reviewed-by: Aurelien Degremont <degremoa@amazon.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>

diff --git a/lustre/include/obd.h b/lustre/include/obd.h
index f0a762b..ce22ac7 100644 (file)
--- a/lustre/include/obd.h
+++ b/lustre/include/obd.h
@@ -1049,7 +1049,8 @@ struct obd_ops {
                          struct obd_export *exp, struct obdo *oa,
                          int objcount, struct obd_ioobj *obj,
                          struct niobuf_remote *remote, int pages,
-                         struct niobuf_local *local, int rc);
+                         struct niobuf_local *local, int rc, int nob,
+                         ktime_t kstart);
        int (*o_init_export)(struct obd_export *exp);
        int (*o_destroy_export)(struct obd_export *exp);
 

diff --git a/lustre/include/obd_class.h b/lustre/include/obd_class.h
index 06658d2..cd2421b 100644 (file)
--- a/lustre/include/obd_class.h
+++ b/lustre/include/obd_class.h
@@ -1138,7 +1138,8 @@ static inline int obd_commitrw(const struct lu_env *env, int cmd,
                               struct obd_export *exp, struct obdo *oa,
                               int objcount, struct obd_ioobj *obj,
                               struct niobuf_remote *rnb, int pages,
-                              struct niobuf_local *local, const int orig_rc)
+                              struct niobuf_local *local, const int orig_rc,
+                              int nob, ktime_t kstart)
 {
        int rc;
        ENTRY;
@@ -1154,7 +1155,8 @@ static inline int obd_commitrw(const struct lu_env *env, int cmd,
        }
 
        rc = OBP(exp->exp_obd, commitrw)(env, cmd, exp, oa, objcount, obj,
-                                        rnb, pages, local, orig_rc);
+                                        rnb, pages, local, orig_rc, nob,
+                                        kstart);
 
        RETURN(rc);
 }

diff --git a/lustre/mdt/mdt_internal.h b/lustre/mdt/mdt_internal.h
index f3cbbb0..5c8df08 100644 (file)
--- a/lustre/mdt/mdt_internal.h
+++ b/lustre/mdt/mdt_internal.h
@@ -1360,7 +1360,8 @@ int mdt_obd_preprw(const struct lu_env *env, int cmd, struct obd_export *exp,
 int mdt_obd_commitrw(const struct lu_env *env, int cmd, struct obd_export *exp,
                     struct obdo *oa, int objcount, struct obd_ioobj *obj,
                     struct niobuf_remote *rnb, int npages,
-                    struct niobuf_local *lnb, int old_rc);
+                    struct niobuf_local *lnb, int old_rc, int nob,
+                    ktime_t kstart);
 int mdt_punch_hdl(struct tgt_session_info *tsi);
 int mdt_fallocate_hdl(struct tgt_session_info *tsi);
 int mdt_glimpse_enqueue(struct mdt_thread_info *mti, struct ldlm_namespace *ns,

diff --git a/lustre/mdt/mdt_io.c b/lustre/mdt/mdt_io.c
index 4c8bff6..4989d1d 100644 (file)
--- a/lustre/mdt/mdt_io.c
+++ b/lustre/mdt/mdt_io.c
@@ -362,10 +362,8 @@ static int mdt_preprw_read(const struct lu_env *env, struct obd_export *exp,
                           struct mdt_device *mdt, struct mdt_object *mo,
                           struct lu_attr *la, int niocount,
                           struct niobuf_remote *rnb, int *nr_local,
-                          struct niobuf_local *lnb, char *jobid)
+                          struct niobuf_local *lnb)
 {
-       struct tgt_session_info *tsi = tgt_ses_info(env);
-       struct ptlrpc_request *req = tgt_ses_req(tsi);
        struct dt_object *dob;
        int i, j, rc, tot_bytes = 0;
        int maxlnb = *nr_local;
@@ -424,7 +422,6 @@ static int mdt_preprw_read(const struct lu_env *env, struct obd_export *exp,
        if (unlikely(rc))
                GOTO(buf_put, rc);
 
-       mdt_counter_incr(req, LPROC_MDT_IO_READ, tot_bytes);
        RETURN(0);
 buf_put:
        dt_bufs_put(env, dob, lnb, *nr_local);
@@ -437,10 +434,8 @@ static int mdt_preprw_write(const struct lu_env *env, struct obd_export *exp,
                            struct lu_attr *la, struct obdo *oa,
                            int objcount, struct obd_ioobj *obj,
                            struct niobuf_remote *rnb, int *nr_local,
-                           struct niobuf_local *lnb, char *jobid)
+                           struct niobuf_local *lnb)
 {
-       struct tgt_session_info *tsi = tgt_ses_info(env);
-       struct ptlrpc_request *req = tgt_ses_req(tsi);
        struct dt_object *dob;
        int i, j, k, rc = 0, tot_bytes = 0;
        int maxlnb = *nr_local;
@@ -500,7 +495,6 @@ static int mdt_preprw_write(const struct lu_env *env, struct obd_export *exp,
        if (likely(rc))
                GOTO(err, rc);
 
-       mdt_counter_incr(req, LPROC_MDT_IO_WRITE, tot_bytes);
        RETURN(0);
 err:
        dt_bufs_put(env, dob, lnb, *nr_local);
@@ -525,7 +519,6 @@ int mdt_obd_preprw(const struct lu_env *env, int cmd, struct obd_export *exp,
        struct lu_attr *la = &info->mti_attr.ma_attr;
        struct mdt_device *mdt = mdt_dev(exp->exp_obd->obd_lu_dev);
        struct mdt_object *mo;
-       char *jobid;
        int rc = 0;
 
        /* The default value PTLRPC_MAX_BRW_PAGES is set in tgt_brw_write()
@@ -533,8 +526,6 @@ int mdt_obd_preprw(const struct lu_env *env, int cmd, struct obd_export *exp,
        if (*nr_local > MD_MAX_BRW_PAGES)
                *nr_local = MD_MAX_BRW_PAGES;
 
-       jobid = tsi->tsi_jobid;
-
        if (!oa || objcount != 1 || obj->ioo_bufcnt == 0) {
                CERROR("%s: bad parameters %p/%i/%i\n",
                       exp->exp_obd->obd_name, oa, objcount, obj->ioo_bufcnt);
@@ -551,13 +542,11 @@ int mdt_obd_preprw(const struct lu_env *env, int cmd, struct obd_export *exp,
        if (cmd == OBD_BRW_WRITE) {
                la_from_obdo(la, oa, OBD_MD_FLGETATTR);
                rc = mdt_preprw_write(env, exp, mdt, mo, la, oa,
-                                     objcount, obj, rnb, nr_local, lnb,
-                                     jobid);
+                                     objcount, obj, rnb, nr_local, lnb);
        } else if (cmd == OBD_BRW_READ) {
                tgt_grant_prepare_read(env, exp, oa);
                rc = mdt_preprw_read(env, exp, mdt, mo, la,
-                                    obj->ioo_bufcnt, rnb, nr_local, lnb,
-                                    jobid);
+                                    obj->ioo_bufcnt, rnb, nr_local, lnb);
                obdo_from_la(oa, la, LA_ATIME);
        } else {
                CERROR("%s: wrong cmd %d received!\n",
@@ -753,8 +742,11 @@ void mdt_dom_obj_lvb_update(const struct lu_env *env, struct mdt_object *mo,
 int mdt_obd_commitrw(const struct lu_env *env, int cmd, struct obd_export *exp,
                     struct obdo *oa, int objcount, struct obd_ioobj *obj,
                     struct niobuf_remote *rnb, int npages,
-                    struct niobuf_local *lnb, int old_rc)
+                    struct niobuf_local *lnb, int old_rc, int nob,
+                    ktime_t kstart)
 {
+       struct tgt_session_info *tsi = tgt_ses_info(env);
+       struct ptlrpc_request *req = tgt_ses_req(tsi);
        struct mdt_thread_info *info = mdt_th_info(env);
        struct mdt_device *mdt = mdt_dev(exp->exp_obd->obd_lu_dev);
        struct mdt_object *mo = info->mti_object;
@@ -808,6 +800,8 @@ int mdt_obd_commitrw(const struct lu_env *env, int cmd, struct obd_export *exp,
 
                la_from_obdo(la, oa, valid);
 
+               mdt_counter_incr(req, LPROC_MDT_IO_WRITE, nob);
+
                rc = mdt_commitrw_write(env, exp, mdt, mo, la, oa, objcount,
                                        npages, lnb, oa->o_grant_used, old_rc);
                if (rc == 0)
@@ -860,6 +854,9 @@ int mdt_obd_commitrw(const struct lu_env *env, int cmd, struct obd_export *exp,
                 * will also get the updated value. bug 5972 */
                if (oa)
                        mdt_dom_obj_lvb_update(env, mo, NULL, true);
+
+               mdt_counter_incr(req, LPROC_MDT_IO_READ, nob);
+
                rc = mdt_commitrw_read(env, mdt, mo, objcount, npages, lnb);
                if (old_rc)
                        rc = old_rc;

diff --git a/lustre/obdecho/echo.c b/lustre/obdecho/echo.c
index 5183047..b65d01a 100644 (file)
--- a/lustre/obdecho/echo.c
+++ b/lustre/obdecho/echo.c
@@ -384,7 +384,8 @@ static int echo_commitrw(const struct lu_env *env, int cmd,
                         struct obd_export *export, struct obdo *oa,
                         int objcount, struct obd_ioobj *obj,
                         struct niobuf_remote *rb, int niocount,
-                        struct niobuf_local *res, int rc)
+                        struct niobuf_local *res, int rc, int nob,
+                        ktime_t kstart)
 {
        struct obd_device *obd;
        int pgs = 0;

diff --git a/lustre/obdecho/echo_client.c b/lustre/obdecho/echo_client.c
index 8dca248..3c6bc10 100644 (file)
--- a/lustre/obdecho/echo_client.c
+++ b/lustre/obdecho/echo_client.c
@@ -2714,7 +2714,7 @@ static int echo_client_prep_commit(const struct lu_env *env,
                }
 
                ret = obd_commitrw(env, rw, exp, oa, 1, &ioo, &rnb, npages, lnb,
-                                  ret);
+                                  ret, rnb.rnb_len, ktime_set(0, 0));
                if (ret != 0)
                        break;
 

diff --git a/lustre/ofd/ofd_internal.h b/lustre/ofd/ofd_internal.h
index 12da352..cb2280e 100644 (file)
--- a/lustre/ofd/ofd_internal.h
+++ b/lustre/ofd/ofd_internal.h
@@ -352,7 +352,8 @@ int ofd_preprw(const struct lu_env *env,int cmd, struct obd_export *exp,
 int ofd_commitrw(const struct lu_env *env, int cmd, struct obd_export *exp,
                 struct obdo *oa, int objcount, struct obd_ioobj *obj,
                 struct niobuf_remote *rnb, int npages,
-                struct niobuf_local *lnb, int old_rc);
+                struct niobuf_local *lnb, int old_rc, int nob,
+                ktime_t kstart);
 
 /* ofd_trans.c */
 struct thandle *ofd_trans_create(const struct lu_env *env,

diff --git a/lustre/ofd/ofd_io.c b/lustre/ofd/ofd_io.c
index 0bae6eb..3f1fd54 100644 (file)
--- a/lustre/ofd/ofd_io.c
+++ b/lustre/ofd/ofd_io.c
@@ -570,14 +570,13 @@ static int ofd_preprw_read(const struct lu_env *env, struct obd_export *exp,
                           struct ofd_device *ofd, const struct lu_fid *fid,
                           struct lu_attr *la, struct obdo *oa, int niocount,
                           struct niobuf_remote *rnb, int *nr_local,
-                          struct niobuf_local *lnb, char *jobid)
+                          struct niobuf_local *lnb)
 {
        struct ofd_object *fo;
        int i, j, rc, tot_bytes = 0;
        enum dt_bufs_type dbt = DT_BUFS_TYPE_READ;
        int maxlnb = *nr_local;
        __u64 begin, end;
-       ktime_t kstart = ktime_get();
 
        ENTRY;
        LASSERT(env != NULL);
@@ -644,9 +643,6 @@ static int ofd_preprw_read(const struct lu_env *env, struct obd_export *exp,
                niocount,
                READ);
 
-       ofd_counter_incr(exp, LPROC_OFD_STATS_READ_BYTES, jobid, tot_bytes);
-       ofd_counter_incr(exp, LPROC_OFD_STATS_READ, jobid,
-                        ktime_us_delta(ktime_get(), kstart));
        RETURN(0);
 
 buf_put:
@@ -685,14 +681,13 @@ static int ofd_preprw_write(const struct lu_env *env, struct obd_export *exp,
                            struct lu_attr *la, struct obdo *oa,
                            int objcount, struct obd_ioobj *obj,
                            struct niobuf_remote *rnb, int *nr_local,
-                           struct niobuf_local *lnb, char *jobid)
+                           struct niobuf_local *lnb)
 {
        struct ofd_object *fo;
        int i, j, k, rc = 0, tot_bytes = 0;
        enum dt_bufs_type dbt = DT_BUFS_TYPE_WRITE;
        int maxlnb = *nr_local;
        __u64 begin, end;
-       ktime_t kstart = ktime_get();
        struct range_lock *range = &ofd_info(env)->fti_write_range;
 
        ENTRY;
@@ -868,9 +863,6 @@ static int ofd_preprw_write(const struct lu_env *env, struct obd_export *exp,
        range_lock(&fo->ofo_write_tree, range);
        ofd_info(env)->fti_range_locked = 1;
 
-       ofd_counter_incr(exp, LPROC_OFD_STATS_WRITE_BYTES, jobid, tot_bytes);
-       ofd_counter_incr(exp, LPROC_OFD_STATS_WRITE, jobid,
-                        ktime_us_delta(ktime_get(), kstart));
        RETURN(0);
 err:
        dt_bufs_put(env, ofd_object_child(fo), lnb, *nr_local);
@@ -913,7 +905,6 @@ int ofd_preprw(const struct lu_env *env, int cmd, struct obd_export *exp,
        struct tgt_session_info *tsi = tgt_ses_info(env);
        struct ofd_device       *ofd = ofd_exp(exp);
        struct ofd_thread_info  *info;
-       char                    *jobid;
        const struct lu_fid     *fid = &oa->o_oi.oi_fid;
        int                      rc = 0;
 
@@ -925,10 +916,8 @@ int ofd_preprw(const struct lu_env *env, int cmd, struct obd_export *exp,
 
        if (tgt_ses_req(tsi) == NULL) { /* echo client case */
                info = ofd_info_init(env, exp);
-               jobid = NULL;
        } else {
                info = tsi2ofd_info(tsi);
-               jobid = tsi->tsi_jobid;
        }
 
        LASSERT(oa != NULL);
@@ -960,12 +949,11 @@ int ofd_preprw(const struct lu_env *env, int cmd, struct obd_export *exp,
        if (cmd == OBD_BRW_WRITE) {
                la_from_obdo(&info->fti_attr, oa, OBD_MD_FLGETATTR);
                rc = ofd_preprw_write(env, exp, ofd, fid, &info->fti_attr, oa,
-                                     objcount, obj, rnb, nr_local, lnb, jobid);
+                                     objcount, obj, rnb, nr_local, lnb);
        } else if (cmd == OBD_BRW_READ) {
                tgt_grant_prepare_read(env, exp, oa);
                rc = ofd_preprw_read(env, exp, ofd, fid, &info->fti_attr, oa,
-                                    obj->ioo_bufcnt, rnb, nr_local, lnb,
-                                    jobid);
+                                    obj->ioo_bufcnt, rnb, nr_local, lnb);
        } else {
                CERROR("%s: wrong cmd %d received!\n",
                       exp->exp_obd->obd_name, cmd);
@@ -1447,23 +1435,40 @@ out:
 int ofd_commitrw(const struct lu_env *env, int cmd, struct obd_export *exp,
                 struct obdo *oa, int objcount, struct obd_ioobj *obj,
                 struct niobuf_remote *rnb, int npages,
-                struct niobuf_local *lnb, int old_rc)
+                struct niobuf_local *lnb, int old_rc, int nob, ktime_t kstart)
 {
+       struct tgt_session_info *tsi = tgt_ses_info(env);
        struct ofd_thread_info *info = ofd_info(env);
        struct ofd_device *ofd = ofd_exp(exp);
        const struct lu_fid *fid = &oa->o_oi.oi_fid;
        struct ldlm_namespace *ns = ofd->ofd_namespace;
        struct ldlm_resource *rs = NULL;
+       char *jobid;
        __u64 valid;
        int rc = 0;
        int root_squash = 0;
 
        LASSERT(npages > 0);
 
+       if (tgt_ses_req(tsi) == NULL) { /* echo client case */
+               jobid = NULL;
+       } else {
+               jobid = tsi->tsi_jobid;
+       }
+
        if (cmd == OBD_BRW_WRITE) {
                struct lu_nodemap *nodemap;
                __u32 mapped_uid, mapped_gid, mapped_projid;
 
+               /* doing this before the commit operation places the counter
+                * update almost immediately after reply to the client, which
+                * gives reasonable time stats and lets us use the actual
+                * bytes of i/o (rather than requested)
+                */
+               ofd_counter_incr(exp, LPROC_OFD_STATS_WRITE_BYTES, jobid, nob);
+               ofd_counter_incr(exp, LPROC_OFD_STATS_WRITE, jobid,
+                                ktime_us_delta(ktime_get(), kstart));
+
                nodemap = nodemap_get_from_exp(exp);
                if (IS_ERR(nodemap))
                        RETURN(PTR_ERR(nodemap));
@@ -1560,6 +1565,11 @@ int ofd_commitrw(const struct lu_env *env, int cmd, struct obd_export *exp,
                oa->o_gid = mapped_gid;
                oa->o_projid = mapped_projid;
        } else if (cmd == OBD_BRW_READ) {
+               /* see comment on LPROC_OFD_STATS_WRITE_BYTES usage above */
+               ofd_counter_incr(exp, LPROC_OFD_STATS_READ_BYTES, jobid, nob);
+               ofd_counter_incr(exp, LPROC_OFD_STATS_READ, jobid,
+                        ktime_us_delta(ktime_get(), kstart));
+
                rc = ofd_commitrw_read(env, ofd, fid, objcount,
                                       npages, lnb);
                if (old_rc)

diff --git a/lustre/target/tgt_handler.c b/lustre/target/tgt_handler.c
index 4123d47..3198cc8 100644 (file)
--- a/lustre/target/tgt_handler.c
+++ b/lustre/target/tgt_handler.c
@@ -2264,6 +2264,7 @@ int tgt_brw_read(struct tgt_session_info *tsi)
                                 npages_read;
        struct tgt_thread_big_cache *tbc = req->rq_svc_thread->t_data;
        const char *obd_name = exp->exp_obd->obd_name;
+       ktime_t kstart;
 
        ENTRY;
 
@@ -2362,6 +2363,7 @@ int tgt_brw_read(struct tgt_session_info *tsi)
        repbody->oa = body->oa;
 
        npages = PTLRPC_MAX_BRW_PAGES;
+       kstart = ktime_get();
        rc = obd_preprw(tsi->tsi_env, OBD_BRW_READ, exp, &repbody->oa, 1,
                        ioo, remote_nb, &npages, local_nb);
        if (rc != 0)
@@ -2379,7 +2381,6 @@ int tgt_brw_read(struct tgt_session_info *tsi)
                        GOTO(out_commitrw, rc = -ENOMEM);
        }
 
-       nob = 0;
        npages_read = npages;
        for (i = 0; i < npages; i++) {
                int page_rc = local_nb[i].lnb_rc;
@@ -2478,7 +2479,7 @@ int tgt_brw_read(struct tgt_session_info *tsi)
 out_commitrw:
        /* Must commit after prep above in all cases */
        rc = obd_commitrw(tsi->tsi_env, OBD_BRW_READ, exp, &repbody->oa, 1, ioo,
-                         remote_nb, npages, local_nb, rc);
+                         remote_nb, npages, local_nb, rc, nob, kstart);
 out_lock:
        tgt_brw_unlock(exp, ioo, remote_nb, &lockh, LCK_PR);
 
@@ -2622,6 +2623,8 @@ int tgt_brw_write(struct tgt_session_info *tsi)
        const char *obd_name = exp->exp_obd->obd_name;
        /* '1' for consistency with code that checks !mpflag to restore */
        unsigned int mpflags = 1;
+       ktime_t kstart;
+       int nob = 0;
 
        ENTRY;
 
@@ -2748,6 +2751,7 @@ int tgt_brw_write(struct tgt_session_info *tsi)
        repbody->oa = body->oa;
 
        npages = PTLRPC_MAX_BRW_PAGES;
+       kstart = ktime_get();
        rc = obd_preprw(tsi->tsi_env, OBD_BRW_WRITE, exp, &repbody->oa,
                        objcount, ioo, remote_nb, &npages, local_nb);
        if (rc < 0)
@@ -2832,13 +2836,25 @@ skip_transfer:
        OBD_FAIL_TIMEOUT(OBD_FAIL_OST_BRW_PAUSE_BULK2, cfs_fail_val);
 
 out_commitrw:
+       /* calculate the expected actual write bytes (nob) for OFD stats.
+        * Technically, if commit fails this would be wrong, but that should be
+        * very rare
+        */
+       for (i = 0; i < niocount; i++) {
+               int len = remote_nb[i].rnb_len;
+
+               nob += len;
+       }
+
        /* Must commit after prep above in all cases */
        rc = obd_commitrw(tsi->tsi_env, OBD_BRW_WRITE, exp, &repbody->oa,
-                         objcount, ioo, remote_nb, npages, local_nb, rc);
+                         objcount, ioo, remote_nb, npages, local_nb, rc, nob,
+                         kstart);
        if (rc == -ENOTCONN)
                /* quota acquire process has been given up because
                 * either the client has been evicted or the client
-                * has timed out the request already */
+                * has timed out the request already
+                */
                no_reply = true;
 
        for (i = 0; i < niocount; i++) {
@@ -2855,13 +2871,10 @@ out_commitrw:
        repbody->oa.o_valid &= ~(OBD_MD_FLMTIME | OBD_MD_FLATIME);
 
        if (rc == 0) {
-               int nob = 0;
-
                /* set per-requested niobuf return codes */
                for (i = j = 0; i < niocount; i++) {
                        int len = remote_nb[i].rnb_len;
 
-                       nob += len;
                        rcs[i] = 0;
                        do {
                                LASSERT(j < npages);