|
Bobijam
Could you please comment?
thanks
Peter
|
|
So I tried the script on master and it also happens there.
|
|
I can also reproduce it on master. The exact details of the lfs and dd task stacks are a little different but the deadlock is still there.
|
|
It relates to the lfs migrate implementation (LU-2445), lfs migrate first takes a group lock to limite concurrent OST access from other clients, then vfs reads and writes the file to migrate data from OSTs to OSTs, at last drops the group lock.
In this case, lfs migrate gets the group lock, then the other client from the same node tries to truncate the file which takes the inode truncate semaphore (lli_trunc_sem) and enqueues OST lock and waits for it to be granted.
The lfs migrate comes to the read phase, it tries get the truncate semaphore as well.
The other client cannot get its OST lock granted, since OST cannot revoke it from the lfs migrate process. Deadlock happens.
|
|
It's a good chance to reimplement migration with open lease.
Aurelien and JC, do you have any inputs on this?
|
|
I see no objection to replacing grouplock-based code with an open-lease mechanism. Current code was using this lock because open lease did not exist as that time and we were advised to do this this way as they was no mechanism to protect from concurrent access. I think it will be cleaner.
|
|
That's true, Aurelien. As long as the migration was implemented by CEA, would it be possible for CEA to pick it up again to reimplement it with open lease?
Jinshan
|
|
OK, CEA will do it.
Could you confirm some behaviour of open lease to be sure we are in line. There is only exclusive open lease for now? When lease are revoked by concurrent access?
|
|
Here's a patch aimed to solve it:
http://review.whamcloud.com/10013
|
|
Use file lease to implement migration.
|
|
Please check the attachment for the implementation of migration.
The procedure is a little bit like HSM release where close and swap layout should be an atomic operation. Also, you need to check if the lease is valid in the middle of data copying periodically therefore data copying can abort if the file is being opened by others.
Please take a look at it and I'll be happy to answer questions.
|
|
Thanks Jinshan.
I still have a question regarding when to check data version. In which case could it fail if we both get and check it under the file lease? Also swap layout will perform a dataversion check. Is the following sequence flawed?
- open source file
- get lease
- open volatile
- get data version
- copy file content
- put lease
- swap layout (pass in data version for check)
- close volatile
- close source file
|
|
We have to make `put lease', `swap layout', and `close source file' in one atomic operation. Otherwise, if the source is opened for writing after `put lease' and generate some dirty pages, it will produce data corruption.
|
|
Cf. the new patchset.
Indeed, I do see the race window between llapi_lease_put() and llapi_fswap_layouts() but I can't see any userland API that would allow us to get rid of it. Am I missing something? You're stressing the need to do operations atomically, do you have something in mind, like making the SWAP_LAYOUT ioctl lease-aware?
|
|
No, there is no API in user space ready to use.
You're stressing the need to do operations atomically, do you have something in mind, like making the SWAP_LAYOUT ioctl lease-aware?
Yes, similar. Instead of making SWAP_LAYOUT lease-aware, what we need is to make lease SWAP-LAYOUT aware.
When we release a lease, a CLOSE RPC will be sent to MDT. We're going to pack the FID of volatile file into the RPC, with a special bias (similar to MDS_HSM_RELEASE, please check ll_close_inode_openhandle() for 'op_data->op_bias |= MDS_HSM_RELEASE', and mdt_hsm_release()), for example, MDS_CLOSE_SWAP_LAYOUT to tell MDT that we want to unlock the release and swap the layout. We're going to extend `struct close_data' to include those information. And the ioctl() of LL_IOC_SET_LEASE with F_UNLCK will be extended as well.
It's totally fine for me to write a design but I think you have understood the problem and are capable of doing it yourself, based on your questions above.
|
|
It took me a little while but I buckled down and just pushed a patch that follows the guidelines you gave me. In userland I've extended the swap layouts ioctl to leverage the existing API and preserve compatibility (I was reluctant to change the parameters of the set lease ioctl). In kernel land it's very close to hsm_release. Hope this is fine conceptually.
|
|
Any update on this?
|
|
I'll look at the patch this week, really sorry for delay.
|
|
I tested rev 12 of this patch on top of head of tree. Nothing fancy:
After lfs decided to output the content of ls (???) to stdout, the node proceeded to not like me and crash.
Without this patch, I still get the junk output, but not the crash.
|
|
Thanks Frank. Null pointer (sbi) dereference in ll_mdscapa_get(). Fixed in patchset #13. The file content ending up in the console remains unexplained to me so far. You said it was present before, is there an open ticket for that?
|
|
Thanks. The fix works, and I can migrate a file between osts now.
Regarding the junk output, I found the bug in llapi_file_open_param(). I'll submit a patch soon.
|
|
Follow-up patch, fixes numerous issues with the first one: http://review.whamcloud.com/#/c/12616/
Both patches can be merged if need be, just let me know what's preferred.
|
|
Henri, I agree with Frank that we should not be landing a patch with significant known defects,since this would break the code for anyone testing this. Please merge the patches.
|
|
One advantage to the old approach of using group locks for migration was that it was theoretically possible to create a version of lfs migrate that could migrate a file in parallel using multiple clients. Is this still possible with the new approach?
|
|
Yes, it is still possible. Though an early version of the patch removed grouplock-protected migration, it has now been re-introduced. Migration can be either grouplock-protected and blocking (as before), or based on exclusive open and non-blocking (would safely abort if a concurrent process opens the file). We would need file leases to provide a notion of "group" to be able to implement non-blocking parallel migration too.
|
|
Thanks for the response, Henri. I'm glad to hear the group lock option was retained, and I see the deadlock with truncate was resolved as well.
|
|
Just to draw attention to my comment in gerrit.
The latest patch still deadlocks in racer on mds, also seems to be leaking ost locks at times?
|
|
Dropping this from Blocker to Critical, since it is not a new issue for 2.7.0 (it exists since migrate was added in 2.4.0), and only affects a subset of users of the migrate functionality, and not anyone else.
|
|
I'm investigating this issue.
|
|
please apply http://review.whamcloud.com/13344 to your tree. It worked well after that patch was applied in my test.
|
|
Patch that adds some tests for the new API: http://review.whamcloud.com/13441/
It has a couple questions left (see BUG?? in source) but otherwise is complete.
It has to be applied on top on Henri's patch.
|
|
for the record: using Jinshan's patch did not help all that much and I was still seeing deadlocks on mdt
|
|
I couldn't reproduce the deadlock problem on MDT. Please collect a core dump when you see the deadlock issue again.
|
|
Frank, Henri, Jinshan,
according to Oleg's last comments, he was still able to hit this deadlock even when the patch was applied, which raises a concern whether the risk of landing this complex patch is worth the risk at this late stage.
Could you please confirm that the current patch has resolved the deadlock in your testing? It may be that Oleg is hitting a second issue that is not directly related.
The second question is whether you are currently running with this patch in your other testing and can confirm that it doesn't introduce other problems?
|
|
1. I can confirm that the original problem is fixed by this patch;
2. I tried to reproduce the problem mentioned by Oleg, i.e., running racer, for over 10 hours but I couldn't reproduce the problem. I'm not saying that this issue doesn't exist but is hard to reproduce.
|
|
Hello,
same here, I have not been able to reproduce the issues aforementioned... I can try investigating from crash dumps if Oleg has any that he can share, though it's indeed harder than with a reproducer.
This patch introduces no regression I'm aware of, and it fixes the original problem. If its size/complexity makes it "unlandable" I can try to split it into two subpatches (one fixing the blocking mode, another one adding the non-blocking mode). I was actually not expecting it to grow that much 
Edit: I have just triggered deadlocks with racer, as descibed by Oleg, with all rpms installed, not simply running from the source tree. There was concurrent activity going on, but I'll try to reproduce in a non-disturbed environment.
|
|
Henri, that's great. Please let me know if you need any help from me.
|
|
So in order to move things forward, and based on my understanding that this patch actually helps some user-ereported problems (and also on the assumption thatthe racer problems are now possibly a separate bug)- let's split the patch into two parts: the actual fix and the racer test patch.
We can then land the code fix (if it otherwise does not introduce any more failures) and we can wait with the racer test patch until we better understand why it fails and fix those. I am a bit nervous about landing tests that tend to fail as that invalidates our testing strategy quite a bit though.
|
|
Henri Doreau (henri.doreau@cea.fr) uploaded a new patch: http://review.whamcloud.com/13669
Subject: LU-4840 tests: Add file migration to racer
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 739d73d58b2877711893e2df0b5f487e6b141060
|
|
Racer patch: http://review.whamcloud.com/#/c/13669
|
|
Oleg Drokin (oleg.drokin@intel.com) merged in patch http://review.whamcloud.com/10013/
Subject: LU-4840 lfs: Use file lease to implement migration
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 85bd36cc69563d7a79e3ed34f8fadb4ed1a72b7c
|
|
Hi,
Now that patch at http://review.whamcloud.com/10013 has been merged into master, is it possible to have a backport to b2_5?
Indeed, the original issue was met on one of our customer clusters running Lustre 2.4, that was updated afterwards in Lustre 2.5.
Thanks,
Sebastien.
|
|
Closing LU-4840 since the patches here are landed to fix the problems described here, except http://review.whamcloud.com/13669 to enable racer with object migrate. LU-7073 has been opened to track the new racer + object migrate failure.
|
|
Backport to b2_7_fe http://review.whamcloud.com/#/c/21513/
|
Generated at Sat Feb 10 01:46:18 UTC 2024 using Jira 9.4.14#940014-sha1:734e6822bbf0d45eff9af51f82432957f73aa32c.
migration.png