Page MenuHomeFreeBSD

add non-blocking variants of rangelock_rlock() and rangelock_wlock()

Authored by rmacklem on Jun 15 2019, 12:29 AM.



D20584 requires a way to acquire rangelocks on two vnodes concurrently without the risk of deadlock.
To do this, an non-blocking variant of at least one of rangelock_rlock() and rangelock_wlock() is needed.
This patch adds both rangelock_rlock_nonblock() and rangelock_wlock_nonblock() to do this and also
vn_rangelock_rlock_nonblock() and vn_rangelock_wlock_nonblock() using the above.

Not sure if rangelock_rlock()/rangelock_wlock() is in any man page?

Test Plan

By putting a long tsleep() in the copy_file_range() patch after the thread acquires the first lock for the first call,
a second copy_file_range() call will hit the conflict and tsleep() after acquiring the other rangelock, then a third
concurrent call to copy_file_range() should have the non-blocking call return NULL.

Trivial tests without the conflicts work ok.

Diff Detail

rS FreeBSD src repository
Lint Skipped
Unit Tests Skipped
Build Status
Buildable 25095

Event Timeline

726 ↗(On Diff #58643)

As far as names go, I suggest the "try" idiom, as in vn_rangelock_try_rlock

This variant of the patch has the "_nonblock" suffixes replaced by "_trylock" as suggested by asomers@.
A quick grep of src/sys/sys shows that "nonblock" is used for flags, but "_trylock" is used as a suffix
for non-blocking lock functions.

No logic change from the previous version.

253 ↗(On Diff #58648)

You should assert that the entry is indeed last, i..e. that its next is NULL.

256 ↗(On Diff #58648)

I do not think that unlock_locked() is needed. Any non-granted entry can be removed from the list directly, and its removal cannot affect the list of currently granted range locks because it was not granted.

277 ↗(On Diff #58648)

I disagree with the addition of non-sleepable flag. This would make trylock fail due to issue not related to the range lock state.

I do not this that a right semantic for rangelock try is to not ever sleep, it should be only 'do not sleep waiting for granting the rangelock'.

This variant allows a sleep to be done for allocation of a new list entry. It only returns NULL
if it would need to sleep waiting to be able to lock the range.

It also now has an extra argument for rangelock_unlock_locked(), which tells it not to call
rangelock_calc_block() when used to remove the new entry that rangelock_enqueue() added
if trylock != 0 and there is a conflict.
This was what kib@ suggested, except I didn't want to crib the rest of the code in
rangelock_unlock_locked() except the rangelock_calc_block() call.
(It might be better to rename rangelock_unlock_locked() to rangelock_remove_locked(), since
the lock hasn't yet been acquired for this case?)

Added replies to inline comments.

256 ↗(On Diff #58648)

I agree that the rangelock_calc_block() call in rangelock_unlock_locked() is a no op
in this case. However, I didn't want to duplicate the rest of the code that removes
an entry from the list, so I added an argument to rangelock_unlock_locked() to
make calling rangelock_calc_block() optional.

277 ↗(On Diff #58648)

Yes. I was thinking "nonblock" should mean doesn't sleep, but "trylock" means
don't sleep for the lock.

Feel free to do the rename in the preliminary commit.

144 ↗(On Diff #58680)

Please use bool and true/false.

204 ↗(On Diff #58680)

Same, please use bool.

rmacklem marked 2 inline comments as done.

int arguments changed to C99 bool as requested by kib@.

rmacklem added inline comments.
726 ↗(On Diff #58643)

I added _trylock suffix, so the basic function name is recognized as the same as
the one that waits for the lock to be available.

This revision is now accepted and ready to land.Jun 19 2019, 7:45 PM

I believe that the code is fine from the algorithmic PoV, but I suggest some minor renaming and rearrangement.

259 ↗(On Diff #58684)

I think that all code inside if (trylock) {... from start up to the rangelock_unlock_locked() call (except the call itself) would be more naturally placed in the rangelock_unlock_locked() itself. In _unlock_locked(), you would call rangelock_calc_block() if trylock is true, and do reset of rl_currdep as needed, otherwise.

276 ↗(On Diff #58684)

I suggest rangelock_tryrlock() name.

291 ↗(On Diff #58684)


rmacklem marked an inline comment as done.

This version of the patch incorporates kib@'s suggestions.

This revision now requires review to proceed.Jun 27 2019, 10:09 PM
This revision is now accepted and ready to land.Jun 27 2019, 10:29 PM