Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvs: refactor kvs cache to handle raw data as "primary" data #1246

Merged
merged 13 commits into from
Oct 24, 2017
Merged

kvs: refactor kvs cache to handle raw data as "primary" data #1246

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
53fe60e
modules/kvs/cache: Remove type parameter
chu11 Oct 18, 2017
66b864c
modules/kvs/cache: Rename internal variable
chu11 Oct 18, 2017
eaa151e
modules/kvs/cache: Refactor data store/free
chu11 Oct 18, 2017
1c97de0
modules/kvs/cache: Code re-org
chu11 Oct 18, 2017
a5e0358
modules/kvs/test: Remove duplicate cache tests
chu11 Oct 18, 2017
d9b2547
modules/kvs: Refactor cache API
chu11 Oct 18, 2017
25d6f58
modules/kvs/lookup: Update for KVS cache changes
chu11 Oct 19, 2017
2b258aa
modules/kvs/commit: Update for KVS cache changes
chu11 Oct 19, 2017
6c9fbce
modules/kvs: Update main kvs for cache changes
chu11 Oct 19, 2017
a905d9c
t/kvs: Adjust kvs tests for cache changes
chu11 Oct 20, 2017
7dcee34
modules/kvs: Remove cache_entry_clear_data()
chu11 Oct 23, 2017
f312169
modules/kvs: Remove unused/rarely used functions
chu11 Oct 23, 2017
b8b8c87
modules/kvs/cache: Set EINVAL in several functions
chu11 Oct 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 32 additions & 50 deletions src/modules/kvs/cache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,20 @@
#include "waitqueue.h"
#include "cache.h"

typedef enum {
CACHE_DATA_TYPE_NONE,
CACHE_DATA_TYPE_JSON,
CACHE_DATA_TYPE_RAW,
} cache_data_type_t;

struct cache_entry {
waitqueue_t *waitlist_notdirty;
waitqueue_t *waitlist_valid;
void *data; /* value object/data */
int len;
bool data_valid; /* flag indicating if data set, don't use
* data == NULL as test */
bool entry_valid; /* flag indicating if data set, don't use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: rename to valid and use same declaration style as dirty (either bool or bitfield is fine with me).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed that this should be consistent, however I'll make a new issue and clean up in another PR. Some functions (such as cache_entry_clear_dirty()) return the current setting of the dirty bit as a 1 or 0. This should be a bool, not a 1 or 0. It'll be a bit more cleanup than I'd like in this PR.

* data == NULL as test, as zero length
* data can be valid */
cache_data_type_t type; /* what does data point to */
int lastuse_epoch; /* time of last use for cache expiry */
uint8_t dirty:1;
Expand All @@ -64,22 +71,15 @@ struct cache {
zhash_t *zh;
};

struct cache_entry *cache_entry_create (cache_data_type_t t)
struct cache_entry *cache_entry_create (void)
{
struct cache_entry *hp;

if (t != CACHE_DATA_TYPE_NONE
&& t != CACHE_DATA_TYPE_JSON
&& t != CACHE_DATA_TYPE_RAW) {
errno = EINVAL;
return NULL;
}

if (!(hp = calloc (1, sizeof (*hp)))) {
errno = ENOMEM;
return NULL;
}
hp->type = t;
hp->type = CACHE_DATA_TYPE_NONE;
return hp;
}

Expand All @@ -92,10 +92,11 @@ struct cache_entry *cache_entry_create_json (json_t *o)
return NULL;
}

if (!(hp = cache_entry_create (CACHE_DATA_TYPE_JSON)))
if (!(hp = cache_entry_create ()))
return NULL;
hp->data = o;
hp->data_valid = true;
hp->entry_valid = true;
hp->type = CACHE_DATA_TYPE_JSON;
return hp;
}

Expand All @@ -108,51 +109,32 @@ struct cache_entry *cache_entry_create_raw (void *data, int len)
return NULL;
}

if (!(hp = cache_entry_create (CACHE_DATA_TYPE_RAW)))
if (!(hp = cache_entry_create ()))
return NULL;

if (data) {
hp->data = data;
hp->len = len;
}
/* true even if data == NULL */
hp->data_valid = true;
hp->entry_valid = true;
hp->type = CACHE_DATA_TYPE_RAW;
return hp;
}

int cache_entry_type (struct cache_entry *hp, cache_data_type_t *t)
{
if (hp) {
if (t)
(*t) = hp->type;
return 0;
}
return -1;
}

bool cache_entry_is_type_json (struct cache_entry *hp)
{
return (hp && hp->type == CACHE_DATA_TYPE_JSON);
}

