linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0 of 2] Stackfs: generic stackable filesystem helper functions
@ 2006-10-13 11:18 Josef Jeff Sipek
  2006-10-13 11:18 ` [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_* Josef Jeff Sipek
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Josef Jeff Sipek @ 2006-10-13 11:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: akpm, torvalds, hch, viro, linux-fsdevel, penberg, ezk, mhalcrow

The following patches introduce stackfs_copy_* functions. These functions
copy inode attributes (such as {a,c,m}time, mode, etc.) from one inode to
another.

While, intended for stackable filesystems any portion of the kernel wishing
to copy inode attributes can use them.

This series consists of two patches, the first introduces the stackfs
functions, and the second makes eCryptfs a user.

Signed-off-by: Josef "Jeff" Sipek <jsipek@cs.sunysb.edu>

4 files changed, 85 insertions(+), 59 deletions(-)
fs/ecryptfs/file.c       |    3 +
fs/ecryptfs/inode.c      |   71 +++++++++-------------------------------------
fs/ecryptfs/main.c       |    5 +--
include/linux/stack_fs.h |   65 ++++++++++++++++++++++++++++++++++++++++++




^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_*
  2006-10-13 11:18 [PATCH 0 of 2] Stackfs: generic stackable filesystem helper functions Josef Jeff Sipek
@ 2006-10-13 11:18 ` Josef Jeff Sipek
  2006-10-13 19:04   ` Andrew James Wade
  2006-10-13 19:27   ` Andrew Morton
  2006-10-13 11:18 ` [PATCH 2 of 2] eCryptfs: Use stackfs's generic copy inode attr functions Josef Jeff Sipek
  2006-10-13 12:02 ` [PATCH 0 of 2] Stackfs: generic stackable filesystem helper functions Pekka Enberg
  2 siblings, 2 replies; 11+ messages in thread
From: Josef Jeff Sipek @ 2006-10-13 11:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: akpm, torvalds, hch, viro, linux-fsdevel, penberg, ezk, mhalcrow

From: Josef "Jeff" Sipek <jsipek@cs.sunysb.edu>

The following patch introduces several stackfs_copy_* functions which allow
stackable filesystems (such as eCryptfs and Unionfs) to easily copy over
(currently only) inode attributes. This prevents code duplication and allows
for code reuse.

Signed-off-by: Josef "Jeff" Sipek <jsipek@cs.sunysb.edu>

---

1 file changed, 65 insertions(+)
include/linux/stack_fs.h |   65 ++++++++++++++++++++++++++++++++++++++++++++++

