[LU-2484] md_stats not used or used incorrectly

• Status: Resolved
• Project: Lustre
• Affects Version/s: Lustre 2.4.0
• Fix Version/s: Lustre 2.4.0, Lustre 2.5.0
• Type: Improvement
• Priority: Minor
• Reporter: John Hammond
• Assignee: John Hammond
• Resolution: Fixed
• Labels: patch, procfs

Description

The stats maintained by the EXP_MD_COUNTER_INCREMENT in the md_ops member wrappers and stored in struct obd_device's md_stats are not used. The classes that implement the the md_ops interface (lmv and mdc) do not allocate the them and the classes that allocate them (mgs and mdt) do not implement md_ops. (This is not a terrible issue but it does add to the internal and external cognitive crud of lustre proc.)

It should be easy to add these stats to lmv and mds but the value of doing so seems low. md_ops contains several groups of low-level function whose counts would be highly correlated and there are already counters in llite for the high level metadata operations.

If they are to be enabled anywhere then EXP_MD_COUNTER_INCREMENT() should check exp->exp_obd->md_stats rather than exp->exp_obd->obd_stats; lprocfs_alloc_md_stats() should check that obd->obd_type->typ_md_ops is not NULL; and exp_md_stats should be used somewhere or removed.

Comments

[Andreas Dilger]

John, with the advent of multiple MDTs, the stats per MDC will no longer be in lockstep with those at the llite/LMV level. It would be nice to have the per-MDC metadata stats available on the client (e.g. for http://review.whamcloud.com/4403) if you think they provide some value.

In any case, it definitely seems like there is some cleanup to do, and as always I'm appreciative of your efforts.

[John Hammond]

See http://review.whamcloud.com/4813 for a patch to remove md_stats from mgs and add some error handling to mgs proc initialization.

[John Hammond]

> John, with the advent of multiple MDTs, the stats per MDC will no longer be in lockstep with those at the llite/LMV level. It would be nice to have the per-MDC metadata stats available on the client

That seems fine to me. I had planned to do this in three parts:
1) Remove from mgs (see above)
2) Remove from mdt.
3) Remove common functions and members.

I'll go ahead with 1 and 2. Rather than just turning these on for lmv and mdc, I think the calls to EXP_MD_COUNTER_INCREMENT() should first be pruned. I don't believe that knowing the call count of mdc_init_ea_size() assists anyone in mentally modeling the behavior of lustre.

[John Hammond]

See http://review.whamcloud.com/4815 for the mdt patch.

[John Hammond]

And http://review.whamcloud.com/4827 for the mdc patch.

[John Hammond]

I don't think these stats should be added to MDCs or any other class. Before I say why here's what they will look like:

~# cat /proc/fs/lustre/mdc/lustre-MDT0000-mdc-ffff88020176ec00/md_stats
snapshot_time             1359065590.107282 secs.usecs
getstatus                 1 samples [reqs]
change_cbdata             2246 samples [reqs]
find_cbdata               2413 samples [reqs]
close                     157 samples [reqs]
create                    1667 samples [reqs]
enqueue                   12 samples [reqs]
getattr                   42 samples [reqs]
getattr_name              5 samples [reqs]
intent_lock               13239 samples [reqs]
link                      1 samples [reqs]
rename                    17 samples [reqs]
setattr                   113 samples [reqs]
readpage                  17 samples [reqs]
unlink                    537 samples [reqs]
getxattr                  155 samples [reqs]
init_ea_size              4 samples [reqs]
get_lustre_md             2857 samples [reqs]
free_lustre_md            2752 samples [reqs]
set_open_replay_data      157 samples [reqs]
clear_open_replay_data    157 samples [reqs]
set_lock_data             9897 samples [reqs]
lock_match                2306 samples [reqs]
intent_getattr_async      33 samples [reqs]
revalidate_lock           3375 samples [reqs]

Now imagine someone reading this and saying "Whenever my jobs run the clients show a lot of change_cbdata and revalidate_lock reqs. Is that bad?"

What would you say to them? I would say that I'm sorry I submitted a patch to enable these stats.

The point is that these stats are counts of calls, and not of high-level metadata operations that help admins and users model their filesystem.

[Keith Mannthey (Inactive)]

Have we thought about moving part of this stat data to a perf tracepoints sort of implementation with the idea to keep admin friendly items in /proc and developer data elsewhere?

[John Hammond]

Keith, this data is well used by admins. The big sites support a healthy cottage industry of lustre monitoring tools.

[John Hammond]

Patch landed to master.

[Jodi Levi (Inactive)]

Added 2.5.0 FixVersion