[sheepdog] [PATCH V4 1/2] sheep: refactor gateway_forward_request

[Liu Yuan]

On 09/03/2012 06:59 PM, Yunkai Zhang wrote:
> On Mon, Sep 3, 2012 at 6:20 PM, Liu Yuan <namei.unix at gmail.com> wrote:
>> On 09/02/2012 08:59 PM, Yunkai Zhang wrote:
>>> +static int forward_request_concurrently(struct sd_req *hdr,
>>> +                                     void *data, unsigned int *wlen,
>>> +                                     struct sd_vnode *target_vnodes[],
>>> +                                     struct sd_rsp target_rsps[],
>>> +                                     void *target_data[],
>>> +                                     int nr_targets)
>>
>> This interface looks unnecessarily complex to me. I'd suggest changes like following
>> untested patch , it is more generic and simpler, simply adding one more caller provided
>> buffer to read response data if any.
>>
>> Thanks,
>> Yuan
>>
>> =====================================================
>> diff --git a/sheep/gateway.c b/sheep/gateway.c
>> index 41d712b..690f0ac 100644
>> --- a/sheep/gateway.c
>> +++ b/sheep/gateway.c
>> @@ -150,10 +150,12 @@ static inline void pfd_info_init(struct write_info *wi, struct pfd_info *pi)
>>   *
>>   * Return error code if any one request fails.
>>   */
>> -static int wait_forward_request(struct write_info *wi, struct sd_rsp *rsp)
>> +static int wait_forward_request(struct write_info *wi, void *read_buffer)
>>  {
>>         int nr_sent, err_ret = SD_RES_SUCCESS, ret, pollret, i;
>> -       struct pfd_info pi;;
>> +       struct pfd_info pi;
>> +       struct sd_rsp rsp;
>> +       char *rb_pos = (char *)read_buffer;
>>  again:
>>         pfd_info_init(wi, &pi);
>>         pollret = poll(pi.pfds, pi.nr, -1);
>> @@ -174,23 +176,37 @@ again:
>>                 if (re & (POLLERR | POLLHUP | POLLNVAL)) {
>>                         err_ret = SD_RES_NETWORK_ERROR;
>>                         finish_one_write_err(wi, i);
>> -               } else if (re & POLLIN) {
>> -                       if (do_read(pi.pfds[i].fd, rsp, sizeof(*rsp))) {
>> +                       goto finish_write;
>> +               }
>> +               if (!(re & POLLIN)) {
>> +                       eprintf("unhandled poll event\n");
>> +                       goto finish_write;
>> +               }
>> +               if (do_read(pi.pfds[i].fd, &rsp, sizeof(rsp))) {
>> +                       eprintf("remote node might have gone away\n");
>> +                       err_ret = SD_RES_NETWORK_ERROR;
>> +                       finish_one_write_err(wi, i);
>> +                       goto finish_write;
>> +               }
>> +
>> +               if (rsp.data_length && read_buffer) {
>> +                       memcpy(rb_pos, wi->ent[i].nid, sizeof(wi->ent[i].nid));
>> +                       rb_pos += sizeof(wi->ent[i].nid);
>> +                       if (do_read(pi.pfds[i].fd, &read_buffer,
>> +                                   rsp.data_length)) {
>>                                 eprintf("remote node might have gone away\n");
>>                                 err_ret = SD_RES_NETWORK_ERROR;
>>                                 finish_one_write_err(wi, i);
>>                                 goto finish_write;
>>                         }
>> -
>> -                       ret = rsp->result;
>> -                       if (ret != SD_RES_SUCCESS) {
>> -                               eprintf("fail %"PRIx32"\n", ret);
>> -                               err_ret = ret;
>> -                       }
>> -                       finish_one_write(wi, i);
>> -               } else {
>> -                       eprintf("unhandled poll event\n");
>> +                       rb_pos += rsp.data_length;
>>                 }
>> +               ret = rsp.result;
>> +               if (ret != SD_RES_SUCCESS) {
>> +                       eprintf("fail %"PRIx32"\n", ret);
>> +                       err_ret = ret;
>> +               }
>> +               finish_one_write(wi, i);
>>         }
>>  finish_write:
>>         if (wi->nr_sent > 0)
>> @@ -225,11 +241,10 @@ static inline void gateway_init_fwd_hdr(struct sd_req *fwd, struct sd_req *hdr)
>>         fwd->proto_ver = SD_SHEEP_PROTO_VER;
>>  }
>>
>> -static int gateway_forward_request(struct request *req)
>> +static int gateway_forward_request(struct request *req, void *read_buffer)
>>  {
>> -       int i, err_ret = SD_RES_SUCCESS, ret, local = -1;
>> +       int i, err_ret = SD_RES_SUCCESS, ret;
>>         unsigned wlen;
>> -       struct sd_rsp *rsp = (struct sd_rsp *)&req->rp;
>>         struct sd_vnode *v;
>>         struct sd_vnode *obj_vnodes[SD_MAX_COPIES];
>>         uint64_t oid = req->rq.obj.oid;
>> @@ -253,10 +268,6 @@ static int gateway_forward_request(struct request *req)
>>                 struct sockfd *sfd;
>>
>>                 v = obj_vnodes[i];
>> -               if (vnode_is_local(v)) {
>> -                       local = i;
>> -                       continue;
>> -               }
>>
>>                 sfd = sheep_get_sockfd(&v->nid);
>>                 if (!sfd) {
>> @@ -274,21 +285,9 @@ static int gateway_forward_request(struct request *req)
>>                 write_info_advance(&wi, v, sfd);
>>         }
>>
>> -       if (local != -1 && err_ret == SD_RES_SUCCESS) {
>> -               v = obj_vnodes[local];
>> -
>> -               assert(op);
>> -               ret = sheep_do_op_work(op, req);
>> -
>> -               if (ret != SD_RES_SUCCESS) {
>> -                       eprintf("fail to write local %"PRIx32"\n", ret);
>> -                       err_ret = ret;
>> -               }
>> -       }
>> -
>>         dprintf("nr_sent %d, err %x\n", wi.nr_sent, err_ret);
>>         if (wi.nr_sent > 0) {
>> -               ret = wait_forward_request(&wi, rsp);
>> +               ret = wait_forward_request(&wi, read_buffer);
>>                 if (ret != SD_RES_SUCCESS)
>>                         err_ret = ret;
>>         }
>> @@ -301,7 +300,7 @@ int gateway_write_obj(struct request *req)
>>         if (sys->enable_write_cache && !req->local && !bypass_object_cache(req))
>>                 return object_cache_handle_request(req);
>>
>> -       return gateway_forward_request(req);
>> +       return gateway_forward_request(req, NULL);
>>  }
>>
>>  int gateway_create_and_write_obj(struct request *req)
>> @@ -309,10 +308,10 @@ int gateway_create_and_write_obj(struct request *req)
>>         if (sys->enable_write_cache && !req->local && !bypass_object_cache(req))
>>                 return object_cache_handle_request(req);
>>
>> -       return gateway_forward_request(req);
>> +       return gateway_forward_request(req, NULL);
>>  }
>>
>>  int gateway_remove_obj(struct request *req)
>>  {
>> -       return gateway_forward_request(req);
>> +       return gateway_forward_request(req, NULL);
>>  }
> 
> 
> It's ok for me.
> 

Feel free to take up my naive patch and continue with your patch set.

Thanks,
Yuan