Gerrit Review Dashboard

(style) This isn't a bug, but the C changes are entirely under lustre/target/, so `target:` would be the specific component tag here. `lustre:` is normally reserved for tree-wide work (compat header cleanups, dropping old kernel support).

(minor) The body says what server_lnet_servicenode_update() registers, but not when it runs, and three code hunks are left unaccounted for.

The helper is also called from tgt_import_update() and tgt_nid_notifier(), so the entry is re-published on every MGC reconnect and every LNet NID change. server_put_super() withdraws it with LNetDiscServiceNodeClear(), and has to do so after tgt_del_notifier() so a notifier worker cannot re-add it. tgt_mod_exit() turns the LNet policy back off.

Could the body describe the whole registration lifecycle rather than just the initial publish?

(minor) The body describes server_lnet_servicenode_update() and the module option, but the LNet half of the change is not mentioned at all:

- the new exported API LNetDiscPeerServiceRegister() / LNetDiscPeerServiceUnregister()
- struct lnet_peer::lp_disc_services and struct obd_export::exp_disc_service
- service names carried on struct lnet_disc_group, and the union/fold handling for them
- the replacement of dvp_partition_matches_peer / dvp_partition_fallback_peer for the servicenode policy, and the new early return in lnet_servicenode_unrestricted_matches_peer()
- tgt_disc_service_register() in tgt_connect(), and the exp_disc_service teardown in class_disconnect()

That is most of the non-test diff. Could the body say what per-peer service context is for and how it changes Push selection, so none of those hunks reads as unrelated?

(style) This isn't a bug, but the trailer order is inverted here: Signed-off-by: normally comes before Change-Id:, and a Change-Id: appearing first usually means the Lustre commit hooks aren't installed in the working tree.

The body explains server_lnet_servicenode_update() and the `ptlrpc.target_nid_policy` knob, but roughly two thirds of the diff is a second mechanism that is never described: LNetDiscPeerServiceRegister()/LNetDiscPeerServiceUnregister(), struct lnet_peer::lp_disc_services, struct obd_export::exp_disc_service, the tgt_connect()/class_disconnect() hooks, and the dg_services[] plumbing plus the new servicenode dvp_partition_matches_peer/dvp_partition_fallback_peer/full-view handling in lib-disc-view.c.

As written a reader cannot tell that the patch also binds each client export to the target it connected to, and uses that binding to pick which group view a Push carries. Could the body name those symbols and say why the binding is needed?

The body explains the target-side registration, the module option and the man page, but nothing in it covers the LNet half of the diff: the new exported LNetDiscPeerServiceRegister()/LNetDiscPeerServiceUnregister() API, struct lnet_peer::lp_disc_services and its transfer/detach at the peer merge, delete and recreate sites, dg_services/dg_alloc_nnids in struct lnet_disc_group, the prefer_service selection added to lnet_disc_view_for_push(), and the new obd_export::exp_disc_service plus the class_disconnect() hook.

That is around 390 of the ~440 lines of non-test, non-doc change. Could the message describe what a peer service registration is, who owns it, and how it changes Push view selection?

(minor) Worth saying here that groups sharing a local NID are folded together. lnet_group_fold_overlapping() unions any two groups with a NID in common, so the usual case - every target on an OSS carrying the same --servicenode list - collapses to a single group, and two targets with partially overlapping lists give one group spanning the union rather than two. As written, "each target contributes one group" reads as though per-target groups stay separate.

(minor) 9770942f89 is not a commit in the tree - `git describe` for this change is v2_17_55-26-g5c9327823f, and the sha changes on every rebase anyway. For a man page added in the same patch as the parameter, this should carry just the parent tag (2.17.55), as llite.sync_on_close.4 does; the exact hash is only meaningful for a page added after the parameter landed.

(minor) Lustre man pages are expected to carry an EXAMPLES section. There is a modprobe.d snippet up in DESCRIPTION -- would it be worth promoting it into a real .SH EXAMPLES here, together with a reading of /sys/module/ptlrpc/parameters/target_nid_policy to confirm the value in force?

(minor) The PROPERTIES block stops at Config. This parameter has both a default and a closed set of accepted values, which is exactly what the remaining man4 fields are for, e.g. llite.enable_setstripe_gid(4):

    .TP
    .B Default
    .br
    .RB target_nid_policy= legacy
    .TP
    .B Valid Values
    .br
    .BR legacy " | " servicenode

