[LU-10579] chlg: -1(null) changelog for setfattr on trusted.*

• Status: Resolved
• Project: Lustre
• Fix Version/s: Lustre 2.10.6
• Type: Bug
• Priority: Minor
• Reporter: CEA
• Assignee: John Hammond
• Resolution: Fixed
• Severity: 3

Description

There seems to be an issue with XATTR changelogs for some specific prefixes such as trusted or security, but not user.

# bash lustre/tests/llmount.sh
# touch /mnt/lustre/blob
# lctl --device lustre-MDT0000 changelog_register
# setfattr -n trusted.test -v test /mnt/lustre/blob
# lfs changelog lustre-MDT0000
1 -1(null) 08:13:35.501636480 2018.01.30 0x0 t=[0x200000401:0x1:0x0] j=setfattr.0 ef=0x1 u=0:0
# setfattr -n user.test -v test /mnt/lustre/blob
# lfs changelog lustre-MDT0000
1 -1(null) 08:13:35.501636480 2018.01.30 0x0 t=[0x200000401:0x1:0x0] j=setfattr.0 ef=0x1 u=0:0
2 15XATTR 08:13:21.191376815 2018.01.30 0x0 t=[0x200000401:0x1:0x0] j=setfattr.0 ef=0x1 u=0:0

Comments

[Peter Jones]

Bruno

Could you please investigate?

Thanks

Peter

[John Hammond]

It seems that we're not handling these prefixes in mdd_xattr_changelog_type():

static enum changelog_rec_type
mdd_xattr_changelog_type(const struct lu_env *env, struct mdd_device *mdd,
                         const char *xattr_name)
{
        /* Layout changes systematically recorded */
        if (strcmp(XATTR_NAME_LOV, xattr_name) == 0 ||
            strncmp(XATTR_LUSTRE_LOV, xattr_name,
                    strlen(XATTR_LUSTRE_LOV)) == 0)
                return CL_LAYOUT;

        /* HSM information changes systematically recorded */
        if (strcmp(XATTR_NAME_HSM, xattr_name) == 0)
                return CL_HSM;

        if (has_prefix(xattr_name, XATTR_USER_PREFIX) ||
            has_prefix(xattr_name, XATTR_NAME_POSIX_ACL_ACCESS) ||
            has_prefix(xattr_name, XATTR_NAME_POSIX_ACL_DEFAULT))
                return CL_XATTR;

        return -1;
}

[Peter Jones]

Mike

Can you please investigate?

Peter

[Olaf Weber]

I ran into this working on changelog processing for HPE DMF 7.

mdd_xattr_changelog_type() returns -1 to indicate that no record should be emitted. This is itself a rather dubious practice: I'd prefer that CL_XATTR records be generated for those cases as well. At present I got half my wish, because they're generated as -1 records.

The reason is that the return type is not int but enum changelog_rec_type, and this is an unsigned type. So checks like the one in  mdd_declare_xattr_set() always fail, and the record is generated with type ((enum changelog_rec_type)-1).

static int mdd_declare_xattr_set(const struct lu_env *env,
                                 struct mdd_device *mdd,
                                 struct mdd_object *obj,
                                 const struct lu_buf *buf,
                                 const char *name,
                                 int fl, struct thandle *handle)
{
        enum changelog_rec_type type;
        int rc;

        rc = mdo_declare_xattr_set(env, obj, buf, name, fl, handle);
        if (rc)
                return rc;

        type = mdd_xattr_changelog_type(env, mdd, name);
>>>     if (type < 0)
>>>             return 0; /* no changelog to store */

        return mdd_declare_changelog_store(env, mdd, type, NULL, NULL, handle);
}

My preferred solution is that these changelog records be generated for these cases, rather than dropped on the floor. This can be done by changing mdd_xattr_changelog_type() to look like this:

static enum changelog_rec_type
mdd_xattr_changelog_type(const struct lu_env *env, struct mdd_device *mdd,
                         const char *xattr_name)
{
        /* Layout changes systematically recorded */
        if (strcmp(XATTR_NAME_LOV, xattr_name) == 0 ||
            strncmp(XATTR_LUSTRE_LOV, xattr_name,
                    strlen(XATTR_LUSTRE_LOV)) == 0)
                return CL_LAYOUT;

        /* HSM information changes systematically recorded */
        if (strcmp(XATTR_NAME_HSM, xattr_name) == 0)
                return CL_HSM;

        /* Everything else systematically recorded */
        return CL_XATTR;
}

 If this is done then the checks of the return value can also be removed of course.

[Olaf Weber]

This looks like fallout from LU-7264 so would be present in 2.10 and later.

[John Hammond]

Fixed in 2.11 by https://review.whamcloud.com/28251 LU-9727 lustre: add CL_GETXATTR for Changelogs.

[Gerrit Updater]

John L. Hammond ([e-mail]) uploaded a new patch: https://review.whamcloud.com/33048
Subject: LU-10579 mdd: emit changelogs for securit* and trusted xattrs
Project: fs/lustre-release
Branch: b2_10
Current Patch Set: 1
Commit: 9958d6f33212d818a439801b88c480207e449908

[Olaf Weber]

Note that 33048 doesn't address the issue that returning -1 from mdd_xattr_changelog_type() does not, in fact, suppress the generation of a changelog entry.

[John Hammond]

Hi Olaf,

Sorry, I didn't read your comments as carefully as I should have.

I think we can add a negative sentinel value to the enum that forces the compiler to use a signed type for it. Or return CL_SETXATTR as a catch all type.

[Olaf Weber]

Hi John,

As noted, my preference would be to just emit the changelog event in all cases. It is simpler to code, and I do not see the value of suppressing the events for certain xattrs. So returning CL_SETXATTR for all the "other" cases has my vote.

[Olaf Weber]

Expanding a bit on my previous comment.

What I'm looking for is the completion of this sentence: "No changelog processor should be notified of changes to an extended attribute named like this because ..."

Maybe there is one. But until there is one, I continue to think we should emit changelog entries for changes to every extended attribute.

[Gerrit Updater]

John L. Hammond ([e-mail]) merged in patch https://review.whamcloud.com/33048/
Subject: LU-10579 mdd: emit changelogs for security and trusted xattrs
Project: fs/lustre-release
Branch: b2_10
Current Patch Set:
Commit: ab246d23566d7da1a41479b688833f47976b6428

[Peter Jones]

Landed for 2.10.6. Not needed for master