[LU-9676] mgc config code cleanup

• Status: Open
• Project: Lustre
• Type: Improvement
• Priority: Minor
• Reporter: John Hammond
• Assignee: WC Triage
• Resolution: Unresolved
• Labels: easy, mgc

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.