DataDog / glommio

Glommio is a thread-per-core crate that makes writing highly parallel asynchronous applications in a thread-per-core architecture easier for rustaceans.
Other
2.93k stars 161 forks source link

Any interest in adding advisory lock support? #663

Open vlovich opened 2 months ago

vlovich commented 2 months ago

Adding advisory locks outside of Glommio is unsafe because it's a single status bit on the fd - i.e. locking shared & then locking exclusively works, but it's dangerous to coordinate unlocking safely as the first lock to unlock unlocks the file. Doing this safely needs to happen within Glommio where it can hold some status about the state & coordinate acquiring a lock safely.

This adds some extra state but hopefully that's not the main concern. There's two approaches for tackling this.

The first is the most featureful where we can try to implement a thread-safe RwLock-like implementation for advisory locks. After thinking about it though, that seems kinda complicated as it involves having a way to signal across threads. I think the only place right now where that happens is the task queue management and it's not a reusable piece of code - likely I'd want to pull in AtomicLInk from intrusive-collections to make this easier. But even with that, it's tricky to implement correctly.

The alternative implementation is that we only allow 1 lock to be held (or try to be held) at a time & simply return an error if anyone else has started trying to acquire the lock. That's much simpler to reason about and implement and likely fits with typical usage where you want to make sure there's exactly 1 owner of the advisory lock & it happens sometime early on start (at least that's my use-case). I would prefer the alternative implementation as it's less likely to contain bugs & the feature set is still useful.

The returned guard would hold a clone of the lock to ensure that it's impossible to close the FD until the lock is released / dropped. There's either an unsafe fn leak method on the guard that consumes self & drops the file letting close work or a close_locked(AdvisoryLock) method on the DmaFile to give ownership of the lock back to the file to close - I think close_locked makes the most sense but I could see leak also being useful so maybe both?

The APIs added to DmaFile would be:

/// If the file is currently locked in the kernel, an error is returned immediately without blocking indefinitely. This returns an
/// error if a lock has already been acquired (or is attempting to be acquired) on this file handle instance (including duped instances).
async fn try_lock_exclusive(&self) -> Result<AdvisoryFileGuard>;
/// If the file is currently locked exclusively in the kernel, an error is returned immediately without blocking indefinitely. This returns an
/// error if a lock has already been acquired (or is attempting to be acquired) on this file handle instance (including duped fds). /// Otherwise a handle holding the shared lock is returned.
async fn try_lock_shared(&self) -> Result<AdvisoryFileGuard>;
/// If the file is currently locked in the kernel, the function will wait indefinitely until an exclusive lock is acquired. This
/// returns an error if a lock has already been acquired on this file handle (including duped instances).
async fn lock_exclusive(&self) -> Result<AdvisoryFileGuard>;
/// If the file is currently exclusively locked in the kernel, the function will wait indefinitely until an exclusive lock is acquired. This
/// returns an error if a lock has already been acquired on this file handle (including duped instances).
async fn lock_shared(&self) -> Result<AdvisoryFileGuard>;

The main advantage around doing this within glommio is that I can enforce the behavior across clones. Externally to glommio I have to make the advisory lock functions unsafe because there's all sorts of soundness issues (e.g. being able to acquire a shared & exclusive lock on the same fd with no way to try to protect against that)