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