[LU-4092] mdt_attr_set should acquire layout lock in some cases

• Type: Bug
• Resolution: Unresolved
• Priority: Minor
• Affects Version/s: Lustre 2.5.0
• Labels: mdt
• Severity: 3
• Assignee: WC Triage
• Reporter: John Hammond

Description

When changing file ownership mdt_attr_set() should add MDS_INODELOCK_LAYOUT to the ibits (MDS_INODELOCK_UPDATE or MDS_INODELOCK_UPDATE | MDS_INODELOCK_LOOKUP | MDS_INODELOCK_PERM). Semantically it only needs a PR layout lock but I believe that acquiring two separate locks on an object in the same handler is a bad idea.

Comments

[John Hammond]

Ownership changes are declared and performed on all subobjects in LOD:

static int lod_declare_attr_set(const struct lu_env *env,
                                struct dt_object *dt,
                                const struct lu_attr *attr,
                                struct thandle *handle)
        struct dt_object  *next = dt_object_child(dt);
        struct lod_object *lo = lod_dt_obj(dt);
        int                rc, i;
        ENTRY;

        /*
         * declare setattr on the local object
         */
        rc = dt_declare_attr_set(env, next, attr, handle);
        if (rc)
                RETURN(rc);

        /* osp_declare_attr_set() ignores all attributes other than                    
         * UID, GID, and size, and osp_attr_set() ignores all but UID                  
         * and GID.  Declaration of size attr setting happens through                  
         * lod_declare_init_size(), and not through this function.                     
         * Therefore we need not load striping unless ownership is                     
         * changing.  This should save memory and (we hope) speed up                   
         * rename(). */
        if (!(attr->la_valid & (LA_UID | LA_GID)))
                RETURN(rc);

        /*                                                                             
         * load striping information, notice we don't do this when object              
         * is being initialized as we don't need this information till                 
         * few specific cases like destroy, chown                                      
         */
        rc = lod_load_striping(env, lo);
        if (rc)
                RETURN(rc);

        /*                                                                             
         * if object is striped declare changes on the stripes                         
         */
        LASSERT(lo->ldo_stripe || lo->ldo_stripenr == 0);
        for (i = 0; i < lo->ldo_stripenr; i++) {
                LASSERT(lo->ldo_stripe[i]);
                rc = dt_declare_attr_set(env, lo->ldo_stripe[i], attr, handle);
                if (rc) {
                        CERROR("failed declaration: %d\n", rc);
                        break;
                }
        }

        RETURN(rc);
}

[Oleg Drokin]

I do not understand why would we want a layout lock taken for setattr, can you please elaborate on this?