by Tejun Heo

[permalink] [raw]

Subject: Re: [Bcache v13 02/16] Bio pool freeing

2012-05-15 16:25:32

by Tejun Heo

[permalink] [raw]

Subject: Re: [Bcache v13 02/16] Bio pool freeing

2012-05-15 16:37:40

by Tejun Heo

[permalink] [raw]

Subject: Re: [Bcache v13 03/16] Revert "rw_semaphore: remove up/down_read_non_owner"

On Wed, May 09, 2012 at 11:08:49PM -0400, Kent Overstreet wrote:
> This reverts commit 11b80f459adaf91a712f95e7734a17655a36bf30.
>
> Signed-off-by: Kent Overstreet <[email protected]>

Please cc Christoph and Al and justify why it needs to be reverted.

Thanks.

--
tejun

2012-05-15 16:38:38

by Tejun Heo

[permalink] [raw]

2012-05-22 22:13:10

by Tejun Heo

[permalink] [raw]

Subject: Re: [Bcache v13 11/16] bcache: Core btree code

Hello, Kent.

On Wed, May 09, 2012 at 11:10:48PM -0400, Kent Overstreet wrote:
> +#define BKEY_PADDED(key) \
> + union { struct bkey key; uint64_t key ## _pad[8]; }

What does the '8' mean? And why does struct bbio use 3 instead? Does
this have to be a macro? Can't we have struct bkey_padded (or
whatever)?

> +struct io {
> + /* Used to track sequential IO so it can be skipped */
> + struct hlist_node hash;
> + struct list_head lru;
> +
> + unsigned long jiffies;
> + unsigned sequential;
> + sector_t last;
> +};

I don't think bcache can have struct io. :P

