linux-kernel - Re: [PATCH 21/41] union-mount: Drive the union cache via dcache

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 30 Nov 2009 03:57:46 -0500
From:	Erez Zadok <ezk@...sunysb.edu>
To:	Valerie Aurora <vaurora@...hat.com>
Cc:	Jan Blunck <jblunck@...e.de>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Christoph Hellwig <hch@...radead.org>,
	Andy Whitcroft <apw@...onical.com>,
	Scott James Remnant <scott@...onical.com>,
	Sandu Popa Marius <sandupopamarius@...il.com>,
	Jan Rekorajski <baggins@...h.mimuw.edu.pl>,
	"J. R. Okajima" <hooanon05@...oo.co.jp>,
	Arnd Bergmann <arnd@...db.de>,
	Vladimir Dronnikov <dronnikov@...il.com>,
	Felix Fietkau <nbd@...nwrt.org>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 21/41] union-mount: Drive the union cache via dcache 

In message <1256152779-10054-22-git-send-email-vaurora@...hat.com>, Valerie Aurora writes:
> From: Jan Blunck <jblunck@...e.de>
> 
> If a dentry is removed from dentry cache because its usage count drops to
> zero, the references to the underlying layer of the unions the dentry is in
> are droped too. Therefore the union cache is driven by the dentry cache.

Hmm, in my review for patch 20, I suggested a way to simplify is_unionized()
by marking relevant dentries with a flag whether they are in a union or
not.  If you're driving the entire union cache from the dcache, can't this
be done easily then?

> Signed-off-by: Jan Blunck <jblunck@...e.de>
> Signed-off-by: Valerie Aurora <vaurora@...hat.com>
> ---
>  fs/dcache.c            |   10 ++++++-
>  fs/union.c             |   74 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/dcache.h |    8 +++++
>  include/linux/union.h  |    6 ++++
>  4 files changed, 97 insertions(+), 1 deletions(-)

> diff --git a/fs/union.c b/fs/union.c
> index d1950c2..6b99393 100644
> --- a/fs/union.c
> +++ b/fs/union.c
> @@ -14,6 +14,7 @@
>  
>  #include <linux/bootmem.h>
>  #include <linux/init.h>
> +#include <linux/module.h>
>  #include <linux/types.h>
>  #include <linux/hash.h>
>  #include <linux/fs.h>
> @@ -255,6 +256,8 @@ int append_to_union(struct vfsmount *mnt, struct dentry *dentry,
>  		union_put(this);
>  		return 0;
>  	}
> +	list_add(&this->u_unions, &dentry->d_unions);
> +	dest_dentry->d_unionized++;
>  	__union_hash(this);
>  	spin_unlock(&union_lock);
>  	return 0;
> @@ -330,3 +333,74 @@ int follow_union_mount(struct vfsmount **mnt, struct dentry **dentry)
>  
>  	return res;
>  }
> +
> +/*
> + * This must be called when unhashing a dentry. This is called with dcache_lock
> + * and unhashes all unions this dentry is in.
> + */
> +void __d_drop_unions(struct dentry *dentry)
> +{
> +	struct union_mount *this, *next;
> +
> +	spin_lock(&union_lock);
> +	list_for_each_entry_safe(this, next, &dentry->d_unions, u_unions)
> +		__union_unhash(this);
> +	spin_unlock(&union_lock);
> +}
> +EXPORT_SYMBOL_GPL(__d_drop_unions);

I thought the convention was that internal functions prefixed with __ are
.. internal, not to be extern'ed and exported.

Besides, why export this symbol?  Which modules need it in your patchset?

> +/*
> + * This must be called after __d_drop_unions() without holding any locks.
> + * Note: The dentry might still be reachable via a lookup but at that time it
> + * already a negative dentry. Otherwise it would be unhashed. The union_mount
> + * structure itself is still reachable through mnt->mnt_unions (which we
> + * protect against with union_lock).
> + */
> +void shrink_d_unions(struct dentry *dentry)
> +{
> +	struct union_mount *this, *next;

See my comments about this/that vs. upper/lower in my review for patch #20.

> +repeat:
> +	spin_lock(&union_lock);
> +	list_for_each_entry_safe(this, next, &dentry->d_unions, u_unions) {
> +		BUG_ON(!hlist_unhashed(&this->u_hash));
> +		BUG_ON(!hlist_unhashed(&this->u_rhash));
> +		list_del(&this->u_unions);
> +		this->u_next.dentry->d_unionized--;
> +		spin_unlock(&union_lock);
> +		union_put(this);
> +		goto repeat;
> +	}
> +	spin_unlock(&union_lock);
> +}
> +
> +extern void __dput(struct dentry *, struct list_head *, int);

Why this extern here?  Isn't there some header file you can #include more
cleanly at the top of this .c file?

>  static inline void d_drop(struct dentry *dentry)
> diff --git a/include/linux/union.h b/include/linux/union.h
> index 0c85312..b035a82 100644
> --- a/include/linux/union.h
> +++ b/include/linux/union.h
> @@ -46,6 +46,9 @@ extern int append_to_union(struct vfsmount *, struct dentry *,
>  			   struct vfsmount *, struct dentry *);
>  extern int follow_union_down(struct vfsmount **, struct dentry **);
>  extern int follow_union_mount(struct vfsmount **, struct dentry **);
> +extern void __d_drop_unions(struct dentry *);
> +extern void shrink_d_unions(struct dentry *);
> +extern void __shrink_d_unions(struct dentry *, struct list_head *);

Again, I don't understand why the two out of three functions above are
prefixed with __ while one of them isn't.  I always prefer to name things
for what they actually do, not rely on magic prefixes and conventions to
guess what it means.  I suggest trying to find better names to avoid having
so many FOO and __FOO names in this entire series of patchset.

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