[ https://issues.apache.org/jira/browse/HBASE-26465?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Duo Zhang updated HBASE-26465: ------------------------------ Hadoop Flags: Reviewed Resolution: Fixed Status: Resolved (was: Patch Available) Pushed to master and branch-2. Thanks [~comnetwork]. > MemStoreLAB may be released early when its SegmentScanner is scanning > --------------------------------------------------------------------- > > Key: HBASE-26465 > URL: https://issues.apache.org/jira/browse/HBASE-26465 > Project: HBase > Issue Type: Bug > Affects Versions: 2.5.0, 3.0.0-alpha-2 > Reporter: chenglei > Assignee: chenglei > Priority: Critical > Fix For: 2.5.0, 3.0.0-alpha-2 > > > HBASE-26144 moved {{MemStore.clearSnapshot}} out of write lock of > {{HStore#lock}}, but because {{MemStore.clearSnapshot}} closed > {{DefaultMemStore#snapshot}} which may be used by > {{DefaultMemStore#getScanners}}, when {{DefaultMemStore#getScanners}} and > {{MemStore}} flushing execute conurrently, {{MemStoreLAB}} used by > {{DefaultMemStore#snapshot}} may be released early when its > {{SegmentScanner}} is scanning. > Considering follow thread sequences: > 1.The flush thread starts {{DefaultMemStore}} flushing after some cells have > be added to {{DefaultMemStore}}. > 2.The flush thread stopping before {{DefaultMemStore#clearSnapshot}} in > following line 1238 in > {{HStore#updateStorefiles}} after completed flushing memStore to hfile. > {code:java} > 1221 private boolean updateStorefiles(List<HStoreFile> sfs, long > snapshotId) throws IOException { > 1222 this.lock.writeLock().lock(); > 1223 try { > 1224 this.storeEngine.getStoreFileManager().insertNewFiles(sfs); > 1225 } finally { > 1226 // We need the lock, as long as we are updating the storeFiles > 1227 // or changing the memstore. Let us release it before calling > 1228 // notifyChangeReadersObservers. See HBASE-4485 for a possible > 1229 // deadlock scenario that could have happened if continue to hold > 1230 // the lock. > 1231 this.lock.writeLock().unlock(); > 1232 } > 1233 // We do not need to call clearSnapshot method inside the write lock. > 1234 // The clearSnapshot itself is thread safe, which can be called at > the same time with other > 1235 // memstore operations expect snapshot and clearSnapshot. And for > these two methods, in HRegion > 1236 // we can guarantee that there is only one onging flush, so they will > be no race. > 1237 if (snapshotId > 0) { > 1238 this.memstore.clearSnapshot(snapshotId); > 1239 } > ...... > {code} > 3.The scan thread starts and stopping after > {{DefaultMemStore.snapshot.getAllSegments}} in following line 141 in > {{DefaultMemStore#getScanners}},here the scan thread gets the > {{DefaultMemStore#snapshot}} which is created by the flush thread. > {code:java} > 138 public List<KeyValueScanner> getScanners(long readPt) throws IOException { > 139 List<KeyValueScanner> list = new ArrayList<>(); > 140 addToScanners(getActive(), readPt, list); > 141 addToScanners(snapshot.getAllSegments(), readPt, list); > 142 return list; > 143 } > {code} > 4.The flush thread continues {{DefaultMemStore#clearSnapshot}} and close > {{DefaultMemStore#snapshot}} in following line 253,because the reference > count of the corresponding {{MemStoreLABImpl}} is decreased to 0, the Chunks > in corresponding {{MemStoreLABImpl}} are recycled. > {code:java} > 240 public void clearSnapshot(long id) throws UnexpectedStateException { > ...... > 248 Segment oldSnapshot = this.snapshot; > 249 if (!this.snapshot.isEmpty()) { > 250 this.snapshot = > SegmentFactory.instance().createImmutableSegment(this.comparator); > 251 } > 252 this.snapshotId = NO_SNAPSHOT_ID; > 253 oldSnapshot.close(); > 254 } > {code} > 5.The scan thread continues {{DefaultMemStore#getScanners}}, and create a > {{SegmentScanner}} for this {{DefaultMemStore#snapshot}}, and invokes > {{MemStoreLABImpl.incScannerCount}} in line 281 to increase the reference > count , but here {{MemStoreLABImpl.incScannerCount}} does not check that the > reference count is already 0 and {{Chunks}} are already recycled by step 4, > so these {{Chunks}} may be overwritten by other write threads when the > {{SegmentScanner}} is scanning, which may cause serious problem. > {code:java} > 280 public void incScannerCount() { > 281 this.openScannerCount.incrementAndGet(); > 282 } > {code} > I simulate above case in my PR, and my Fix is as following: > 1.Moving {{MemStore.clearSnapshot}} back under write lock of > {{HStore#lock}}, because {{DefaultMemStore#getScanners}} is protected under > the read lock of {{HStore#lock}}, so {{DefaultMemStore#getScanners}} and > {{DefaultMemStore#clearSnapshot}} could not execute concurrently. > 2. Introducing {{RefCnt}} into {{MemStoreLABImpl}} to replace its flawed > reference count implementation, so checking and increasing or decreasing is > done in atomicity and if there is illegal state in reference count, it would > throw exception rather than using corrupt data, but this modification is not > included in PR now because I find that there is another bug in flushing would > disrupt the reference count in {{MemStoreLABImpl}} , I would Introduce > {{RefCnt}} after fixing this another bug. -- This message was sent by Atlassian Jira (v8.20.1#820001)