LU-10422 lfsck: misc fixes to avoid unexpected repairing

There are several issues that will misguide LFSCK to
trigger unexpected RPC or repairing by wrong, including:

1) object_update_result_insert() should pack the OUT RPC
   result (not the return value) into the reply buffer via
   object_update_result::our_data. But it did that in some
   wrong address.

2) out_xattr_get() used wrong index to obtain the EA buffer
   as to may overwrite former update (such as OUT_XATTR_GET)
   results.

3) osp_declare_xattr_get() does not consider the last '0'
   of the EA name for the length parameter for
   osp_insert_async_request().

4) osp_xattr_get_interpterer() missed to handle the positive
   value for the given parameter @rc. That will cause the PFID
   EA to be double read when the target OST-object has it.

Signed-off-by: Fan Yong <fan.yong@intel.com>
Change-Id: Ibf0e095ae2735c60b9b88e4b0992389c906728f9
Reviewed-on: https://review.whamcloud.com/30612
Tested-by: Jenkins
Tested-by: Maloo <hpdd-maloo@intel.com>
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
Reviewed-by: Lai Siyao <lai.siyao@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>

diff --git a/lustre/include/lustre_update.h b/lustre/include/lustre_update.h
index d1ceb12..f8766f6 100644 (file)
--- a/lustre/include/lustre_update.h
+++ b/lustre/include/lustre_update.h
@@ -241,20 +241,14 @@ object_update_result_insert(struct object_update_reply *reply,
                            int rc)
 {
        struct object_update_result *update_result;
-       char *ptr;
 
        update_result = object_update_result_get(reply, index, NULL);
        LASSERT(update_result);
 
        update_result->our_rc = ptlrpc_status_hton(rc);
        if (rc >= 0) {
-               if (data_len > 0) {
-                       LASSERT(data);
-
-                       ptr = (char *)update_result +
-                       cfs_size_round(sizeof(struct object_update_reply));
-                       memcpy(ptr, data, data_len);
-               }
+               if (data_len > 0 && data)
+                       memcpy(update_result->our_data, data, data_len);
                update_result->our_datalen = data_len;
        }
 

diff --git a/lustre/osp/osp_object.c b/lustre/osp/osp_object.c
index ae35a71..0604892 100644 (file)
--- a/lustre/osp/osp_object.c
+++ b/lustre/osp/osp_object.c
@@ -769,42 +769,36 @@ static int osp_xattr_get_interpterer(const struct lu_env *env,
                                     void *data, int index, int rc)
 {
        struct osp_xattr_entry *oxe = data;
-       struct lu_buf *rbuf = &osp_env_info(env)->osi_lb2;
 
-       if (!rc) {
+       spin_lock(&obj->opo_lock);
+       if (rc >= 0) {
+               struct lu_buf *rbuf = &osp_env_info(env)->osi_lb2;
                size_t len = sizeof(*oxe) + oxe->oxe_namelen + 1;
 
                rc = object_update_result_data_get(reply, rbuf, index);
-               spin_lock(&obj->opo_lock);
-               if (rc < 0 || rbuf->lb_len == 0 ||
-                   rbuf->lb_len > (oxe->oxe_buflen - len)) {
-                       if (unlikely(rc == -ENODATA)) {
-                               oxe->oxe_exist = 0;
-                               oxe->oxe_ready = 1;
-                       } else {
-                               oxe->oxe_ready = 0;
-                       }
-                       spin_unlock(&obj->opo_lock);
-                       /* Put the reference obtained in the
-                        * osp_declare_xattr_get(). */
-                       osp_oac_xattr_put(oxe);
+               if (rc == -ENOENT || rc == -ENODATA || rc == 0) {
+                       oxe->oxe_exist = 0;
+                       oxe->oxe_ready = 1;
+                       goto unlock;
+               }
 
-                       return rc < 0 ? rc : -ERANGE;
+               if (unlikely(rc < 0) ||
+                   rbuf->lb_len > (oxe->oxe_buflen - len)) {
+                       oxe->oxe_ready = 0;
+                       goto unlock;
                }
 
                __osp_oac_xattr_assignment(obj, oxe, rbuf);
-               spin_unlock(&obj->opo_lock);
        } else if (rc == -ENOENT || rc == -ENODATA) {
-               spin_lock(&obj->opo_lock);
                oxe->oxe_exist = 0;
                oxe->oxe_ready = 1;
-               spin_unlock(&obj->opo_lock);
        } else {
-               spin_lock(&obj->opo_lock);
                oxe->oxe_ready = 0;
-               spin_unlock(&obj->opo_lock);
        }
 
+unlock:
+       spin_unlock(&obj->opo_lock);
+
        /* Put the reference obtained in the osp_declare_xattr_get(). */
        osp_oac_xattr_put(oxe);
 
@@ -835,8 +829,8 @@ static int osp_declare_xattr_get(const struct lu_env *env, struct dt_object *dt,
        struct osp_object       *obj     = dt2osp_obj(dt);
        struct osp_device       *osp     = lu2osp_dev(dt->do_lu.lo_dev);
        struct osp_xattr_entry  *oxe;
-       __u16 namelen;
        int                      rc      = 0;
+       __u16 len;
 
        LASSERT(buf != NULL);
        LASSERT(name != NULL);
@@ -848,10 +842,10 @@ static int osp_declare_xattr_get(const struct lu_env *env, struct dt_object *dt,
        if (oxe == NULL)
                return -ENOMEM;
 
-       namelen = strlen(name);
+       len = strlen(name) + 1;
        mutex_lock(&osp->opd_async_requests_mutex);
        rc = osp_insert_async_request(env, OUT_XATTR_GET, obj, 1,
-                                     &namelen, (const void **)&name,
+                                     &len, (const void **)&name,
                                      oxe, buf->lb_len,
                                      osp_xattr_get_interpterer);
        if (rc != 0) {

diff --git a/lustre/target/out_handler.c b/lustre/target/out_handler.c
index 481877b..91f0012 100644 (file)
--- a/lustre/target/out_handler.c
+++ b/lustre/target/out_handler.c
@@ -266,7 +266,7 @@ static int out_xattr_get(struct tgt_session_info *tsi)
                RETURN(PTR_ERR(name));
        }
 
-       update_result = object_update_result_get(reply, 0, NULL);
+       update_result = object_update_result_get(reply, idx, NULL);
        if (update_result == NULL) {
                CERROR("%s: empty name for xattr get: rc = %d\n",
                       tgt_name(tsi->tsi_tgt), -EPROTO);
@@ -294,7 +294,10 @@ static int out_xattr_get(struct tgt_session_info *tsi)
               tgt_name(tsi->tsi_tgt), PFID(lu_object_fid(&obj->do_lu)),
               name, (int)lbuf->lb_len, rc);
 
-       object_update_result_insert(reply, lbuf->lb_buf, lbuf->lb_len, idx, rc);
+       /* Since we directly use update_result->our_data as the lbuf->lb_buf,
+        * then use NULL for result_insert to avoid unnecessary memory copy. */
+       object_update_result_insert(reply, NULL, lbuf->lb_len, idx, rc);
+
        RETURN(0);
 }