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

[Liu Yuan]

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.

Thanks
Yuan