|
I looked through that patch and didn't see anything obvious that would introduce a performance regression, since none of those proc/seq files should be anywhere close to the IO path. I think the likely source of the problem is that the change from /proc to /sys/kernel/debug has resulted in some tunable parameter to be lost completely (random example readcache_max_io_mb), or is somehow "disconnected" from the actual parameter (e.g. the parameter can be set, but it doesn't actually change the behavior of the kernel).
There weren't any parameters that I could easily see, beyond the readcache_max_io_mb and related osd-ldiskfs parameters that could be the culprit.
|
|
it might be related to what cpu is quite busy rather than IO? A test case was 128 IOR process against 128 CPU cores and client's cpu was quite busy and I saw some ior threads were much slower than others in bad case. Let me dig in this detail.
|
|
I don't think any of the code changed by the patch would actually be used during operation. Are there any of tunable parameters set on the client or server that were modified by this patch? I think the most likely cause is that one of the modified parameter settings is not working properly. It might be possible to revert part (ideally half) of the parameter changes to see if that avoids or reintroduces the problem, to isolate it down to a specific part of the patch?
|
|
I only reverted patch on client and performance was back.
Also, what parameters applied was below. No other parameters applied on both server and client.
|
|
Any thoughts on this one? Should we just revert this patch while we are working out the details?
|
|
I looked through the patch again and I do not see anything obvious.
It would be fine to revert this for now.
|
|
I can't reproduce this problem. Is it only a Ubunut issue? Can you reproduce it Shaun? Does it break mpirun or mdtest / IOR. Lustre is only used by mdtest to call llapi_dir_set_default_lmv_stripe() for the -g option. What setup do you have ? Everything on one node or all VMs?
|
|
No I don't have a reproducer here. My ubuntu testing is just a couple of VMs.
If it is 5.3/4 kernel related and not specific to Ubuntu then I expect I will have something available soonish but not immediately.
|
|
Shaun, would you be able to post a handful of different patches that revert different parts of patch https://review.whamcloud.com/37834 to see if Ihara can isolate which part is slowing things down?
|
|
Sounds good. I will post a series of reverts.
|
|
Shaun Tancheff (shaun.tancheff@hpe.com) uploaded a new patch: https://review.whamcloud.com/40833
Subject: LU-14055 all: Enable debugfs macros fallback to PDE_DATA
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 6ef6b0b9b9ee670930a7a1863f4903f783f58055
|
|
Shaun Tancheff (shaun.tancheff@hpe.com) uploaded a new patch: https://review.whamcloud.com/40835
Subject: LU-14055 all: Revert LU-13344 1 of 7
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 3a42232a0a536758121cd1ebcb4c73334d1d898f
|
|
Shaun Tancheff (shaun.tancheff@hpe.com) uploaded a new patch: https://review.whamcloud.com/40836
Subject: LU-14055 all: Revert LU-13344 2 of 7
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: e2c1be578c090153aaaa752ac8bad2f6668923d2
|
|
Shaun Tancheff (shaun.tancheff@hpe.com) uploaded a new patch: https://review.whamcloud.com/40837
Subject: LU-14055 all: Revert LU-13344 3 of 7
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: a650c8a62227589adc940b53a0432d28a4276274
|
|
Shaun Tancheff (shaun.tancheff@hpe.com) uploaded a new patch: https://review.whamcloud.com/40838
Subject: LU-14055 all: Revert LU-13344 4 of 7
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: e6b994fbaf5e0b7a6eef7266c6f9b0836115cbad
|
|
Shaun Tancheff (shaun.tancheff@hpe.com) uploaded a new patch: https://review.whamcloud.com/40839
Subject: LU-14055 all: Revert LU-13344 5 of 7
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: a8b7b7877beea237164312f5581fe21919df87b9
|
|
Shaun Tancheff (shaun.tancheff@hpe.com) uploaded a new patch: https://review.whamcloud.com/40840
Subject: LU-14055 all: Revert LU-13344 6 of 7
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 43fdb7aa30367545452d242e41b2b95fbd6832f4
|
|
Shaun Tancheff (shaun.tancheff@hpe.com) uploaded a new patch: https://review.whamcloud.com/40841
Subject: LU-14055 all: Revert LU-13344 7 of 7
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: ca7e834e5472c520b8a4443fd13d44fdacd230a6
|
|
Shuichi, could you please give these patches a try to help isolate the source of the regression.
|
|
Shuichi I pushed a fixed patch set that Shaun started. Please give it a try. Some of the RHEL8.2 builds failed due to an unrelated bug. Those patches do build as you can see in later patches building properly.
|
|
Thank you James! I will test new patches on my test node and let you know.
|
|
Here is test results after reverted patch. ("Write" performance only measured) So, it still needed to revert all patches to get the performance back.
I didn't test 3,2,1 of 7 patch since regression already started when 6 of 7 patch reverted. should we split a bit 7 of 7 patch to narrow down?
|
|
Shuichi, just to confirm, the "good" number beside "7 of 7" is with all 7 of the patches applied (ie. the entire original patch reverted), and if the "7 of 7" patch is removed then the performance is bad?
What is strange is that the "7 of 7" patch is not really doing much except renaming a few structs and adding one variable near the end of struct obd_device. I had previously wondered if the increase in struct obd_device size was causing something bad with cache lines on SMP systems, or the struct became too large for some reason (it is just over 4KB now)?
You could test this by reverting all of the patches, the only adding a "void *dummy;" pointer where "struct ldebugfs_vars *obd_debugfs_vars" is located in the "7 of 7" patch in https://review.whamcloud.com/#/c/40841/2/lustre/include/obd.h
Other than that, I can't see anything in the "7 of 7" patch that would cause any problems. The /proc files changed are for "fid" and "fld" services, which I don't think are being tuned at all, so it shouldn't matter even if the tunables were broken.
|
Shuichi, just to confirm, the "good" number beside "7 of 7" is with all 7 of the patches applied (ie. the entire original patch reverted), and if the "7 of 7" patch is removed then the performance is bad?
That's correct. Once patch "7 of 7" applied (reverted entire LU-13344 patch), the performance was good.
You could test this by reverting all of the patches, the only adding a "void *dummy;" pointer where "struct ldebugfs_vars *obd_debugfs_vars" is located in the "7 of 7" patch in https://review.whamcloud.com/#/c/40841/2/lustre/include/obd.h
okey, I will test this idea.
|
|
I noticed that even before the problematic patch is applied, there are already some places in the code using "ldebugfs" that are affected by the changes in the patch. Most places are fid, fld, seq, which I don't think can be harmful. There is a bit of usage in ldlm, but it doesn't look harmful either.
One important parameter that may be affected is llite.*.max_cached_mb".
Shuichi, the other thing you could try with and without the patch applied is "lctl get_param -R '*'" to see if the parameters available changes between the two versions?
|
|
For the test runs, was there a default striping set on the filesystem?
|
For the test runs, was there a default striping set on the filesystem?
yes, stripe count was 1 for all tests.
|
You could test this by reverting all of the patches, the only adding a "void *dummy;" pointer where "struct ldebugfs_vars *obd_debugfs_vars" is located in the "7 of 7" patch in https://review.whamcloud.com/#/c/40841/2/lustre/include/obd.h
Andreas, here is test resutls of your suggestion. And it seems that results tells us a root cause that you exaclty pointed out. it was tested 3 times to verify.
| Write(MB/s) |
"7 of 7" revert patch |
"7 of 7" revert patch + adding "struct ldebugfs_vars *obd_debugfs_vars" |
| iteration1 |
36.225 |
19.280 |
| iteration2 |
35.410 |
18.781 |
| iteration3 |
33.871 |
19.033 |
|
|
If it isn't too much trouble (and if you didn't already) can you try adding that field at the end of the structure, rather than where it currently is?
If that makes a difference, then my guess is that there is a hot cache-line in that struct which is being split.
If it doesn't make a difference, then my guess is that the struct size crosses a boundary resulting in obd_device_cache being shared with a different other cache, which maybe doesn't something strange. I doubt that though.
|
|
I should have to inform that adding a "void *dummy;" top of "7 of 7" revert patch also caused same performance problem, then tested adding "struct ldebugfs_vars *obd_debugfs_vars" after of that.
|
|
Very strange that adding an 8-byte pointer to the already-large struct obd_device causes such a huge performance problem. According to pahole:
} __attribute__((__aligned__(8)));
Maybe it pushes the struct from 106 cachelines to 107, but I wouldn't think that would make a huge difference in performance. We could pack the two 4-byte holes in the struct to get back 8 bytes to solve this problem quickly before 2.14. That means the next patch that touches this struct is going to cause problems again, but would also give us some breathing room to resolve this issue more completely in 2.15. I will make a patch for this.
pahole shows a number of other major offenders in the struct that could be cleaned up:
struct rhashtable obd_uuid_hash;
struct rhltable obd_nid_hash;
struct obd_llog_group obd_olg;
struct hrtimer obd_recovery_timer __attribute__((__aligned__
(8)));
union {
struct obd_device_target obt;
struct filter_obd filter;
struct ost_obd ost;
struct echo_obd echo;
struct client_obd cli;
struct echo_client_obd echo_client;
struct lov_obd lov;
struct lmv_obd lmv;
} u;
There seems to have been a lot of cruft added to obd_device over the years that is server-specific and could be moved into struct obd_device_target instead of being kept in the common struct:
- obd_nid_hash usage is already under HAVE_SERVER_SUPPORT and could be declared that way also. I have a patch for this. Moving it into obd_device_target would be better since it would also reduce memory usage on clients that are built with server support.
- obd_uuid_hash entry is also only needed on the server. While it is referenced by __class_new_export() (also used on the client for "self" exports), it should only really be needed for targets that add remote connections. The whole block that checks obd_uuid_equals(cluuid, obd_uuid) could be under HAVE_SERVER_SUPPORT to allow obd_uuid_hash to also be moved into obd_device_target, along with obd_uuid_add() and obd_uuid_del(). I have a patch for this.
- obd_olg is larger than it needs to be, since many of the LLOG_*_CTXT contexts are no longer used (at least LLOG_MDS_OST_REPL_CTXT, LLOG_SIZE_ORIG,REPL_CTXT, and LLOG_TEST_REPL_CTXT.
LU-5218 explains some of the details. OST_SZ_REC which seems to imply that LLOG_SIZE_ORIG_CTXT is used is itself not actually set anywhere but test code, and was never used in production (it was for an old Size-on-MDS implementation that was never finished). Removing the unused LLOG_*_CTXT constants will not directly shrink LLOG_MAX_CTXTS, but may allow a more efficient mapping to be used (e.g. a simple mapping to an in-memory dense enum).
- obd_recovery_timer is only needed on the server and can move into obd_device_target, along with all of the other obd_recovery_*, obd_replay_*, obd_lwp_export, obd_exports_timed, obd_eviction_timer, obd_*_clients, and at least some of the obd_*transno* fields (though clients may use some of them). The target_*() functions under HAVE_SERVER_SUPPORT in ldlm_lib.c should all be moved into lustre/target/tgt_recovery.c (or similar) since they don't really have anything to do with LDLM.
The worst offender is the device-specific union u, with client_obd and lmv_obd being the largest members (though obd_device_target may increase in size in the future). One option is to dynamically allocate this member depending on the type used, since there is typically only one lmv_obd on a client, though client_obd is used for most of the devices on the client so will not help much.
The (almost only) offender in lmv_obd is struct lu_tgt_descs lmv_mdt_descs, which is a static array for all of the potential MDT devices the LMV may have (struct lu_tgt_desc_idx *ltd_tgt_idx[TGT_PTRS] being the major contributor). This is really unnecessary, and could be dynamically allocated for the maximum current MDT count. I have included that into patch https://review.whamcloud.com/40901 "LU-13601 llite: avoid needless large allocations" but it deserves to be split into its own patch, since making ltd_tgt_index dynamically sized is relatively easy compared to the llite changes also in that patch. I have a patch for this already.
The worst offender in client_obd are the seven obd_histogram fields, consuming 1848 of 2664 bytes, with 34 bytes of holes. I had a patch to dynamically allocate these structures on an as-needed basis, but it was complex and never landed. Maybe I need to revive that patch again.
|
|
As for fields that might be affected by the addition of obd_debugfs_vars there are only a few that are after this new field, and of those only a subset are used on the client:
struct ldebugfs_vars * obd_debugfs_vars;
atomic_t obd_evict_inprogress;
wait_queue_head_t obd_evict_inprogress_waitq;
struct list_head obd_evict_list;
rwlock_t obd_pool_lock;
__u64 obd_pool_slv;
int obd_pool_limit;
int obd_conn_inprogress;
struct lu_ref obd_reference;
struct kset obd_kset;
struct kobj_type obd_ktype;
struct completion obd_kobj_unregister;
Likely candidates might be obd_pool_slv being split from obd_pool_lock, or something in the recently-added obd_kset or obd_ktype fields. I'm hoping that shrinking obd_device will resolve the problem, since random field alignment issues causing such huge performance swings is a nightmare.
|
|
Andreas Dilger (adilger@whamcloud.com) uploaded a new patch: https://review.whamcloud.com/41161
Subject: LU-14055 obdclass: fill hole in struct obd_device
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 29898c50da924ad146b71046ee066371058cbb6a
|
|
Andreas Dilger (adilger@whamcloud.com) uploaded a new patch: https://review.whamcloud.com/41162
Subject: LU-14055 lmv: reduce struct lmv_obd size
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 4f6159a3f2175cdcb8eee4017ba8e0a3d70268f2
|
|
Shuichi, could you please try with these two patches. The first one just decreases the struct size by 4 bytes, but I'm not sure if that will be enough. The second reduces it by over 2KB, which hopefully is enough. Otherwise, it may be that there is an alignment issue with some struct (likely the new "obd_k*" ones at the end) that will need some specific alignment requests.
|
|
I cannot tell if this is already understood but it would be useful to know if the change in performance is due to the debugfs changes being applied to the client, to the server, or both.
|
|
John, it is on the client only, AFAIK.
Shuichi, could you please run "pahole" on the obdclass.ko module with and without the problem and attach it here. That program is part of the "dwarves RPM.
|
|
Andreas,
I just tested two patches, but the performrance was even worse below.
|
|
Andreas Dilger (adilger@whamcloud.com) uploaded a new patch: https://review.whamcloud.com/41178
Subject: LU-14055 obdclass: move obd_debugfs_vars to end obd_device
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: dd0a0df383387e6455bbad565503883433516454
|
|
Shuichi, if shrinking struct obd_device does not solve the problem, then it seems the problem is caused by a misalignment of some data structure that follows the added obd_debugfs_vars field.
Can you please try another set of tests that move the "obd_debugfs_vars" line until we isolate the problematic field. The first test would be to move obd_debugfs_vars to the end of the struct:
- struct ldebugfs_vars * obd_debugfs_vars;
atomic_t obd_evict_inprogress;
wait_queue_head_t obd_evict_inprogress_waitq;
struct list_head obd_evict_list;
rwlock_t obd_pool_lock;
__u64 obd_pool_slv;
int obd_pool_limit;
int obd_conn_inprogress;
struct lu_ref obd_reference;
struct kset obd_kset;
struct kobj_type obd_ktype;
struct completion obd_kobj_unregister;
+ struct ldebugfs_vars * obd_debugfs_vars;
to see if this solves the problem (without my other patches). I've pushed a patch to do this. If it fixes the problem, then this confirms that the problem is caused by the alignment or cacheline contention on of one of the fields between obd_evict_inprogress and obd_kobj_unregister. This would be enough to land for 2.14.0 to solve the problem, but I don't want to leave the reason for the problem unsolved, since it is likely to be accidentally returned again in the future (e.g. by landing my patches to shrink lu_tgt_desc or anything else).
To isolate the reason for the problem you would need to "bisect" the 11 fields/366 bytes to see which one is causing the slowdown.
First try moving obd_debugfs_vars after obd_kset to see if this causes the slowdown again. If not, then the problem is obd_kset or earlier, so try moving it immediately before obd_kset (this is the largest field so makes it difficult to "bisect" exactly). If the problem is still not seen, move it after obd_evict_list, etc. Essentially, when obd_debugfs_vars is immediately before the offending struct the performance will be bad, and when it is immediately after the struct then the performance problem should go away. Once you find out what the structure is, try moving that field to be at the start of struct obd_device so that there is no chance of it being misaligned, after obd_lu_dev and after obd_recovery_expired. If these also show good performance, then this can be a permanent solution (I would prefer after obd_recovery_expired since these bitfields are very commonly used).
Please run the "pahole" command on the obdclass.ko module to show the "good" and "bad" structures to see what the problem is, and attach the results here.
Neil, James, since the obd_kobj and obd_ktype fields are recent additions and the largest fields in this area, it seems likely that they are the culprit here. Is there anything "special" about them that would require their alignment, or to avoid cacheline contention? Are they "hot" and referenced/refcounted continuously during object access?
|
|
patch: https://review.whamcloud.com/41178 didn't help and it was still same slow down. let me try your "bisect" ideas to find if it shows good (or still bad) performance.
|
|
Strange that the patch to reduce the structure to the previous size (-8 bytes) and the patch to move the new 8-byte field to the end didn't help performance and the patch to reduce the struct size -2000 bytes also didn't help. It is possible my "-8 byte" patch was too complex, and I moved too many fields around? I had tried to keep the related fields together.
Do you have any idea where the performance is being lost? Is it high CPU usage or lock contention or something else?
|
|
Attached are two frame graphes on good (reverted whole LU-13344 patches) and bad (master) cases.
I see huge amount of spin_locks in osc_queue_sync_pages() under ll_direct_IO() in bad case. There are also several spin_lock impacts in the functions under brw_interpret().
|
|
It looks like the contended lock in master.svg is cl_loi_list_lock in both osc_queue_sync_pages() and brw_interpret(). The cl_loi_list_lock is in struct client_obd. What is strange is that this field is before the added structure, so I can't see how that would be affected by the new field. I'll have to check if there is something later in obd_device that is being accessed under that lock.
|
|
> Attached are two frame graphes on good (reverted whole LU-13344 patches) and bad (master) cases.
How long did perf run for the two flame graphs? One has 5x the samples as the other. master-revert.svg shows 20% in swapper and 5% in open. Had ior completed startup when perf was run?
|
|
Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/41162/
Subject: LU-14055 lmv: reduce struct lmv_obd size
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: e11deeb1e6d114608eac4ee998d4cea22e30b0f5
|
|
Ihara,
Ignoring entirely the apparent memory layout issue, I have two suggestions that may reduce the pain on cl_loi_list_lock.
This patch just landed to master:
https://review.whamcloud.com/38214
And this one which has not yet landed:
https://review.whamcloud.com/#/c/39482/
If you're able to give these a try, that would be very interesting. Both should cut the contention on cl_loi_list_lock when doing dio. I'm not sure precisely what effect they'll have here, but it seems worth trying.
|
|
tappro made this comment over on LU-14580, and I wanted to bring it here:
"I don't see problems with patch itself. Increment in osc_consume_write_grant() was removed because it is done by atomic_long_add_return() now outside that call and it is done in both places where it is called. But maybe the patch "LU-12687 osc: consume grants for direct I/O" itself causes slowdown? Now grants are taken for Direct IO as well, so maybe that is related to not enough grants problem or similar. Are there any complains about grants on client during IOR run?"
That patch definitely has performance implications. Direct i/o will keep sending even when there are no grants - since it is already synchronous - but it significantly increases the load on the cl_loi_list_lock in some cases. The patches I noted above are aimed at that.
There's still very likely a memory layout issue here, but perhaps these will help...
|
|
Ihara,
FYI I had to rebase that second patch above:
https://review.whamcloud.com/#/c/39482/
It was not compatible with current master, should be fixed now.
|
|
sihara can this issue now be considered fixed?
|
|
My understanding is this issue is resolved, though we never fully understood it. Let me knw if that's wrong and we can reopen or open a new ticket.
|
Generated at Sat Feb 10 03:06:27 UTC 2024 using Jira 9.4.14#940014-sha1:734e6822bbf0d45eff9af51f82432957f73aa32c.
master-revert.svg
