[sheepdog] [PATCH v2 3/3] sheep: remove cache object for discard operation

[Hitoshi Mitake]

At Tue, 16 Jul 2013 10:36:22 +0800,
Liu Yuan wrote:
> 
> On Mon, Jul 15, 2013 at 11:49:38PM +0900, Hitoshi Mitake wrote:
> > At Mon, 15 Jul 2013 14:28:48 +0800,
> > Liu Yuan wrote:
> > > 
> > > Signed-off-by: Liu Yuan <namei.unix at gmail.com>
> > > ---
> > >  sheep/object_cache.c |   28 ++++++++++++++++++++++++++++
> > >  sheep/sheep_priv.h   |    1 +
> > >  sheep/store.c        |    6 ++++++
> > >  3 files changed, 35 insertions(+)
> > > 
> > > diff --git a/sheep/object_cache.c b/sheep/object_cache.c
> > > index e3270df..1579a30 100644
> > > --- a/sheep/object_cache.c
> > > +++ b/sheep/object_cache.c
> > > @@ -1317,6 +1317,34 @@ out:
> > >  	return ret;
> > >  }
> > >  
> > > +int object_cache_remove(uint64_t oid)
> > > +{
> > > +	/* Inc the entry refcount to exclude the reclaimer */
> > > +	struct object_cache_entry *entry = oid_to_entry(oid);
> > > +	struct object_cache *oc = entry->oc;
> > > +	int ret;
> > > +
> > > +	if (!entry)
> > > +		return SD_RES_NO_OBJ;
> > > +
> > > +	sd_dprintf("%" PRIx64, oid);
> > > +	while (refcount_read(&entry->refcnt) > 1)
> > > +		usleep(100000); /* Object might be in push */
> > 
> > I believe this sort of magic number based coding should be
> > avoided.
> 
> just #define 100000 xxx doesn't make anything better. I think of wait-queue
> later to replace all the busy loop.
> 
> > coroutine would be suitable for writing this type of code cleanly. How
> > about recover it? I've heard that the previous sheepdog had coroutine
> > but it was reverted because of poor compatibility with valgrind.
> > 
> > But I believe coroutine is worth considering to use, because other
> > code in sheepdog (e.g. swithc_journal_file()) would be able to enjoy
> > it.
> 
> coroutine is notorious for:
> 1. bad code readability, very hard to maintain and extend
> 2. not work with gdb, very hard to debug
> 3. limited stack size
> 
> It might look smart but we should completely avoid this kind of trick.
> 
> What we really need, is wait queue mechanism, then above whie loop will simply
> be transformed into:
> 
>   wait_event(queue, refcount_read(&entry->refcnt) <= 1)
> 
> wait_event(q, condition) is much more straightforward scheduling mechanism than
> coroutine.

OK, I agree with not using coroutine.

BTW, it seems that object_cache_remove() contains a bug. There is a
possibility that other threads can acquire the removed object cache
between the end of usleep() loop and write_lock_cache(). We need a
better locking scheme for object cache.

(The sequence means a VM issues read or write direct after its remove
request. It would be rare but we don't have any assumption about
behaviour of VMs.)

Thanks,
Hitoshi