Re: [PATCH 3/3] kref: create karef and use for sysfs_dirent->s_active

[Eric W. Biederman]

NeilBrown <neilb@xxxxxxx> writes:

> ->s_active is almost a kref, but needs atomic_inc_not_zero which
> is not generally appropriate for a kref, as you can only access a
> kreffed object if you hold a reference, and in that case the counter
> cannot possibly be zero.
>
> So introduce 'karef' - a counter of active references.  An active
> reference is separate from an existential reference and normally
> implies one.
> For a karef, get_not_zero have be appropriate providing a separate
> existential reference is held.
>
> Then change sysfs_dirent->s_active to be the first (and so-far only)
> karef. super_block->s_active is another candidate to be a karef.

If this actually captured the semantics of active references I would find
this interesting.  But you currently miss the inconvenient but crucial
part of preventing new references after deletion, and you miss the
lockdep bits.

Given that with the completion we act enough like a lock we can cause
deadlocks I would expect a generic version for dummies (aka a kref flavor)
to include lockdep annotations from the start.

Lock ordering issues combined with ref counts are rare but they really
suck, are hard to find (without lockdep) and hard to reason about.

The logical semantics are:

read_trylock() sysfs_get_active
read_unlock() sysfs_put_active
write_lock() sysfs_deactivate

Maybe your karef stuff is good for something but it models the sysfs
usage poorly.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/