bool cache_entry_is_type_raw (struct cache_entry *hp)
{
return (hp && hp->type == CACHE_DATA_TYPE_RAW);
}

bool cache_entry_get_valid (struct cache_entry *hp)
{
return (hp && hp->data_valid);
return (hp && hp->entry_valid);
}

bool cache_entry_get_dirty (struct cache_entry *hp)
{
return (hp && hp->data_valid && hp->dirty);
return (hp && hp->entry_valid && hp->dirty);
}

int cache_entry_set_dirty (struct cache_entry *hp, bool val)
{
if (hp && hp->data_valid) {
if (hp && hp->entry_valid) {
if ((val && hp->dirty) || (!val && !hp->dirty))
; /* no-op */
else if (val && !hp->dirty)
Expand All @@ -174,7 +156,7 @@ int cache_entry_set_dirty (struct cache_entry *hp, bool val)

int cache_entry_clear_dirty (struct cache_entry *hp)
{
if (hp && hp->data_valid) {
if (hp && hp->entry_valid) {
if (hp->dirty
&& (!hp->waitlist_notdirty
|| !wait_queue_length (hp->waitlist_notdirty)))
Expand All @@ -186,7 +168,7 @@ int cache_entry_clear_dirty (struct cache_entry *hp)

int cache_entry_force_clear_dirty (struct cache_entry *hp)
{
if (hp && hp->data_valid) {
if (hp && hp->entry_valid) {
if (hp->dirty) {
if (hp->waitlist_notdirty) {
wait_queue_destroy (hp->waitlist_notdirty);
Expand All @@ -201,7 +183,7 @@ int cache_entry_force_clear_dirty (struct cache_entry *hp)

json_t *cache_entry_get_json (struct cache_entry *hp)
{
if (!hp || !hp->data_valid || hp->type != CACHE_DATA_TYPE_JSON)
if (!hp || !hp->entry_valid || hp->type != CACHE_DATA_TYPE_JSON)
return NULL;
/* should be non-NULL for json */
assert (hp->data);
Expand All @@ -214,18 +196,18 @@ int cache_entry_set_json (struct cache_entry *hp, json_t *o)
&& o
&& (hp->type == CACHE_DATA_TYPE_NONE
|| hp->type == CACHE_DATA_TYPE_JSON)) {
if (hp->data_valid) {
if (hp->entry_valid) {
assert (hp->data);
json_decref (o); /* no-op, 'o' is assumed identical to hp->data */
} else {
assert (!hp->data);
hp->data = o;
hp->data_valid = true;
hp->entry_valid = true;
if (hp->waitlist_valid) {
if (wait_runqueue (hp->waitlist_valid) < 0) {
/* set back to orig */
hp->data = NULL;
hp->data_valid = false;
hp->entry_valid = false;
return -1;
}
}
Expand All @@ -238,7 +220,7 @@ int cache_entry_set_json (struct cache_entry *hp, json_t *o)

int cache_entry_get_raw (struct cache_entry *hp, void **data, int *len)
{
if (!hp || !hp->data_valid || hp->type != CACHE_DATA_TYPE_RAW)
if (!hp || !hp->entry_valid || hp->type != CACHE_DATA_TYPE_RAW)
return -1;
if (data)
(*data) = hp->data;
Expand All @@ -257,7 +239,7 @@ int cache_entry_set_raw (struct cache_entry *hp, void *data, int len)
if (hp
&& (hp->type == CACHE_DATA_TYPE_NONE
|| hp->type == CACHE_DATA_TYPE_RAW)) {
if (hp->data_valid) {
if (hp->entry_valid) {
if ((data && hp->data) || (!data && !hp->data))
free (data); /* no-op, 'data' is assumed identical to hp->data */
else {
Expand All @@ -270,14 +252,14 @@ int cache_entry_set_raw (struct cache_entry *hp, void *data, int len)
else {
hp->data = data;
hp->len = len;
hp->data_valid = true;
hp->entry_valid = true;

if (hp->waitlist_valid) {
if (wait_runqueue (hp->waitlist_valid) < 0) {
/* set back to orig */
hp->data = NULL;
hp->len = 0;
hp->data_valid = false;
hp->entry_valid = false;
return -1;
}
}
Expand All @@ -299,7 +281,7 @@ int cache_entry_clear_data (struct cache_entry *hp)
}
hp->data = NULL;
hp->len = 0;
hp->data_valid = false;
hp->entry_valid = false;
return 0;
}
return -1;
Expand All @@ -309,7 +291,7 @@ void cache_entry_destroy (void *arg)
{
struct cache_entry *hp = arg;
if (hp) {
if (hp->data_valid) {
if (hp->entry_valid) {
if (hp->type == CACHE_DATA_TYPE_JSON)
json_decref (hp->data);
else if (hp->type == CACHE_DATA_TYPE_RAW)
Expand Down
61 changes: 13 additions & 48 deletions src/modules/kvs/cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,57 +6,32 @@
#include "src/common/libutil/tstat.h"
#include "waitqueue.h"

typedef enum {
CACHE_DATA_TYPE_NONE,
CACHE_DATA_TYPE_JSON,
CACHE_DATA_TYPE_RAW,
} cache_data_type_t;

struct cache_entry;
struct cache;


/* Create/destroy cache entry.
*
* cache_entry_create() creates an entry, setting the cache entry type
* to specified type. CACHE_DATA_TYPE_NONE indicates user is not yet
* sure of the type of data to be stored, and it will be determined
* later when cache_entry_set_X() function is called.
* cache_entry_get_valid() will return false after
* cache_entry_create() is initially called, regardless of the type
* passed in.
* cache_entry_create() creates an empty cache entry.
*
* cache_entry_create_json() creates an entry, setting the cache entry
* type to CACHE_DATA_TYPE_JSON. The create transfers ownership of
* cache_entry_create_json() creates an entry storing the data
* associated with a json object. The create transfers ownership of
* 'o' to the cache entry. On destroy, json_decref() will be called
* on 'o'. 'o' cannot be NULL.
*
* cache_entry_create_raw() creates an entry, setting the cache entry
* type to CACHE_DATA_TYPE_RAW. The create transfers ownership of
* 'data' to the cache entry. On destroy, free() will be called on
* 'data'. If 'data' is NULL, 'len' must be zero. If 'data' is
* non-NULL, 'len' must be > 0.
* cache_entry_create_raw() creates an entry storing the data passed
* in. The create transfers ownership of 'data' to the cache entry.
* On destroy, free() will be called on 'data'. If 'data' is NULL,
* 'len' must be zero. If 'data' is non-NULL, 'len' must be > 0.
*
* cache_entry_get_valid() will return true on entries when
* cache_entry_create_json() and cache_entry_get_raw() return success.
*/
struct cache_entry *cache_entry_create (cache_data_type_t t);
struct cache_entry *cache_entry_create (void);
struct cache_entry *cache_entry_create_json (json_t *o);
struct cache_entry *cache_entry_create_raw (void *data, int len);
void cache_entry_destroy (void *arg);

/* Return what data type is stored in the cache entry or will be
* stored in the cache entry. CACHE_DATA_TYPE_NONE means it has not
* yet been determined. Returns 0 on success, -1 on error.
*
* For convenience, cache_entry_is_type_json() checks specifically if
* an entry is of type json. cache_entry_is_type_raw() checks
* specifically if an entry is of type raw.
*/
int cache_entry_type (struct cache_entry *hp, cache_data_type_t *t);
bool cache_entry_is_type_json (struct cache_entry *hp);
bool cache_entry_is_type_raw (struct cache_entry *hp);

/* Return true if cache entry contains valid json or raw data.
* False would indicate that a load RPC is in progress.
*/
Expand Down Expand Up @@ -88,22 +63,12 @@ int cache_entry_force_clear_dirty (struct cache_entry *hp);

/* Accessors for cache entry data.
*
* json get accessor must have type of CACHE_DATA_TYPE_JSON to
* retrieve json object.
*
* raw get accessor must have type of CACHE_DATA_TYPE_RAW to
* retrieve raw data.
*
* json set accessor must have type of CACHE_DATA_TYPE_NONE or
* CACHE_DATA_TYPE_JSON to set json object. After setting, the type
* is converted to CACHE_DATA_TYPE_JSON. 'o' must be non-NULL. Set
* transfers ownership of 'o' to the cache entry.
* json set accessor transfers ownership of 'o' to the cache entry.
* 'o' must be non-NULL.
*
* raw set accessor must have type of CACHE_DATA_TYPE_NONE or
* CACHE_DATA_TYPE_RAW to set raw data. After setting, the type is
* converted to CACHE_DATA_TYPE_RAW. If 'data' is NULL, 'len' must be
* zero. If 'data' is non-NULL, 'len' must be > 0. If non-NULL, set
* transfers ownership of 'data' to the cache entry.
* raw set accessor transfers ownership of 'data' to the cache entry
* if it is non-NULL. If 'data' is non-NULL, 'len' must be > 0. If
* 'data' is NULL, 'len' must be zero.
*
* cache_entry_clear_data () will clear any data in the entry.
*
Expand Down
Loading