Re: Fileassoc locking

[Elad Efrat <elad%NetBSD.org@localhost>]

Hey--

Should have answered your questions in the other mail... but I forgot. :)

On Sat, Dec 26, 2009 at 3:02 AM, Adam Hamsik <haaaad%gmail.com@localhost> wrote:

>>> Where faf->faf_lock was entered here ?
>>
>> file_free() expects it to be locked -- see comment.
>
> Locking structure in one routine and then releasing it in other is not 
> standard behavior. I think that you should revisit it. In best case you 
> should do mutex_enter(foo) and mutex_exit(foo) in same routine not in two or 
> more.

I've changed that and now (hopefully) there are no obscure locking patterns...

>> I have removed the locking on "assoc" here. There's going to be a
>> favorable reader:writer ratio forfileassoc_list_lock -- should it
>> still be a mutex?
>
> I think that mutexes are much cheaper then rw locks in locking. bu you should 
> hold mutex on fileassoc_list_lock only for absolutely required time (you 
> should not do any heavy operations like, memory allocation, memory free etc.)

Right; my other mail discusses this subject more thoroughly.

>> I think maybe to make fileassoc_lookup() always increase the reference
>> count on the file entry before it returns, and require the caller
>> (user of the KPI) to fileassoc_release() once it's done to avoid the
>> data being freed while in use.
>
> Yes this seems like a good approach to me. protocol can be
> mutex_enter(faf_list)
> ... find faf ...
>  mutex_enter(faf)
>  increase ref counter
>  mutex_exit(faf)
> mutex_exit(faf_list)

It is unclear to me why we need to hold a mutex (rather than merely a
reference) when traversing a list that will not change until reference
count has dropped. What am I missing? (See other mail about this as
well.)

> But you need to be sure that there is no other code who is doing mutex_enter 
> on faf_list and faf locks in a reversed order.

I think this is addressed in the new version of the file. Also,
locking is done internally, and fileassoc(9) users should (eventually)
only care about referencing the entry they're using -- "entry" being
(as my other mail explains) a combination of a file entry and an
"assoc."

Thanks,

-e.