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.u...@gmail.com>;
> Date: Wed, Aug 6, 2014 07:02 PM
> To: "朱炳鹏"<nku...@foxmail.com>;
> Cc: "sheepdog"<sheepdog@lists.wpkg.org>; "Yu
> Fang"<bingpeng....@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 <nku...@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....@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
--
sheepdog mailing list
sheepdog@lists.wpkg.org
http://lists.wpkg.org/mailman/listinfo/sheepdog
