[LU-7334] Refactor lprocfs helpers

• Status: Resolved
• Project: Lustre
• Fix Version/s: Lustre 2.9.0
• Type: Improvement
• Priority: Minor
• Reporter: Giuseppe Di Natale (Inactive)
• Assignee: Emoly Liu
• Resolution: Fixed
• Labels: llnl, patch

Description

The lprocfs helper functions for parsing __u64 and ints from strings are currently a bit messy. These should be re-written to leverage a single set of internal functions. This change will replace these helpers with a small set of new helpers that will return an __s64. Any functions calling the old helpers will be changed to accommodate their replacement.

Issue Links

• is related to: LU-8139 sanity test_101g: 'max_pages_per_rpc ...
• is related to: LU-8338 lov.*-clilov-*.stripe* params not con...
• is related to: LU-9611 fix default stripe count/offset proc ...
• is related to: LU-9091 Replace lprocfs_str_with_units_to_s64...

Comments

[Giuseppe Di Natale (Inactive)]

I will be submitting a patch for testing this shortly. No need for code reviews yet.

[Gerrit Updater]

Giuseppe Di Natale ([e-mail]) uploaded a new patch: http://review.whamcloud.com/16930
Subject: LU-7334 lprocfs: Refactored string to value helpers
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: fb65e6b0b6ec11cc2521e32396fa1ccd4855d434

[Peter Jones]

Emoly

Could you please review and look after this patch?

Thanks

Peter

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch http://review.whamcloud.com/16930/
Subject: LU-7334 lprocfs: Refactored string to value helpers
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 1cd2189806c82c31256c5ef9756d1dbf97784844

[Emoly Liu]

Landed for 2.9.0.

[Andreas Dilger]

We've noticed some regressions with this patch, in particular LU-8139 was causing sanity failures because both this http://review.whamcloud.com/16930 patch and http://review.whamcloud.com/19368 were tested and passed independently, but caused failures when both were landed.

It seems that several /proc files that previously were using lprocfs_write_u64_helper() and accepted a unit suffix have been changed to lprocfs_str_to_s64() and no longer accept units. These include:

lod_stripesize_seq_write()
lod_stripeoffset_seq_write()
lov_stripesize_seq_write()*
lov_stripeoffset_seq_write()*
osc_cur_grant_bytes_seq_write()
osc_obd_max_pages_per_rpc_seq_write()+
ldiskfs_osd_readcache_seq_write()

[*] The "lov_stripesize_seq_*()" tunables are non-functional since 2.4.0 and should just be deleted, they were replaced by the "lod_*" equivalents.
[+] Oleg has already submitted a patch to fix osc_obd_max_pages_per_rpc_seq_write() (http://review.whamcloud.com/20243) since this was causing sanity.sh to fail.

The others are just potential sources of failures that should be fixed to avoid problems with end-user configurations that are setting these parameters.

[Giuseppe Di Natale (Inactive)]

Does this imply that we need a version of the function which doesn't accept a default unit, but will allow for units to be present in the string? The reason I ask is because Oleg's change appears to be a work around to have a default multiplier of 1, unless of course there is a unit in the string. I really don't like the idea of passing in '%' as the work around to accomplish a default multiplier of 1.

The other option is to have a new unit 'B'/'b' for bytes with a multiplier of 1, but that can clash with the hex value B. Also, if we allow the new unit, we could disallow units to be part of hex strings? It seems unlikely that a hex value would represent a quantity, but I could be wrong.

[Andreas Dilger]

Also, "b" is used in some cases to mean "blocks", which would be confusing. Since the default unit is passed as a separate string, using "1" for the units would work and also be clear?

[Frank Zago (Inactive)]

I also found the following failure:

# echo 1000 > /proc/fs/lustre/mdt/lustre-MDT0000/hsm/max_requests
-bash: echo: write error: Invalid argument

No weird values involved. That worked fine up to that patch.

[Giuseppe Di Natale (Inactive)]

That's because my processing function doesn't allow a newline before the null terminator. I will fix that too.

[Gerrit Updater]

Giuseppe Di Natale ([e-mail]) uploaded a new patch: http://review.whamcloud.com/20353
Subject: LU-7334 lprocfs: Allow a new line before null terminator
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 9f3e8fca14009bf0372a470f333a68e7e6e4c9b9

[Giuseppe Di Natale (Inactive)]

I'm not sure a "1" unit is the way to go either. Since using 'B/b' is appearing to be some what confusing, I'm leaning towards a version of lprocfs_str_with_units_to_s64 that has no default unit, meaning it is expecting just a number (multiplier is 1 by default) and will parse a unit if it encounters one. But even that I'm not 100% happy with.

[Christopher Morrone]

I like using '1' for the units better than creating a new function.

[Giuseppe Di Natale (Inactive)]

Andreas, when you say remove the "lov_stripesize_seq_*()" tunables, you mean removing the proc file as a whole, correct?

[Andreas Dilger]

Correct. These /proc tunables were replaced on the MDS with "lod_stripesize_seq_()" and the "lov_" code is now only used on the client, where these tunables are totally useless.

[Gerrit Updater]

Giuseppe Di Natale ([e-mail]) uploaded a new patch: http://review.whamcloud.com/20455
Subject: LU-7334 lprocfs: Few proc files are numbers, but use units.
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: e8bb349c927eced4c927850ce69a981ac38664a1

[Giuseppe Di Natale (Inactive)]

Should I introduce a change to re-enable sanity test 101g?

[Gerrit Updater]

Giuseppe Di Natale ([e-mail]) uploaded a new patch: http://review.whamcloud.com/20465
Subject: LU-7334 lproc: Remove lov_stripesize and lov_stripeoffset
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 2475a20d42b6d423578f35953d260897ac466da5

[Andreas Dilger]

The enabling of test 101g is being done via http://review.whamcloud.com/20281

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch http://review.whamcloud.com/20353/
Subject: LU-7334 lprocfs: Allow a new line before null terminator
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: db44a9e35a9c115d69bca82014ceec4c5fc336dd

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch http://review.whamcloud.com/20465/
Subject: LU-7334 lov: Cleanup lov_stripe proc files
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: d34b9dfbebc8d313be9c12a96b660a34ad45edcb

[Gerrit Updater]

Oleg Drokin ([e-mail]) uploaded a new patch: http://review.whamcloud.com/21129
Subject: Revert "LU-7334 lov: Cleanup lov_stripe proc files"
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: dd2bbdb66a2f5a43b63dd5a42f1702a652af9ffd

[Giuseppe Di Natale (Inactive)]

Just noticed http://review.whamcloud.com/20465/ caused some issues. Should that change be abandoned?

[James A Simmons]

I thought setting the default lov with proc was a bad idea? Patch 20465 seems to not break anything but just adds a bogus error at client mount time.Is this not true?

[Giuseppe Di Natale (Inactive)]

It was reverted. From the description in LU-8338, the error at mount time might be harmless.

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch http://review.whamcloud.com/20455/
Subject: LU-7334 lprocfs: Allow default multiplier of 1
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 2fc4838533668135d2efe98b90e56a9ac5dadaea

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch http://review.whamcloud.com/21129/
Subject: Revert "LU-7334 lov: Cleanup lov_stripe proc files"
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 4af430de697249d07bcb7fe3401a6caa4415fe31

[Peter Jones]

It looks like all patches have landed for 2.9 now