[sheepdog] [PATCH 2/2] sheep: cleanup prepare_schedule_oid and oid_in_recovery

[MORITA Kazutaka]

At Sat, 31 Aug 2013 20:51:43 +0800,
Liu Yuan wrote:
> 
> On Fri, Aug 30, 2013 at 11:24:32PM +0900, MORITA Kazutaka wrote:
> > From: MORITA Kazutaka <morita.kazutaka at lab.ntt.co.jp>
> > 
> > This moves all the check of recovering objects from
> > prepare_schedule_oid() to oid_in_recovery(), and makes the code easier
> > to read.
> > 
> > This also fixes a problem that sheep doesn't recover an object when
> >  - sys->cinfo.disable_recovery is true,
> >  - rinfo->state is RW_PREPARE_LIST, and
> >  - oid_in_recovery() is called against the object
> > and fixes an error of tests/functional/010, which happens in rare
> > cases.
> > 
> > Signed-off-by: MORITA Kazutaka <morita.kazutaka at lab.ntt.co.jp>
> > ---
> >  include/sheep.h  |    5 ++++
> >  sheep/recovery.c |   77 ++++++++++++++++++++++++++++++------------------------
> >  2 files changed, 48 insertions(+), 34 deletions(-)
> > 
> > diff --git a/include/sheep.h b/include/sheep.h
> > index 68c852f..f9945c6 100644
> > --- a/include/sheep.h
> > +++ b/include/sheep.h
> > @@ -251,6 +251,11 @@ static inline const char *sd_strerror(int err)
> >  	return descs[err];
> >  }
> >  
> > +static inline int oid_cmp(const uint64_t *oid1, const uint64_t *oid2)
> > +{
> > +	return intcmp(*oid1, *oid2);
> > +}
> > +
> >  static inline int node_id_cmp(const struct node_id *node1,
> >  			      const struct node_id *node2)
> >  {
> > diff --git a/sheep/recovery.c b/sheep/recovery.c
> > index 6faaeab..4305e48 100644
> > --- a/sheep/recovery.c
> > +++ b/sheep/recovery.c
> > @@ -331,24 +331,11 @@ bool node_in_recovery(void)
> >  static inline void prepare_schedule_oid(uint64_t oid)
> >  {
> >  	struct recovery_info *rinfo = main_thread_get(current_rinfo);
> > -	uint64_t i;
> >  
> > -	for (i = 0; i < rinfo->nr_prio_oids; i++)
> > -		if (rinfo->prio_oids[i] == oid)
> > -			return;
> > -	/*
> > -	 * We need this check because oid might not be recovered.
> > -	 * Very much unlikely though, but it might happen indeed.
> > -	 */
> > -	for (i = 0; i < rinfo->done; i++)
> > -		if (rinfo->oids[i] == oid) {
> > -			sd_debug("%"PRIx64" not recovered, don't schedule it",
> > -				 oid);
> > -			return;
> > -		}
> 
> This check is removed entirely? I don't remember exactly how this check was
> introduced though.

This check is moved to oid_in_recovery():

+               if (xlfind(&oid, rinfo->oids, rinfo->done, oid_cmp)) {
+                       sd_debug("%" PRIx64 " has been already recovered", oid);
+                       return false;
+               }

> 
> > -	/* When recovery is not suspended, oid is currently being recovered */
> > -	if (!rinfo->suspended && rinfo->oids[rinfo->done] == oid)
> > +	if (xlfind(&oid, rinfo->prio_oids, rinfo->nr_prio_oids, oid_cmp)) {
> > +		sd_debug("%" PRIx64 " has been already in prio_oids", oid);
> >  		return;
> > +	}
> 
> is this correct? prio_oids are never sorted.

xlfind() does a linear search and the array doesn't need to be sorted.

Thanks,

Kazutaka