diff --git a/include/linux/stack_fs.h b/include/linux/stack_fs.h
new file mode 100644
--- /dev/null
+++ b/include/linux/stack_fs.h
@@ -0,0 +1,65 @@
+#ifndef _LINUX_STACK_FS_H
+#define _LINUX_STACK_FS_H
+
+/* This file defines generic functions used primarily by stackable
+ * filesystems
+ */
+
+static inline void stackfs_copy_inode_size(struct inode *dst,
+					   const struct inode *src)
+{
+	i_size_write(dst, i_size_read((struct inode *)src));
+	dst->i_blocks = src->i_blocks;
+}
+
+static inline void stackfs_copy_attr_atime(struct inode *dest,
+					   const struct inode *src)
+{
+	dest->i_atime = src->i_atime;
+}
+
+static inline void stackfs_copy_attr_times(struct inode *dest,
+					   const struct inode *src)
+{
+	dest->i_atime = src->i_atime;
+	dest->i_mtime = src->i_mtime;
+	dest->i_ctime = src->i_ctime;
+}
+
+static inline void stackfs_copy_attr_timesizes(struct inode *dest,
+					       const struct inode *src)
+{
+	dest->i_atime = src->i_atime;
+	dest->i_mtime = src->i_mtime;
+	dest->i_ctime = src->i_ctime;
+	stackfs_copy_inode_size(dest, src);
+}
+
+static inline void __stackfs_copy_attr_all(struct inode *dest,
+					   const struct inode *src,
+					   int (*get_nlinks)(struct inode *))
+{
+	if (!get_nlinks)
+		dest->i_nlink = src->i_nlink;
+	else
+		dest->i_nlink = get_nlinks(dest);
+
+	dest->i_mode = src->i_mode;
+	dest->i_uid = src->i_uid;
+	dest->i_gid = src->i_gid;
+	dest->i_rdev = src->i_rdev;
+	dest->i_atime = src->i_atime;
+	dest->i_mtime = src->i_mtime;
+	dest->i_ctime = src->i_ctime;
+	dest->i_blkbits = src->i_blkbits;
+	dest->i_flags = src->i_flags;
+}
+
+static inline void stackfs_copy_attr_all(struct inode *dest,
+					 const struct inode *src)
+{
+	__stackfs_copy_attr_all(dest, src, NULL);
+}
+
+#endif /* _LINUX_STACK_FS_H */
+

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2 of 2] eCryptfs: Use stackfs's generic copy inode attr functions
  2006-10-13 11:18 [PATCH 0 of 2] Stackfs: generic stackable filesystem helper functions Josef Jeff Sipek
  2006-10-13 11:18 ` [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_* Josef Jeff Sipek
@ 2006-10-13 11:18 ` Josef Jeff Sipek
  2006-10-13 12:02 ` [PATCH 0 of 2] Stackfs: generic stackable filesystem helper functions Pekka Enberg
  2 siblings, 0 replies; 11+ messages in thread
From: Josef Jeff Sipek @ 2006-10-13 11:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: akpm, torvalds, hch, viro, linux-fsdevel, penberg, ezk, mhalcrow

From: Josef "Jeff" Sipek <jsipek@cs.sunysb.edu>

Replace eCryptfs specific code & calls with the more generic stackfs
equivalents and remove the eCryptfs specific functions.

Signed-off-by: Josef "Jeff" Sipek <jsipek@cs.sunysb.edu>

---

3 files changed, 20 insertions(+), 59 deletions(-)
fs/ecryptfs/file.c  |    3 +-
fs/ecryptfs/inode.c |   71 ++++++++++-----------------------------------------
fs/ecryptfs/main.c  |    5 ++-

diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
--- a/fs/ecryptfs/file.c
+++ b/fs/ecryptfs/file.c
@@ -30,6 +30,7 @@
 #include <linux/security.h>
 #include <linux/smp_lock.h>
 #include <linux/compat.h>
+#include <linux/stack_fs.h>
 #include "ecryptfs_kernel.h"
 
 /**
@@ -192,7 +193,7 @@ retry:
 		goto retry;
 	file->f_pos = lower_file->f_pos;
 	if (rc >= 0)
-		ecryptfs_copy_attr_atime(inode, lower_file->f_dentry->d_inode);
+		stackfs_copy_attr_atime(inode, lower_file->f_dentry->d_inode);
 	return rc;
 }
 
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -30,6 +30,7 @@
 #include <linux/namei.h>
 #include <linux/mount.h>
 #include <linux/crypto.h>
+#include <linux/stack_fs.h>
 #include "ecryptfs_kernel.h"
 
 static struct dentry *lock_parent(struct dentry *dentry)
@@ -51,48 +52,6 @@ static void unlock_dir(struct dentry *di
 {
 	mutex_unlock(&dir->d_inode->i_mutex);
 	dput(dir);
-}
-
-void ecryptfs_copy_inode_size(struct inode *dst, const struct inode *src)
-{
-	i_size_write(dst, i_size_read((struct inode *)src));
-	dst->i_blocks = src->i_blocks;
-}
-
-void ecryptfs_copy_attr_atime(struct inode *dest, const struct inode *src)
-{
-	dest->i_atime = src->i_atime;
-}
-
-static void ecryptfs_copy_attr_times(struct inode *dest,
-				     const struct inode *src)
-{
-	dest->i_atime = src->i_atime;
-	dest->i_mtime = src->i_mtime;
-	dest->i_ctime = src->i_ctime;
-}
-
-static void ecryptfs_copy_attr_timesizes(struct inode *dest,
-					 const struct inode *src)
-{
-	dest->i_atime = src->i_atime;
-	dest->i_mtime = src->i_mtime;
-	dest->i_ctime = src->i_ctime;
-	ecryptfs_copy_inode_size(dest, src);
-}
-
-void ecryptfs_copy_attr_all(struct inode *dest, const struct inode *src)
-{
-	dest->i_mode = src->i_mode;
-	dest->i_nlink = src->i_nlink;
-	dest->i_uid = src->i_uid;
-	dest->i_gid = src->i_gid;
-	dest->i_rdev = src->i_rdev;
-	dest->i_atime = src->i_atime;
-	dest->i_mtime = src->i_mtime;
-	dest->i_ctime = src->i_ctime;
-	dest->i_blkbits = src->i_blkbits;
-	dest->i_flags = src->i_flags;
 }
 
 /**
@@ -171,8 +130,8 @@ ecryptfs_do_create(struct inode *directo
 		ecryptfs_printk(KERN_ERR, "Failure in ecryptfs_interpose\n");
 		goto out_lock;
 	}
-	ecryptfs_copy_attr_timesizes(directory_inode,
-				     lower_dir_dentry->d_inode);
+	stackfs_copy_attr_timesizes(directory_inode,
+				    lower_dir_dentry->d_inode);
 out_lock:
 	unlock_dir(lower_dir_dentry);
 out:
@@ -372,7 +331,7 @@ static struct dentry *ecryptfs_lookup(st
        		"d_name.name = [%s]\n", lower_dentry,
 		lower_dentry->d_name.name);
 	lower_inode = lower_dentry->d_inode;
-	ecryptfs_copy_attr_atime(dir, lower_dir_dentry->d_inode);
+	stackfs_copy_attr_atime(dir, lower_dir_dentry->d_inode);
 	BUG_ON(!atomic_read(&lower_dentry->d_count));
 	ecryptfs_set_dentry_private(dentry,
 				    kmem_cache_alloc(ecryptfs_dentry_info_cache,
@@ -478,7 +437,7 @@ static int ecryptfs_link(struct dentry *
 	rc = ecryptfs_interpose(lower_new_dentry, new_dentry, dir->i_sb, 0);
 	if (rc)
 		goto out_lock;
-	ecryptfs_copy_attr_timesizes(dir, lower_new_dentry->d_inode);
+	stackfs_copy_attr_timesizes(dir, lower_new_dentry->d_inode);
 	old_dentry->d_inode->i_nlink =
 		ecryptfs_inode_to_lower(old_dentry->d_inode)->i_nlink;
 	i_size_write(new_dentry->d_inode, file_size_save);
@@ -503,7 +462,7 @@ static int ecryptfs_unlink(struct inode 
 		ecryptfs_printk(KERN_ERR, "Error in vfs_unlink\n");
 		goto out_unlock;
 	}
-	ecryptfs_copy_attr_times(dir, lower_dir_inode);
+	stackfs_copy_attr_times(dir, lower_dir_inode);
 	dentry->d_inode->i_nlink =
 		ecryptfs_inode_to_lower(dentry->d_inode)->i_nlink;
 	dentry->d_inode->i_ctime = dir->i_ctime;
@@ -542,7 +501,7 @@ static int ecryptfs_symlink(struct inode
 	rc = ecryptfs_interpose(lower_dentry, dentry, dir->i_sb, 0);
 	if (rc)
 		goto out_lock;
-	ecryptfs_copy_attr_timesizes(dir, lower_dir_dentry->d_inode);
+	stackfs_copy_attr_timesizes(dir, lower_dir_dentry->d_inode);
 out_lock:
 	unlock_dir(lower_dir_dentry);
 	dput(lower_dentry);
@@ -565,7 +524,7 @@ static int ecryptfs_mkdir(struct inode *
 	rc = ecryptfs_interpose(lower_dentry, dentry, dir->i_sb, 0);
 	if (rc)
 		goto out;
-	ecryptfs_copy_attr_timesizes(dir, lower_dir_dentry->d_inode);
+	stackfs_copy_attr_timesizes(dir, lower_dir_dentry->d_inode);
 	dir->i_nlink = lower_dir_dentry->d_inode->i_nlink;
 out:
 	unlock_dir(lower_dir_dentry);
@@ -601,7 +560,7 @@ static int ecryptfs_rmdir(struct inode *
 		d_delete(tlower_dentry);
 		tlower_dentry = NULL;
 	}
-	ecryptfs_copy_attr_times(dir, lower_dir_dentry->d_inode);
+	stackfs_copy_attr_times(dir, lower_dir_dentry->d_inode);
 	dir->i_nlink = lower_dir_dentry->d_inode->i_nlink;
 	unlock_dir(lower_dir_dentry);
 	if (!rc)
@@ -629,7 +588,7 @@ ecryptfs_mknod(struct inode *dir, struct
 	rc = ecryptfs_interpose(lower_dentry, dentry, dir->i_sb, 0);
 	if (rc)
 		goto out;
-	ecryptfs_copy_attr_timesizes(dir, lower_dir_dentry->d_inode);
+	stackfs_copy_attr_timesizes(dir, lower_dir_dentry->d_inode);
 out:
 	unlock_dir(lower_dir_dentry);
 	if (!dentry->d_inode)
@@ -658,9 +617,9 @@ ecryptfs_rename(struct inode *old_dir, s
 			lower_new_dir_dentry->d_inode, lower_new_dentry);
 	if (rc)
 		goto out_lock;
-	ecryptfs_copy_attr_all(new_dir, lower_new_dir_dentry->d_inode);
+	stackfs_copy_attr_all(new_dir, lower_new_dir_dentry->d_inode);
 	if (new_dir != old_dir)
-		ecryptfs_copy_attr_all(old_dir, lower_old_dir_dentry->d_inode);
+		stackfs_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);
@@ -714,8 +673,8 @@ ecryptfs_readlink(struct dentry *dentry,
 				rc = -EFAULT;
 		}
 		kfree(decoded_name);
-		ecryptfs_copy_attr_atime(dentry->d_inode,
-					 lower_dentry->d_inode);
+		stackfs_copy_attr_atime(dentry->d_inode,
+					lower_dentry->d_inode);
 	}
 out_free_lower_buf:
 	kfree(lower_buf);
@@ -945,7 +904,7 @@ static int ecryptfs_setattr(struct dentr
 	}
 	rc = notify_change(lower_dentry, ia);
 out:
-	ecryptfs_copy_attr_all(inode, lower_inode);
+	stackfs_copy_attr_all(inode, lower_inode);
 	return rc;
 }
 
diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -35,6 +35,7 @@
 #include <linux/pagemap.h>
 #include <linux/key.h>
 #include <linux/parser.h>
+#include <linux/stack_fs.h>
 #include "ecryptfs_kernel.h"
 
 /**
@@ -115,10 +116,10 @@ int ecryptfs_interpose(struct dentry *lo
 		d_add(dentry, inode);
 	else
 		d_instantiate(dentry, inode);
-	ecryptfs_copy_attr_all(inode, lower_inode);
+	stackfs_copy_attr_all(inode, lower_inode);
 	/* This size will be overwritten for real files w/ headers and
 	 * other metadata */
-	ecryptfs_copy_inode_size(inode, lower_inode);
+	stackfs_copy_inode_size(inode, lower_inode);
 out:
 	return rc;
 }



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0 of 2] Stackfs: generic stackable filesystem helper functions
  2006-10-13 11:18 [PATCH 0 of 2] Stackfs: generic stackable filesystem helper functions Josef Jeff Sipek
  2006-10-13 11:18 ` [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_* Josef Jeff Sipek
  2006-10-13 11:18 ` [PATCH 2 of 2] eCryptfs: Use stackfs's generic copy inode attr functions Josef Jeff Sipek
@ 2006-10-13 12:02 ` Pekka Enberg
  2 siblings, 0 replies; 11+ messages in thread
From: Pekka Enberg @ 2006-10-13 12:02 UTC (permalink / raw)
  To: Josef Jeff Sipek
  Cc: linux-kernel, akpm, torvalds, hch, viro, linux-fsdevel, ezk,
	mhalcrow

Hi Jeff,

On 10/13/06, Josef Jeff Sipek <jsipek@cs.sunysb.edu> wrote:
> The following patches introduce stackfs_copy_* functions. These functions
> copy inode attributes (such as {a,c,m}time, mode, etc.) from one inode to
> another.

Looks ok to me. Thanks!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_*
  2006-10-13 11:18 ` [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_* Josef Jeff Sipek
@ 2006-10-13 19:04   ` Andrew James Wade
  2006-10-15 17:38     ` [PATCH] " Jan Engelhardt
  2006-10-13 19:27   ` Andrew Morton
  1 sibling, 1 reply; 11+ messages in thread
From: Andrew James Wade @ 2006-10-13 19:04 UTC (permalink / raw)
  To: Josef Jeff Sipek
  Cc: linux-kernel, akpm, torvalds, hch, viro, linux-fsdevel, penberg,
	ezk, mhalcrow

On Friday 13 October 2006 07:18, Josef Jeff Sipek wrote:
> +static inline void stackfs_copy_inode_size(struct inode *dst,
> +					   const struct inode *src)
> +{
> +	i_size_write(dst, i_size_read((struct inode *)src));

Instead of casting, I'd change the signature of i_size_read.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_*
  2006-10-13 11:18 ` [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_* Josef Jeff Sipek
  2006-10-13 19:04   ` Andrew James Wade
@ 2006-10-13 19:27   ` Andrew Morton
  2006-10-13 20:07     ` Josef Sipek
  1 sibling, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2006-10-13 19:27 UTC (permalink / raw)
  To: Josef Jeff Sipek
  Cc: linux-kernel, torvalds, hch, viro, linux-fsdevel, penberg, ezk,
	mhalcrow

On Fri, 13 Oct 2006 07:18:49 -0400
Josef "Jeff" Sipek <jsipek@cs.sunysb.edu> wrote:

> From: Josef "Jeff" Sipek <jsipek@cs.sunysb.edu>
> 
> The following patch introduces several stackfs_copy_* functions which allow
> stackable filesystems (such as eCryptfs and Unionfs) to easily copy over
> (currently only) inode attributes. This prevents code duplication and allows
> for code reuse.

Fair enough.

> include/linux/stack_fs.h |   65 ++++++++++++++++++++++++++++++++++++++++++++++

The name stack_fs implies that there's a filesystem called stackfs.  Only
there isn't.  I wonder if we can choose a better name for all of this. 
Maybe fs_stack_*?

> 
> diff --git a/include/linux/stack_fs.h b/include/linux/stack_fs.h
> new file mode 100644
> --- /dev/null
> +++ b/include/linux/stack_fs.h
> @@ -0,0 +1,65 @@
> +#ifndef _LINUX_STACK_FS_H
> +#define _LINUX_STACK_FS_H
> +
> +/* This file defines generic functions used primarily by stackable
> + * filesystems
> + */
> +
> +static inline void stackfs_copy_inode_size(struct inode *dst,
> +					   const struct inode *src)
> +{
> +	i_size_write(dst, i_size_read((struct inode *)src));
> +	dst->i_blocks = src->i_blocks;
> +}

What are the locking requirements for these functions?  Presumably the
caller must hold i_mutex on at least the source inode, and perhaps the
destination one?

If i_mutex is held, i_size_read() isn't needed.

If i_mutex is held, i_size_write() isn't needed either.

So please document the locking requirements via source comments and then
see if this can be simplified.

If this function stays as it is, it's too big to inline.

> +static inline void stackfs_copy_attr_atime(struct inode *dest,
> +					   const struct inode *src)
> +{
> +	dest->i_atime = src->i_atime;
> +}
> +
> +static inline void stackfs_copy_attr_times(struct inode *dest,
> +					   const struct inode *src)
> +{
> +	dest->i_atime = src->i_atime;
> +	dest->i_mtime = src->i_mtime;
> +	dest->i_ctime = src->i_ctime;
> +}
> +
> +static inline void stackfs_copy_attr_timesizes(struct inode *dest,
> +					       const struct inode *src)
> +{
> +	dest->i_atime = src->i_atime;
> +	dest->i_mtime = src->i_mtime;
> +	dest->i_ctime = src->i_ctime;
> +	stackfs_copy_inode_size(dest, src);
> +}
> +
> +static inline void __stackfs_copy_attr_all(struct inode *dest,
> +					   const struct inode *src,
> +					   int (*get_nlinks)(struct inode *))
> +{
> +	if (!get_nlinks)
> +		dest->i_nlink = src->i_nlink;
> +	else
> +		dest->i_nlink = get_nlinks(dest);

I cannot find a get_nlinks() in 2.6.19-rc2?

> +	dest->i_mode = src->i_mode;
> +	dest->i_uid = src->i_uid;
> +	dest->i_gid = src->i_gid;
> +	dest->i_rdev = src->i_rdev;
> +	dest->i_atime = src->i_atime;
> +	dest->i_mtime = src->i_mtime;
> +	dest->i_ctime = src->i_ctime;
> +	dest->i_blkbits = src->i_blkbits;
> +	dest->i_flags = src->i_flags;
> +}
>
> +static inline void stackfs_copy_attr_all(struct inode *dest,
> +					 const struct inode *src)
> +{
> +	__stackfs_copy_attr_all(dest, src, NULL);
> +}

Many of these functions are too large to be inlined.  Suggest they be
placed in fs/fs-stack.c (or whatever we call it).

The functions themselves seem a bit arbitrary. 
stackfs_copy_attr_timesizes() copy the three timestamps and the size.  Is
there actually any methodical reason for that, or is it simply some
sequence which happens to have been observed in ecryptfs?

And please - if I asked these questions when reviewing the patch, others
will ask them when reading the code two years from now.  So please treat my
questions as "gosh, I should have put a comment in there".

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_*
  2006-10-13 19:27   ` Andrew Morton
@ 2006-10-13 20:07     ` Josef Sipek
  2006-10-13 20:16       ` Josef Sipek
                         ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Josef Sipek @ 2006-10-13 20:07 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, torvalds, hch, viro, linux-fsdevel, penberg, ezk,
	mhalcrow

On Fri, Oct 13, 2006 at 12:27:06PM -0700, Andrew Morton wrote:
> On Fri, 13 Oct 2006 07:18:49 -0400
> Josef "Jeff" Sipek <jsipek@cs.sunysb.edu> wrote:
> > include/linux/stack_fs.h |   65 ++++++++++++++++++++++++++++++++++++++++++++++
> 
> The name stack_fs implies that there's a filesystem called stackfs.  Only
> there isn't.  I wonder if we can choose a better name for all of this. 
> Maybe fs_stack_*?
 
fs_stack_* sounds good to me.
 
> What are the locking requirements for these functions?  Presumably the
> caller must hold i_mutex on at least the source inode, and perhaps the
> destination one?
> 
> If i_mutex is held, i_size_read() isn't needed.
> 
> If i_mutex is held, i_size_write() isn't needed either.
> 
> So please document the locking requirements via source comments and then
> see if this can be simplified.

Fair enough. I'll submit an updated version.

> > +static inline void __stackfs_copy_attr_all(struct inode *dest,
> > +					   const struct inode *src,
> > +					   int (*get_nlinks)(struct inode *))
> > +{
> > +	if (!get_nlinks)
> > +		dest->i_nlink = src->i_nlink;
> > +	else
> > +		dest->i_nlink = get_nlinks(dest);
> 
> I cannot find a get_nlinks() in 2.6.19-rc2?
 
It is the last argument to the function. Perhaps the function name is
deceiving.
 
> Many of these functions are too large to be inlined.  Suggest they be
> placed in fs/fs-stack.c (or whatever we call it).
 
Ack. As a rule of thumb, for functions like these (laundry list of
assignments), what's a good threshold?
 
> The functions themselves seem a bit arbitrary.
> stackfs_copy_attr_timesizes() copy the three timestamps and the size.  Is
> there actually any methodical reason for that, or is it simply some
> sequence which happens to have been observed in ecryptfs?

These functions come from the FiST templates [1]. Some of these can
definitely removed, or split up.

> And please - if I asked these questions when reviewing the patch, others
> will ask them when reading the code two years from now.  So please treat my
> questions as "gosh, I should have put a comment in there".

That's exactly how I look at it.

Josef "Jeff" Sipek.

-- 
Bad pun of the week: The formula 1 control computer suffered from a race
condition

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_*
  2006-10-13 20:07     ` Josef Sipek
@ 2006-10-13 20:16       ` Josef Sipek
  2006-10-13 20:37       ` Andrew Morton
  2006-10-15 12:36       ` Jan Engelhardt
  2 siblings, 0 replies; 11+ messages in thread
From: Josef Sipek @ 2006-10-13 20:16 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, torvalds, hch, viro, linux-fsdevel, penberg, ezk,
	mhalcrow

On Fri, Oct 13, 2006 at 04:07:05PM -0400, Josef Sipek wrote:
... 
> These functions come from the FiST templates [1]. Some of these can
> definitely removed, or split up.

Forgot to say what [1] was...

[1] http://www.filesystems.org/

Josef "Jeff" Sipek.

-- 
It used to be said [...] that AIX looks like one space alien discovered
Unix, and described it to another different space alien who then implemented
AIX. But their universal translators were broken and they'd had to gesture a
lot.
		- Paul Tomblin 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_*
  2006-10-13 20:07     ` Josef Sipek
  2006-10-13 20:16       ` Josef Sipek
@ 2006-10-13 20:37       ` Andrew Morton
  2006-10-15 12:36       ` Jan Engelhardt
  2 siblings, 0 replies; 11+ messages in thread
From: Andrew Morton @ 2006-10-13 20:37 UTC (permalink / raw)
  To: Josef Sipek
  Cc: linux-kernel, torvalds, hch, viro, linux-fsdevel, penberg, ezk,
	mhalcrow

On Fri, 13 Oct 2006 16:07:05 -0400
Josef Sipek <jsipek@fsl.cs.sunysb.edu> wrote:

> > > +static inline void __stackfs_copy_attr_all(struct inode *dest,
> > > +					   const struct inode *src,
> > > +					   int (*get_nlinks)(struct inode *))
> > > +{
> > > +	if (!get_nlinks)
> > > +		dest->i_nlink = src->i_nlink;
> > > +	else
> > > +		dest->i_nlink = get_nlinks(dest);
> > 
> > I cannot find a get_nlinks() in 2.6.19-rc2?
>  
> It is the last argument to the function. Perhaps the function name is
> deceiving.

doh.

That's why us old farts like to see

	dest->i_nlink = (*get_nlinks)(dest);


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_*
  2006-10-13 20:07     ` Josef Sipek
  2006-10-13 20:16       ` Josef Sipek
  2006-10-13 20:37       ` Andrew Morton
@ 2006-10-15 12:36       ` Jan Engelhardt
  2 siblings, 0 replies; 11+ messages in thread
From: Jan Engelhardt @ 2006-10-15 12:36 UTC (permalink / raw)
  To: Josef Sipek
  Cc: Andrew Morton, linux-kernel, torvalds, hch, viro, linux-fsdevel,
	penberg, ezk, mhalcrow


>> Many of these functions are too large to be inlined.  Suggest they be
>> placed in fs/fs-stack.c (or whatever we call it).

fs/stack.c would probably be enough -- fs/fs-stack.c is like
include/linux/reiserfs_fs.h

>Ack. As a rule of thumb, for functions like these (laundry list of
>assignments), what's a good threshold?

3 or 4 I guess. Might want to take a look at other static-inline functions.


	-`J'
-- 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] Re: [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_*
  2006-10-13 19:04   ` Andrew James Wade
@ 2006-10-15 17:38     ` Jan Engelhardt
  0 siblings, 0 replies; 11+ messages in thread
From: Jan Engelhardt @ 2006-10-15 17:38 UTC (permalink / raw)
  To: Andrew James Wade
  Cc: Josef Jeff Sipek, Linux Kernel Mailing List, akpm, torvalds, hch,
	viro, linux-fsdevel, penberg, ezk, mhalcrow


>On Friday 13 October 2006 07:18, Josef Jeff Sipek wrote:
>> +static inline void stackfs_copy_inode_size(struct inode *dst,
>> +					   const struct inode *src)
>> +{
>> +	i_size_write(dst, i_size_read((struct inode *)src));
>
>Instead of casting, I'd change the signature of i_size_read.


Change the signature of i_size_read(), IMINOR() and IMAJOR() because 
they, or the functions they call, will never modify the argument.
Compile-tested on x86 allmodconfig.

Signed-off-by: Jan Engelhardt <jengelh@gmx.de>


Index: linux-2.6.19-rc1/include/linux/fs.h
===================================================================
--- linux-2.6.19-rc1.orig/include/linux/fs.h
+++ linux-2.6.19-rc1/include/linux/fs.h
@@ -633,7 +633,7 @@ enum inode_i_mutex_lock_class
  * cmpxchg8b without the need of the lock prefix). For SMP compiles
  * and 64bit archs it makes no difference if preempt is enabled or not.
  */
-static inline loff_t i_size_read(struct inode *inode)
+static inline loff_t i_size_read(const struct inode *inode)
 {
 #if BITS_PER_LONG==32 && defined(CONFIG_SMP)
 	loff_t i_size;
@@ -672,12 +672,12 @@ static inline void i_size_write(struct i
 #endif
 }
 
-static inline unsigned iminor(struct inode *inode)
+static inline unsigned iminor(const struct inode *inode)
 {
 	return MINOR(inode->i_rdev);
 }
 
-static inline unsigned imajor(struct inode *inode)
+static inline unsigned imajor(const struct inode *inode)
 {
 	return MAJOR(inode->i_rdev);
 }
#<EOF>


	-`J'
-- 

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2006-10-15 17:52 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-10-13 11:18 [PATCH 0 of 2] Stackfs: generic stackable filesystem helper functions Josef Jeff Sipek
2006-10-13 11:18 ` [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_* Josef Jeff Sipek
2006-10-13 19:04   ` Andrew James Wade
2006-10-15 17:38     ` [PATCH] " Jan Engelhardt
2006-10-13 19:27   ` Andrew Morton
2006-10-13 20:07     ` Josef Sipek
2006-10-13 20:16       ` Josef Sipek
2006-10-13 20:37       ` Andrew Morton
2006-10-15 12:36       ` Jan Engelhardt
2006-10-13 11:18 ` [PATCH 2 of 2] eCryptfs: Use stackfs's generic copy inode attr functions Josef Jeff Sipek
2006-10-13 12:02 ` [PATCH 0 of 2] Stackfs: generic stackable filesystem helper functions Pekka Enberg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).