Details

    • Type: Improvement
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Labels:
    • Rank (Obsolete):
      9223372036854775807

      Description

      There are several easy cleanups that can be done in the mgc config code and its interfaces.

      As noted on https://review.whamcloud.com/#/c/27320/:

      his looks like it will fix the problem, but the code here could be further improved to avoid potential errors in the future.
      lustre/mgc/mgc_request.c
      Line 300:
      
      (style) would be better to do this in one check:
      
              if (cfg->cfg_sub_clds & CONFIG_T_SPTLRPC)
                      lcfg.cfg_instance = NULL;
              else if (sb != NULL)
                      lcfg.cfg_instance = sb;
              else
                      lcfg.cfg_instance = obd;
      
      no need to cast the pointers to void.
      
      Also, blech on having CONFIG_T_{PARAMS,SPTLRPC,BARRIER,...} and CONFIG_SUB_{PARAMS,...}.  This is obviously very error prone.
      
      Since CONFIG_T_is part of lustre_idl.h and is an enum, what about adding:
      
       enum config_sub_mask {
              CONFIG_SUB_SPTLRPC = BIT(CONFIG_T_SPTLRPC),
              CONFIG_SUB_RECOVER = BIT(CONFIG_T_PARAMS),
              CONFIG_SUB_PARAMS  = BIT(CONFIG_T_PARAMS),
              CONFIG_SUB_NODEMAP = BIT(CONFIG_T_NODEMAP),
              CONFIG_SUB_BARRIER = BIT(CONFIG_T_BARRIER),
       };
      
      and naming the CONFIG_T_* enum, and adding the enums as arguments where they are used so that it is clear which one is intended,
      and/or renaming these a bit so that "T" and "SUB" are more self-explanatory?
      

      The definition of struct config_llog_data can be moved to mgc_request.c. Same for cld_is_*() and the declaration of mgc_process_log().

      The declarations mgc_fsname2resid() and mgc_logname2resid() can be moved to mgc_internal.h and neither function needs to be exported.

      mgc_process_log() can be made static.

      The data and datalen parameters of obd_process_config() should be replaces with a struct lustre_cfg * parameter:

       static inline int
      -obd_process_config(struct obd_device *obd, int datalen, void *data)
      +obd_process_config(struct obd_device *obd, struct lustre_cfg *lcfg)
      

      Most of the char *logname or char *fsname parameters in lustre/mgc/ should be converted to const char *.

      Similarly, most of the struct config_llog_instance * parameters should be const qualified.

      The void *cfg_instance member of struct config_llog_instance should be const qualified.

      The ordering of parameters is inconsistent:

      static struct config_llog_data *config_log_find_or_add(struct obd_device *obd,
                                      char *logname, struct super_block *sb, int type,
                                      struct config_llog_instance *cfg)
      
      static struct config_llog_data *
      config_log_add(struct obd_device *obd, char *logname,
                     struct config_llog_instance *cfg, struct super_block *sb)
      
      

      Several functions can be simplified if we use a const void *instance parameter (pointing to the sb or obd device) rather than a struct config_llog_instance * parameter. These include config_log_find() and config_log_end().

      After this is done, the lustre_cfg for LCFG_LOG_END can be simplified to pass just the sb or obd pointer instead of a whole struct config_llog_instance.

      We should also consider removing LCFG_LOG_START and LCFG_LOG_END and replacing their uses with calls to new obd methods. There's no reason to pack in kernel objects into a lustre_cfg for this and it obscures analysis of these functions.

        Attachments

          Activity

            People

            • Assignee:
              wc-triage WC Triage
              Reporter:
              jhammond John Hammond
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated: