fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

[Erez Zadok]

Andrew, this patch has been in my git tree for a while.  It affects current
mainline users (i.e., ecryptfs).  Do you think it can go to mainline soon?
If not, post-25? (I can remind you then).  This patch has a few changes, and
I can break it into several smaller ones if desired:

1. remove the 3rd arg to fsstack_copy_attr_all.  There are no users for it:
   ecryptfs never used the 3rd arg; unionfs stopped using it a long time
   ago.  Halcrow ok'ed this patch some time ago.

2. add necessary locking for 32-bit smp systems in fsstack_copy_inode_size
   (courtesy Hugh Dickins).

3. minor commenting style changes, and addition of copyrights which were
   missing.

Thanks,
Erez.

--cut--here----cut--here----cut--here----cut--here----cut--here--


fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

Acked-by: Mike Halcrow <mhalcrow@us.ibm.com>
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>

diff --git a/include/linux/fs_stack.h b/include/linux/fs_stack.h
index bb516ce..6b52faf 100644
--- a/include/linux/fs_stack.h
+++ b/include/linux/fs_stack.h
@@ -1,17 +1,28 @@
+/*
+ * Copyright (c) 2006-2007 Erez Zadok
+ * Copyright (c) 2006-2007 Josef 'Jeff' Sipek
+ * Copyright (c) 2006-2007 Stony Brook University
+ * Copyright (c) 2006-2007 The Research Foundation of SUNY
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
 #ifndef _LINUX_FS_STACK_H
 #define _LINUX_FS_STACK_H
 
-/* This file defines generic functions used primarily by stackable
+/*
+ * This file defines generic functions used primarily by stackable
  * filesystems; none of these functions require i_mutex to be held.
  */
 
 #include <linux/fs.h>
 
 /* externs for fs/stack.c */
-extern void fsstack_copy_attr_all(struct inode *dest, const struct inode *src,
-				int (*get_nlinks)(struct inode *));
-
-extern void fsstack_copy_inode_size(struct inode *dst, const struct inode *src);
+extern void fsstack_copy_attr_all(struct inode *dest, const struct inode *src);
+extern void fsstack_copy_inode_size(struct inode *dst,
+				    const struct inode *src);
 
 /* inlines */
 static inline void fsstack_copy_attr_atime(struct inode *dest,
diff --git a/fs/stack.c b/fs/stack.c
index 67716f6..4336f2b 100644
--- a/fs/stack.c
+++ b/fs/stack.c
@@ -1,24 +1,42 @@
+/*
+ * Copyright (c) 2006-2007 Erez Zadok
+ * Copyright (c) 2006-2007 Josef 'Jeff' Sipek
+ * Copyright (c) 2006-2007 Stony Brook University
+ * Copyright (c) 2006-2007 The Research Foundation of SUNY
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/fs_stack.h>
 
-/* does _NOT_ require i_mutex to be held.
+/*
+ * does _NOT_ require i_mutex to be held.
  *
  * This function cannot be inlined since i_size_{read,write} is rather
  * heavy-weight on 32-bit systems
  */
 void fsstack_copy_inode_size(struct inode *dst, const struct inode *src)
 {
-	i_size_write(dst, i_size_read((struct inode *)src));
+#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
+	spin_lock(&dst->i_lock);
+#endif
+	i_size_write(dst, i_size_read(src));
 	dst->i_blocks = src->i_blocks;
+#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
+	spin_unlock(&dst->i_lock);
+#endif
 }
 EXPORT_SYMBOL_GPL(fsstack_copy_inode_size);
 
-/* copy all attributes; get_nlinks is optional way to override the i_nlink
+/*
+ * copy all attributes; get_nlinks is optional way to override the i_nlink
  * copying
  */
-void fsstack_copy_attr_all(struct inode *dest, const struct inode *src,
-				int (*get_nlinks)(struct inode *))
+void fsstack_copy_attr_all(struct inode *dest, const struct inode *src)
 {
 	dest->i_mode = src->i_mode;
 	dest->i_uid = src->i_uid;
@@ -29,14 +47,6 @@ void fsstack_copy_attr_all(struct inode *dest, const struct inode *src,
 	dest->i_ctime = src->i_ctime;
 	dest->i_blkbits = src->i_blkbits;
 	dest->i_flags = src->i_flags;
-
-	/*
-	 * Update the nlinks AFTER updating the above fields, because the
-	 * get_links callback may depend on them.
-	 */
-	if (!get_nlinks)
-		dest->i_nlink = src->i_nlink;
-	else
-		dest->i_nlink = (*get_nlinks)(dest);
+	dest->i_nlink = src->i_nlink;
 }
 EXPORT_SYMBOL_GPL(fsstack_copy_attr_all);
diff --git a/fs/ecryptfs/dentry.c b/fs/ecryptfs/dentry.c
index 5e59658..4621f89 100644
--- a/fs/ecryptfs/dentry.c
+++ b/fs/ecryptfs/dentry.c
@@ -62,7 +62,7 @@ static int ecryptfs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
 		struct inode *lower_inode =
 			ecryptfs_inode_to_lower(dentry->d_inode);
 
-		fsstack_copy_attr_all(dentry->d_inode, lower_inode, NULL);
+		fsstack_copy_attr_all(dentry->d_inode, lower_inode);
 	}
 out:
 	return rc;
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index e238611..687819d 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -575,9 +575,9 @@ ecryptfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 			lower_new_dir_dentry->d_inode, lower_new_dentry);
 	if (rc)
 		goto out_lock;
-	fsstack_copy_attr_all(new_dir, lower_new_dir_dentry->d_inode, NULL);
+	fsstack_copy_attr_all(new_dir, lower_new_dir_dentry->d_inode);
 	if (new_dir != old_dir)
-		fsstack_copy_attr_all(old_dir, lower_old_dir_dentry->d_inode, NULL);
+		fsstack_copy_attr_all(old_dir, lower_old_dir_dentry->d_inode);
 out_lock:
 	unlock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
 	dput(lower_new_dentry->d_parent);
@@ -910,7 +910,7 @@ static int ecryptfs_setattr(struct dentry *dentry, struct iattr *ia)
 
 	rc = notify_change(lower_dentry, ia);
 out:
-	fsstack_copy_attr_all(inode, lower_inode, NULL);
+	fsstack_copy_attr_all(inode, lower_inode);
 	return rc;
 }
 
diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index d25ac95..7eac062 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -211,7 +211,7 @@ int ecryptfs_interpose(struct dentry *lower_dentry, struct dentry *dentry,
 		d_add(dentry, inode);
 	else
 		d_instantiate(dentry, inode);
-	fsstack_copy_attr_all(inode, lower_inode, NULL);
+	fsstack_copy_attr_all(inode, lower_inode);
 	/* This size will be overwritten for real files w/ headers and
 	 * other metadata */
 	fsstack_copy_inode_size(inode, lower_inode);

Re: fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

[Andrew Morton]

On Wed, 2 Apr 2008 21:49:11 -0400 Erez Zadok <ezk@cs.sunysb.edu> wrote:

> Andrew, this patch has been in my git tree for a while.  It affects current
> mainline users (i.e., ecryptfs).  Do you think it can go to mainline soon?
> If not, post-25? (I can remind you then).  This patch has a few changes, and
> I can break it into several smaller ones if desired:
> 
> 1. remove the 3rd arg to fsstack_copy_attr_all.  There are no users for it:
>    ecryptfs never used the 3rd arg; unionfs stopped using it a long time
>    ago.  Halcrow ok'ed this patch some time ago.
> 
> 2. add necessary locking for 32-bit smp systems in fsstack_copy_inode_size
>    (courtesy Hugh Dickins).
> 
> 3. minor commenting style changes, and addition of copyrights which were
>    missing.
> 
> Thanks,
> Erez.
> 
> --cut--here----cut--here----cut--here----cut--here----cut--here--
> 
> 
> fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

I'm already carrying this, via git-unionfs.  It makes it a bit hard to merge.
I guess resending it during the 2.6.26 merge window would suit.

Re: fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

[Josef 'Jeff' Sipek]

On Wed, Apr 02, 2008 at 09:49:11PM -0400, Erez Zadok wrote:
...
> +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
> +	spin_lock(&dst->i_lock);
> +#endif

I think you need to check CONFIG_PREEMPT as well.

Josef 'Jeff' Sipek.

-- 
The reasonable man adapts himself to the world; the unreasonable one
persists in trying to adapt the world to himself. Therefore all progress
depends on the unreasonable man.
		- George Bernard Shaw

Re: fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

[Hugh Dickins]

On Thu, 3 Apr 2008, Josef 'Jeff' Sipek wrote:
> On Wed, Apr 02, 2008 at 09:49:11PM -0400, Erez Zadok wrote:
> ...
> > +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
> > +	spin_lock(&dst->i_lock);
> > +#endif
> 
> I think you need to check CONFIG_PREEMPT as well.

Don't i_size_read() and i_size_write() handle that case adequately
themselves with preempt_dis/enable?

If you mean it'd be better to have the preempt_dis/enable bracketing
around the i_size_read() along with the i_size_write(): well, could
do, but if lower i_size is changing underneath us in that way,
it can just as well change again a moment later, so no point.

The bit that worries me, that prevents me from saying Acked-by,
is that I'm unsure of the extent to which lower filesystems are
relying on i_mutex or something else for their i_size serialization.
Using the i_lock here plays well with unionfs internally, and fixes
the hangs I used to see; but relying on one locking scheme here and
another below is uncomfortable for me.  But it is an improvement.

Hugh

Re: fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

[Erez Zadok]

In message <20080403182001.GB30189@josefsipek.net>, "Josef 'Jeff' Sipek" writes:
> On Wed, Apr 02, 2008 at 09:49:11PM -0400, Erez Zadok wrote:
> ...
> > +#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)
> > +	spin_lock(&dst->i_lock);
> > +#endif
> 
> I think you need to check CONFIG_PREEMPT as well.
> 
> Josef 'Jeff' Sipek.

I'm not sure if it's needed in case of CONFIG_PREEMPT.  Anyone?  The code
for i_size_write (below), and the comment at the top of the function,
suggest that the spinlock is needed only to prevent the lots seqcount.

/*
 * NOTE: unlike i_size_read(), i_size_write() does need locking around it
 * (normally i_mutex), otherwise on 32bit/SMP an update of i_size_seqcount
 * can be lost, resulting in subsequent i_size_read() calls spinning forever.
 */
static inline void i_size_write(struct inode *inode, loff_t i_size)
{
#if BITS_PER_LONG==32 && defined(CONFIG_SMP)
	write_seqcount_begin(&inode->i_size_seqcount);
	inode->i_size = i_size;
	write_seqcount_end(&inode->i_size_seqcount);
#elif BITS_PER_LONG==32 && defined(CONFIG_PREEMPT)
	preempt_disable();
	inode->i_size = i_size;
	preempt_enable();
#else
	inode->i_size = i_size;
#endif
}


BTW, some time ago I reviewed all callers of i_size_write.  I did so again
just now, and the results were the same:

- a LOT of callers of i_size_write don't take any lock
- some take another spinlock in a different data structure
- those that do take the spinlock, do so unconditionally
- only unionfs and fs/stack.c wrap the spinlock in

	#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)

Erez.

Re: fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

[Hugh Dickins]

On Thu, 3 Apr 2008, Erez Zadok wrote:
> In message <20080403182001.GB30189@josefsipek.net>, "Josef 'Jeff' Sipek" writes:
> > I think you need to check CONFIG_PREEMPT as well.
> 
> I'm not sure if it's needed in case of CONFIG_PREEMPT.  Anyone?  The code
> for i_size_write (below), and the comment at the top of the function,
> suggest that the spinlock is needed only to prevent the lots seqcount.

Correct.

> BTW, some time ago I reviewed all callers of i_size_write.  I did so again
> just now, and the results were the same:
> 
> - a LOT of callers of i_size_write don't take any lock

They mostly know that i_mutex is already held (as i_size_write comment
mentions); but I believe that's up to the individual filesystem.

> - some take another spinlock in a different data structure
> - those that do take the spinlock, do so unconditionally
> - only unionfs and fs/stack.c wrap the spinlock in
> 
> 	#if BITS_PER_LONG == 32 && defined(CONFIG_SMP)

I chose to follow the #ifdeffery of i_size_write(),
but you could do it unconditionally if you prefer:
just a little more overhead when it's not needed.

As I've said elsewhere, I don't think the result can be entirely
safe against concurrent changes in the lower filesystem, using
different locking; but I don't know how resilient unionfs is
expected to be against messing directly with lower at the same
time as upper level.

Hugh

Re: fs_stack/eCryptfs: remove 3rd arg of copy_attr_all, add locking to copy_inode_size

[Josef 'Jeff' Sipek]

On Thu, Apr 03, 2008 at 08:26:54PM +0100, Hugh Dickins wrote:
> On Thu, 3 Apr 2008, Erez Zadok wrote:
> > In message <20080403182001.GB30189@josefsipek.net>, "Josef 'Jeff' Sipek" writes:
> > > I think you need to check CONFIG_PREEMPT as well.
> > 
> > I'm not sure if it's needed in case of CONFIG_PREEMPT.  Anyone?  The code
> > for i_size_write (below), and the comment at the top of the function,
> > suggest that the spinlock is needed only to prevent the lots seqcount.
> 
> Correct.
 
True...

> > BTW, some time ago I reviewed all callers of i_size_write.  I did so again
> > just now, and the results were the same:
> > 
> > - a LOT of callers of i_size_write don't take any lock
> 
> They mostly know that i_mutex is already held (as i_size_write comment
> mentions); but I believe that's up to the individual filesystem.

Is this function (fsstack_copy_inode_size) used only in unlocked paths? If
there's little chance that it'll ever need to be used in locked paths, then
sure, why not have the lock right in the function.

Josef 'Jeff' Sipek.

-- 
My public GPG key can be found at
http://www.josefsipek.net/gpg/public-0xC7958FFE.txt