On Tue, Mar 13, 2018 at 2:37 PM, Adam C. Emerson <aemerson@xxxxxxxxxx> wrote:
> On 13/03/2018, Gregory Farnum wrote:
>> Oooo, yeah. So there are *quick grep* 66 of those. In the Objecter, we
>> pretty much wrap everything in a unique_lock, pass that through the
>> function call chains, and make use of the "owns_lock" functionality.
>> unique_lock is mostly analogous to our Locker but not quite (you can
>> lock and unlock through it) and might be a simpler way to swap it in
>> on a per-thread basis, if we wanted to get gross. (Or do the same
>> thing as Objecter where we force you to pass a unique_lock in to all
>> of those internal functions; I think they mostly only assert in cases
>> where they're going to change the lock state? So it might be good prep
>> work for stuff like the MDS going multi-lock anyway.)
>>
>> Straight-up losing that safety check without replacement would be a
>> bit sad. Not sure it's worth holding anything up for or not, though.
>> -Greg
>
> My thought is we probably don't really /need/ these checks. If we
> /want/ them, especially in places like Objecter where there's One Lock
> per Class, we should do them statically. This is fairly easy to do in
> the type system if a bit fiddly. The big downside is we'd be replacing
> unique_lock shared_lock with magical template Foo.
Well, no, we don't *need* them. They're an assert to make sure the
function isn't being called from an invalid context.
On Tue, Mar 13, 2018 at 3:16 PM, Jesse Williamson <jwilliamson@xxxxxxx> wrote:
>
> // JFW: these are race conditions waiting to happen... should be done away
> with!
> bool is_locked() const { return nlock > 0; }
> bool is_locked_by_me() const { return is_locked() &&
> this_thread::get_id() == locked_by; }
So let me restate that super clearly: these checks are *absolutely*
not used to make run-time decisions. (So I don't think there's any
racing that can happen.)
I am...not a super big fan of the Objecter passing around a
unique_lock parameter to a huge set of functions. It's not bad,
exactly, it's just icky, especially since the Objecter (unlike our
other single-lock users) is actually using different locks in
different contexts.
But if we can strip that down to not have any run-time cost, or
especially if there's some static check we can use that doesn't make
the code check out any differently, it's probably worth it to let us
programmatically check interfaces instead of hold them in people's
heads during review.
-Greg
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
