HDDS-1672. Improve locking in OzoneManager. #949
Conversation
bharatviswa504
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
Unless I am completely out of my mind, this is a dangerous patch to commit. We need to prove that taking user locks after volume locks is consistent across all paths. Right now, we think of user/volume/bucket/key as a logical hierarchy. Yes, there are cases where you only have volume and you have to look back up the path, and in that case, you might have to release a lock already taken. But that is not an excuse to change the locking hierarchy if you want to do that, You need to write down a locking conflict resolution table and prove that all paths are consistent. That is what we did when we first developed this locking hierarchy table, Yes, it is a bit of work now, but will save us countless hours of work and pain in the future. Let us not commit this unless we have such a table. As I said, it might be that this is correct, but this is a place where we have to be sure that this is correct. |
Thank You @anuengineer for the comments. |
I will try to find it, @nandakumar131 and I worked on this together, I think Nanda wrote that document. These are the kind of docs I wish were committed it the design repo so that we can find them when we need. I don't think that doc was posted to Apache, But I will see if I can find it, else we will have to rebuild that lock conflict table. |
@anuengineer could you please clarify what this table looks like? This is the first time in 20+ years of writing multi-threaded code that I have have heard of a locking conflict resolution table. I have only heard of locking order so far. 🙂 |
Something like Lock Levels - User=0, Volume=1,Bucket=2, Key =3 Create Volume -> Locking Order -> Lock User, LockVolume , Release Volume, Release User For each path, you know what the locking order is. We have a lock hierarchy very clearly defined today, changing that order means that you need to make sure no other path which is taking those locks are now violating the proposed order. We need to manually list out each lock path in the OM to move user lock from level O to level 1. In other words, changing lock hierarchy in any code path is a non-trivial. This process will prove that our hierarchy itself is not buggy, and you don't run into this same problem is some other path, since you have optimized code for this path. |
|
Thank you that makes perfect sense. We will write that up. |
bharatviswa504
force-pushed
the
HDDS-1672
branch
from
3f8e9f5 to
9b29425
Compare
bharatviswa504
changed the title
|
Thank You @anuengineer for the suggestion to write up the document. |
bharatviswa504
force-pushed
the
HDDS-1672
branch
from
02deced to
c88e645
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bharatviswa504
force-pushed
the
HDDS-1672
branch
from
d1a8f84 to
2000d08
Compare
|
/retest |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
anuengineer
left a comment
anuengineer
left a comment
There was a problem hiding this comment.
Let us chat offline, there are some complicated issues here.
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
Outdated
Show resolved
Hide resolved
|
|
||
| // Or do we need to add this for future safe? | ||
|
|
||
| if (hasAnyVolumeLock() || hasAnyBucketLock() || hasAnyUserLock()) { |
There was a problem hiding this comment.
There is a small bug in this line of code. Since we have Prefix and S3Secret locks, we have to check for them here. Otherwise our model is incompletely enforced. I do agree that we currently have no code path which would do something like that; but the enforcer must enforce all rules. Not dependent on people doing the right thing.
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerLock.java
Outdated
Show resolved
Hide resolved
| * @return true if current thread holds user lock, else false | ||
| */ | ||
| private boolean hasAnyUserLock() { | ||
| return myLocks.get().get(USER_LOCK).get() != 0; |
There was a problem hiding this comment.
This works , but is very inconsistent ? because we do not call into this call when we take multi-user lock -- ?
and that is because this function does not give the semantics we need for the user locking ?
There was a problem hiding this comment.
Yes, added a call for hasAnyUserLock() in acquireUserLock() so that if some one is trying to acquire multiple user locks, he will immediately fail with RunTimeException. As said in code comments in acquireUserLock() this is a protection logic, in avoiding users do that.
|
|
||
| omMetadataManager.getLock().acquireUserLock(owner); | ||
| omMetadataManager.getLock().acquireVolumeLock(volume); | ||
| omMetadataManager.getLock().acquireUserLock(owner); |
There was a problem hiding this comment.
If acquireUserLock throws an exception, we can try to release a lock in the finally block that we don't actually hold. You may need nested try catch here, or alternative a boolean flag that is set to true when the user lock is acquired.
There was a problem hiding this comment.
That is why we checked in finally owner!=null and then only release the lock in finally.
| // Release and reacquire lock for now it will not be a problem for now, as | ||
| // applyTransaction serializes the operation's. | ||
| // TODO: Revisit this logic once HDDS-1672 checks in. | ||
| // Release and reacquire lock for now it will not be a problem for now, as |
|
|
||
| // We cannot acquire user lock holding volume lock, so released volume | ||
| // lock, and acquiring user and volume lock. | ||
| // We cannot acquire user lock holding volume lock, so released volume |
|
|
||
| if (compare < 0) { | ||
| manager.lock(OM_USER_PREFIX + newUser); | ||
| manager.lock(OM_USER_PREFIX + oldUser); |
There was a problem hiding this comment.
If the second lock call fails then we should release the first lock before returning.
The method should either get both locks or none.
| metrics.incNumBucketCreates(); | ||
| try { | ||
| metadataManager.getLock().acquireS3BucketLock(s3BucketName); | ||
| metadataManager.getLock().acquireVolumeLock( |
There was a problem hiding this comment.
If getting the volume lock fails, the finally block will still attempt to release it. Same with the S3 bucket lock.
You will either need to use nested try-catch or a boolean flag per lock indicating it was successfully acquired.
There was a problem hiding this comment.
Many of these bugs around proper releasing on failure have always been in the codebase. However this is a good time to fix them.
There was a problem hiding this comment.
I see the only case for failing is with RunTimeException. So, do we still need the flags?
There was a problem hiding this comment.
On a side note: once we use new HA code this will be cleaned up, so not considered much refactoring here.
|
|
||
| omMetadataManager.getS3Table().put(bucketName, finalName); | ||
| } finally { | ||
| omMetadataManager.getLock().releaseS3Lock(bucketName); |
There was a problem hiding this comment.
Sorry I didn't get why we removed the acquire/release bucket lock. Is the caller now supposed to get the lock?
There was a problem hiding this comment.
Do you mean s3 bucket lock, as we need to acquire that before creating volume. So, that is acquired in caller in OzoneManager.
bharatviswa504
force-pushed
the
HDDS-1672
branch
from
2000d08 to
2b29eea
Compare
|
Opened #1016 |
|
💔 -1 overall
This message was automatically generated. |
…apache#949) the references of the scala 2.10 version from the documentation.
4 participants
No description provided.