> +struct dirty_io {
> + struct closure cl;
> + struct cached_dev *d;
> + struct bio bio;
> +};
> +
> +struct dirty {
> + struct rb_node node;
> + BKEY_PADDED(key);
> + struct dirty_io *io;
> +};
...
> +struct cache {

Nor these and so on.

> +/* Bkey fields: all units are in sectors */
> +
> +#define KEY_FIELD(name, field, offset, size) \
> + BITMASK(name, struct bkey, field, offset, size)
> +
> +#define PTR_FIELD(name, offset, size) \
> + static inline uint64_t name(const struct bkey *k, unsigned i) \
> + { return (k->ptr[i] >> offset) & ~(((uint64_t) ~0) << size); } \
> + \
> + static inline void SET_##name(struct bkey *k, unsigned i, uint64_t v)\
> + { \
> + k->ptr[i] &= ~(~((uint64_t) ~0 << size) << offset); \
> + k->ptr[i] |= v << offset; \
> + }
> +
> +KEY_FIELD(KEY_PTRS, header, 60, 3)
> +KEY_FIELD(HEADER_SIZE, header, 58, 2)
> +KEY_FIELD(KEY_CSUM, header, 56, 2)
> +KEY_FIELD(KEY_PINNED, header, 55, 1)
> +KEY_FIELD(KEY_DIRTY, header, 36, 1)
> +
> +KEY_FIELD(KEY_SIZE, header, 20, 16)
> +KEY_FIELD(KEY_DEV, header, 0, 20)
> +
> +KEY_FIELD(KEY_SECTOR, key, 16, 47)
> +KEY_FIELD(KEY_SNAPSHOT, key, 0, 16)
> +
> +PTR_FIELD(PTR_DEV, 51, 12)
> +PTR_FIELD(PTR_OFFSET, 8, 43)
> +PTR_FIELD(PTR_GEN, 0, 8)

So, apart from the the macros, key is 64bit containing the backend
device and extent offset and size with the ptr fields somehow point to
cache. Am I understanding it correctly? If right, I'm *tiny* bit
apprehensive about using only 43bits for offset. While the block 9
bits means 52bit addressing, the 9 bit block size is now there mostly
to work as buffer between memory bitness growth and storage device
size growth so that we have 9 bit buffer as storage device reaches
ulong addressing limit. Probably those days are far out enough.

> +void btree_read_done(struct closure *cl)
> +{
> + struct btree *b = container_of(cl, struct btree, io.cl);
> + struct bset *i = b->sets[0].data;
> + struct btree_iter *iter = b->c->fill_iter;
> + const char *err = "bad btree header";
> + BUG_ON(b->nsets || b->written);
> +
> + bbio_free(b->bio, b->c);
> + b->bio = NULL;
> +
> + mutex_lock(&b->c->fill_lock);
> + iter->used = 0;
> +
> + if (btree_node_io_error(b) ||
> + !i->seq)
> + goto err;
> +
> + for (;
> + b->written < btree_blocks(b) && i->seq == b->sets[0].data->seq;
> + i = write_block(b)) {
> + err = "unsupported bset version";
> + if (i->version > BCACHE_BSET_VERSION)
> + goto err;
> +
> + err = "bad btree header";
> + if (b->written + set_blocks(i, b->c) > btree_blocks(b))
> + goto err;
> +
> + err = "bad magic";
> + if (i->magic != bset_magic(b->c))
> + goto err;
> +
> + err = "bad checksum";
> + switch (i->version) {
> + case 0:
> + if (i->csum != csum_set(i))
> + goto err;
> + break;
> + case BCACHE_BSET_VERSION:
> + if (i->csum != btree_csum_set(b, i))
> + goto err;
> + break;
> + }
> +
> + err = "empty set";
> + if (i != b->sets[0].data && !i->keys)
> + goto err;
> +
> + btree_iter_push(iter, i->start, end(i));
> +
> + b->written += set_blocks(i, b->c);
> + }
> +
> + err = "corrupted btree";
> + for (i = write_block(b);
> + index(i, b) < btree_blocks(b);
> + i = ((void *) i) + block_bytes(b->c))
> + if (i->seq == b->sets[0].data->seq)
> + goto err;
> +
> + btree_sort_and_fix_extents(b, iter);
> +
> + i = b->sets[0].data;
> + err = "short btree key";
> + if (b->sets[0].size &&
> + bkey_cmp(&b->key, &b->sets[0].end) < 0)
> + goto err;
> +
> + if (b->written < btree_blocks(b))
> + bset_init_next(b);
> +
> + if (0) {
> +err: set_btree_node_io_error(b);
> + cache_set_error(b->c, "%s at bucket %lu, block %zu, %u keys",
> + err, PTR_BUCKET_NR(b->c, &b->key, 0),
> + index(i, b), i->keys);
> + }

Please don't do that. Just define out: label, move error specific
path to the end of the function and jump to out at the end of that.

> +
> + mutex_unlock(&b->c->fill_lock);
> +
> + spin_lock(&b->c->btree_read_time_lock);
> + time_stats_update(&b->c->btree_read_time, b->io_start_time);
> + spin_unlock(&b->c->btree_read_time_lock);
> +
> + smp_wmb(); /* read_done is our write lock */
> + set_btree_node_read_done(b);
> +
> + closure_return(cl);
> +}
> +
> +static void btree_read_resubmit(struct closure *cl)
> +{
> + struct btree *b = container_of(cl, struct btree, io.cl);
> +
> + submit_bbio_split(b->bio, b->c, &b->key, 0);
> + continue_at(&b->io.cl, btree_read_done, system_wq);
> +}

I suppose submit_bbio_split() can't fail here somehow unlike from
btree_read() path? If so, please add a comment to explain and maybe
WARN_ON_ONCE() on failure. Subtlety to comment ratio is *way* off.

> +static struct btree *mca_bucket_alloc(struct cache_set *c,
> + struct bkey *k, gfp_t gfp)
> +{
> + struct btree *b = kzalloc(sizeof(struct btree), gfp);
> + if (!b)
> + return NULL;
> +
> + init_rwsem(&b->lock);
> + INIT_LIST_HEAD(&b->list);
> + INIT_DELAYED_WORK(&b->work, btree_write_work);
> + b->c = c;
> + closure_init_unlocked(&b->io);
> +
> + mca_data_alloc(b, k, gfp);
> + return b->sets[0].data ? b : NULL;
> +}

mca_data_alloc() failure path seems like a resource leak but it isn't
because mca_data_alloc() puts it in free list. Is the extra level of
caching necessary? How is it different from sl?b allocator cache?
And either way, comments please.

> +static int mca_reap(struct btree *b, struct closure *cl)
> +{
> + lockdep_assert_held(&b->c->bucket_lock);
> +
> + if (!down_write_trylock(&b->lock))
> + return -1;
> +
> + BUG_ON(btree_node_dirty(b) && !b->sets[0].data);
> +
> + if (cl && btree_node_dirty(b))
> + btree_write(b, true, NULL);
> +
> + if (cl)
> + closure_wait_event_async(&b->io.wait, cl,
> + atomic_read(&b->io.cl.remaining) == -1);
> +
> + if (btree_node_dirty(b) ||
> + atomic_read(&b->io.cl.remaining) != -1 ||
> + work_pending(&b->work.work)) {
> + rw_unlock(true, b);
> + return -EAGAIN;
> + }
> +
> + return 0;
> +}

Mixing -1 and -EAGAIN returns usually isn't a good idea.

> +static struct btree *alloc_bucket(struct cache_set *c, struct bkey *k,
> + int level, struct closure *cl)
> +{
> + struct btree *b, *i;
> + unsigned page_order = ilog2(KEY_SIZE(k) / PAGE_SECTORS ?: 1);
> +
> + lockdep_assert_held(&c->bucket_lock);
> +retry:
> + if (find_bucket(c, k))
> + return NULL;
> +
> + /* btree_free() doesn't free memory; it sticks the node on the end of
> + * the list. Check if there's any freed nodes there:
> + */
> + list_for_each_entry(b, &c->btree_cache_freeable, list)
> + if (page_order <= b->page_order &&
> + !b->key.ptr[0] &&
> + !mca_reap(b, NULL))
> + goto out;
> +
> + /* We never free struct btree itself, just the memory that holds the on
> + * disk node. Check the freed list before allocating a new one:
> + */
> + list_for_each_entry(b, &c->btree_cache_freed, list)
> + if (!mca_reap(b, NULL)) {
> + mca_data_alloc(b, k, __GFP_NOWARN|GFP_NOIO);
> + if (!b->sets[0].data) {
> + rw_unlock(true, b);
> + goto err;
> + } else
> + goto out;
> + }
> +
> + b = mca_bucket_alloc(c, k, __GFP_NOWARN|GFP_NOIO);
> + if (!b)
> + goto err;
> +
> + BUG_ON(!down_write_trylock(&b->lock));
> +out:
> + BUG_ON(!closure_is_unlocked(&b->io.cl));
> +
> + bkey_copy(&b->key, k);
> + list_move(&b->list, &c->btree_cache);
> + hlist_del_init_rcu(&b->hash);
> + hlist_add_head_rcu(&b->hash, hash_bucket(c, k));
> + lock_set_subclass(&b->lock.dep_map, level + 1, _THIS_IP_);
> +
> + b->flags = 0;
> + b->level = level;
> + b->written = 0;
> + b->nsets = 0;
> + for (int i = 0; i < MAX_BSETS; i++)
> + b->sets[i].size = 0;
> + for (int i = 1; i < MAX_BSETS; i++)
> + b->sets[i].data = NULL;

Why separate loops?

> +
> + return b;
> +err:
> + if (current->bio_list)
> + return ERR_PTR(-EAGAIN);

What does this test do?

Thanks.

--
tejun

2012-05-22 22:39:39

by Tejun Heo

[permalink] [raw]

Subject: Re: [Bcache v13 12/16] bcache: Bset code (lookups within a btree node)

Hello, again.

On Wed, May 09, 2012 at 11:11:00PM -0400, Kent Overstreet wrote:
> +void bkey_copy_single_ptr(struct bkey *dest, const struct bkey *src, unsigned i)
> +{
> + BUG_ON(i > KEY_PTRS(src));
> +
> + /* Only copy the header, key, and one pointer. */
> + memcpy(dest, src, 2 * sizeof(uint64_t));

I hope this could be better somehow.

> + dest->ptr[0] = src->ptr[i];
> + SET_KEY_PTRS(dest, 1);
> + /* We didn't copy the checksum so clear that bit. */
> + SET_KEY_CSUM(dest, 0);
> +}
> +/* I have no idea why this code works... and I'm the one who wrote it
> + *
> + * However, I do know what it does:
> + * Given a binary tree constructed in an array (i.e. how you normally implement
> + * a heap), it converts a node in the tree - referenced by array index - to the
> + * index it would have if you did an inorder traversal.
> + *
> + * The binary tree starts at array index 1, not 0
> + * extra is a function of size:
> + * extra = (size - rounddown_pow_of_two(size - 1)) << 1;
> + */

It's kinda problematic if you can't understand or explain it. I'm
scared. :(

> +static unsigned __to_inorder(unsigned j, unsigned size, unsigned extra)
> +{
> + unsigned b = fls(j);
> + unsigned shift = fls(size - 1) - b;
> +
> + j ^= 1U << (b - 1);
> + j <<= 1;
> + j |= 1;
> + j <<= shift;
> +
> + if (j > extra)
> + j -= (j - extra) >> 1;
> +
> + return j;
> +}
...
> +static void bset_alloc_tree(struct btree *b, struct bset_tree *t)
> +{
> + if (t != b->sets) {
> + unsigned j = roundup(t[-1].size,
> + 64 / sizeof(struct bkey_float));
> +
> + t->tree = t[-1].tree + j;
> + t->prev = t[-1].prev + j;
> + }
> +
> + while (t < b->sets + MAX_BSETS)
> + t++->size = 0;
> +}

I don't know. This is cryptic and I feel dirty and stupid.

> +static void bset_build_unwritten_tree(struct btree *b)
> +{
> + struct bset_tree *t = b->sets + b->nsets;

Why not &b->sets[b->nsets]?

> +
> + bset_alloc_tree(b, t);
> +
> + if (t->tree != b->sets->tree + bset_tree_space(b)) {

And why are we switching between sets[N].XXX and sets->XXX? Do they
signify something? The usages don't seem to be consistent tho.

> + t->prev[0] = bkey_to_cacheline_offset(t->data->start);
> + t->size = 1;
> + }
> +}
...
> +void bset_init_next(struct btree *b)
> +{
> + struct bset *i = write_block(b);
> +
> + if (i != b->sets[0].data) {
> + b->sets[++b->nsets].data = i;
> + i->seq = b->sets[0].data->seq;
> + } else
> + get_random_bytes(&i->seq, sizeof(uint64_t));
> +
> + i->magic = bset_magic(b->c);
> + i->version = 0;
> + i->keys = 0;
> +
> + bset_build_unwritten_tree(b);
> +}
> +
> +struct bset_search_iter {
> + struct bkey *l, *r;
> +};
> +
> +__attribute__((optimize(3)))

So, this sets different optimization level per function. Is this
really necessary? If so, you need to make compiler independent in
include/linux/compiler*.h.

> +static void __btree_sort(struct btree *b, struct btree_iter *iter,
> + unsigned start, unsigned order, bool fixup)
> +{
> + uint64_t start_time;
> + bool remove_stale = !b->written;
> + struct bset *out = (void *) __get_free_pages(__GFP_NOWARN|GFP_NOIO,
> + order);
> + if (!out) {
> + mutex_lock(&b->c->sort_lock);
> + out = b->c->sort;
> + order = ilog2(bucket_pages(b->c));
> + }

So, this is implementing simplistic mempool, right? If so, why not
just use mempool?

> + start_time = local_clock();
> +
> + btree_mergesort(b, out, iter, fixup, remove_stale);
> + b->nsets = start;
> +
> + if (!fixup && !start && b->written)
> + btree_verify(b, out);
> +
> + if (!start && order == b->page_order) {
> + out->magic = bset_magic(b->c);
> + out->seq = b->sets[0].data->seq;
> + out->version = b->sets[0].data->version;
> + swap(out, b->sets[0].data);
> +
> + if (b->c->sort == b->sets[0].data)
> + b->c->sort = out;
> + } else {
> + b->sets[start].data->keys = out->keys;
> + memcpy(b->sets[start].data->start, out->start,
> + (void *) end(out) - (void *) out->start);
> + }
> +
> + if (out == b->c->sort)

And is this test correct given the above swap(out, b->sets[0].data)?

> + mutex_unlock(&b->c->sort_lock);
> + else
> + free_pages((unsigned long) out, order);
> +
> + if (b->written)
> + bset_build_written_tree(b);
> +
> + if (!start) {
> + spin_lock(&b->c->sort_time_lock);
> + time_stats_update(&b->c->sort_time, start_time);
> + spin_unlock(&b->c->sort_time_lock);
> + }
> +}
...
> +#define BKEY_MID_BITS 3
> +#define BKEY_MID_MAX (~(~0 << (BKEY_MID_BITS - 1)))
> +#define BKEY_MID_MIN (-1 - BKEY_MID_MAX)
> +
> +#define BKEY_EXPONENT_BITS 7
> +#define BKEY_MANTISSA_BITS 22
> +#define BKEY_MANTISSA_MASK ((1 << BKEY_MANTISSA_BITS) - 1)
> +
> +struct bkey_float {
> + unsigned exponent:BKEY_EXPONENT_BITS;
> + unsigned m:BKEY_MID_BITS;
> + unsigned mantissa:BKEY_MANTISSA_BITS;
> +} __packed;

I *suppose* bkey_float is used to somehow compactly represent key
values for compacter search tree. Wouldn't things like this need
boatload of explanation?

> +#define BSET_CACHELINE 128
> +#define BSET_CACHELINE_BITS ilog2(BSET_CACHELINE)

Hmm... what if CACHELINE isn't 128 bytes? What are the effects?
Wouldn't it be better to name it something else (I don't know,
BSET_UNIT_BYTES or whatever) and then explain that it's defined to be
close to popular cacheline size and the effects of it not coinciding
with actual cacheline size? Another nit, it's more customary to
define BITS or SHIFT first and then define size as << SHIFT.

> +#define bset_tree_space(b) (btree_data_space(b) >> BSET_CACHELINE_BITS)
> +
> +#define bset_tree_bytes(b) (bset_tree_space(b) * sizeof(struct bkey_float))
> +#define bset_prev_bytes(b) (bset_tree_bytes(b) >> 2)

Ummm.... cryptic. What does that >> 2 mean? Why not
bset_tree_space(b) * sizeof(whatever prev type is)?

> +void bset_init_next(struct btree *);
> +
> +void bset_fix_invalidated_key(struct btree *, struct bkey *);
> +void bset_fix_lookup_table(struct btree *, struct bkey *);
> +
> +struct bkey *__bset_search(struct btree *, struct bset_tree *,
> + const struct bkey *);
> +#define bset_search(b, t, search) \
> + ((search) ? __bset_search(b, t, search) : (t)->data->start)

Why oh why is this a macro? And can we please have descriptive
variable names in prototypes? btree_sort_into(btree *, btree *) - how
is one supposed to know from which into which?

> +bool bkey_try_merge(struct btree *, struct bkey *, struct bkey *);
> +void btree_sort_lazy(struct btree *);
> +void btree_sort_into(struct btree *, struct btree *);
> +void btree_sort_and_fix_extents(struct btree *, struct btree_iter *);
> +void btree_sort_partial(struct btree *, unsigned);
> +#define btree_sort(b) btree_sort_partial(b, 0)
> +
> +int bset_print_stats(struct cache_set *, char *);

Thanks.

--
tejun

2012-05-22 22:41:07

by Tejun Heo

[permalink] [raw]

Subject: Re: [Bcache v13 11/16] bcache: Core btree code

2012-05-22 22:43:03

by Tejun Heo

[permalink] [raw]

Subject: Re: [Bcache v13 14/16] bcache: Request, io and allocation code

On Wed, May 09, 2012 at 11:11:25PM -0400, Kent Overstreet wrote:
> +int submit_bbio_split(struct bio *bio, struct cache_set *c,
> + struct bkey *k, unsigned ptr)
> +{
> + struct closure *cl = bio->bi_private;
> + struct bbio *b;
> + struct bio *n;
> + unsigned sectors_done = 0;
> +
> + closure_get(cl);
> +
> + bio->bi_sector = PTR_OFFSET(k, ptr);
> + bio->bi_bdev = PTR_CACHE(c, k, ptr)->bdev;
> +
> + do {
> + n = bio_split_get(bio, bio_max_sectors(bio), c);
> + if (!n) {
> + closure_put(cl);
> + return -ENOMEM;
> + }
> +
> + b = container_of(n, struct bbio, bio);
> +
> + bkey_copy_single_ptr(&b->key, k, ptr);
> + SET_KEY_SIZE(&b->key, KEY_SIZE(k) - sectors_done);
> + SET_PTR_OFFSET(&b->key, 0, PTR_OFFSET(k, ptr) + sectors_done);
> +
> + b->submit_time_us = local_clock_us();
> + generic_make_request(n);
> + } while (n != bio);
> +
> + return 0;
> +}

Hmmm... where is @sectors_done updated?

Thanks.

--
tejun

2012-05-22 22:44:09

by Tejun Heo

[permalink] [raw]

On Wed, 2012-05-23 at 02:04 -0400, Kent Overstreet wrote:
> Only trouble is we lose the existing method of specifying the type of
> the integer.

Hi again Kent.

My first suggestion to you was:
---------------------
> or maybe yet another lib/vsprintf pointer extension
> like %pD with some descriptors after the %pD for
> type, width and precision.
---------------------

For instance, internet addresses use pointer extensions
%pI4 and %pI6 for ipv4 and ipv6 addresses.

> New flag character would make much more sense... if gcc
> could be persuaded.

It can't be so persuaded though. Maybe a future gcc.

2012-05-23 15:15:46

by Vivek Goyal

[permalink] [raw]

2012-05-31 02:46:03

by Tejun Heo

[permalink] [raw]

Hello, again, Kent.

On Thu, May 31, 2012 at 03:45:15PM +0900, Tejun Heo wrote:
> I still don't understand what's being made better by closure. At
> least the ones I've seen didn't seem to be justified, so if you have
> some examples which highlight the benefits of closure, please go ahead
> and explain them.
>
> I'll write more about it later but here are some of my current
> thoughts.

I'm gonna try my (personal, of course) reasoning on closure. It's
gonna be long-winded and fairly abstract. The issue at hand is an
abstract one and I don't know how to communicate it better. :(

My objection to closure is largely two-fold.

1. Excessive abstraction / mosaicing of functions.

Abstraction itself isn't a goal onto itself - it is a (pretty
important) tool that we rely on to manage complexities. By the very
nature, it's heavily dependent on what concepts one develops on the
subjct and those concepts may grow substantially removed from the
concrete requirements and functionality, and as they get further
removed from the concrete, different people tend to arrive at
conclusions vastly far apart. One man's paradise can easily be
another's hell. Concepts clear to one mind can be completely
ambiguous or disturbing to others.

It's a problem with no fast solution but IMHO trying to minimize the
distance between the needed functionality (requirements) and the
abstraction. You brought up RB tree earlier in the discussion and
that's a great example. It provides a clear functionality of balanced
search tree without much extra abstracting. Most software developers
can agree on the need of searching an element based on a key value and
RB tree implements that specific functionality. If one understands
the need, the abstraction is fundamental and immediate and thus
self-evident.

One effective way of reaching at a bad abstraction, I think, is overly
generalizing limited local usage patterns. When one sees certain
pattern repeated, it's tempting to overly generalize and make it some
fundamental abstraction when the only things which actually exist are
local usage patterns.

Such over-generalization often ends up bundling things which aren't
fundamentally associated. After all, there aren't clear fundamental
requirements such an abstraction is serving - it is a mosaic of
incidental set of features which happen to be in use for the specific
part of code. Generalized ambiguity can be very generous - combining
A and B resulted in this awesome generic super construct, adding C on
top would woule make it super-duper, right?

This harms both the local usage and the code base in general. Local
code develops unnecessary (generic things tend to have lives of their
own) distance between the actual requirements and the mechanism
implementing them making it more difficult to understand, and the rest
of code base gets something which seems generic but is ambiguos,
difficult to understand and easy to misuse.

I personally think kobject is a good example of this. Its name seems
to suggest it's something like the base type for kernel objects.
Pretty ambiguous and ambitious. If you look at it, it's a combination
of refcnting, elements for sysfs representation, weird looking type
system mostly born out from sysfs usage and device driver model,
userland notification mechanism and some more. It's ass-ugly but
seems to fit device driver model okish. Unfortunately, as soon as it
gets dragged out of device driver model, it isn't clear what different
parts of the dang thing are supposed to do.

For some device-driver-model specific reason, a directory in sysfs
hierarchy maps to a kobject (there are attr dirs which are exceptions
tho). As soon as you start using sysfs outside device driver model,
it becomes weird. An entity which wants to expose something via sysfs
somehow has to use kobject which brings in its object lifetime
management which may not fit the existing usage model.

Let's say there are entities A, B and C, where A and B follow mostly
the same life-cycle rules and C is dependent on B, and A and C have
something to expose via sysfs. So, A and C embed kobjects inside
them. What about kobj life-cycle management then? Let's say A and C
use kobj life-cycle management. Hmmm... C is dependent on B. How
does that play out? What if some in-kernel entity represents two
separate userland-visible entities? What happens then? Do people
have to suppress / circumvent kobj lifecycle mechanism? Wouldn't that
make the code unnecessarily difficult to understand - extra cruft
tends to be very effective at confusing people?

The stupid thing is that sysfs doesn't need any of the kobject
silliness. sysfs should have provided interface fundamental to its
features which device driver model should have used to implement
whatever specific abstraction necessary and keep that inside. The
association between certain type of life cycle management and sysfs
usage wasn't anything fundamental. It was a device driver model local
thing and the mechanism too should have stayed local inside the device
driver model.

At least for me, closure triggers all the alarms for bad generic
abstraction. The patch description you wrote for closure starts with
"asynchronous refcounty thingies" and it's quite apt. It's a
combination of object life-cycle management, async execution, object
hierarchy, event mechanism and even locking. It isn't anything
fundamental. It's a kitchen sink with spoon, knife, chainsaw and
toilet welded together.

2. Making async programming better, or glaze-coating turd.

As I wrote before, I personally think that async programming - using
something other than stack for execution state - is inherently
inferior. The programming languages we program in and the processors
the resulting programs run on depend on stack for execution context
after all. To my mind, async programming is inherently a turd.

That isn't to say we can do away with async programming completely.
At times, turds are unavoidable and even essential (and the line
between turds and !turds isn't clear to begin with). Process context
can be expensive in terms of switching and/or memory overhead. It
would be silly to keep ten thousand threads lying around listening to
ten thousand idle clients which may or may not make another request.
It could also be silly to switch to a process context just to relay
completion notification to another process context (although block
layer kinda does that for a different reason).

In block layer, the need for async programming rises mostly from the
disconnection between issue and completion contexts. The issuing
context may not wait for the completion and we end up with IOs without
matching process contexts. The problem isn't too bad tho. We require
and thus have process context on the issue path and the completion
paths tend to be not too complex.

When we have to deal with async programming, I think the best approach
is to identify why they're necessary, restrict their scopes to minimum
and clearly contain them. For example, bounce to wq as soon as things
start to get complex, don't try to be smart with process contexts
while something complex is going on, and relinquish context judicially
when not doing so results in noticeable performance / resource hit and
hopefully where doing so doesn't complicate things too much (e.g. it's
a clear retry boundary).

If you follow such patterns, there isn't too much need for fancy async
mechanism to blur the pain. Properly identified and consolidated,
sync / async boundaries shouldn't be too painful to manage and the
kernel already provides ample mechanisms to support such patterns.

The problem with closure is that it's a mechanism to glaze-coat turd.
By glaze-coating, it encourages littering turds around instead of
consolidating and isolating them, so in that sense, I prefer async
programming to remain painful and explicit. People tend to do
bat-shit crazy stuff otherwise.

To conclude, I *hope* that bcache followed more usual async
programming conventions, much like I hope it followed more usual
coding style. These aren't cut-and-dry issues and there probably are
aspects that I haven't considered or lost balance on. Please feel
free to point them out.

Thanks.

--
tejun