[sheepdog] [PATCH] sheep: cleaning vdi deletion process of sheep daemon

[Hitoshi Mitake]

At Fri, 27 Dec 2013 15:19:28 +0800,
Liu Yuan wrote:
> 
> On Thu, Dec 26, 2013 at 03:53:51PM +0900, Hitoshi Mitake wrote:
> > Current code of vdi deletion process in sheep deamon is a real
> > disaster. Especially, its irregular usage of work queue is terrible
> > thing for maintenance. In addition, the vdi deletion process can
> > potentially notify a completion of deletion to a dog process before
> > actual completion.
> > 
> > This patch cleans the VDI deletion code and removes the above bug.
> > 
> > Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
> > ---
> >  sheep/vdi.c |  301 ++++++++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 176 insertions(+), 125 deletions(-)
> > 
> > diff --git a/sheep/vdi.c b/sheep/vdi.c
> > index eca4f43..49038c5 100644
> > --- a/sheep/vdi.c
> > +++ b/sheep/vdi.c
> > @@ -754,22 +754,6 @@ int vdi_snapshot(const struct vdi_iocb *iocb, uint32_t *new_vid)
> >  				  info.vid);
> >  }
> >  
> > -static int start_deletion(struct request *req, uint32_t vid);
> > -
> > -int vdi_delete(const struct vdi_iocb *iocb, struct request *req)
> > -{
> > -	struct vdi_info info;
> > -	int ret;
> > -
> > -	ret = vdi_lookup(iocb, &info);
> > -	if (ret != SD_RES_SUCCESS)
> > -		goto out;
> > -
> > -	ret = start_deletion(req, info.vid);
> > -out:
> > -	return ret;
> > -}
> > -
> >  int read_vdis(char *data, int len, unsigned int *rsp_len)
> >  {
> >  	if (len != sizeof(sys->vdi_inuse))
> > @@ -781,28 +765,35 @@ int read_vdis(char *data, int len, unsigned int *rsp_len)
> >  	return SD_RES_SUCCESS;
> >  }
> >  
> > -struct deletion_work {
> > -	uint32_t done;
> > +struct deletion_info {
> > +	uint32_t target_vid;
> > +
> > +	int delete_vid_count;
> > +	uint32_t *delete_vid_array;
> > +
> > +	int done_deletion;
> >  
> > +	int finish_fd;
> > +	struct list_node list;	/* connected deletion_info_queue */
> > +};
> > +
> > +struct deletion_work {
> >  	struct work work;
> > -	struct list_node list;
> > -	struct request *req;
> >  
> >  	uint32_t vid;
> > -
> > -	int count;
> > -	uint32_t *buf;
> > +	struct deletion_info *di;
> >  };
> >  
> > -static LIST_HEAD(deletion_work_list);
> > +static pthread_mutex_t deletion_info_queue_lock = PTHREAD_MUTEX_INITIALIZER;
> > +static LIST_HEAD(deletion_info_queue);
> >  
> > -static int delete_inode(struct deletion_work *dw)
> > +static int delete_inode(uint32_t vid)
> >  {
> >  	struct sd_inode *inode = NULL;
> >  	int ret = SD_RES_SUCCESS;
> >  
> >  	inode = xzalloc(sizeof(*inode));
> > -	ret = sd_read_object(vid_to_vdi_oid(dw->vid), (char *)inode,
> > +	ret = sd_read_object(vid_to_vdi_oid(vid), (char *)inode,
> >  			     SD_INODE_HEADER_SIZE, 0);
> >  	if (ret != SD_RES_SUCCESS) {
> >  		ret = SD_RES_EIO;
> > @@ -811,7 +802,7 @@ static int delete_inode(struct deletion_work *dw)
> >  
> >  	memset(inode->name, 0, sizeof(inode->name));
> >  
> > -	ret = sd_write_object(vid_to_vdi_oid(dw->vid), (char *)inode,
> > +	ret = sd_write_object(vid_to_vdi_oid(vid), (char *)inode,
> >  			      SD_INODE_HEADER_SIZE, 0, false);
> >  	if (ret != 0) {
> >  		ret = SD_RES_EIO;
> > @@ -871,16 +862,15 @@ static void delete_cb(void *data, enum btree_node_type type, void *arg)
> >  	}
> >  }
> >  
> > -static void delete_one(struct work *work)
> > +static void delete_one_vdi(struct work *work)
> >  {
> > -	struct deletion_work *dw = container_of(work, struct deletion_work, work);
> > -	uint32_t vdi_id = *(dw->buf + dw->count - dw->done - 1);
> > +	struct deletion_work *dw =
> > +		container_of(work, struct deletion_work, work);
> > +	uint32_t vdi_id = dw->vid;
> >  	int ret;
> >  	uint32_t i, nr_deleted, nr_objs;
> >  	struct sd_inode *inode = NULL;
> >  
> > -	sd_debug("%d %d, %16x", dw->done, dw->count, vdi_id);
> > -
> >  	inode = malloc(sizeof(*inode));
> >  	if (!inode) {
> >  		sd_err("failed to allocate memory");
> > @@ -942,74 +932,111 @@ out:
> >  	free(inode);
> >  }
> >  
> > -static void delete_one_done(struct work *work)
> > +static void delete_one_vdi_done(struct work *work);
> > +
> > +static struct deletion_work *create_next_deletion_work(struct deletion_info *di)
> >  {
> > -	struct deletion_work *dw = container_of(work, struct deletion_work, work);
> > -	struct request *req = dw->req;
> > +	struct deletion_work *dw = NULL;
> >  
> > -	dw->done++;
> > -	if (dw->done < dw->count) {
> > -		queue_work(sys->deletion_wqueue, &dw->work);
> > -		return;
> > +	if (!(di->done_deletion < di->delete_vid_count))
> > +		/* no more deletion work, this deletion info is completed */
> 
> simply check di->done_deletion >= di->delete_vid_count

I dont't like the comparison operators > and >=. We put smaller
numbers on left side of number lines and put larger numbers on right
side. I think current expression is natural.

Thanks,
Hitoshi