CAS is broken

[Tomash Brechko]

On Mon, Nov 19, 2007 at 23:26:13 -0800, dormando wrote:
> I fixed this with an extensively ugly patch in r646. I didn't commit
> your tests with it though; if you think they cover things enough I'll
> happily apply.

My test case doesn't cove the whole case, so a better test should be
added, because the problem is not a simple one.

I looked through your patch, here are my comments:


> diff --git a/trunk/server/memcached.c b/trunk/server/memcached.c
> index dbbb154..74bf06c 100644
> --- a/trunk/server/memcached.c
> +++ b/trunk/server/memcached.c
> @@ -527,6 +541,66 @@ static void conn_set_state(conn *c, int state) {
>      }
>  }
>  
> +/*
> + * Free list management for suffix buffers.
> + */
> +
> +static char **freesuffix;
> +static int freesuffixtotal;
> +static int freesuffixcurr;
> +
> +static void suffix_init(void) {
> +    freesuffixtotal = 500;
> +    freesuffixcurr  = 0;
> +
> +    freesuffix = (char **)malloc( sizeof(char *) *
> +                                  21 * freesuffixtotal );

freesuffix is the array of pointers, '21' is not required here.


> +    if (freesuffix == NULL) {
> +        perror("malloc()");
> +    }
> +    return;
> +}
> +
> +/*
> + * Returns a suffix buffer from the freelist, if any. Should call this using
> + * suffix_from_freelist() for thread safety.
> + */
> +char *do_suffix_from_freelist() {
> +    char *s;
> +
> +    if (freesuffixcurr > 0) {
> +        s = freesuffix[--freesuffixcurr];
> +    } else {
> +        /* FIXME: global define? */
> +        /* If malloc fails, let the logic fall through without spamming
> +         * STDERR on the server. */
> +        s = malloc( sizeof(char *) * SUFFIX_SIZE );
> +    }

[*] (see below).


> +
> +    return s;
> +}
> +
> +/*
> + * Adds a connection to the freelist. 0 = success. Should call this using
> + * conn_add_to_freelist() for thread safety.
> + */
> +bool do_suffix_add_to_freelist(char *s) {
> +    if (freesuffixcurr < freesuffixtotal) {
> +        freesuffix[freesuffixcurr++] = s;
> +        return false;
> +    } else {
> +        /* try to enlarge free connections array */
> +        char **new_freesuffix = realloc(freesuffix,
> +                                        SUFFIX_SIZE * freesuffixtotal * 2);

Same here, no need to multiply by SUFFIX_SIZE.


> +        if (new_freesuffix) {
> +            freesuffixtotal *= 2;
> +            freesuffix = new_freesuffix;
> +            freesuffix[freesuffixcurr++] = s;
> +            return false;
> +        }
> +    }
> +    return true;
> +}

You never check the result of suffix_add_to_freelist() later.  Since
you allocate memory in suffix_from_freelist() (see [*] mark above), it
would be natural to free it here, if failed to put it to the free
list.



> @@ -1158,12 +1232,33 @@ static inline void process_get_command(conn *c, token_t *tokens, size_t ntokens,
>                   *   " " + flags + " " + data length + "\r\n" + data (with \r\n)
>                   */
>  
> -                if(return_key_ptr == true)
> +                if(return_cas == true)
>                  {
> -                  sprintf(suffix," %d %d %llu\r\n", atoi(ITEM_suffix(it) + 1),
> -                      it->nbytes - 2, it->cas_id);
> +                  /* Goofy mid-flight realloc. */
> +                  if (i >= c->suffixsize) {
> +                    char **new_suffix_list = realloc(c->suffixlist,
> +                                           sizeof(char *) * c->suffixsize * 2);
> +                    if (new_suffix_list) {
> +                      c->suffixsize *= 2;
> +                      c->suffixlist  = new_suffix_list;
> +                    }

What if new_suffix_list is NULL?  You have to abort on that too.



> +                  }
> +
> +                  suffix = suffix_from_freelist();
> +                  if (suffix == NULL) {
> +                    STATS_LOCK();
> +                    stats.get_cmds   += stats_get_cmds;
> +                    stats.get_hits   += stats_get_hits;
> +                    stats.get_misses += stats_get_misses;
> +                    STATS_UNLOCK();
> +                    out_string(c, "SERVER_ERROR out of memory");
> +                    return;
> +                  }
> +                  *(c->suffixlist + i) = suffix;
> +                  sprintf(suffix, " %llu\r\n", it->cas_id);

suffix is 21 chars long, but you need ' ' + 20 + '\r' + '\n' + '\0' ==
24 (sprintf() will copy that '\0' from the format, even if you don't
need it).


> @@ -1223,6 +1322,10 @@ static inline void process_get_command(conn *c, token_t *tokens, size_t ntokens,
>          || (c->udp && build_udp_headers(c) != 0)) {
>          out_string(c, "SERVER_ERROR out of memory");
>      }
> +    else if (return_cas) {
> +        conn_set_state(c, conn_caswrite);
> +        c->msgcurr = 0;
> +    }
>      else {
>          conn_set_state(c, conn_mwrite);
>          c->msgcurr = 0;
> @@ -2102,9 +2205,10 @@ static void drive_machine(conn *c) {
>              /* fall through... */
>  
>          case conn_mwrite:
> +        case conn_caswrite:
>              switch (transmit(c)) {
>              case TRANSMIT_COMPLETE:
> -                if (c->state == conn_mwrite) {
> +                if (c->state == conn_mwrite || c->state == conn_caswrite) {
>                      while (c->ileft > 0) {
>                          item *it = *(c->icurr);
>                          assert((it->it_flags & ITEM_SLABBED) == 0);
> @@ -2112,6 +2216,14 @@ static void drive_machine(conn *c) {
>                          c->icurr++;
>                          c->ileft--;
>                      }
> +                    if (c->state == conn_caswrite) {
> +                        while (c->suffixleft > 0) {
> +                            char *suffix = *(c->suffixcurr);
> +                            suffix_add_to_freelist(suffix);
> +                            c->suffixcurr++;
> +                            c->suffixleft--;
> +                        }
> +                    }

You don't need a separate conn_caswrite state, just output suffixes
whenever c->suffixleft > 0, and reset the counter afterwards.


But if we are going to use an ugly solution, there's a simpler one,
not involving global free list and mutexes: let conn have a pointer to
a single chunk of memory, and put suffixes there one after another (in
the format " %llu\r\n" as above).

Of course you can't save pointers to the suffixes in this chunk into
IOV, because on the realloc() the chunk it may move.  But you can
temporarily place suffix _offsets_ (cast to pointers) into IOV.
Before leaving process_get_command() you just add another pass and
convert all offsets stored in IOV to pointers (you know where they
are, because the loop has the same number of calls to add_iov(), so
every nth entry is the suffix offset).


-- 
   Tomash Brechko