[sheepdog] [PATCH v1] sheep/http: fix error in bucket_delete

[Liu Yuan]

On Wed, Aug 06, 2014 at 07:29:29PM +0800, 朱炳鹏 wrote:
> Thanks, yuan.
> I have already modified bnode_lookup() in this patch. Please review it.
> ‍
> 

I didn't see any modification on bnode_lookup() in this patch. Am I missing
something?

Thanks
Yuan

> 
> 
> 
> ------------------ Original ------------------
> From:  "Liu Yuan";<namei.unix at gmail.com>;
> Date:  Wed, Aug 6, 2014 07:02 PM
> To:  "朱炳鹏"<nkuzbp at foxmail.com>; 
> Cc:  "sheepdog"<sheepdog at lists.wpkg.org>; "Yu Fang"<bingpeng.zbp at alibaba-inc.com>; 
> Subject:  Re: [sheepdog] [PATCH v1] sheep/http: fix error in bucket_delete
> 
> 
> 
> On Tue, Aug 05, 2014 at 08:33:46PM +0800, Bingpeng Zhu wrote:
> > From: NankaiZBP <nkuzbp at foxmail.com>
> > 
> > In current implementation, When we create a bucket, we decide
> > the bnode's location in account VDI using sd_hash(bucket_name)
> > as key. We handle hash conflict by linear probing hash table.
> > Here is the bug:
> > When we delete a bucket, we can't discard its bnode. Because
> > bnode_lookup() need it to find if some bucket exists or not
> > by checking adjacent bnodes. Therefore, we just zero its
> > bnode.name when client want to delete a bucket. When we create
> > a bucket later, we can reuse the deleted bnode if they hash to
> > the same location in account VDI.
> > 
> > Signed-off-by: Yu Fang <bingpeng.zbp at alibaba-inc.com>
> > ---
> >  sheep/http/kv.c |   37 +++++++++++++++++++++++++++++--------
> >  1 files changed, 29 insertions(+), 8 deletions(-)
> > 
> > diff --git a/sheep/http/kv.c b/sheep/http/kv.c
> > index 5c4b386..2060bb1 100644
> > --- a/sheep/http/kv.c
> > +++ b/sheep/http/kv.c
> > @@ -247,18 +247,21 @@ int kv_delete_account(struct http_request *req, const char *account)
> >   */
> >  
> >  static int bnode_do_create(struct kv_bnode *bnode, struct sd_inode *inode,
> > -			   uint32_t idx)
> > +			   uint32_t idx, bool create)
> >  {
> >  	uint32_t vid = inode->vdi_id;
> >  	uint64_t oid = vid_to_data_oid(vid, idx);
> >  	int ret;
> >  
> >  	bnode->oid = oid;
> > -	ret = sd_write_object(oid, (char *)bnode, sizeof(*bnode), 0, true);
> > +	ret = sd_write_object(oid, (char *)bnode, sizeof(*bnode), 0, create);
> >  	if (ret != SD_RES_SUCCESS) {
> >  		sd_err("failed to create object, %" PRIx64, oid);
> >  		goto out;
> >  	}
> > +	if (!create)
> > +		goto out;
> > +
> >  	sd_inode_set_vid(inode, idx, vid);
> >  	ret = sd_inode_write_vid(inode, idx, vid, vid, 0, false, false);
> >  	if (ret != SD_RES_SUCCESS) {
> > @@ -276,6 +279,7 @@ static int bnode_create(struct kv_bnode *bnode, uint32_t account_vid)
> >  	uint32_t tmp_vid, idx;
> >  	uint64_t hval, i;
> >  	int ret;
> > +	bool create = true;
> >  
> >  	ret = sd_read_object(vid_to_vdi_oid(account_vid), (char *)inode,
> >  			     sizeof(*inode), 0);
> > @@ -289,16 +293,27 @@ static int bnode_create(struct kv_bnode *bnode, uint32_t account_vid)
> >  	for (i = 0; i < MAX_DATA_OBJS; i++) {
> >  		idx = (hval + i) % MAX_DATA_OBJS;
> >  		tmp_vid = sd_inode_get_vid(inode, idx);
> > -		if (tmp_vid)
> > -			continue;
> > -		else
> > +		if (tmp_vid) {
> > +			uint64_t oid = vid_to_data_oid(account_vid, idx);
> > +			char name[SD_MAX_BUCKET_NAME] = { };
> > +
> > +			ret = sd_read_object(oid, name, sizeof(name), 0);
> > +			if (ret != SD_RES_SUCCESS)
> > +				goto out;
> > +			if (name[0] == 0) {
> > +				create = false;
> > +				goto create;
> > +			}
> > +		} else
> >  			break;
> >  	}
> >  	if (i == MAX_DATA_OBJS) {
> >  		ret = SD_RES_NO_SPACE;
> >  		goto out;
> >  	}
> > -	ret = bnode_do_create(bnode, inode, idx);
> > +
> > +create:
> > +	ret = bnode_do_create(bnode, inode, idx, create);
> >  out:
> >  	free(inode);
> >  	return ret;
> > @@ -423,6 +438,7 @@ static int bucket_delete(const char *account, uint32_t avid, const char *bucket)
> >  	struct kv_bnode bnode;
> >  	char onode_name[SD_MAX_VDI_LEN];
> >  	char alloc_name[SD_MAX_VDI_LEN];
> > +	char name[SD_MAX_BUCKET_NAME] = {};
> >  	int ret;
> >  
> >  	snprintf(onode_name, SD_MAX_VDI_LEN, "%s/%s", account, bucket);
> > @@ -436,9 +452,14 @@ static int bucket_delete(const char *account, uint32_t avid, const char *bucket)
> >  	if (bnode.object_count > 0)
> >  		return SD_RES_VDI_NOT_EMPTY;
> >  
> > -	ret = sd_discard_object(bnode.oid);
> > +	/*
> > +	 * We can't discard bnode because bnode_lookup() need it to find
> > +	 * if some bucket exists or not by checking adjacent bnodes.
> > +	 * So we just zero bnode.name to indicate a deleted bucket.
> > +	 */
> > +	ret = sd_write_object(bnode.oid, name, sizeof(name), 0, false);
> >  	if (ret != SD_RES_SUCCESS) {
> > -		sd_err("failed to discard bnode for %s", bucket);
> > +		sd_err("failed to zero bnode for %s", bucket);
> >  		return ret;
> >  	}
> >  	sd_delete_vdi(onode_name);
> 
> If we try to mimic the onode lookup algorithm as your patch did, we should also
> modify the bnode_lookup() too. See onode_lookup_nolock().
> 
> Thanks
> Yuan