[#LU-5030] "lctl {get,set}_param" should also check in /sys/fs/{lnet,lustre}

[LU-5030] "lctl {get,set}_param" should also check in /sys/fs/{lnet,lustre} Created: 08/May/14 Updated: 31/Mar/17 Resolved: 04/Feb/16
Status:	Resolved
Project:	Lustre
Component/s:	None
Affects Version/s:	None
Fix Version/s:	Lustre 2.8.0

Type:

Bug

Priority:

Critical

Reporter:

Andreas Dilger

Assignee:

Yang Sheng

Resolution:

Fixed

Votes:

Labels:

usk

Issue Links:

Related
is related to	~~LU-2633~~	Base sysfs support	Closed
is related to	~~LU-6215~~	Sync Lustre external tree with lustre...	Resolved
is related to	~~LU-7437~~	"lctl list_param -R" can't list the p...	Resolved
is related to	~~LU-4768~~	ost-survey hangs on client 2.4	Resolved
is related to	~~LU-7757~~	Interop master<->2.7.1 - sanity-sec t...	Closed
is related to	~~LU-5330~~	remove deprecated full path tunable a...	Resolved

Sub-Tasks:

Key

Summary

Type

Status

Assignee

LU-5330

remove deprecated full path tunable a...

Technical task

Resolved

Nathaniel Clark

Severity:

Rank (Obsolete):

13911

Description

In the upstream Linux kernel there is a requ irement to move files out of /proc/fs/lustre and into /sys/fs/lustre (lnet too). In order to facilitate this move it makes sense to start checking for files in /sys/fs/lustre in addition to the current glob that is used.

This will allow moving incrementally (e.g. ET first

Comments

Comment by John Hammond [ 08/May/14 ]

This reminds me that our e2fsprogs uses "/proc/fs/lustre" and has several "/proc/fs/lustre" paths hard coded in the source.

Comment by James A Simmons [ 08/May/14 ]

Can I ask you a favor. The new guy I have been training at ORNL to be a Lustre developer has been working on this project. He is on vaction but should be back in the next few weeks. Is it okay for him to finish this work.

Comment by James A Simmons [ 08/May/14 ]

The sysfs port work for LNET has already been done. Please link ~~LU-2633~~ to this one.

Comment by Andreas Dilger [ 09/May/14 ]

James, that would be very much appreciated.

I'd ask that the patch to fix "lctl get_param" be done first, so it can go into the released versions earlier, and then the work to actually convert over to sysfs can be done on master and on upstream clients in parallel.

Comment by Andreas Dilger [ 12/May/14 ]

This is a patch that I had partially completed in my tree, and is probably a good starting point for this work: http://review.whamcloud.com/10300

Comment by James A Simmons [ 30/May/14 ]

Do you want the work to be based on top of that patch or be added to this patch?

This week I looked at the pros and cons of going to sysfs. The one con I see for going to sysfs is we end up with the one page for data issue that we had with the old proc handling. Going to seq_files now allows use to use more than one page for the data buffer.
Now we could work around this by using debugfs. The second con is the very nature of sysfs. Sysfs is designed with a single data item per file. Most of lustre can easily be converted over but we have things like import which would need to be broken up.

Chao is back from vacation so I will be meeting with him today to discuss this work. He also has a patch that moves nearly all the test suite from accessing proc directly. The only test we could not port over is sanity 900 due to lctl not being able to handle setting multiple proc entries at the same time. This is the same issue ~~LU-3970~~ was created for but in that case it was to handle parallel access to ldlm drop_cache.

Comment by Andreas Dilger [ 03/Jun/14 ]

I think the 10300 patch is a good starting point for changes to the lctl and liblustreapi code to consolidate pathname handling, instead of having it spread throughout the code. Ideally, this would eventually result in usable llapi_get_param() and llapi_set_param() functions which can be used by lctl, lfs, and other applications that hide the details of the interface and the location of the files in /proc or /sys or /debugfs or whatever.

As for sanity.sh test_900, I'm not clear on why cancel_lru_locks() is not usable for the MGC. Ah, it seems that the filename is /proc/fs/lustre/ldlm/namespaces/MGC192.168.20.1@tcp/lru_size which has '.' in the filename, which clashes with lctl get_param and lctl set_param using '.' as a separator. One solution would be to have "lctl get_param" print these parameter names with the '.' escaped like '
.' but that also can have problems if it is being evaluated multiple times (e.g. via remote shell). Alternately, the MGC proc name could be printed without '.' in the filename, though that wouldn't be consistent with other /proc files that print a NID.

Comment by Ryan Haasken [ 04/Jun/14 ]

Perhaps I'm missing something, why does cancel_lru_locks in test-framework.sh do this:

    for d in `lctl get_param -N ldlm.namespaces.*.lru_size | egrep -i $1`; do
        $LCTL set_param -n $d=clear
    done

instead of just doing an "lctl set_param" with a wildcard? Like this:

$LCTL set_param ldlm.namespaces.*$1*.lru_size

Sure, cancel_lru_locks would no longer be case insensitive, but is that really important? If it is important, the input to cancel_lru_locks() could be coerced into the right letter case beforehand.

Comment by James A Simmons [ 06/Jun/14 ]

I update the base patch but there is much more work to do. Currently I'm thinking of using llapi_target_iterate as the base for llapi_[g|s]set_param. I'm also looking at using it for get_param_obdvar.

Comment by Yang Sheng [ 17/Sep/14 ]

Hi, James, Could you please give a status update for this ticket? TIA.

Comment by James A Simmons [ 17/Sep/14 ]

In the latest version of patch 10300 the function llapi_get_param was defined. This function returns a 2D array of data the user can the parse for what he/she needs. This is a good step forward and can be used to define the needed functions for listing parameters and setting parameters. A few bugs remain in the patch which I have handed off to the other Lustre developer at ORNL, Chao. He will be carrying the rest of the work forward for me. Chao can you give a update were you are at?

Comment by Chao Wang [ 18/Sep/14 ]

There are some bugs with the latest patch (patch set 7) of patch 10300 that I'm working on right now. After these bugs are fixed, would work on the functions of llapi_set_param and llapi_list_param. After then, need to fix the hard-coded /proc/fs/lustre in a few .c files by replacing them with the calls to these new functions.

Comment by Gerrit Updater [ 05/May/15 ]

James Simmons (uja.ornl@yahoo.com) uploaded a new patch: http://review.whamcloud.com/14675
Subject: ~~LU-5030~~ utils: create a libcfs function to handle tunable paths
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: cd1cf0b72ddcdaaf32ef7687cd0061b3b860a33c

Comment by Dmitry Eremin (Inactive) [ 18/May/15 ]

I revised http://review.whamcloud.com/14675/ patch. Now it should work with /proc, /sys and /sys/kernel/debug file systems. I think it's an intermediate step forward. This patch aggregates all information about proc tree in one function, but we also have other parameters that can not be reached for now. For example, "/sys/module/libcfs/parameters/*". I'd like to use them also but we have explicit name "lnet/" or "lustre/" in pattern for all requests to cfs_get_procpath(). While we elaborating 10300 patch could we take this into account and remove those names? Could we append those name always in llapi_get_param() or we should have a separate API for lustre and lnet parameters? Does it make sense to have a separate API for them at all or all parameters should be common? What do you think about this?

Comment by Gerrit Updater [ 20/May/15 ]

Dmitry Eremin (dmitry.eremin@intel.com) uploaded a new patch: http://review.whamcloud.com/14887
Subject: ~~LU-5030~~ utils: create llapi_get_param function
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: f8d7ae7a6e58a1fa870c320af53284287558bef9

Comment by James A Simmons [ 02/Jul/15 ]

I started to work on this again. Will have a update in a few days.

Comment by Oleg Drokin [ 05/Jul/15 ]

I tried the current patch http://review.whamcloud.com/#/c/14675/ and it works with current staging tree.

Due to the way llite/lov was reimplemented (it used to be llite/lov/common_name that contained lov name which is way too convluluted, so tha was replaced with llite/lov being a symlink to the proper lov dir) we either need to adopt this same change in our tree and update the test scripts or just update the test scripts with something like (this is what I am running now):

diff --git a/lustre/tests/sanity.sh b/lustre/tests/sanity.sh
index 0a9e496..b25dd83 100644
--- a/lustre/tests/sanity.sh
+++ b/lustre/tests/sanity.sh
@@ -95,6 +95,10 @@ assert_DIR
 MDT0=$($LCTL get_param -n mdc.*.mds_server_uuid |
        awk '{ gsub(/_UUID/,""); print $1 }' | head -n1)
 LOVNAME=$($LCTL get_param -n llite.*.lov.common_name | tail -n 1)
+if [ -z "$LOVNAME" ] ; then
+LOVNAME=$(readlink /sys/fs/lustre/llite/*/lov)
+LOVNAME=$(basename $LOVNAME)
+fi
 OSTCOUNT=$($LCTL get_param -n lov.$LOVNAME.numobd)
 STRIPECOUNT=$($LCTL get_param -n lov.$LOVNAME.stripecount)
 STRIPESIZE=$($LCTL get_param -n lov.$LOVNAME.stripesize)

(similar patch is also needed for sanity-quota.sh)

Comment by James A Simmons [ 06/Jul/15 ]

That is still a complicated way to do things. Plus if you have more than one file system LOVNAME might fetch the wrong one since only the first answer is respected. Why not set LOVNAME=lov.$FSNAME-clilov-* or just plain LOVNAME="lov-~~clilov~~" when you don't care about which file system. Looking at the proc/sys tree IMO I don't see a need for lov or lmv in the llite directory tree. The lov/lmv subdirectory in llite and llite's uuid all match. All we have it common_name which is unique but we can use wildcards to the lctl command to do the same thing. I can update my patch with this change.

Comment by James A Simmons [ 06/Jul/15 ]

I updated the patch to remove the last bits of glob being used directly. This was especially true for lustre_cfg.c which defines the lctl [s|g]et_params functionality. Only place left to move away from glob is the perl script utilities. Also more clean ups can be done for the liblustreapi internal parameter helper functions. Also for some reason /sys/module/*/parameters/* still don't work. I'm debugging to figure out why. Feedback welcomed.

Comment by Oleg Drokin [ 06/Jul/15 ]

"llite.*.lov.common_name" would potentially match wrong fs too, I think, so it's ok to either way, no?

Also don't bother with module parameters just yet - I am installing debugfs symlinks there so we should be fine (assuming this is accepted, see the patch series that just went to the lists).

Comment by Oleg Drokin [ 14/Jul/15 ]

the symlinks pointing to module parameters did get accepted

So we can take them for granted assuming debugfs is enabled, and if it's not do we care enough about those configurations?

Comment by Andreas Dilger [ 20/Oct/15 ]

I needed to update one of my MythTV clients to run the vanilla 4.2.3 kernel to support a new motherboard chipset/CPU. The good news is that the vanilla 4.2.3 Lustre client mounts my 2.5.3 server w/o problem, the bad news is that this patch isn't landed yet so get/set the tunable parameters work, not even conf_param settings from the MGS config log, which is a bit strange (I didn't think lctl conf_param tunables depended on "lctl" working, only lctl set_param -P tunables?). I'll be able to test this patch when I have some spare time.

Comment by James A Simmons [ 29/Oct/15 ]

Sorry all our test resources have been devoted to DNE2 and 2.8 release testing. I'm hoping a new test system will be brought up very soon so this work can be completed.

Comment by Gerrit Updater [ 07/Nov/15 ]

Andreas Dilger (andreas.dilger@intel.com) uploaded a new patch: http://review.whamcloud.com/17081
Subject: ~~LU-5030~~ utils: add -R parameter to lctl get_param
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: a86b456de9fd9232c34155a266447e88de937001

Comment by Gerrit Updater [ 07/Nov/15 ]

Andreas Dilger (andreas.dilger@intel.com) uploaded a new patch: http://review.whamcloud.com/17082
Subject: ~~LU-5030~~ tests: remove /proc paths from tests
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 6fcb5eeaef446ce2fc03ccc12256382038a66770