Re: [PATCH 1/7] shared subtree

[Miklos Szeredi]

> +static int do_make_shared(struct vfsmount *mnt)
> +{
> +	int err=0;
> +	struct vfspnode *old_pnode = NULL;
> +	/*
> +	 * if the mount is already a slave mount,
> +	 * allocate a new pnode and make it
> +	 * a slave pnode of the original pnode.
> +	 */
> +	if (IS_MNT_SLAVE(mnt)) {
> +		old_pnode = mnt->mnt_pnode;
> +		pnode_del_slave_mnt(mnt);
> +	}
> +	if(!IS_MNT_SHARED(mnt)) {
> +		mnt->mnt_pnode = pnode_alloc();
> +		if(!mnt->mnt_pnode) {
> +			pnode_add_slave_mnt(old_pnode, mnt);
> +			err = -ENOMEM;
> +			goto out;
> +		}
> +		pnode_add_member_mnt(mnt->mnt_pnode, mnt);
> +	}
> +	if(old_pnode)
> +		pnode_add_slave_pnode(old_pnode, mnt->mnt_pnode);
> +	set_mnt_shared(mnt);
> +out:
> +	return err;
> +}

This is an example, where having struct pnode just complicates things.
If there was no struct pnode, this function would be just one line:
setting the shared flag.

> +static kmem_cache_t * pnode_cachep;
> +
> +/* spinlock for pnode related operations */
> + __cacheline_aligned_in_smp DEFINE_SPINLOCK(vfspnode_lock);
> +
> +enum pnode_vfs_type {
> +	PNODE_MEMBER_VFS = 0x01,
> +	PNODE_SLAVE_VFS = 0x02
> +};
> +
> +void __init pnode_init(unsigned long mempages)
> +{
> +	pnode_cachep = kmem_cache_create("pnode_cache",
> +                       sizeof(struct vfspnode), 0,
> +                       SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL, NULL);
> +}
> +
> +struct vfspnode * pnode_alloc(void)
> +{
> +	struct vfspnode *pnode =  kmem_cache_alloc(pnode_cachep, GFP_KERNEL);
> +	INIT_LIST_HEAD(&pnode->pnode_vfs);
> +	INIT_LIST_HEAD(&pnode->pnode_slavevfs);
> +	INIT_LIST_HEAD(&pnode->pnode_slavepnode);
> +	INIT_LIST_HEAD(&pnode->pnode_peer_slave);
> +	pnode->pnode_master = NULL;
> +	pnode->pnode_flags = 0;
> +	atomic_set(&pnode->pnode_count,0);
> +	return pnode;
> +}
> +
> +void inline pnode_free(struct vfspnode *pnode)
> +{
> +	kmem_cache_free(pnode_cachep, pnode);
> +}
> +
> +/*
> + * __put_pnode() should be called with vfspnode_lock held
> + */
> +void __put_pnode(struct vfspnode *pnode)
> +{
> +	struct vfspnode *tmp_pnode;
> +	do {
> +		tmp_pnode = pnode->pnode_master;
> +		list_del_init(&pnode->pnode_peer_slave);
> +		BUG_ON(!list_empty(&pnode->pnode_vfs));
> +		BUG_ON(!list_empty(&pnode->pnode_slavevfs));
> +		BUG_ON(!list_empty(&pnode->pnode_slavepnode));
> +		pnode_free(pnode);
> +		pnode = tmp_pnode;
> +		if (!pnode || !atomic_dec_and_test(&pnode->pnode_count))
> +			break;
> +	} while(pnode);
> +}
> +

All these are really unnecessary IMO.

> +/*
> + * merge 'pnode' into 'peer_pnode' and get rid of pnode
> + * @pnode: pnode the contents of which have to be merged
> + * @peer_pnode: pnode into which the contents are merged
> + */
> +int pnode_merge_pnode(struct vfspnode *pnode, struct vfspnode *peer_pnode)
> +{
> +	struct vfspnode *slave_pnode, *pnext;
> +	struct vfsmount *mnt, *slave_mnt, *next;
> +
> +	list_for_each_entry_safe(slave_pnode,  pnext,
> +			&pnode->pnode_slavepnode, pnode_peer_slave) {
> +		slave_pnode->pnode_master = peer_pnode;
> +		list_move(&slave_pnode->pnode_peer_slave,
> +				&peer_pnode->pnode_slavepnode);
> +		put_pnode_locked(pnode);
> +		get_pnode(peer_pnode);
> +	}
> +
> +	list_for_each_entry_safe(slave_mnt,  next,
> +			&pnode->pnode_slavevfs, mnt_pnode_mntlist) {
> +		slave_mnt->mnt_pnode = peer_pnode;
> +		list_move(&slave_mnt->mnt_pnode_mntlist,
> +				&peer_pnode->pnode_slavevfs);
> +		put_pnode_locked(pnode);
> +		get_pnode(peer_pnode);
> +	}
> +
> +	list_for_each_entry_safe(mnt, next,
> +			&pnode->pnode_vfs, mnt_pnode_mntlist) {
> +		mnt->mnt_pnode = peer_pnode;
> +		list_move(&mnt->mnt_pnode_mntlist,
> +				&peer_pnode->pnode_vfs);
> +		put_pnode_locked(pnode);
> +		get_pnode(peer_pnode);
> +	}
> +	return 0;
> +}