Both facts are in the prose below, but a reader skimming PROPERTIES will not find them there.

(style) The other LNet servicenode entry points added by this series (LNetDiscServiceNodePolicySet(), ...Set(), ...AddNID(), ...DelNID(), ...Clear()) all carry kernel-doc with Context: and Return: in lib-disc-view.c. These two have none, and the contract is not obvious: a return of 0 with *handle left NULL means "nothing was registered", and Unregister() is NULL-safe. Worth documenting both, since callers have to handle the NULL-handle success case.

The continuation lines are also one column short of the open paren (column 32 vs 33); LNetDiscServiceNodeSet() just below lines up.

(style) Not a bug, but LIBCFS_ALLOC() zeroes (LIBCFS_ALLOC_GFP uses kzalloc/vzalloc), so dg_nservices = 0 and dg_services = NULL are already true here; only dg_alloc_nnids needs setting.

Similarly at line 295, LIBCFS_FREE() (and so CFS_FREE_PTR_ARRAY()) already checks for NULL, so the `if (g->dg_services)` guard can go.

(minor) The gate is placed on the common LDD_F_OPC_REG path, so it also rejects a plain remount of an already-registered target that carries neither LDD_F_WRITECONF nor LDD_F_UPDATE. test_158 covers exactly that ("Existing target remount (startup ping) must also be denied"), and the in-code comment lists it too, but the subject and this paragraph only mention new registrations and post-writeconf re-registration.

Could the body name the remount case as well? It is the one an admin is most likely to hit after enabling nodemaps.

(defect) mgs_check_index() opens with

    LASSERT(!(mti->mti_flags & LDD_F_NEED_INDEX));

and the two lines above only OR in LDD_F_WRITECONF, they never clear LDD_F_NEED_INDEX. So an mti that arrives with that flag set now LBUGs the MGS here.

Before this patch the call was inside `if (!allow_register)`, and allow_register defaults to ALLOW_REGISTER_UNLIMITED (class_obd.c), so the default path never reached it. The other call site, mgs_check_target(), is guarded by `!(mti_flags & (LDD_F_WRITECONF | LDD_F_UPDATE))`, which cannot be true once WRITECONF has been added just above. Moving the call out of the allow_register block makes the assertion reachable on every registration.

mti_flags is unvalidated wire data from the registering node, and LDD_F_NEED_INDEX is a value the MGS otherwise still honours - mgs_set_index() in mgs_llog.c picks a free index for it. Current mount.lustre refuses to mount such a device locally, but that is the peer's own check, not the MGS's, and it is the peer this patch is trying not to trust.

Would clearing the flag before the call work?

    if (mti->mti_flags & LDD_F_NEED_INDEX)
        mti->mti_flags |= LDD_F_WRITECONF;

    if (!(mti->mti_flags & LDD_F_NEED_INDEX))
        rc = mgs_check_index(...);

or returning -EINVAL for that flag instead of asserting on it.

Looks like this should be fixed.

(defect) The comment above says every LDD_F_OPC_REG must come from a trusted nodemap, but by the time the check runs the untrusted peer has already changed MGS state:

  - line 497 sets `b_fsdb->fsdb_barrier_disabled = 1` when an MDT registration arrives without OBD_CONNECT_BARRIER. That is sticky for the lifetime of the fsdb and makes `lctl barrier_freeze` return -EOPNOTSUPP (mgs_barrier.c). Both mti_flags and the connect flags come from the peer, so a rejected node can still turn off the write barrier for a filesystem it names.
  - mgs_find_or_make_fsdb() has been called twice for peer-supplied names (barrier_name above, mti_fsname via mgs_check_index()).
  - and the LASSERT noted on line 504 fires before the peer is ever rejected.

Moving the nodemap check up to just after the `opc != LDD_F_OPC_REG` test would close all three: nothing the peer supplies gets acted on until it is known to be trusted.

Looks straight forward enough and resolves both issues.

(minor) Not a bug today, but `rc` now has to carry the mgs_check_index() result across ~60 lines that include a block which writes `rc` itself (`rc = PTR_ERR(nodemap)` above). It only works because that assignment is immediately followed by a GOTO. If the patch is refreshed, a separate `int index_rc` would make the intent obvious and stop a later edit in the nodemap block from silently changing what mgs_check_target() sees.

(defect?) Does this strand the peer on ln_dc_working?