|
Did this happen on only 2.5.52, as in 2.5.51 servers were fine? Any chance you can arrive at the patch that introduced this with a bit of git bisect?
|
|
Cliff
Have you seen any performance drops like this on Hyperion?
Peter
|
|
At least 2.5.0 and 2.5.51 are also fine. It seems something happened between 2.5.51 and 2.5.52. I will try git bisect to find exactly commit which caused this performance differences.
2.5.0 client
2.5.51 client
|
|
Here is "git bisect" results.
File removal operation to shared directory
| commit |
Removal(ops/sec) |
result |
| 98ac0fe3a45dde62759ecaa4c84e6250ac2067f8(HEAD) |
46818 |
bad |
| 2.5.51 |
61824 |
good |
| e9a1f308b5359c2de1fda67816ef662ce727d275 |
45919 |
bad |
| cbab0aa32ed2d21f59aae3a28285b49802b734f2 |
46917 |
bad |
| 2b13169cd86b4868730f2c45432645b7d2cc0073 |
62137 |
good |
| a9ae2181f3efd811e17843ebf951b00fb9ea0366 |
63721 |
good |
| 12d2b04f2204bc087f380cb214a29c126f50d709 |
63157 |
good |
| b17d23fd01557c0e23f5c3b4eeea237c08fe2bc5 |
44786 |
bad |
| 55989b17c7391266740d68e3c62418e184364ed7 |
46392 |
bad |
55989b17c7391266740d68e3c62418e184364ed7 LU-3544 llite: simplify dentry revalidate
This commit is exactly point where we have been getting metadata performance regression.
And, for double check, I also tested curent HDAD of master branch with revert of LU-3544 patch. Here is result. The removal performance is back.
|
|
Lai
Are you able to comment?
Thanks
Peter
|
|
Hi Ihara, could you test with createmany and unlinkmany? I'm afraid it's not unlink performance drop, but mdtest causes file revalidation failure and relookup, because 55989b17c7391266740d68e3c62418e184364ed7 LU-3544 llite: simplify dentry revalidate only touches the code path of dentry revalidation. I'll do mdtest locally to reproduce this.
|
|
Hi Lai,
Sorry delay response on this. I have tested with createmany and unlinkmany on 16 clients and total 64 processes simultaneously.
Here is summary of results. The number was not impacted whether LU-3544 patch applied or not.
| |
iteration 1 |
iteration 2 |
iteration 3 |
| 2.5.52 |
45491 |
44416 |
44200 |
| 2.5.52/wo LU3544 patch |
44157 |
43648 |
44182 |
|
|
Hi Lai, any adviseses and updates of this?
|
|
I don't find any clue yet, will need more time on testing, I'll update the progress next week.
|
|
I tested on different setup, but I didn't see the unlink performance drop. If possible, could you use oprofile to find which function consumes more time for 2.5.52 client?
I noticed that you only tested with a small set of files (20000 total files) and iterated three times. Could you test with more files and only one iteration? And could you also test with one client to see if unlink gets slow?
|
|
Lai,
what kind of system did you test on? I suspect that this slowdown is only visible with a fast MDS and IB network and enough OSTs so that unlinking the OST objects is not the bottleneck. I don't know that changing the parameters of what is being tested is needed, since there is clearly a slowdown in this test which is significantly larger than the standard deviation between tests (-15000 unlinks/sec with stddev 1000).
Also, I think it is important to note that this is only an issue during unlink, and in fact with it is +3000 unlink/sec faster than 2.5.0/2.5.51 once the LU-3544 patch is reverted. However, it does appear that the open-for-read ("File read" with 0 bytes read) performance is +60000 open/sec faster with the LU-3544 patch applied, which is also important not to lose.
I suspect there is some subtle difference in the new ll_revalidate_dentry() code that is only triggering in the unlink case, possibly forcing an extra RPC to the MDS to revalidate the dentry just before it is being unlinked? Rather than spending time trying to reproduce the performance loss, it might make more sense to just get a debug log of unlink with and without the 55989b17c73912 patch applied and see what the difference is in the callpath and RPCs sent. Hopefully, there is just a minor change that can be done to fix the unlink path and not impact the other performance.
|
|
I tested on three testnodes in Toro, one client, one MDS, two OSS on same OST.
I was suspecting that in LU-3544 patch .revalidate just return 0 if dentry is invalid, and let .lookup to do real lookup, instead of lookup in .revalidate in old code, and this may introduce a small overhead. I'll double check the call trace of unlink to see whether there are extra lookups.
|
|
command like `mdtest -d /lustre/dir.0 -n 20000 -F -i 3` executed following syscalls on each file:
1. creat
2. close
3. stat
4. open
5. I/O
6. close
7. unlink
For old code, syscall open in step 4 called .revalidate(IT_OPEN), which opened file, and close in step 6 called .release and did close the file.
While for new code, .revalidate doesn't execute intent any more, but return 1 directly, and later ll_intent_file_open() will open file with MDS_INODELOCK_OPEN, so close in step 6 doesn't really close the file because open lock is cached. And in step 7 unlink needs to close file before unlinking file, and this is the cause of unlink performance drop.
IMHO this is not a real bug, because no extra RPC was sent, but because mdtest opened file twice, so in new code open lock is fetched. A possible fix might be to add a timestamp so .open can know just now .revalidate(IT_OPEN) was called, so no need to fetch open lock. But I'm not sure this is necessary.
|
|
Yes, even this might not be bug, we see perforamnce drop under mdtest IO senario at least. mdtest is one of major benchmark tool for metadata and it's one of metadata scenario. we would be keeping (at least) same performance with newer version of Lustre. If you have idea of workaorund, please share with us. I would like to test them.
|
|
During test I saw other places that can be improved to increase file creation, stat, and maybe read performance, and I composed two patches:
http://review.whamcloud.com/#/c/9696/
http://review.whamcloud.com/#/c/9697/
Would you apply these two patches and get some result?
|
|
sure, will test those patches very soon and keep you updates! Thanks a lot, again!
|
|
Lai, these patches are broken. can't copy file from local filesystem to Lustre.
This worked.
|
|
this is debugfile when the problem happens.
echo "+trace" > /proc/sys/lnet/debug
lctl debug_daemon start /tmp/debuglog 100
touch /tmp/a
cp /tmp/a /lustre
lctl debug_daemon stop
echo "-trace" > /proc/sys/lnet/debug
|
|
Thanks Ihara, patches updated, previously I only tested mdtest, and didn't do a full test because they are intended to get mdtest performance data, and may not be final patches yet, sorry for the trouble made.
|
|
Lai, it looks like the patches http://review.whamcloud.com/9696 and http://review.whamcloud.com/9697 are improving the open performance, but do not address the unlink performance. Is there something that can be done to improve the unlink performance back to the 2.5.0 level so that 2.6.0 does not have a performance regression?
|
|
The root cause is that revalidate(IT_OPEN) enqueued open lock, so that close is deferred to unlink which caused unlink performance drop, but totally there is no extra RPC. I don't find a clear way to handle this, so I think if we can improve open and stat performance a lot, it's worthwhile keeping the status quo.
|
|
It might be possible to combine the close and unlink RPCs (unlink with close flag, or close with unlink flag?) so that the number of RPCs is actually reduced? We already do something similar with early lock cancellation, so it might be possible to do something similar with the close.
|
|
I've thought of that, but considering the complication of open replay, and possibly SOM, I think it's not a trivial work. I'll think about it more and do some test later (maybe next week).
|
|
Patch to combine close in unlink RPC: http://review.whamcloud.com/#/c/10398/
Ihara, could you apply this only and get results from mdtest?
|
|
Hi Lai,
it seems that there are several updates after you posted initial patches. please adivse me which patches should be applied?
|
|
Lai should confirm, but I think the most important patch for addressing the unlink regression is http://review.whamcloud.com/10398 so that one should be tested first.
There is also a potential improvement in http://review.whamcloud.com/9696 that is next, but it doesn't affect unlink. I think the http://review.whamcloud.com/9697 is too complex to land for 2.6.0 at this point, but if it gives a significant improvement then it could be landed for 2.7.0 and IEEL.
|
|
Ihara, did you get a chance to test if 10398 fixes the unlink regression? We are ready to land that patch.
|
|
I'm testing patches. will post results shortly.
|
|
Ihara, any chance to post the results from your tests?
|
|
Hi Ihara, is there a chance for you to post the mdtest results for the testing you did on 06-09 for patch http://review.whamcloud.com/10398 ?
|
|
Andreas, sorry dely on this... Here is recent our test resutls.
Unique Directory Operation
master branch(47cde804ddc9019ff0793229030211d536d0612f)
Unique Directory Operation
master branch(47cde804ddc9019ff0793229030211d536d0612f) + patch 10426 + patch 10398
Shared Directory Operation
master branch(47cde804ddc9019ff0793229030211d536d0612f)
Shared Directory Operation
master branch(47cde804ddc9019ff0793229030211d536d0612f) + patch 10426 + patch 10398
We see performance improvements with patch for unlink operations to unique directories as well as shared directory. I will also want to check with lustre-2.5 to comapre. btw, file/directory creation to shared directory is lower than I expected.. I will check later on other lustre version (e.g. b2_5) as well.
|
|
It appears that the unlink performance has gone up, but the create and stat rate have gone down. Can you please test those two patches separately? If the 10398 patch is fixing the unlink performance without hurting the other performance it could land. It might be that 10426 patch is changing the other performance and needs to be reworked.
|
|
First, I tried only 10398 patch, but build fails since OBD_CONNECT_UNLINK_CLOSE is defined in 10426 patch. So, I needed both patches at same time to compile.
BTW, here is same mdtest benchmark on same hardware, but lustre version is 2.5.2RC2.
Unique Directory Operation
Shared Directory Operation
Unique directory metadata operation, overall, the result of master + 10398 + 10426 patches are close to 2.5.2RC2 results except directory stats. (stats operation, 2.5 is better than master)
However, metadata operations to a shared directory, most of 2.5.2RC2's numbers are still much higher than master or master + 10398 + 10426 patch's resutls. That's oritinal issue on this ticket, but still big performance gap there. Read operation, master branch much improved against 2.5 branch.
|
|
So it looks like we have all of this extra file handle caching that should not really be happening at all.
Originally when opencache was implemented - it did cache everything and that resulted in performance drop specifically due to slow lock cancellation.
That's when we decided to only restrict this caching to nfs-originated requests and some races - by only setting the flag in ll_file_intent_open where we could only get via nfs.
Now it appears that this assumption is broken?
I am planning to take a deeper lok to understand what is happening with the cache now.
|
|
Sorry about my earlier confusion with 10426 - I thought that was a different patch, but I see now that it is required for 10398 to work.
It looks like the 10398 patch does improve the unlink performance, but at the expense of almost every other operation. Since unlink is already faster than create, it doesn't make sense to speed it up and slow down create. It looks like there is also some other change(s) that slowed down the create and stat operations on master compared to 2.5.2.
It doesn't seem reasonable to land 10398 for 2.6.0 at this point.
|
|
Oleg, the cause is simplified revalidate (see 7475), originally revalidate will execute IT_OPEN, but this code is replicate of lookup, and this opened handle can be lost if other client canceled this lock. So 7475 simplified revalidate, which just return 1 if dentry is valid, and let .open to really open file, but this can't be differentiate from NFS export open, so both open after revalidate and NFS export open take open lock.
|
|
So, it looks like we still can infer if the open originated from vfs or not.
When we come from do_filp_open (this is for real open path), we go through filename_lookup with LOOKUP_OPEN set, when we go through dentry_open, LOOKUP_OPEN is not set.
As such the most brute-force way I see to address this is in ll_revalidate_dentry to always return 0 if LOOKUP_OPEN is set and LOOKUP_CONTINUE is NOT set (i.e. we are looking up last component).
We already do a similar trick for LOOKUP_OPEN|LOOKUP_CONTINUE
BTW, while looking at the ll_revalidate_dentry logic, I think we can improve it quite a bit too in the area of intermediate path component lookup.
All of this is in this patch: http://review.whamcloud.com/11062
Ihara-san, please give it a try to see if it helps for your workload?
This patch passes medium level of my testing (does not include any performance testing).
|
All of this is in this patch: http://review.whamcloud.com/11062
Ihara-san, please give it a try to see if it helps for your workload?
sure, will test that patches as soon as I can run benchmark. maybe early next week, thanks!
|
|
I ran the patch on Hyperion, 1,32,64,100 clients. Mdtest dir-per-process and single-shared-dir.
Spreadsheet with graphs attached
|
|
Patches landed to Master. Please reopen ticket if more work is needed.
|
Generated at Sat Feb 10 01:42:05 UTC 2024 using Jira 9.4.14#940014-sha1:734e6822bbf0d45eff9af51f82432957f73aa32c.
LU-4367.xlsx
debugfile
unlinkmany-result.zip