westerndigitalcorporation / libzbd

Zoned block device manipulation library and tools
69 stars 27 forks source link

Initialisation challenges of the per thread zbd_log_level variable from RocksDB+ZenFS #17

Closed imukherjee-wdc closed 2 years ago

imukherjee-wdc commented 2 years ago

The variable zbd_log_level is defined per thread. In RocksDB+ZenFS combo, only one thread opens the zbd device and creates the top level ZenFS object instance. The call to zbd_set_log_level(ZBD_LOG_ERROR) is in this path within ZenFS. Later, other RocksDB IO threads just use the pre-initialised ZenFS object and start issuing IOs. As a result, the per thread zbd_log_level variable is initialised only for the first thread(that opens the zbd device) and remains uninitialised for other RocksDB threads. Note that the threads are created by RocksDB.

Would it make more sense to define the zbd_log_level per FD and embed it within the zbd_info structure? Normally one application (like an instance of RocksDB) would completely own a zbd device and it's likely that the threads will use the same debug level. In this approach, calling the zbd_set_log_level() api before opening the zbd device would suffice.

floatious commented 2 years ago

Hello Indro,

Just curious.

Is there no callback function that zenfs can hook into, that rocksdb will call after creating the threads, where the zenfs callback could call zbd_set_log_level() on each thread?

imukherjee-wdc commented 2 years ago

Once ZenFS is registered by the main thread and mounted, the other worker threads just do IO. There are no one time init calls for these other worker threads. Adding the call to znd_set_log_level() anywhere in these other paths(fs operations) would mean invoking the call for every file operation of that type. I'll check once again but I already did check before initiating this discussion.

Not related to this discussion: It might be nice to have a call to get current debug level as well.

damien-lemoal commented 2 years ago

Changing the log level to be per fd would be an ABI change, so would require a major version increase. Same if we changed zbd_log_level to be per process instead of per thread, not to mention that this would not cover the cases of applications doing fork. So I am not warm to make these changes. libzbd log level is convenient to understand what goes wrong if you get a failure opening the device. Apart from zbd_open(), all other libzbd functions only use zbd_error() to print that the ioctl() failed... Not super useful, and the application can print the exact same messages if it wants since errno will be set for the failing libzbd function. So my recommendation is simply to add error messages in zenfs for failed libzbd calls and not rely on libzbd log level.

imukherjee-wdc commented 2 years ago

Discussed with the team - the issue can be closed.