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

[Liu Yuan]

On Tue, Jul 16, 2013 at 02:21:25PM +0900, Hitoshi Mitake wrote:
> 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.)
> 
 
Seem not a bug, we don't return for discard command yet and VM won't proceed
with subsequent IO requests, no?

Thanks
Yuan