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
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rmacklem created this revision.Jun 15 2019, 12:29 AM
asomers added inline comments.Jun 15 2019, 2:40 AM
726 ↗(On Diff #58643)

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

rmacklem updated this revision to Diff 58648.Jun 15 2019, 5:35 AM

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.

kib added inline comments.Jun 15 2019, 9:32 PM
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'.

rmacklem updated this revision to Diff 58680.Jun 15 2019, 11:20 PM

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?)

rmacklem marked 2 inline comments as done.Jun 15 2019, 11:23 PM

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.

kib added a comment.Jun 16 2019, 12:00 AM

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 updated this revision to Diff 58684.Jun 16 2019, 5:12 AM
rmacklem marked 2 inline comments as done.

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

rmacklem marked 4 inline comments as done.Jun 16 2019, 5:14 AM
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.

asomers accepted this revision.Jun 19 2019, 7:45 PM
This revision is now accepted and ready to land.Jun 19 2019, 7:45 PM
kib added a comment.Jun 26 2019, 10:36 AM

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 updated this revision to Diff 59121.Jun 27 2019, 10:09 PM
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
kib accepted this revision.Jun 27 2019, 10:29 PM
This revision is now accepted and ready to land.Jun 27 2019, 10:29 PM
This revision was automatically updated to reflect the committed changes.