Much overcomplication.  It would just be a list_splice(), if there was
no struct pnode.


> +static void empty_pnode(struct vfspnode *pnode) { struct vfsmount *slave_mnt,
> +	*next; struct vfspnode *master_pnode, *slave_pnode, *pnext;
> +
> +	if ((master_pnode = pnode->pnode_master)) {
> +		pnode->pnode_master = NULL;
> +		list_del_init(&pnode->pnode_peer_slave);
> +		pnode_merge_pnode(pnode, master_pnode);
> +		put_pnode_locked(master_pnode);
> +	} else {
> +		list_for_each_entry_safe(slave_mnt, next,
> +			&pnode->pnode_slavevfs, mnt_pnode_mntlist) {
> +			list_del_init(&slave_mnt->mnt_pnode_mntlist);
> +			set_mnt_private(slave_mnt);
> +			put_pnode_locked(pnode);
> +		}
> +		list_for_each_entry_safe(slave_pnode,  pnext,
> +			&pnode->pnode_slavepnode, pnode_peer_slave) {
> +			slave_pnode->pnode_master = NULL;
> +			list_del_init(&slave_pnode->pnode_peer_slave);
> +			put_pnode_locked(pnode);
> +		}
> +	}
> +}
> +

Unnecessary.

> +static int pnode_next(struct pcontext *context)
> +{
> +	struct vfspnode *pnode = context->pnode;
> +	struct vfspnode	*master_pnode=context->master_pnode;
> +	struct list_head *next;
> +
> +	if (!pnode) {
> +		BUG_ON(!context->start);
> +		get_pnode(context->start);
> +		context->pnode = context->start;
> +		context->master_pnode = NULL;
> +		context->level = 0;
> +		return 1;
> +	}
> +
> +	spin_lock(&vfspnode_lock);
> +	next = pnode->pnode_slavepnode.next;
> +	if (next == &pnode->pnode_slavepnode) {
> +		while (1) {
> +			int flag;
> +
> +			if (pnode == context->start) {
> +				put_pnode_locked(pnode);
> +				spin_unlock(&vfspnode_lock);
> +				BUG_ON(context->level != 0);
> +				return 0;
> +			}
> +
> +			next = pnode->pnode_peer_slave.next;
> +			flag = (next != &pnode->pnode_master->pnode_slavepnode);
> +			put_pnode_locked(pnode);
> +
> +			if (flag)
> +				break;
> +
> +			pnode = master_pnode;
> +			master_pnode = pnode->pnode_master;
> +			context->level--;
> +		}
> +	} else {
> +		master_pnode = pnode;
> +		context->level++;
> +	}
> +
> +	pnode = list_entry(next, struct vfspnode, pnode_peer_slave);
> +	get_pnode(pnode);
> +
> +	context->pnode = pnode;
> +	context->master_pnode = master_pnode;
> +	spin_unlock(&vfspnode_lock);
> +	return 1;
> +}
> +
> +/*
> + * skip the rest of the tree, cleaning up
> + * reference to pnodes held in pnode_next().
> + */
> +static void pnode_end(struct pcontext *context)
> +{
> +	struct vfspnode *p = context->pnode;
> +	struct vfspnode *start = context->start;
> +
> +	do {
> +		put_pnode(p);
> +	} while (p != start && (p = p->pnode_master));
> +	return;
> +}
> +
> +/*
> + * traverse the pnode tree and at each pnode encountered, execute the
> + * pnode_fnc(). For each vfsmount encountered call the vfs_fnc().
> + *
> + * @pnode: pnode tree to be traversed
> + * @in_data: input data
> + * @out_data: output data
> + * @pnode_func: function to be called when a new pnode is encountered.
> + * @vfs_func: function to be called on each slave and member vfs belonging
> + * 		to the pnode.
> + */
> +static int pnode_traverse(struct vfspnode *pnode,
> +		void *in_data,
> +		void **out_data,
> +		int (*pnode_pre_func)(struct vfspnode *,
> +			void *, void **, va_list),
> +		int (*pnode_post_func)(struct vfspnode *,
> +			void *, va_list),
> +		int (*vfs_func)(struct vfsmount *,
> +			enum pnode_vfs_type, void *,  va_list),
> +		...)
> +{
> +	va_list args;
> +	int ret = 0, level;
> +	void *my_data, *data_from_master;
> +     	struct vfspnode *master_pnode;
> +     	struct vfsmount *slave_mnt, *member_mnt, *t_m;
> +	struct pcontext context;
> +	static void *p_array[PNODE_MAX_SLAVE_LEVEL];
> +
> +	context.start = pnode;
> +	context.pnode = NULL;
> +	/*
> +	 * determine whether to process vfs first or the
> +	 * slave pnode first
> +	 */
> +	while (pnode_next(&context)) {
> +		level = context.level;
> +
> +		if (level >= PNODE_MAX_SLAVE_LEVEL)
> +			goto error;
> +
> +		pnode = context.pnode;
> +		master_pnode = context.master_pnode;
> +
> +		if (master_pnode) {
> +			data_from_master = p_array[level-1];
> +			my_data = NULL;
> +		} else {
> +			data_from_master = NULL;
> +			my_data = in_data;
> +		}
> +
> +		if (pnode_pre_func) {
> +			va_start(args, vfs_func);
> +			if((ret = pnode_pre_func(pnode,
> +				data_from_master, &my_data, args)))
> +				goto error;
> +			va_end(args);
> +		}
> +
> +		// traverse member vfsmounts
> +		spin_lock(&vfspnode_lock);
> +		list_for_each_entry_safe(member_mnt,
> +			t_m, &pnode->pnode_vfs, mnt_pnode_mntlist) {
> +
> +			spin_unlock(&vfspnode_lock);
> +			va_start(args, vfs_func);
> +			if ((ret = vfs_func(member_mnt,
> +				PNODE_MEMBER_VFS, my_data, args)))
> +				goto error;
> +			va_end(args);
> +			spin_lock(&vfspnode_lock);
> +		}
> +		list_for_each_entry_safe(slave_mnt, t_m,
> +			&pnode->pnode_slavevfs, mnt_pnode_mntlist) {
> +
> +			spin_unlock(&vfspnode_lock);
> +			va_start(args, vfs_func);
> +			if ((ret = vfs_func(slave_mnt, PNODE_SLAVE_VFS,
> +				my_data, args)))
> +				goto error;
> +			va_end(args);
> +			spin_lock(&vfspnode_lock);
> +		}
> +		spin_unlock(&vfspnode_lock);
> +
> +		if (pnode_post_func) {
> +			va_start(args, vfs_func);
> +			if((ret = pnode_post_func(pnode,
> +				my_data, args)))
> +				goto error;
> +			va_end(args);
> +		}
> +
> +		p_array[level] = my_data;
> +	}
> +out:
> +	if (out_data)
> +		*out_data = p_array[0];
> +	return ret;
> +error:
> +	va_end(args);
> +	pnode_end(&context);
> +	goto out;
> +}
> 

And this is the worst part.  As I said earlier, void pointers and
variable argument functions have no place in this kind of code.

I think you could get rid of all these if you'd implement a simple
iterator function which returns the traversed vfsmounts.  That's
another big argument for getting rid of struct pnode: you could do
iteration simply by holding onto a vfsmount pointer, instead of having
to do a two level iteration, once over pnodes, then over vfsmounts.

> +extern spinlock_t vfspnode_lock;
> +extern void __put_pnode(struct vfspnode *);
> +
> +static inline struct vfspnode *
> +get_pnode(struct vfspnode *pnode)
> +{
> +	if (!pnode)
> +		return NULL;
> +	atomic_inc(&pnode->pnode_count);
> +	return pnode;
> +}
> +
> +static inline void
> +put_pnode(struct vfspnode *pnode)
> +{
> +	if (!pnode)
> +		return;
> +	if (atomic_dec_and_lock(&pnode->pnode_count, &vfspnode_lock)) {
> +		__put_pnode(pnode);
> +		spin_unlock(&vfspnode_lock);
> +	}
> +}

Unnecessary.

> +#define MNT_PRIVATE	0x10  /* if the vfsmount is private, by default it is private*/

If by default it's private, why is this flag needed?

Miklos
-
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/