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 <[email protected]>
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 ++++++++++++++++++++++++++++++++++++++++++
From: Josef "Jeff" Sipek <[email protected]>
Replace eCryptfs specific code & calls with the more generic stackfs
equivalents and remove the eCryptfs specific functions.
Signed-off-by: Josef "Jeff" Sipek <[email protected]>
---
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;
}
From: Josef "Jeff" Sipek <[email protected]>
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 <[email protected]>
---
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 */
+
Hi Jeff,
On 10/13/06, Josef Jeff Sipek <[email protected]> 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!
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.
On Fri, 13 Oct 2006 07:18:49 -0400
Josef "Jeff" Sipek <[email protected]> wrote:
> From: Josef "Jeff" Sipek <[email protected]>
>
> 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".
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 <[email protected]> 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
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
On Fri, 13 Oct 2006 16:07:05 -0400
Josef Sipek <[email protected]> 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);
>> 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'
--
>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 <[email protected]>
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'
--