On Tue, Apr 17, 2007 at 05:08:48PM -0500, Serge E. Hallyn wrote:
> Quoting Bharata B Rao (bharata@xxxxxxxxxxxxxxxxxx):
> > From: Jan Blunck <j.blunck@xxxxxxxxxxxxx>
> > Subject: Introduce union stack.
> >
> > Adds union stack infrastructure to the dentry structure and provides
> > locking routines to walk the union stack.
> >
> > Signed-off-by: Jan Blunck <j.blunck@xxxxxxxxxxxxx>
> > Signed-off-by: Bharata B Rao <bharata@xxxxxxxxxxxxxxxxxx>
> > ---
> > fs/Makefile | 2
> > fs/dcache.c | 5
> > fs/union.c | 53 +++++++++
> > include/linux/dcache.h | 11 +
> > include/linux/dcache_union.h | 243 +++++++++++++++++++++++++++++++++++++++++++
> > 5 files changed, 314 insertions(+)
> >
> > --- a/fs/Makefile
> > +++ b/fs/Makefile
> > @@ -49,6 +49,8 @@ obj-$(CONFIG_FS_POSIX_ACL) += posix_acl.
> > obj-$(CONFIG_NFS_COMMON) += nfs_common/
> > obj-$(CONFIG_GENERIC_ACL) += generic_acl.o
> >
> > +obj-$(CONFIG_UNION_MOUNT) += union.o
> > +
> > obj-$(CONFIG_QUOTA) += dquot.o
> > obj-$(CONFIG_QFMT_V1) += quota_v1.o
> > obj-$(CONFIG_QFMT_V2) += quota_v2.o
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -936,6 +936,11 @@ struct dentry *d_alloc(struct dentry * p
> > #ifdef CONFIG_PROFILING
> > dentry->d_cookie = NULL;
> > #endif
> > +#ifdef CONFIG_UNION_MOUNT
> > + dentry->d_overlaid = NULL;
> > + dentry->d_topmost = NULL;
> > + dentry->d_union = NULL;
> > +#endif
> > INIT_HLIST_NODE(&dentry->d_hash);
> > INIT_LIST_HEAD(&dentry->d_lru);
> > INIT_LIST_HEAD(&dentry->d_subdirs);
> > --- /dev/null
> > +++ b/fs/union.c
> > @@ -0,0 +1,53 @@
> > +/*
> > + * VFS based union mount for Linux
> > + *
> > + * Copyright ? 2004-2007 IBM Corporation
> > + * Author(s): Jan Blunck (j.blunck@xxxxxxxxxxxxx)
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the Free
> > + * Software Foundation; either version 2 of the License, or (at your option)
> > + * any later version.
> > + */
> > +
> > +#include <linux/fs.h>
> > +
> > +struct union_info * union_alloc(void)
> > +{
> > + struct union_info *info;
> > +
> > + info = kmalloc(sizeof(*info), GFP_ATOMIC);
> > + if (!info)
> > + return NULL;
> > +
> > + mutex_init(&info->u_mutex);
> > + mutex_lock(&info->u_mutex);
> > + atomic_set(&info->u_count, 1);
> > + UM_DEBUG_LOCK("allocate union %p\n", info);
> > + return info;
> > +}
> > +
> > +struct union_info * union_get(struct union_info *info)
> > +{
> > + BUG_ON(!info);
> > + BUG_ON(!atomic_read(&info->u_count));
> > + atomic_inc(&info->u_count);
> > + UM_DEBUG_LOCK("get union %p (count=%d)\n", info,
> > + atomic_read(&info->u_count));
> > + return info;
> > +}
>
> The locking here needs to be laid out. It looks like union_get() needs
> to be called under union_lock(), while union_get2() (horrible name)
> grabs that lock itself, and returns with the lock held?
>
> Similarly union_put clearly needs to be called under union_lock(), so
> that should be commented here.
Agreed. This whole thing needs a fresh look and we need to establish
some rules and consistency here. After you pointed out this, even
union_alloc2() is looking suspect to me. Same goes for union_release()
also. Thanks for pointing this out.
Regards,
Bharata.
-
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
