Uploaded image for project: 'Lustre'
  1. Lustre
  2. LU-10579

chlg: -1(null) changelog for setfattr on trusted.*

Details

    • Bug
    • Resolution: Fixed
    • Minor
    • Lustre 2.10.6
    • None
    • None
    • 3
    • 9223372036854775807

    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
      

      Attachments

        Activity

          [LU-10579] chlg: -1(null) changelog for setfattr on trusted.*
          pjones Peter Jones added a comment -

          Landed for 2.10.6. Not needed for master

          pjones Peter Jones added a comment - Landed for 2.10.6. Not needed for master

          John L. Hammond (jhammond@whamcloud.com) 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

          gerrit Gerrit Updater added a comment - John L. Hammond (jhammond@whamcloud.com) 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

          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.

          olaf Olaf Weber (Inactive) added a comment - 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.

          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 Olaf Weber (Inactive) added a comment - 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.
          jhammond John Hammond added a comment -

          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.

          jhammond John Hammond added a comment - 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.

          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.

          olaf Olaf Weber (Inactive) added a comment - 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 L. Hammond (jhammond@whamcloud.com) 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

          gerrit Gerrit Updater added a comment - John L. Hammond (jhammond@whamcloud.com) 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
          jhammond John Hammond added a comment -

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

          jhammond John Hammond added a comment - Fixed in 2.11 by https://review.whamcloud.com/28251 LU-9727 lustre: add CL_GETXATTR for Changelogs.

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

          olaf Olaf Weber (Inactive) added a comment - This looks like fallout from  LU-7264 so would be present in 2.10 and later.
          olaf Olaf Weber (Inactive) added a comment - - edited

          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 Olaf Weber (Inactive) added a comment - - edited 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.

          People

            jhammond John Hammond
            cealustre CEA
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: