On May 14 2007 15:10, Bharata B Rao wrote:
>+struct union_info * union_alloc(void)
Ultimate nitpick: try s/\* /*/; (also elsewhere)
>+static inline void union_lock(struct dentry *dentry)
>+{
>+ if (unlikely(dentry && dentry->d_union)) {
>+ struct union_info *ui = dentry->d_union;
>+
>+ UM_DEBUG_LOCK("\"%s\" locking %p (count=%d)\n",
>+ dentry->d_name.name, ui,
>+ atomic_read(&ui->u_count));
>+ __union_lock(dentry->d_union);
>+ }
>+}
>+
>+static inline void union_unlock(struct dentry *dentry)
>+{
>+ if (unlikely(dentry && dentry->d_union)) {
>+ struct union_info *ui = dentry->d_union;
>+
>+ UM_DEBUG_LOCK("\"%s\" unlocking %p (count=%d)\n",
>+ dentry->d_name.name, ui,
>+ atomic_read(&ui->u_count));
>+ __union_unlock(dentry->d_union);
>+ }
>+}
Do we really need the unlikely()? d_union may be a new feature,
but it may very well be possible that someone puts the bigger
part of his/her files under a union. And when d_unions get
stable, people will probably begin making their root filesystem
unioned for livecds, and then unlikely() will rather be a
likely penalty. My stance: just
if (dentry != NULL && dentry->d_union != NULL)
This also goes for union_trylock.
>+static inline int union_trylock(struct dentry *dentry)
>+{
>+ int locked = 1;
>+
>+ if (unlikely(dentry && dentry->d_union)) {
>+ UM_DEBUG_LOCK("\"%s\" try locking %p (count=%d)\n",
>+ dentry->d_name.name, dentry->d_union,
>+ atomic_read(&dentry->d_union->u_count));
>+ BUG_ON(!atomic_read(&dentry->d_union->u_count));
>+ locked = mutex_trylock(&dentry->d_union->u_mutex);
>+ UM_DEBUG_LOCK("\"%s\" trylock %p %s\n", dentry->d_name.name,
>+ dentry->d_union,
>+ locked ? "succeeded" : "failed");
>+ }
>+ return (locked ? 1 : 0);
>+}
return locked ? 1 : 0
or even
return !!locked;
or since we're just passing up from mutex_trylock:
return locked;
?
>+/*
>+ * This is a *I can't get no sleep* helper
More commonly known as "insomnia". :)
Jan
--
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
