HDDS-10076. SnapshotCache closes RocksDB instance with Reference.

[aswinshakil]

What changes were proposed in this pull request?

While accessing snapshots, We use SnapshotCache to load and retrieve a snapshot's RocksDB instance. The function call is as follows get()->cleanup(), When multiple background process calls get() some threads can also be doing thecleanup() of pending eviction list.

There is a scenario, where Thread 1(KeyDeletingService) is executing get()->cleanup() method and Thread 2(SSTFilteringService) is executing get() (Hasn't reached cleanup() yet), The reference count of the snapshot is incremented by get()(Thread 2) but we still close the rocksDB instance because the cleanup()(Thread1) method assumes everything in the pending eviction list has a reference count of 0. This is not the case in the above-mentioned scenario, We need to recheck if the reference count is still 0 when closing the RocksDB instance.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10076

How was this patch tested?

Manually tested with an existing test.

[smengcl]

Thanks @aswinshakil . The fix lgtm. Some nits inline.

[hemantk-12]

Thanks @aswinshakil for the patch.

Overall looks good to me. Left a inline comment, please check that.

[hemantk-12]

I just realized that we should use compute and log/throw an error if dbMap doesn't contain the key. In case when key is missing from dbMap, it is possible that object was not closed properly or some other inconsistency issue.

[aswinshakil]

Thanks for pointing out. Updated the patch.

[swamirishi]

@aswinshakil Thanks for the quick patch and for identifying the issue. I am not sure if this patch completely fixes the race condition, would like other reviewers as well to double check the same.

[swamirishi]

This particular check is not exactly thread safe there would still be a race condition, this doesn't really take a ref count lock. We should probably put this function check inside ReferenceCounted class which would take a ref count lock when doing the check. Some function like
boolean isTotalRefCountEqual(long expectedValue) which would return if the value is equal by taking a ref count lock. You can refer incrementRefCount function for this.

[swamirishi]

GetKey increaments the ref count after the dbMap.compute() function so the lock is already out of scope. I don't see any change in the get function so this should be the way going forward for the cleanup.

[aswinshakil]

I don't see a need to get refCountLock unless you are actually updating it. By that logic then we would also need to get a refCountLock everytime we do refCount.get(). @smengcl Do you see any problem with this? AFAIK I don't think this is problem.

[swamirishi]

Yeah every time we do a refCount.get() if it is going to impact the cache yes we need to take a lock

[swamirishi]

I guess separating out the referenceCounted and cacheLoader is not the right way to implement the cache. From what I know this has go hand in hand in a single lock. Increasing the reference count or decreasing the reference count or when evicting an instance from the cache which would check if there are any references. BTW even decrementRefCount has this problem. decrement is called with referenceCountLock, and it checks if the reference count is zero and adds it to the pending eviction list which checks the count is 0. But in case of a race condition a get call could have a closed rocksdb instance, cleaner thread could have closed the instance in the background.

[swamirishi]

refCount increment should happen when things are loaded in the cache and within the same lock. i.e. the same db.compute method

[aswinshakil]

refCount increment cannot happen in the same db.compute() because it will cause a deadlock.

[smengcl]

How about a new atomic operation like rcOmSnapshot.closeObjSafely(). It only closes the internal obj if refcount == 0, and it returns true when the internal object is successfully closed?

[hemantk-12]

I guess it comes to the same point when we call rcOmSnapshot.closeObjSafely() it returns true but when actually closing it, object is referenced again.

[aswinshakil]

Closing it as #5986 fixes this issue.