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

[Hitoshi Mitake]

At Fri, 27 Dec 2013 15:42:55 +0800,
Liu Yuan wrote:
> 
> On Fri, Dec 27, 2013 at 04:25:21PM +0900, Hitoshi Mitake wrote:
> > 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.
>  
> I am not sure how gcc will optimize A >= B and !(A < B), but for human mind,
> A >= B means a single operation and !(A < B) means two operations. For code
> readability, this is pointless to choose !(A < B) over (A >= B).
> 
> Most of the time I'll try hard to respect others' taste of coding style, but
> replacing (A >= B) by !(A < B) should be discouraged in terms of code readability.

OK, I'll fix it.

Thanks,
Hitoshi