Read lock on LevelHandler while get

[wangnengjie]

When getting value from LSM, agatedb iters on levelhandlers and calls LevelHandler::get() under a RwLockReadGuard which will cause the whole read I/O path on this level under a read lock. It is always not a good idea to do I/O under lock even it is a read lock.

See: https://github.com/tikv/agatedb/blob/475e17bce85cf5985161004c068616a594923365/src/levels.rs#L931

I think such read lock is only necessary for fetching associated Tables from LevelHandler and the exact read work can be done outside the lock.

This logic is same as what it is in badger. https://github.com/dgraph-io/badger/blob/7d159dd923ac9e470ffa76365f9b29ccbd038b0d/levels.go#L1602 The comment says it will call h.RLock() and h.RUnlock(), but exactly they are called in getTableForKey() which only locked when getting tables. https://github.com/dgraph-io/badger/blob/master/level_handler.go#L241-L243

BTW, as Table might be removed (I/O) when the struct drop and this might happened when modifying LevelHandler under write lock. So this might influence read? I' mot sure whether it's right?

[GanZiheng]

I think such read lock is only necessary for fetching associated Tables from LevelHandler and the exact read work can be done outside the lock.

Yes, we currently perform the whole get process while holding read lock of LevelHandler. In fact, we could release the lock as soon as we get the tables to search.

BTW, as Table might be removed (I/O) when the struct drop and this might happened when modifying LevelHandler under write lock. So this might influence read? I' mot sure whether it's right?

I think if we already get the table, it will not be dropped until nobody use it.

[wangnengjie]

I think if we already get the table, it will not be dropped until nobody use it.

It takes a little more time when do the real file drop and this will occur under write lock if nobody have the table ref. Don't you think doing drop under write lock might block the read process?

[GanZiheng]

I think if we already get the table, it will not be dropped until nobody use it.

It takes a little more time when do the real file drop and this will occur under write lock if nobody have the table ref. Don't you think doing drop under write lock might block the read process?

I see, I misunderstood what you mean previously. Maybe we could just mark them as could be deleted and perform the real delete operation later. But I doubt if it is real expensive to delete a file...

[wangnengjie]

Well, it depends on CPU, OS, filesystem and disk performance. I just have a test on my cloud server. HDD... so it's slow and remove a 64MiB file takes about tens of microseconds. I have to say that it really depends on os cache for meta and disk performance so the result is just for reference. And, this is a system call. For compaction we might drop more than one table here. For cloud env that might use remote storage, it really cost. (well, of course we can spawn an async task to do it)

---

auto start = std::chrono::steady_clock::now();
remove("./64MiB");
auto end = std::chrono::steady_clock::now();
auto nano = std::chrono::duration_cast<std::chrono::nanoseconds>(end-start).count();
std::cout << nano << std::endl;