[LU-5296] lod_attr_set() skips attr_set on osp objects incorrectly

• Status: Resolved
• Project: Lustre
• Affects Version/s: Lustre 2.6.0
• Fix Version/s: Lustre 2.6.0
• Type: Bug
• Priority: Critical
• Reporter: Niu Yawei (Inactive)
• Assignee: Niu Yawei (Inactive)
• Resolution: Fixed
• Labels: HB
• Severity: 3

Description

This is a regression introduced by fix of LU-4690 "lod: separate master object with master stripe" (http://review.whamcloud.com/9511 commit 60e07b972114df24105a3a1bfa7365892f72a4a7).

In lod_attr_set():

        for (i = 0; i < lo->ldo_stripenr; i++) {
                LASSERT(lo->ldo_stripe[i]);
+               if (dt_object_exists(lo->ldo_stripe[i]) == 0)
+                       continue;
                rc = dt_attr_set(env, lo->ldo_stripe[i], attr, handle, capa);
                if (rc) {
                        CERROR("failed declaration: %d\n", rc);

Since dt_object_exists() always return false for the osp object (for OST object), any attr_set to osp object will be skipped mistakenly, and chown/chgrp won't set UID/GID to OST objects at the end.

As per Di's suggestion, we'd add additional IS_DIR checking here.

Issue Links

• is related to: LU-4690 sanity test_4: Expect error removing ...
• is related to: LU-4515 Test failure sanity-quota test_34: Us...

Comments

[Niu Yawei (Inactive)]

http://review.whamcloud.com/10989

[Niu Yawei (Inactive)]

Actually, current osp thread can't handle exceptions gracefully, I opened another ticket to track the problem there. See LU-5297.

[Alex Zhuravlev]

then it's probably better remove this check from LOD ?

[Niu Yawei (Inactive)]

then it's probably better remove this check from LOD ?

I think DNE relies on that check for dir object. Di, any comment on this? Thanks.

[Andreas Dilger]

How does this relate to LU-5006 "chown/chgrp doesn't work for files created by lfs setstripe"?

[Andreas Dilger]

What is the impact of this bug? Does it mean that quota will be incorrect during normal usage? Is it causing test failures?

[Andreas Dilger]

I see in further investigation that this would be causing sanity-quota test_34 failures, but because of LU-4515 this test was being skipped. I've merged the patch from LU-4515 to re-enable test_34 into http://review.whamcloud.com/10989 and increased the priority of this patch to critical so that it will land for 2.6.0.

[Di Wang]

I think DNE relies on that check for dir object. Di, any comment on this? Thanks.

Hmm, If you remove dt_object_exists here then you need move attr_set after real sub-stripes is created in mdd_object_create(), probably something like

diff --git a/lustre/mdd/mdd_dir.c b/lustre/mdd/mdd_dir.c
index d85dfef..b7efe22 100644
--- a/lustre/mdd/mdd_dir.c
+++ b/lustre/mdd/mdd_dir.c
@@ -1834,7 +1834,7 @@ static int mdd_object_initialize(const struct lu_env *env,
                                 struct lu_attr *attr, struct thandle *handle,
                                 const struct md_op_spec *spec)
 {
-        int rc;
+        int rc = 0;
         ENTRY;
 
         /*
@@ -1845,7 +1845,6 @@ static int mdd_object_initialize(const struct lu_env *env,
          *  (2) maybe, the child attributes should be set in OSD when creation.
          */
 
-       rc = mdd_attr_set_internal(env, child, attr, handle, 0);
        /* arguments are supposed to stay the same */
        if (S_ISDIR(attr->la_mode)) {
                 /* Add "." and ".." for newly created dir */
@@ -2187,6 +2186,9 @@ static int mdd_object_create(const struct lu_env *env, struct mdd_object *pobj,
                        GOTO(err_destroy, rc);
        }
 
+       rc = mdd_attr_set_internal(env, son, attr, handle, 0);
+       if (rc != 0)
+               GOTO(err_destroy, rc);
 #ifdef CONFIG_FS_POSIX_ACL
        if (def_acl_buf != NULL && def_acl_buf->lb_len > 0 &&
            S_ISDIR(attr->la_mode)) {

[Niu Yawei (Inactive)]

How does this relate to LU-5006 "chown/chgrp doesn't work for files created by lfs setstripe"?

This is not related to that one.

What is the impact of this bug? Does it mean that quota will be incorrect during normal usage? Is it causing test failures?

Quota will be incorrect after chown/chgrp. s-q test-34 will fail.

I see in further investigation that this would be causing sanity-quota test_34 failures, but because of LU-4515 this test was being skipped. I've merged the patch from LU-4515 to re-enable test_34 into http://review.whamcloud.com/10989 and increased the priority of this patch to critical so that it will land for 2.6.0.

Thank you for doing this. Andreas.

[Alex Zhuravlev]

wouldn't it better to remove this attr_set() - we're creating a new object and we do specify attributes for the creation. iirc, we do additional attr_set because it was easy for MDD to initialize ACL/i_mode, but this case demonstrates some issues..

[Niu Yawei (Inactive)]

wouldn't it better to remove this attr_set() - we're creating a new object and we do specify attributes for the creation. iirc, we do additional attr_set because it was easy for MDD to initialize ACL/i_mode, but this case demonstrates some issues..

Alex, we are talking about chown/chgrp case but not creation here.

[Alex Zhuravlev]

sorry, the monday is my only possible excuse..

[Alex Zhuravlev]

hmm, but if we don't have original attr_set() in create path, then we don't this check as well?

[Jodi Levi (Inactive)]

Patch landed to Master.