[Sheepdog] [PATCH v2] object cache: incorrect lock may lead to update lost

[Liu Yuan]

On 04/16/2012 12:48 AM, Yunkai Zhang wrote:

> I don't think so.
> 
> In fact, I put it out specially for three reasons:
> 
> 1). It will make the lock strategy more obviously when people read
> object_cache_rw/object_cache_push function.
> 2). As the name implies, the target of these xxx_dirty_tree_and_list
> functions is *just* dirty_tree_and_list. Lock logic should not
> included in it.
> 3). Put the lock out of them will make these functions obey the
> principle of orthogonality, so we can compose them like this example
> in somewhere:
>         pthread_mutex_lock(oc->lock);
>         switch_dirty_tree_and_list()
>         ...
>         merge_dirty_tree_and_list()
>         ...
>         add_to_dirty_tree_and_list()
>         ...
>         pthread_mutex_unlock(oc->lock)


I am not 100% against the coding style if you insist it. but I have to
voice out my opinion:

On principle,

1) lock should be part of internal logic that should be encapsulated
completely from upper layer, if you really code any decent interface.

2) back to our case, switch and merge operation is NOT as low level as
add and del, they are more part of logic of object_cache_push() than
low level interface that is expected by others.

>         pthread_mutex_lock(oc->lock);
>         switch_dirty_tree_and_list()
>         ...
>         merge_dirty_tree_and_list()
>         ...
>         add_to_dirty_tree_and_list()
>         ...
>         pthread_mutex_unlock(oc->lock)

  this is quite artificial, both switch and merge is higher level on top
of add and del operation and I don't think there is any REAL case that
we will use switch and merge together in a lock. That is, both switch
and merge should be coded as a independent interface that handles lock
internally.

  If I were you, I would code a another interface
add_to_dirty_tree_and_list() as
  add_to_dirty_tree_and_list(oc, idx, create)
  {
	pthread_mutex_lock(oc->lock);
	add_to_active_dirty_tree_and_list(...);
	pthread_mutex_unlock(oc->lock);
  }

  So orthogonal interface doesn't necessarily mean you should export low
level interface to the users and rely them to do the locking.

Thanks,
Yuan