[sheepdog] [PATCH v3 3/3] sheep/http: add lock to protect container and object

[Robin Dong]

2013/12/12 Liu Yuan <namei.unix at gmail.com>

> On Thu, Dec 12, 2013 at 02:26:19PM +0800, Robin Dong wrote:
> > 2013/12/12 Liu Yuan <namei.unix at gmail.com>
> >
> > > On Wed, Dec 11, 2013 at 06:14:43PM +0800, Robin Dong wrote:
> > > > From: Robin Dong <sanbai at taobao.com>
> > > >
> > > > Add locks to protect containers and objects when users create/delete
> > > > containers or create/delete objects in the same time.
> > > >
> > > > Signed-off-by: Robin Dong <sanbai at taobao.com>
> > > > ---
> > > >  sheep/http/kv.c    | 252
> > > ++++++++++++++++++++++++++++++++---------------------
> > > >  sheep/http/swift.c |  10 ++-
> > > >  2 files changed, 162 insertions(+), 100 deletions(-)
> > > >
> > > > diff --git a/sheep/http/kv.c b/sheep/http/kv.c
> > > > index fb66dfa..4d690cb 100644
> > > > --- a/sheep/http/kv.c
> > > > +++ b/sheep/http/kv.c
> > > > @@ -218,22 +218,22 @@ static int kv_get_account(const char *account,
> > > uint32_t *nr_buckets)
> > > >
> > > >       ret = lookup_vdi(account, &account_vid);
> > > >       if (ret != SD_RES_SUCCESS)
> > > > -             return ret;
> > > > +             goto out;
> > > >
> > > >       /* read account vdi out */
> > > >       oid = vid_to_vdi_oid(account_vid);
> > > >       ret = sd_read_object(oid, (char *)&inode, sizeof(struct
> sd_inode),
> > > 0);
> > > >       if (ret != SD_RES_SUCCESS) {
> > > >               sd_err("Failed to read inode header %lx", oid);
> > > > -             return ret;
> > > > +             goto out;
> > > >       }
> > > >
> > > >       struct list_buckets_arg arg = {NULL, NULL, NULL, 0};
> > > >       traverse_btree(sheep_bnode_reader, &inode, list_buckets_cb,
> &arg);
> > > >       if (nr_buckets)
> > > >               *nr_buckets = arg.bucket_counter;
> > > > -
> > > > -     return SD_RES_SUCCESS;
> > > > +out:
> > > > +     return ret;
> > > >  }
> > > >
> > > >  int kv_read_account(const char *account, uint32_t *nr_buckets)
> > > > @@ -528,21 +528,22 @@ out:
> > > >  }
> > > >
> > > >  static int kv_get_bucket(struct sd_inode *account_inode, const char
> > > *account,
> > > > -                      const char *bucket)
> > > > +                      const char *bucket, uint32_t *account_vid)
> > >
> > > account_inode already has the account_vid, no? So we can remove
> > > account_vdi as
> > > parameter.
> > >
> >
> > No, the account_vid will be set by lookup_vdi() below, so we can't remove
> > it.
> >
> >
> > >
> > > >  {
> > > >       char vdi_name[SD_MAX_VDI_LEN];
> > > >       uint64_t oid;
> > > > -     uint32_t account_vid, bucket_vid;
> > > > +     uint32_t bucket_vid;
> > > >       int ret;
> > > >
> > > > -     ret = lookup_vdi(account, &account_vid);
> > > > +     ret = lookup_vdi(account, account_vid);
> > > >       if (ret != SD_RES_SUCCESS) {
> > > >               sd_err("Failed to find account %s", account);
> > > >               return -1;
> > > >       }
> > > >
> > > > +     sys->cdrv->lock(*account_vid);
> > >
> > > Seems that you don't unlock after exit of this function?
> > >
> >
> > We must lock the account_vid before read the accout_inode and unlock
> after
> > create(or delete) the container,
> > so we call lock() in kv_get_bucket() and call unlock() in the end
> > ofkv_create_bucket() and kv_delete_bucket().
> > If not so, the buckets will corrupt under parallely high pressure tests (
> > https://github.com/RobinDong/misc/blob/master/sheepdog_test/pswift.sh)
> >
>
> If we can't move the ->lock out of kv_get_bucket(), then I'd suggest that
> we
> rename it as kv_get_lock_bucket() to explicitly inform people it locks the
> bucket after calling it.
>

Nice suggestion. I will change it on next version.

>
> Thanks
> Yuan
>



-- 
--
Best Regard
Robin Dong
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.wpkg.org/pipermail/sheepdog/attachments/20131212/d450ce8b/attachment-0004.html>