shared subtree
Get series with b4
b4 am 20050718065311.210001000@localhostVersions newest first
- v1 Latest8 patches 0 repliesNew
Patches in v1 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/8 | shared subtreefs/ fs/ fs/ +4 more | +385 | 0 replies | |
| 2/8 | shared subtreefs/ include/ include/ | +30 | 0 replies | |
| 3/8 | shared subtreefs/ fs/ include/ +3 more | +906 | 0 replies | |
| 4/8 | shared subtreefs/ | +125 | 0 replies | |
| 5/8 | shared subtreefs/ fs/ include/ +1 more | +177 | 0 replies | |
| 6/8 | shared subtreefs/ | +9 | 0 replies | |
| 7/8 | shared subtreefs/ fs/ include/ | +61 | 0 replies | |
| 8/8 | shared subtreefs/ | +224 | 0 replies |
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.
Cover letter [RFC-2 PATCH 0/8] shared subtree
Enclosed 8 patches that implement shared subtree functionality as detailed in Al Viro's RFC found at http://lwn.net/Articles/119232/
I have incorporated all the comments received earlier in first round. Thanks to Miklos and Pekka for the valuable comments. Also I have optimized lots of code, especially in pnode.c . Code is unit tested. However the code in its current form does not handle ENOMEM error gracefully. I am working on it.
The incremental patches provide the following functionality:
1) shared_private_slave.patch : Provides the ability to mark a subtree as shared or private or slave.
2) unclone.patch : provides the ability to mark a subtree as unclonable. NOTE:
this feature is an addition to Al Viro's RFC, to solve the vfsmount explosion. The problem is detailed here:
http://www.ussg.iu.edu/hypermail/linux/kernel/0502.0/0468.html
3) rbind.patch : this patch adds the ability to propagate binds/rbinds across vfsmounts.
4) move.patch : this patch provides the ability to move a shared/private/slave/unclonable subtree to some other mount-point. It also provides the same feature to pivot_root()
5) umount.patch: this patch provides the ability to propagate unmounts.
6) namespace.patch: this patch provides ability to clone a namespace, with propagation set to vfsmounts in the new namespace.
7) automount.patch: this patch provides the automatic propagation for mounts/unmounts done through automounter.
8) pnode_opt.patch: this patch optimizes the redundant code in pnode.c .
Looking forward for comments,
RP
-----------------------------------------------------------------------
CHANGES DONE IN RESPONSE TO COMMENTS RECEIVED IN 1ST ROUND,
response to Pekka J Enberg Comments:
Quoted 2 lines
Quoted 2 lines
Inlining the patches to email would be greatly appreciated. Here are some comments.
done
Quoted 4 lines
(quoted diff) +2
-0+int +_do_make_mounted(struct nameidata *nd, struct vfsmount **mnt)Quoted 2 lines
Use two underscores to follow naming conventions.
Yes done. In fact this function is renamed as make_mounted because that seemed to make more sense. But in general throughout the patches I have changed all newly introduced function that start with one underscore to two underscores.
Quoted 10 lines
Index:
2.6.12/fs/pnode.c
===================================================================fs/pnode.c new file +3
-0@@ -0,0 +1,362 @@ + +#define PNODE_MEMBER_VFS 0x01 +#define PNODE_SLAVE_VFS 0x02Quoted 2 lines
Enums, please.
done
Quoted 12 lines
fs/pnode.c new file +11
-0+ +static kmem_cache_t * pnode_cachep; + +/* spinlock for pnode related operations */ +__cacheline_aligned_in_smp DEFINE_SPINLOCK (vfspnode_lock ); + + +static void +pnode_init_fn (void *data,kmem_cache_t *cachep,unsigned long flags) +{ +struct vfspnode *pnode = (struct vfspnode *)data;
Quoted 1 line
Quoted 1 line
Redundant cast.
yes. removed.
Quoted 24 lines
fs/pnode.c new file +23
-0+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 ); +} + +void __init +pnode_init (unsigned long mempages) +{ + pnode_cachep =kmem_cache_create ("pnode_cache" , +sizeof (struct vfspnode ),0 , +SLAB_HWCACHE_ALIGN |SLAB_PANIC , pnode_init_fn,NULL ); +} + + +struct vfspnode * +pnode_alloc (void ) +{ +struct vfspnode *pnode = (struct vfspnode *)kmem_cache_alloc ( + pnode_cachep,GFP_KERNEL );
Quoted 1 line
Quoted 1 line
Redundant cast.
yes removed.
Quoted 3 lines
+struct inoutdata {
Quoted 1 line
Wants a better name.
This datastructure is gone after optimizing pnode.c
Quoted 20 lines
fs/pnode.c new file +19
-0+void *my_data;/* produced and consumed by me */ +void *in_data;/* produced by master, consumed by slave */ +void *out_data;/* produced by slave, comsume by master */ +}; + +struct pcontext { +struct vfspnode *start; +int flag; +int traversal; +int level; +struct vfspnode *master_pnode; +struct vfspnode *pnode; +struct vfspnode *slave_pnode; +}; + + +#define PNODE_UP 1 +#define PNODE_DOWN 2 +#define PNODE_MID 3
Quoted 1 line
Quoted 1 line
Enums, please.
These #defines are gone after the optimizations and cleanup.
Quoted 15 lines
fs/pnode.c new file +14
-0+ +/* +* Walk the pnode tree for each pnode encountered. A given pnode in the tree +* can be returned a minimum of 2 times. First time the pnode is encountered, +* it is returned with the flag PNODE_DOWN. Every time the pnode is encountered +* after having traversed through each of its children, it is returned with the +* flag PNODE_MID. And finally when the pnode is encountered after having +* walked all of its children, it is returned with the flag PNODE_UP. +* +* @context: provides context on the state of the last walk in the pnode +* tree. +*/ +static int inline +pnode_next (struct pcontext *context)
Quoted 1 line
Quoted 1 line
Rather large function to be an inline.
yes. done.
Quoted 8 lines
fs/pnode.c new file +6
-0+{ +int traversal = context->traversal; +int ret=0 ; +struct vfspnode *pnode = context->pnode, + *slave_pnode=context->slave_pnode, + *master_pnode=context->master_pnode;Quoted 2 lines
Add a separate declaration for each variable. The above is hard to read.
yes done.
Quoted 27 lines
fs/pnode.c new file +26
-0+struct list_head *next; + +spin_lock (&vfspnode_lock); +/* +* the first traversal will be on the root pnode +* with flag PNODE_DOWN +*/ +if (!pnode) { + context->pnode =get_pnode (context->start); + context->master_pnode =NULL ; + context->traversal =PNODE_DOWN ; + context->slave_pnode =NULL ; + context->level =0 ; + ret =1 ; + goto out; + } + +/* +* if the last traversal was PNODE_UP, than the current +* traversal is PNODE_MID on the master pnode. +*/ +if (traversal ==PNODE_UP ) { +if (!master_pnode) { +/* DONE. return */ +put_pnode (pnode); + ret =0 ;
Quoted 1 line
Quoted 1 line
Using goto out and dropping the else branch would make this more readable.
This code is also gone after optimization and cleanups.
Quoted 13 lines
fs/pnode.c new file +12
-0+ }else { + context->traversal =PNODE_MID ; + context->level--; + context->pnode = master_pnode; +put_pnode (slave_pnode); + context->slave_pnode = pnode; + context->master_pnode = (master_pnode ? + master_pnode->pnode_master :NULL ); + ret =1 ; + } + }else { +if (traversal ==PNODE_MID ) {
Quoted 1 line
Quoted 1 line
Missing space before parenthesis.
Yes ensured that throughout the patches. Hope I have not missed any.
Quoted 6 lines
fs/pnode.c new file +4
-0+ next = slave_pnode->pnode_peer_slave.next; + }else { + next = pnode->pnode_slavepnode.next; + }Quoted 2 lines
Please drop the extra braces.
Yes done.
Quoted 23 lines
fs/pnode.c new file +22
-0+put_pnode (slave_pnode); + context->slave_pnode =NULL ; +/* +* if the last traversal was PNODE_MID or PNODE_DOWN, and the +* master pnode has some slaves to traverse, the current +* traversal will be PNODE_DOWN on the slave pnode. +*/ +if ((next != &pnode->pnode_slavepnode) && + (traversal ==PNODE_DOWN || traversal ==PNODE_MID )) { + context->traversal =PNODE_DOWN ; + context->level++; + context->pnode =get_pnode (list_entry (next, + struct vfspnode, pnode_peer_slave)); + context->master_pnode = pnode; + ret =1 ; + }else { +/* +* since there are no more children, the current traversal +* is PNODE_UP on the same pnode +*/ + context->traversal =PNODE_UP ; + ret =1 ;
Quoted 3 lines
Quoted 3 lines
Would probably make more sense to check if
(next ==&pnode->pnode_slavepnode&& traversal ==PNODE_UP) and use goto out to get rid of the else branch.
Again after code-cleanup and optimization, this code is gone.
Quoted 12 lines
fs/pnode.c new file +10
-0+ } + } +out: +spin_unlock (&vfspnode_lock); +return ret; +} + + +static void +_pnode_disassociate_mnt (struct vfsmount *mnt)
Quoted 1 line
Quoted 1 line
Two underscores, please.
Yes. Did it!
Quoted 7 lines
fs/pnode.c new file +5
-0+struct vfsmount * +pnode_make_mounted (struct vfspnode *pnode,struct vfsmount *mnt,struct dentry *dentry) +{ +struct vfsmount *child_mnt; +int ret=0 ,traversal,level;Quoted 2 lines
Spaces, please.
Yes done.
Quoted 4 lines
fs/pnode.c new file +2
-0+struct vfspnode *slave_pnode, *master_pnode, *child_pnode, *slave_child_pnode; +struct vfsmount *slave_mnt, *member_mnt, *t_m;Quoted 2 lines
Formatting damage.
Yep. Done
Quoted 3 lines
fs/pnode.c new file +1
-0+while (pnode_next (&context)) {Quoted 2 lines
Missing space before parenthesis.
Done
Quoted 9 lines
fs/pnode.c new file +7
-0+ traversal = context.traversal; + level = context.level; + pnode = context.pnode; + slave_pnode = context.slave_pnode; + master_pnode = context.master_pnode; + +if (traversal ==PNODE_DOWN ) {Quoted 2 lines
Use switch statement here.
yep! did it.
Quoted 4 lines
fs/pnode.c new file +2
-0+if (master_pnode) { + child_pnode = (struct vfspnode *)p_array[level-1 ].in_data;Quoted 2 lines
Redundant cast.
done.
Quoted 5 lines
fs/pnode.c new file +3
-0+ }else { + child_pnode =NULL ; + }Quoted 2 lines
Extra braces.
done
Quoted 5 lines
fs/pnode.c new file +2
-0+while (!(child_pnode =pnode_alloc ())) +schedule ();Quoted 3 lines
This looks dangerous. Why this must not fail and in other places you return
-ENOMEM?
I have changed it to return -ENOMEM. But its not perfect yet. The code is not graceful returning. It leaves around some data-structures. I am working on this last aspect currently.
Quoted 3 lines
fs/pnode.c new file +1
-0+ p_array[level].my_data = (void *)child_pnode;Quoted 2 lines
Redundant cast.
Yes. done.
Quoted 3 lines
fs/pnode.c new file +1
-0+ child_pnode = (struct vfspnode *)p_array[level].my_data;Quoted 2 lines
Redundant cast.
Yes. done.
Quoted 5 lines
fs/pnode.c new file +3
-0+#define MS_PRIVATE 262144 +#define MS_SLAVE 524288 +#define MS_SHARED 1048576Quoted 2 lines
The expression (1<<bit) would make more sense here.
Yes. done.
Quoted 16 lines
Index:
2.6.12/include/linux/pnode.h
===================================================================include/linux/pnode.h new file +10
-0@@ -0,0 +1,78 @@ +/* +* linux/fs/pnode.c +* +* (C) Copyright IBM Corporation 2005. +* Released under GPL v2. +* +*/ +#ifndef _LINUX_PNODE_H +#define _LINUX_PNODE_H +#ifdef __KERNEL__
Quoted 2 lines
Quoted 2 lines
No need for the above. Kernel headers are not supposed to be included by userspace.
Removed it.
Quoted 20 lines
include/linux/pnode.h new file +19
-0+ +#include <linux/list.h> +#include <linux/mount.h> +#include <linux/spinlock.h> +#include <asm/atomic.h> + +struct vfspnode { +struct list_head pnode_vfs;/* list of vfsmounts anchored here */ +struct list_head pnode_slavevfs;/* list of slave vfsmounts */ +struct list_head pnode_slavepnode;/* list of slave pnode */ +struct list_head pnode_peer_slave;/* going through master's slave pnode +list*/ +struct vfspnode *pnode_master;/* master pnode */ +int pnode_flags; +atomic_t pnode_count; +}; +#define PNODE_MAX_SLAVE_LEVEL 10 +#define PNODE_DELETE 0x01 +#define PNODE_SLAVE 0x02
Quoted 1 line
Quoted 1 line
Enums, please.
I have left it as is for now. These defines can also go. I will fix them.
Quoted 7 lines
include/linux/pnode.h new file +5
-0+ +#define IS_PNODE_DELETE (pn) ((pn->pnode_flags&PNODE_DELETE)==PNODE_DELETE) +#define IS_PNODE_SLAVE (pn) ((pn->pnode_flags&PNODE_SLAVE)==PNODE_SLAVE) +#define SET_PNODE_DELETE (pn) pn->pnode_flags |= PNODE_DELETE +#define SET_PNODE_SLAVE (pn) pn->pnode_flags |= PNODE_SLAVEQuoted 2 lines
Static inline functions are preferred over #define.
done
Quoted 10 lines
include/linux/pnode.h new file +9
-0+ +extern spinlock_t vfspnode_lock; +extern void __put_pnode (struct vfspnode *); + +static inline struct vfspnode * +get_pnode (struct vfspnode *pnode) +{ +if (!pnode) +return NULL ;
Quoted 3 lines
Quoted 3 lines
Can pnode really be NULL here? Looking at the callers in this patch, it can't. Please remember that you should do NULL checks like this only when it makes sense from API point of view to call the function with NULL.
Yes there are cases where it they can be called with null. If I don't have these checks, than the check has to be done in the caller. So I chose it here.
Quoted 2 lines
+//TOBEDONE WRITE BETTER MACROS. ..
Quoted 1 line
Please use static inline functions instead.
yes done.
Response to Miklos comments follows:
Quoted 9 lines
include/linux/pnode.h new file +1
-1-struct vfsmount *lookup_mnt (struct vfsmount *mnt,struct dentry *dentry) +struct vfsmount *lookup_mnt (struct vfsmount *mnt,struct dentry *dentry,struct dentry *root) {Quoted 6 lines
How about changing it to inline and calling it
__lookup_mnt_root(), and calling it fromlookup_mnt()(which could keep the old signature) andlookup_mnt_root(). That way the compiler can optimize away the root check for the plainlookup_mnt()case, and there's no need to modify callers oflookup_mnt().
Yes. DOne.
Quoted 5 lines
include/linux/pnode.h new file +2
-0+struct vfsmount *do_make_mounted (struct vfsmount *mnt,struct dentry *dentry) +{Quoted 2 lines
What does this function do? Can we have a header comment?
Yes added a header.
Quoted 5 lines
include/linux/pnode.h new file +3
-0+int +_do_make_mounted (struct nameidata *nd,struct vfsmount **mnt) +{Quoted 2 lines
Ditto.
Added a header.
Quoted 17 lines
include/linux/pnode.h new file +15
-0+/* +* recursively change the type of the mountpoint. +*/ +static int do_change_type (struct nameidata *nd,int flag) +{ +struct vfsmount *m, *mnt; +struct vfspnode *old_pnode =NULL ; +int err; + +if (!(flag &MS_SHARED ) && !(flag &MS_PRIVATE ) + && !(flag &MS_SLAVE )) +return -EINVAL ; + +if ((err =_do_make_mounted (nd, &mnt))) +return err;
Quoted 2 lines
Quoted 2 lines
Why does this opertation do any mounting? If it's a type change, it should just change the type of something already mounted, no?
Ok. Now it returns -EINVAL.
Quoted 2 lines
include/linux/pnode.h new file +2
-0+case MS_SHARED : +SET_MNT_PRIVATE (m);
....
Quoted 4 lines
include/linux/pnode.h new file +2
-0+break ; +Quoted 2 lines
Can this be split into three functions?
Yes split them into 3 functions.
Quoted 5 lines
include/linux/pnode.h new file +3
-0+static int inline +pnode_next (struct pcontext *context) +{Quoted 2 lines
Is such a generic traversal function really needed? Why?
As I said in my earlier mail, this function is equivalent of next_mnt for traversing vfsmount tree.
I have optimized this function as well as pnode_traverse(). Hopefully you may find the new code palatable.
Quoted 7 lines
include/linux/pnode.h new file +3
-0+struct vfsmount * +pnode_make_mounted (struct vfspnode *pnode,struct vfsmount *mnt,struct dentry *dentry) +{Quoted 4 lines
Again a header comment would be nice, on what exactly this function does. Also the implementation is really cryptic, but I can't even start to decipher without knowing what it's supposed to do.
Yes. added a header comment.
Quoted 5 lines
include/linux/pnode.h new file +3
-0+static inline struct vfspnode * +get_pnode_n (struct vfspnode *pnode,size_t n) +{Quoted 2 lines
Seems to be unused throughout the patch series
True removed.
Quoted 5 lines
+
static inline struct vfsmount *mntget(struct vfsmount *mnt)Quoted 1 line
Please don't add empty lines.
Yes done.
-----------------------------------------------------------------------