2006-11-02 15:20:53

by Josef 'Jeff' Sipek

[permalink] [raw]
Subject: [PATCH 0/3] fsstack updates

These are some more updates to fsstack.

The first patch makes fsstack_copy_attr_all copy the inode size in addition
to all the other attributes.

The second patch introduces generic structures and functions to get/set
lower objects.

The third patch converts eCryptfs to use the generic structures and
functions introduced by the second patch.

For those interested, I set up a git repo with all the fsstack related
patches currently in mm and these three (they're still being mirrored):

git://git.kernel.org/pub/scm/linux/kernel/git/jsipek/fsstack-2.6.git for-2.6.20

Josef "Jeff" Sipek.

Signed-off-by: Josef "Jeff" Sipek <[email protected]>


2006-11-02 15:21:31

by Josef 'Jeff' Sipek

[permalink] [raw]
Subject: [PATCH 2/3] fsstack: Generic get/set lower object functions

From: Josef "Jeff" Sipek <[email protected]>

Every stackable filesystem needs to track what the corresponding lower
objects are. The stackable fs superblock needs to maintain pointers to the
lower superblock(s); the inodes need to maintain pointers to the lower
inodes; dentries need to maintain pointers to the lower dentries and
vfsmounts.

Currently, every stackable filesystem maintains this information in the
private data of the object in question (the inode pointers may be also
stored in the inode's container - which is what the following patch
requires.)

This patch introduces generic structures to maintain the lower object
references, and functions to get/set the lower object structure members.
These functions are the generalized forms, which work with both linear and
fanout stackable filesystem (eCryptfs and Unionfs to name some).

The patch is loosely based on Pekka Enberg's patches from October 13th
(http://lkml.org/lkml/2006/10/13/82).

Cc: Pekka Enberg <[email protected]>
Cc: Michael Halcrow <[email protected]>
Cc: Erez Zadok <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Andrew Morton <[email protected]>
Signed-off-by: Josef "Jeff" Sipek <[email protected]>
---

include/linux/fs_stack.h | 264 ++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 264 insertions(+), 0 deletions(-)

diff --git a/include/linux/fs_stack.h b/include/linux/fs_stack.h
index bb516ce..fd4872e 100644
--- a/include/linux/fs_stack.h
+++ b/include/linux/fs_stack.h
@@ -5,8 +5,272 @@ #define _LINUX_FS_STACK_H
* filesystems; none of these functions require i_mutex to be held.
*/

+#include <linux/namei.h>
#include <linux/fs.h>

+/* structs to maintain pointers to the lower VFS objects */
+struct fsstack_sb_info {
+ union {
+ struct super_block *sb;
+ struct super_block **sbs;
+ };
+};
+
+struct fsstack_inode_info {
+ union {
+ struct inode *inode;
+ struct inode **inodes;
+ };
+};
+
+struct fsstack_dentry_info {
+ union {
+ struct path path;
+ struct path *paths;
+ };
+};
+
+struct fsstack_file_info {
+ union {
+ struct file *file;
+ struct file **files;
+ };
+};
+
+/* DO NOT USE!
+ *
+ * The following structure is used during the container_of calls to allow
+ * for as generic as possible way of accessing fsstack_inode_info given a
+ * pointer to the inode.
+ */
+struct __fsstack_inode_generic_info {
+ struct inode vfs_inode; /* vfs inode */
+ struct fsstack_inode_info info; /* fsstack inode info */
+};
+
+/*
+ * Functions to get lower objects from an upper one.
+ *
+ * NOTE: The filesystem specific info structures (for dentries, superblocks
+ * and files) _must_ have the following layout:
+ *
+ * struct foo {
+ * struct fsstack_{dentry,sb,file}_info info;
+ * ...
+ * };
+ *
+ * Because of the usage of containers, the inode container structure _must_
+ * have the following layout:
+ *
+ * struct bar {
+ * struct inode vfs_inode;
+ * struct fsstack_inode_info info;
+ * ...
+ * };
+ */
+static inline struct super_block *
+__fsstack_lower_sb(struct super_block *sb, unsigned long branch_idx)
+{
+ struct fsstack_sb_info *info = sb->s_fs_info;
+ return info->sbs[branch_idx];
+}
+
+static inline struct super_block **
+__fsstack_lower_sbs(struct super_block *sb)
+{
+ struct fsstack_sb_info *info = sb->s_fs_info;
+ return info->sbs;
+}
+
+static inline void
+__fsstack_set_lower_sb(struct super_block *sb, unsigned long branch_idx,
+ struct super_block *lower_sb)
+{
+ struct fsstack_sb_info *info = sb->s_fs_info;
+ info->sbs[branch_idx] = lower_sb;
+}
+
+static inline void
+__fsstack_set_lower_sbs(struct super_block *sb, struct super_block **lower_sbs)
+{
+ struct fsstack_sb_info *info = sb->s_fs_info;
+ info->sbs = lower_sbs;
+}
+
+static inline struct super_block *fsstack_lower_sb(struct super_block *sb)
+{
+ struct fsstack_sb_info *info = sb->s_fs_info;
+ return info->sb;
+}
+
+static inline void
+fsstack_set_lower_sb(struct super_block *sb, struct super_block *lower_sb)
+{
+ struct fsstack_sb_info *info = sb->s_fs_info;
+ info->sb = lower_sb;
+}
+
+/* get the fs dependent data */
+static inline void * fsstack_inode_data(struct inode *inode)
+{
+ return &((struct __fsstack_inode_generic_info*) inode)->info;
+}
+
+static inline struct inode *
+__fsstack_lower_inode(struct inode *inode, unsigned long branch_idx)
+{
+ struct fsstack_inode_info *info = fsstack_inode_data(inode);
+
+ return info->inodes[branch_idx];
+}
+
+static inline struct inode **
+__fsstack_lower_inodes(struct inode *inode)
+{
+ struct fsstack_inode_info *info = fsstack_inode_data(inode);
+ return info->inodes;
+}
+
+static inline void
+__fsstack_set_lower_inode(struct inode *inode, unsigned long branch_idx,
+ struct inode *lower_inode)
+{
+ struct fsstack_inode_info *info = fsstack_inode_data(inode);
+ info->inodes[branch_idx] = lower_inode;
+}
+
+static inline void
+__fsstack_set_lower_inodes(struct inode *inode, struct inode **lower_inodes)
+{
+ struct fsstack_inode_info *info = fsstack_inode_data(inode);
+ info->inodes = lower_inodes;
+}
+
+static inline struct inode *fsstack_lower_inode(struct inode *inode)
+{
+ struct fsstack_inode_info *info = fsstack_inode_data(inode);
+ return info->inode;
+}
+
+static inline void
+fsstack_set_lower_inode(struct inode *inode, struct inode *lower_inode)
+{
+ struct fsstack_inode_info *info = fsstack_inode_data(inode);
+ info->inode = lower_inode;
+}
+
+static inline struct dentry *
+__fsstack_lower_dentry(struct dentry *dentry, unsigned long branch_idx)
+{
+ struct fsstack_dentry_info *info = dentry->d_fsdata;
+ return info->paths[branch_idx].dentry;
+}
+
+static inline struct path *
+__fsstack_lower_paths(struct dentry *dentry)
+{
+ struct fsstack_dentry_info *info = dentry->d_fsdata;
+ return info->paths;
+}
+
+static inline void
+__fsstack_set_lower_dentry(struct dentry *dentry, unsigned long branch_idx,
+ struct dentry *lower_dentry)
+{
+ struct fsstack_dentry_info *info = dentry->d_fsdata;
+ info->paths[branch_idx].dentry = lower_dentry;
+}
+
+static inline void
+__fsstack_set_lower_paths(struct dentry *dentry, struct path *lower_paths)
+{
+ struct fsstack_dentry_info *info = dentry->d_fsdata;
+ info->paths = lower_paths;
+}
+
+static inline struct dentry *fsstack_lower_dentry(struct dentry *dentry)
+{
+ struct fsstack_dentry_info *info = dentry->d_fsdata;
+ return info->path.dentry;
+}
+
+static inline void
+fsstack_set_lower_dentry(struct dentry *dentry, struct dentry *lower_dentry)
+{
+ struct fsstack_dentry_info *info = dentry->d_fsdata;
+ info->path.dentry = lower_dentry;
+}
+
+static inline struct vfsmount *
+__fsstack_lower_mnt(struct dentry *dentry, unsigned long branch_idx)
+{
+ struct fsstack_dentry_info *info = dentry->d_fsdata;
+ return info->paths[branch_idx].mnt;
+}
+
+static inline void
+__fsstack_set_lower_mnt(struct dentry *dentry, unsigned long branch_idx,
+ struct vfsmount *lower_mnt)
+{
+ struct fsstack_dentry_info *info = dentry->d_fsdata;
+ info->paths[branch_idx].mnt = lower_mnt;
+}
+
+static inline struct vfsmount *fsstack_lower_mnt(struct dentry *dentry)
+{
+ struct fsstack_dentry_info *info = dentry->d_fsdata;
+ return info->path.mnt;
+}
+
+static inline void
+fsstack_set_lower_mnt(struct dentry *dentry, struct vfsmount *lower_mnt)
+{
+ struct fsstack_dentry_info *info = dentry->d_fsdata;
+ info->path.mnt = lower_mnt;
+}
+
+static inline struct file *
+__fsstack_lower_file(struct file *file, unsigned long branch_idx)
+{
+ struct fsstack_file_info *info = file->private_data;
+ return info->files[branch_idx];
+}
+
+static inline struct file **
+__fsstack_lower_files(struct file *file)
+{
+ struct fsstack_file_info *info = file->private_data;
+ return info->files;
+}
+
+static inline void
+__fsstack_set_lower_file(struct file *file, unsigned long branch_idx,
+ struct file *lower_file)
+{
+ struct fsstack_file_info *info = file->private_data;
+ info->files[branch_idx] = lower_file;
+}
+
+static inline void
+__fsstack_set_lower_files(struct file *file, struct file **lower_files)
+{
+ struct fsstack_file_info *info = file->private_data;
+ info->files = lower_files;
+}
+
+static inline struct file *fsstack_lower_file(struct file *file)
+{
+ struct fsstack_file_info *info = file->private_data;
+ return info->file;
+}
+
+static inline void
+fsstack_set_lower_file(struct file *file, struct file *lower_file)
+{
+ struct fsstack_file_info *info = file->private_data;
+ info->file = lower_file;
+}
+
/* externs for fs/stack.c */
extern void fsstack_copy_attr_all(struct inode *dest, const struct inode *src,
int (*get_nlinks)(struct inode *));

2006-11-02 15:20:57

by Josef 'Jeff' Sipek

[permalink] [raw]
Subject: [PATCH 1/3] fsstack: Make fsstack_copy_attr_all copy inode size

From: Josef "Jeff" Sipek <[email protected]>

fsstack_copy_attr_all should copy the inode size in addition to all the
other attributes.

Signed-off-by: Josef "Jeff" Sipek <[email protected]>
---

fs/stack.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/stack.c b/fs/stack.c
index 03987f2..9d3ba4e 100644
--- a/fs/stack.c
+++ b/fs/stack.c
@@ -33,5 +33,7 @@ void fsstack_copy_attr_all(struct inode
dest->i_ctime = src->i_ctime;
dest->i_blkbits = src->i_blkbits;
dest->i_flags = src->i_flags;
+
+ fsstack_copy_inode_size(dest, src);
}
EXPORT_SYMBOL_GPL(fsstack_copy_attr_all);

2006-11-02 15:21:25

by Josef 'Jeff' Sipek

[permalink] [raw]
Subject: [PATCH 3/3] fsstack: Make ecryptfs a user of new fsstack_* functions

From: Josef "Jeff" Sipek <[email protected]>

This patch makes eCryptfs a user of the generic fsstack lower object fsstack
structures and functions.

Cc: Pekka Enberg <[email protected]>
Cc: Michael Halcrow <[email protected]>
Cc: Erez Zadok <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Andrew Morton <[email protected]>
Signed-off-by: Josef "Jeff" Sipek <[email protected]>
---

fs/ecryptfs/crypto.c | 4 +-
fs/ecryptfs/dentry.c | 6 +--
fs/ecryptfs/ecryptfs_kernel.h | 84 ++++++------------------------------------
fs/ecryptfs/file.c | 34 ++++++++---------
fs/ecryptfs/inode.c | 72 ++++++++++++++++++------------------
fs/ecryptfs/main.c | 8 ++--
fs/ecryptfs/mmap.c | 18 ++++-----
fs/ecryptfs/super.c | 10 ++---
8 files changed, 89 insertions(+), 147 deletions(-)

diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index f49f105..736adcb 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -482,7 +482,7 @@ #define ECRYPTFS_PAGE_STATE_MODIFIED 2
#define ECRYPTFS_PAGE_STATE_WRITTEN 3
int page_state;

- lower_inode = ecryptfs_inode_to_lower(ctx->page->mapping->host);
+ lower_inode = fsstack_lower_inode(ctx->page->mapping->host);
inode_info = ecryptfs_inode_to_private(ctx->page->mapping->host);
crypt_stat = &inode_info->crypt_stat;
if (!ECRYPTFS_CHECK_FLAG(crypt_stat->flags, ECRYPTFS_ENCRYPTED)) {
@@ -616,7 +616,7 @@ int ecryptfs_decrypt_page(struct file *f

crypt_stat = &(ecryptfs_inode_to_private(
page->mapping->host)->crypt_stat);
- lower_inode = ecryptfs_inode_to_lower(page->mapping->host);
+ lower_inode = fsstack_lower_inode(page->mapping->host);
if (!ECRYPTFS_CHECK_FLAG(crypt_stat->flags, ECRYPTFS_ENCRYPTED)) {
rc = ecryptfs_do_readpage(file, page, page->index);
if (rc)
diff --git a/fs/ecryptfs/dentry.c b/fs/ecryptfs/dentry.c
index 0b9992a..6559d82 100644
--- a/fs/ecryptfs/dentry.c
+++ b/fs/ecryptfs/dentry.c
@@ -42,8 +42,8 @@ #include "ecryptfs_kernel.h"
*/
static int ecryptfs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
{
- struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
- struct vfsmount *lower_mnt = ecryptfs_dentry_to_lower_mnt(dentry);
+ struct dentry *lower_dentry = fsstack_lower_dentry(dentry);
+ struct vfsmount *lower_mnt = fsstack_lower_mnt(dentry);
struct dentry *dentry_save;
struct vfsmount *vfsmount_save;
int rc = 1;
@@ -73,7 +73,7 @@ static void ecryptfs_d_release(struct de
{
struct dentry *lower_dentry;

- lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ lower_dentry = fsstack_lower_dentry(dentry);
if (ecryptfs_dentry_to_private(dentry))
kmem_cache_free(ecryptfs_dentry_info_cache,
ecryptfs_dentry_to_private(dentry));
diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
index 870a65b..2f46258 100644
--- a/fs/ecryptfs/ecryptfs_kernel.h
+++ b/fs/ecryptfs/ecryptfs_kernel.h
@@ -30,6 +30,7 @@ #include <keys/user-type.h>
#include <linux/fs.h>
#include <linux/namei.h>
#include <linux/scatterlist.h>
+#include <linux/fs_stack.h>

/* Version verification for shared data structures w/ userspace */
#define ECRYPTFS_VERSION_MAJOR 0x00
@@ -218,17 +219,20 @@ #define ECRYPTFS_KEY_VALID 0x00
struct mutex cs_mutex;
};

-/* inode private data. */
+/* inode private data.
+ *
+ * fsstack requires inode to be first, and info to be second
+ */
struct ecryptfs_inode_info {
struct inode vfs_inode;
- struct inode *wii_inode;
+ struct fsstack_inode_info info;
struct ecryptfs_crypt_stat crypt_stat;
};

/* dentry private data. Each dentry must keep track of a lower
- * vfsmount too. */
+ * vfsmount too - fsstack requires info to be first */
struct ecryptfs_dentry_info {
- struct path lower_path;
+ struct fsstack_dentry_info info;
struct ecryptfs_crypt_stat *crypt_stat;
};

@@ -252,15 +256,15 @@ #define ECRYPTFS_PLAINTEXT_PASSTHROUGH_E
unsigned char global_auth_tok_sig[ECRYPTFS_SIG_SIZE_HEX + 1];
};

-/* superblock private data. */
+/* superblock private data - fsstack requires info to be first */
struct ecryptfs_sb_info {
- struct super_block *wsi_sb;
+ struct fsstack_sb_info info;
struct ecryptfs_mount_crypt_stat mount_crypt_stat;
};

-/* file private data. */
+/* file private data - fsstack requires info to be first */
struct ecryptfs_file_info {
- struct file *wfi_file;
+ struct fsstack_file_info info;
struct ecryptfs_crypt_stat *crypt_stat;
};

@@ -284,35 +288,12 @@ ecryptfs_set_file_private(struct file *f
file->private_data = file_info;
}

-static inline struct file *ecryptfs_file_to_lower(struct file *file)
-{
- return ((struct ecryptfs_file_info *)file->private_data)->wfi_file;
-}
-
-static inline void
-ecryptfs_set_file_lower(struct file *file, struct file *lower_file)
-{
- ((struct ecryptfs_file_info *)file->private_data)->wfi_file =
- lower_file;
-}
-
static inline struct ecryptfs_inode_info *
ecryptfs_inode_to_private(struct inode *inode)
{
return container_of(inode, struct ecryptfs_inode_info, vfs_inode);
}

-static inline struct inode *ecryptfs_inode_to_lower(struct inode *inode)
-{
- return ecryptfs_inode_to_private(inode)->wii_inode;
-}
-
-static inline void
-ecryptfs_set_inode_lower(struct inode *inode, struct inode *lower_inode)
-{
- ecryptfs_inode_to_private(inode)->wii_inode = lower_inode;
-}
-
static inline struct ecryptfs_sb_info *
ecryptfs_superblock_to_private(struct super_block *sb)
{
@@ -326,23 +307,10 @@ ecryptfs_set_superblock_private(struct s
sb->s_fs_info = sb_info;
}

-static inline struct super_block *
-ecryptfs_superblock_to_lower(struct super_block *sb)
-{
- return ((struct ecryptfs_sb_info *)sb->s_fs_info)->wsi_sb;
-}
-
-static inline void
-ecryptfs_set_superblock_lower(struct super_block *sb,
- struct super_block *lower_sb)
-{
- ((struct ecryptfs_sb_info *)sb->s_fs_info)->wsi_sb = lower_sb;
-}
-
static inline struct ecryptfs_dentry_info *
ecryptfs_dentry_to_private(struct dentry *dentry)
{
- return (struct ecryptfs_dentry_info *)dentry->d_fsdata;
+ return dentry->d_fsdata;
}

static inline void
@@ -352,32 +320,6 @@ ecryptfs_set_dentry_private(struct dentr
dentry->d_fsdata = dentry_info;
}

-static inline struct dentry *
-ecryptfs_dentry_to_lower(struct dentry *dentry)
-{
- return ((struct ecryptfs_dentry_info *)dentry->d_fsdata)->lower_path.dentry;
-}
-
-static inline void
-ecryptfs_set_dentry_lower(struct dentry *dentry, struct dentry *lower_dentry)
-{
- ((struct ecryptfs_dentry_info *)dentry->d_fsdata)->lower_path.dentry =
- lower_dentry;
-}
-
-static inline struct vfsmount *
-ecryptfs_dentry_to_lower_mnt(struct dentry *dentry)
-{
- return ((struct ecryptfs_dentry_info *)dentry->d_fsdata)->lower_path.mnt;
-}
-
-static inline void
-ecryptfs_set_dentry_lower_mnt(struct dentry *dentry, struct vfsmount *lower_mnt)
-{
- ((struct ecryptfs_dentry_info *)dentry->d_fsdata)->lower_path.mnt =
- lower_mnt;
-}
-
#define ecryptfs_printk(type, fmt, arg...) \
__ecryptfs_printk(type "%s: " fmt, __FUNCTION__, ## arg);
void __ecryptfs_printk(const char *fmt, ...);
diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
index 8064e90..06e322d 100644
--- a/fs/ecryptfs/file.c
+++ b/fs/ecryptfs/file.c
@@ -117,8 +117,8 @@ static ssize_t ecryptfs_read_update_atim
if (-EIOCBQUEUED == rc)
rc = wait_on_sync_kiocb(iocb);
if (rc >= 0) {
- lower_dentry = ecryptfs_dentry_to_lower(file->f_path.dentry);
- lower_vfsmount = ecryptfs_dentry_to_lower_mnt(file->f_path.dentry);
+ lower_dentry = fsstack_lower_dentry(file->f_path.dentry);
+ lower_vfsmount = fsstack_lower_mnt(file->f_path.dentry);
touch_atime(lower_vfsmount, lower_dentry);
}
return rc;
@@ -175,7 +175,7 @@ static int ecryptfs_readdir(struct file
struct inode *inode;
struct ecryptfs_getdents_callback buf;

- lower_file = ecryptfs_file_to_lower(file);
+ lower_file = fsstack_lower_file(file);
lower_file->f_pos = file->f_pos;
inode = file->f_path.dentry->d_inode;
memset(&buf, 0, sizeof(buf));
@@ -243,7 +243,7 @@ static int ecryptfs_open(struct inode *i
struct dentry *ecryptfs_dentry = file->f_path.dentry;
/* Private value of ecryptfs_dentry allocated in
* ecryptfs_lookup() */
- struct dentry *lower_dentry = ecryptfs_dentry_to_lower(ecryptfs_dentry);
+ struct dentry *lower_dentry = fsstack_lower_dentry(ecryptfs_dentry);
struct inode *lower_inode = NULL;
struct file *lower_file = NULL;
struct vfsmount *lower_mnt;
@@ -260,7 +260,7 @@ static int ecryptfs_open(struct inode *i
goto out;
}
memset(file_info, 0, sizeof(*file_info));
- lower_dentry = ecryptfs_dentry_to_lower(ecryptfs_dentry);
+ lower_dentry = fsstack_lower_dentry(ecryptfs_dentry);
crypt_stat = &ecryptfs_inode_to_private(inode)->crypt_stat;
mount_crypt_stat = &ecryptfs_superblock_to_private(
ecryptfs_dentry->d_sb)->mount_crypt_stat;
@@ -277,14 +277,14 @@ static int ecryptfs_open(struct inode *i
lower_flags = (lower_flags & O_ACCMODE) | O_RDWR;
if (file->f_flags & O_APPEND)
lower_flags &= ~O_APPEND;
- lower_mnt = ecryptfs_dentry_to_lower_mnt(ecryptfs_dentry);
+ lower_mnt = fsstack_lower_mnt(ecryptfs_dentry);
/* Corresponding fput() in ecryptfs_release() */
if ((rc = ecryptfs_open_lower_file(&lower_file, lower_dentry, lower_mnt,
lower_flags))) {
ecryptfs_printk(KERN_ERR, "Error opening lower file\n");
goto out_puts;
}
- ecryptfs_set_file_lower(file, lower_file);
+ fsstack_set_lower_file(file, lower_file);
/* Isn't this check the same as the one in lookup? */
lower_inode = lower_dentry->d_inode;
if (S_ISDIR(ecryptfs_dentry->d_inode->i_mode)) {
@@ -338,7 +338,7 @@ static int ecryptfs_open(struct inode *i
ecryptfs_printk(KERN_DEBUG, "inode w/ addr = [0x%p], i_ino = [0x%.16x] "
"size: [0x%.16x]\n", inode, inode->i_ino,
i_size_read(inode));
- ecryptfs_set_file_lower(file, lower_file);
+ fsstack_set_lower_file(file, lower_file);
goto out;
out_puts:
mntput(lower_mnt);
@@ -354,7 +354,7 @@ static int ecryptfs_flush(struct file *f
int rc = 0;
struct file *lower_file = NULL;

- lower_file = ecryptfs_file_to_lower(file);
+ lower_file = fsstack_lower_file(file);
if (lower_file->f_op && lower_file->f_op->flush)
rc = lower_file->f_op->flush(lower_file, td);
return rc;
@@ -362,9 +362,9 @@ static int ecryptfs_flush(struct file *f

static int ecryptfs_release(struct inode *inode, struct file *file)
{
- struct file *lower_file = ecryptfs_file_to_lower(file);
+ struct file *lower_file = fsstack_lower_file(file);
struct ecryptfs_file_info *file_info = ecryptfs_file_to_private(file);
- struct inode *lower_inode = ecryptfs_inode_to_lower(inode);
+ struct inode *lower_inode = fsstack_lower_inode(inode);
int rc;

if ((rc = ecryptfs_close_lower_file(lower_file))) {
@@ -380,8 +380,8 @@ out:
static int
ecryptfs_fsync(struct file *file, struct dentry *dentry, int datasync)
{
- struct file *lower_file = ecryptfs_file_to_lower(file);
- struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ struct file *lower_file = fsstack_lower_file(file);
+ struct dentry *lower_dentry = fsstack_lower_dentry(dentry);
struct inode *lower_inode = lower_dentry->d_inode;
int rc = -EINVAL;

@@ -399,7 +399,7 @@ static int ecryptfs_fasync(int fd, struc
int rc = 0;
struct file *lower_file = NULL;

- lower_file = ecryptfs_file_to_lower(file);
+ lower_file = fsstack_lower_file(file);
if (lower_file->f_op && lower_file->f_op->fasync)
rc = lower_file->f_op->fasync(fd, lower_file, flag);
return rc;
@@ -411,7 +411,7 @@ static ssize_t ecryptfs_sendfile(struct
struct file *lower_file = NULL;
int rc = -EINVAL;

- lower_file = ecryptfs_file_to_lower(file);
+ lower_file = fsstack_lower_file(file);
if (lower_file->f_op && lower_file->f_op->sendfile)
rc = lower_file->f_op->sendfile(lower_file, ppos, count,
actor, target);
@@ -459,9 +459,9 @@ ecryptfs_ioctl(struct inode *inode, stru
struct file *lower_file = NULL;

if (ecryptfs_file_to_private(file))
- lower_file = ecryptfs_file_to_lower(file);
+ lower_file = fsstack_lower_file(file);
if (lower_file && lower_file->f_op && lower_file->f_op->ioctl)
- rc = lower_file->f_op->ioctl(ecryptfs_inode_to_lower(inode),
+ rc = lower_file->f_op->ioctl(fsstack_lower_inode(inode),
lower_file, cmd, arg);
else
rc = -ENOTTY;
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index cfcfcbf..c5c21e8 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -71,8 +71,8 @@ ecryptfs_create_underlying_file(struct i
struct dentry *dentry, int mode,
struct nameidata *nd)
{
- struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
- struct vfsmount *lower_mnt = ecryptfs_dentry_to_lower_mnt(dentry);
+ struct dentry *lower_dentry = fsstack_lower_dentry(dentry);
+ struct vfsmount *lower_mnt = fsstack_lower_mnt(dentry);
struct dentry *dentry_save;
struct vfsmount *vfsmount_save;
int rc;
@@ -109,7 +109,7 @@ ecryptfs_do_create(struct inode *directo
struct dentry *lower_dentry;
struct dentry *lower_dir_dentry;

- lower_dentry = ecryptfs_dentry_to_lower(ecryptfs_dentry);
+ lower_dentry = fsstack_lower_dentry(ecryptfs_dentry);
lower_dir_dentry = lock_parent(lower_dentry);
if (unlikely(IS_ERR(lower_dir_dentry))) {
ecryptfs_printk(KERN_ERR, "Error locking directory of "
@@ -158,7 +158,7 @@ static int grow_file(struct dentry *ecry
fake_file.f_path.dentry = ecryptfs_dentry;
memset(&tmp_file_info, 0, sizeof(tmp_file_info));
ecryptfs_set_file_private(&fake_file, &tmp_file_info);
- ecryptfs_set_file_lower(&fake_file, lower_file);
+ fsstack_set_lower_file(&fake_file, lower_file);
rc = ecryptfs_fill_zeros(&fake_file, 1);
if (rc) {
ECRYPTFS_SET_FLAG(
@@ -194,7 +194,7 @@ static int ecryptfs_initialize_file(stru
struct inode *inode, *lower_inode;
struct vfsmount *lower_mnt;

- lower_dentry = ecryptfs_dentry_to_lower(ecryptfs_dentry);
+ lower_dentry = fsstack_lower_dentry(ecryptfs_dentry);
ecryptfs_printk(KERN_DEBUG, "lower_dentry->d_name.name = [%s]\n",
lower_dentry->d_name.name);
inode = ecryptfs_dentry->d_inode;
@@ -203,7 +203,7 @@ static int ecryptfs_initialize_file(stru
#if BITS_PER_LONG != 32
lower_flags |= O_LARGEFILE;
#endif
- lower_mnt = ecryptfs_dentry_to_lower_mnt(ecryptfs_dentry);
+ lower_mnt = fsstack_lower_mnt(ecryptfs_dentry);
/* Corresponding fput() at end of this function */
if ((rc = ecryptfs_open_lower_file(&lower_file, lower_dentry, lower_mnt,
lower_flags))) {
@@ -291,7 +291,7 @@ static struct dentry *ecryptfs_lookup(st
struct inode *lower_inode;
u64 file_size;

- lower_dir_dentry = ecryptfs_dentry_to_lower(dentry->d_parent);
+ lower_dir_dentry = fsstack_lower_dentry(dentry->d_parent);
dentry->d_op = &ecryptfs_dops;
if ((dentry->d_name.len == 1 && !strcmp(dentry->d_name.name, "."))
|| (dentry->d_name.len == 2
@@ -335,8 +335,8 @@ static struct dentry *ecryptfs_lookup(st
"to allocate ecryptfs_dentry_info struct\n");
goto out_dput;
}
- ecryptfs_set_dentry_lower(dentry, lower_dentry);
- ecryptfs_set_dentry_lower_mnt(dentry, lower_mnt);
+ fsstack_set_lower_dentry(dentry, lower_dentry);
+ fsstack_set_lower_mnt(dentry, lower_mnt);
if (!lower_dentry->d_inode) {
/* We want to add because we couldn't find in lower */
d_add(dentry, NULL);
@@ -409,8 +409,8 @@ static int ecryptfs_link(struct dentry *
int rc;

file_size_save = i_size_read(old_dentry->d_inode);
- lower_old_dentry = ecryptfs_dentry_to_lower(old_dentry);
- lower_new_dentry = ecryptfs_dentry_to_lower(new_dentry);
+ lower_old_dentry = fsstack_lower_dentry(old_dentry);
+ lower_new_dentry = fsstack_lower_dentry(new_dentry);
dget(lower_old_dentry);
dget(lower_new_dentry);
lower_dir_dentry = lock_parent(lower_new_dentry);
@@ -424,7 +424,7 @@ static int ecryptfs_link(struct dentry *
fsstack_copy_attr_times(dir, lower_new_dentry->d_inode);
fsstack_copy_inode_size(dir, lower_new_dentry->d_inode);
old_dentry->d_inode->i_nlink =
- ecryptfs_inode_to_lower(old_dentry->d_inode)->i_nlink;
+ fsstack_lower_inode(old_dentry->d_inode)->i_nlink;
i_size_write(new_dentry->d_inode, file_size_save);
out_lock:
unlock_dir(lower_dir_dentry);
@@ -438,8 +438,8 @@ out_lock:
static int ecryptfs_unlink(struct inode *dir, struct dentry *dentry)
{
int rc = 0;
- struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
- struct inode *lower_dir_inode = ecryptfs_inode_to_lower(dir);
+ struct dentry *lower_dentry = fsstack_lower_dentry(dentry);
+ struct inode *lower_dir_inode = fsstack_lower_inode(dir);

lock_parent(lower_dentry);
rc = vfs_unlink(lower_dir_inode, lower_dentry);
@@ -449,7 +449,7 @@ static int ecryptfs_unlink(struct inode
}
fsstack_copy_attr_times(dir, lower_dir_inode);
dentry->d_inode->i_nlink =
- ecryptfs_inode_to_lower(dentry->d_inode)->i_nlink;
+ fsstack_lower_inode(dentry->d_inode)->i_nlink;
dentry->d_inode->i_ctime = dir->i_ctime;
out_unlock:
unlock_parent(lower_dentry);
@@ -467,7 +467,7 @@ static int ecryptfs_symlink(struct inode
unsigned int encoded_symlen;
struct ecryptfs_crypt_stat *crypt_stat = NULL;

- lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ lower_dentry = fsstack_lower_dentry(dentry);
dget(lower_dentry);
lower_dir_dentry = lock_parent(lower_dentry);
mode = S_IALLUGO;
@@ -502,7 +502,7 @@ static int ecryptfs_mkdir(struct inode *
struct dentry *lower_dentry;
struct dentry *lower_dir_dentry;

- lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ lower_dentry = fsstack_lower_dentry(dentry);
lower_dir_dentry = lock_parent(lower_dentry);
rc = vfs_mkdir(lower_dir_dentry->d_inode, lower_dentry, mode);
if (rc || !lower_dentry->d_inode)
@@ -526,7 +526,7 @@ static int ecryptfs_rmdir(struct inode *
struct dentry *lower_dir_dentry;
int rc;

- lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ lower_dentry = fsstack_lower_dentry(dentry);
dget(dentry);
lower_dir_dentry = lock_parent(lower_dentry);
dget(lower_dentry);
@@ -550,7 +550,7 @@ ecryptfs_mknod(struct inode *dir, struct
struct dentry *lower_dentry;
struct dentry *lower_dir_dentry;

- lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ lower_dentry = fsstack_lower_dentry(dentry);
lower_dir_dentry = lock_parent(lower_dentry);
rc = vfs_mknod(lower_dir_dentry->d_inode, lower_dentry, mode, dev);
if (rc || !lower_dentry->d_inode)
@@ -577,8 +577,8 @@ ecryptfs_rename(struct inode *old_dir, s
struct dentry *lower_old_dir_dentry;
struct dentry *lower_new_dir_dentry;

- lower_old_dentry = ecryptfs_dentry_to_lower(old_dentry);
- lower_new_dentry = ecryptfs_dentry_to_lower(new_dentry);
+ lower_old_dentry = fsstack_lower_dentry(old_dentry);
+ lower_new_dentry = fsstack_lower_dentry(new_dentry);
dget(lower_old_dentry);
dget(lower_new_dentry);
lower_old_dir_dentry = dget_parent(lower_old_dentry);
@@ -608,7 +608,7 @@ ecryptfs_readlink(struct dentry *dentry,
mm_segment_t old_fs;
struct ecryptfs_crypt_stat *crypt_stat;

- lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ lower_dentry = fsstack_lower_dentry(dentry);
if (!lower_dentry->d_inode->i_op ||
!lower_dentry->d_inode->i_op->readlink) {
rc = -EINVAL;
@@ -760,16 +760,16 @@ int ecryptfs_truncate(struct dentry *den
rc = -ENOMEM;
goto out;
}
- lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ lower_dentry = fsstack_lower_dentry(dentry);
/* This dget & mntget is released through fput at out_fput: */
- lower_mnt = ecryptfs_dentry_to_lower_mnt(dentry);
+ lower_mnt = fsstack_lower_mnt(dentry);
if ((rc = ecryptfs_open_lower_file(&lower_file, lower_dentry, lower_mnt,
O_RDWR))) {
ecryptfs_printk(KERN_ERR,
"Error opening dentry; rc = [%i]\n", rc);
goto out_free;
}
- ecryptfs_set_file_lower(&fake_ecryptfs_file, lower_file);
+ fsstack_set_lower_file(&fake_ecryptfs_file, lower_file);
/* Switch on growing or shrinking file */
if (new_length > i_size) {
rc = ecryptfs_fill_zeros(&fake_ecryptfs_file, new_length);
@@ -827,13 +827,13 @@ ecryptfs_permission(struct inode *inode,
struct vfsmount *vfsmnt_save = nd->mnt;
struct dentry *dentry_save = nd->dentry;

- nd->mnt = ecryptfs_dentry_to_lower_mnt(nd->dentry);
- nd->dentry = ecryptfs_dentry_to_lower(nd->dentry);
- rc = permission(ecryptfs_inode_to_lower(inode), mask, nd);
+ nd->mnt = fsstack_lower_mnt(nd->dentry);
+ nd->dentry = fsstack_lower_dentry(nd->dentry);
+ rc = permission(fsstack_lower_inode(inode), mask, nd);
nd->mnt = vfsmnt_save;
nd->dentry = dentry_save;
} else
- rc = permission(ecryptfs_inode_to_lower(inode), mask, NULL);
+ rc = permission(fsstack_lower_inode(inode), mask, NULL);
return rc;
}

@@ -858,9 +858,9 @@ static int ecryptfs_setattr(struct dentr
struct ecryptfs_crypt_stat *crypt_stat;

crypt_stat = &ecryptfs_inode_to_private(dentry->d_inode)->crypt_stat;
- lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ lower_dentry = fsstack_lower_dentry(dentry);
inode = dentry->d_inode;
- lower_inode = ecryptfs_inode_to_lower(inode);
+ lower_inode = fsstack_lower_inode(inode);
if (ia->ia_valid & ATTR_SIZE) {
ecryptfs_printk(KERN_DEBUG,
"ia->ia_valid = [0x%x] ATTR_SIZE" " = [0x%x]\n",
@@ -886,7 +886,7 @@ ecryptfs_setxattr(struct dentry *dentry,
int rc = 0;
struct dentry *lower_dentry;

- lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ lower_dentry = fsstack_lower_dentry(dentry);
if (!lower_dentry->d_inode->i_op->setxattr) {
rc = -ENOSYS;
goto out;
@@ -906,7 +906,7 @@ ecryptfs_getxattr(struct dentry *dentry,
int rc = 0;
struct dentry *lower_dentry;

- lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ lower_dentry = fsstack_lower_dentry(dentry);
if (!lower_dentry->d_inode->i_op->getxattr) {
rc = -ENOSYS;
goto out;
@@ -925,7 +925,7 @@ ecryptfs_listxattr(struct dentry *dentry
int rc = 0;
struct dentry *lower_dentry;

- lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ lower_dentry = fsstack_lower_dentry(dentry);
if (!lower_dentry->d_inode->i_op->listxattr) {
rc = -ENOSYS;
goto out;
@@ -942,7 +942,7 @@ static int ecryptfs_removexattr(struct d
int rc = 0;
struct dentry *lower_dentry;

- lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ lower_dentry = fsstack_lower_dentry(dentry);
if (!lower_dentry->d_inode->i_op->removexattr) {
rc = -ENOSYS;
goto out;
@@ -956,7 +956,7 @@ out:

int ecryptfs_inode_test(struct inode *inode, void *candidate_lower_inode)
{
- if ((ecryptfs_inode_to_lower(inode)
+ if ((fsstack_lower_inode(inode)
== (struct inode *)candidate_lower_inode))
return 1;
else
diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index a4aee57..06f5479 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -79,7 +79,7 @@ int ecryptfs_interpose(struct dentry *lo
int rc = 0;

lower_inode = lower_dentry->d_inode;
- if (lower_inode->i_sb != ecryptfs_superblock_to_lower(sb)) {
+ if (lower_inode->i_sb != fsstack_lower_sb(sb)) {
rc = -EXDEV;
goto out;
}
@@ -449,10 +449,10 @@ static int ecryptfs_read_super(struct su
goto out_free;
}
lower_mnt = nd.mnt;
- ecryptfs_set_superblock_lower(sb, lower_root->d_sb);
+ fsstack_set_lower_sb(sb, lower_root->d_sb);
sb->s_maxbytes = lower_root->d_sb->s_maxbytes;
- ecryptfs_set_dentry_lower(sb->s_root, lower_root);
- ecryptfs_set_dentry_lower_mnt(sb->s_root, lower_mnt);
+ fsstack_set_lower_dentry(sb->s_root, lower_root);
+ fsstack_set_lower_mnt(sb->s_root, lower_mnt);
if ((rc = ecryptfs_interpose(lower_root, sb->s_root, sb, 0)))
goto out_free;
rc = 0;
diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
index 3001189..419b907 100644
--- a/fs/ecryptfs/mmap.c
+++ b/fs/ecryptfs/mmap.c
@@ -219,10 +219,10 @@ int ecryptfs_do_readpage(struct file *fi
const struct address_space_operations *lower_a_ops;

dentry = file->f_path.dentry;
- lower_file = ecryptfs_file_to_lower(file);
- lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ lower_file = fsstack_lower_file(file);
+ lower_dentry = fsstack_lower_dentry(dentry);
inode = dentry->d_inode;
- lower_inode = ecryptfs_inode_to_lower(inode);
+ lower_inode = fsstack_lower_inode(inode);
lower_a_ops = lower_inode->i_mapping->a_ops;
lower_page = read_cache_page(lower_inode->i_mapping, lower_page_index,
(filler_t *)lower_a_ops->readpage,
@@ -542,8 +542,8 @@ process_new_file(struct ecryptfs_crypt_s
int header_pages;
int more_header_data_to_be_written = 1;

- lower_inode = ecryptfs_inode_to_lower(inode);
- lower_file = ecryptfs_file_to_lower(file);
+ lower_inode = fsstack_lower_inode(inode);
+ lower_file = fsstack_lower_file(file);
lower_a_ops = lower_inode->i_mapping->a_ops;
header_pages = ((crypt_stat->header_extent_size
* crypt_stat->num_header_extents_at_front)
@@ -635,8 +635,8 @@ static int ecryptfs_commit_write(struct
int rc;

inode = page->mapping->host;
- lower_inode = ecryptfs_inode_to_lower(inode);
- lower_file = ecryptfs_file_to_lower(file);
+ lower_inode = fsstack_lower_inode(inode);
+ lower_file = fsstack_lower_file(file);
mutex_lock(&lower_inode->i_mutex);
crypt_stat = &ecryptfs_inode_to_private(file->f_path.dentry->d_inode)
->crypt_stat;
@@ -749,7 +749,7 @@ static sector_t ecryptfs_bmap(struct add
struct inode *lower_inode;

inode = (struct inode *)mapping->host;
- lower_inode = ecryptfs_inode_to_lower(inode);
+ lower_inode = fsstack_lower_inode(inode);
if (lower_inode->i_mapping->a_ops->bmap)
rc = lower_inode->i_mapping->a_ops->bmap(lower_inode->i_mapping,
block);
@@ -763,7 +763,7 @@ static void ecryptfs_sync_page(struct pa
struct page *lower_page;

inode = page->mapping->host;
- lower_inode = ecryptfs_inode_to_lower(inode);
+ lower_inode = fsstack_lower_inode(inode);
/* NOTE: Recently swapped with grab_cache_page(), since
* sync_page() just makes sure that pending I/O gets done. */
lower_page = find_lock_page(lower_inode->i_mapping, page->index);
diff --git a/fs/ecryptfs/super.c b/fs/ecryptfs/super.c
index 825757a..904283a 100644
--- a/fs/ecryptfs/super.c
+++ b/fs/ecryptfs/super.c
@@ -85,7 +85,7 @@ static void ecryptfs_destroy_inode(struc
*/
void ecryptfs_init_inode(struct inode *inode, struct inode *lower_inode)
{
- ecryptfs_set_inode_lower(inode, lower_inode);
+ fsstack_set_lower_inode(inode, lower_inode);
inode->i_ino = lower_inode->i_ino;
inode->i_version++;
inode->i_op = &ecryptfs_main_iops;
@@ -119,7 +119,7 @@ static void ecryptfs_put_super(struct su
*/
static int ecryptfs_statfs(struct dentry *dentry, struct kstatfs *buf)
{
- return vfs_statfs(ecryptfs_dentry_to_lower(dentry), buf);
+ return vfs_statfs(fsstack_lower_dentry(dentry), buf);
}

/**
@@ -134,7 +134,7 @@ static int ecryptfs_statfs(struct dentry
*/
static void ecryptfs_clear_inode(struct inode *inode)
{
- iput(ecryptfs_inode_to_lower(inode));
+ iput(fsstack_lower_inode(inode));
}

/**
@@ -146,8 +146,8 @@ static void ecryptfs_clear_inode(struct
static int ecryptfs_show_options(struct seq_file *m, struct vfsmount *mnt)
{
struct super_block *sb = mnt->mnt_sb;
- struct dentry *lower_root_dentry = ecryptfs_dentry_to_lower(sb->s_root);
- struct vfsmount *lower_mnt = ecryptfs_dentry_to_lower_mnt(sb->s_root);
+ struct dentry *lower_root_dentry = fsstack_lower_dentry(sb->s_root);
+ struct vfsmount *lower_mnt = fsstack_lower_mnt(sb->s_root);
char *tmp_page;
char *path;
int rc = 0;

2006-11-02 16:06:47

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH 2/3] fsstack: Generic get/set lower object functions

On Wed, 2006-11-01 at 22:59 -0500, Josef Jeff Sipek wrote:
> From: Josef "Jeff" Sipek <[email protected]>
>
> Every stackable filesystem needs to track what the corresponding lower
> objects are. The stackable fs superblock needs to maintain pointers to the
> lower superblock(s); the inodes need to maintain pointers to the lower
> inodes; dentries need to maintain pointers to the lower dentries and
> vfsmounts.
>
> Currently, every stackable filesystem maintains this information in the
> private data of the object in question (the inode pointers may be also
> stored in the inode's container - which is what the following patch
> requires.)
>
> This patch introduces generic structures to maintain the lower object
> references, and functions to get/set the lower object structure members.
> These functions are the generalized forms, which work with both linear and
> fanout stackable filesystem (eCryptfs and Unionfs to name some).
>
> The patch is loosely based on Pekka Enberg's patches from October 13th
> (http://lkml.org/lkml/2006/10/13/82).
>
> Cc: Pekka Enberg <[email protected]>
> Cc: Michael Halcrow <[email protected]>
> Cc: Erez Zadok <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: Josef "Jeff" Sipek <[email protected]>
> ---
>
> include/linux/fs_stack.h | 264 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 264 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/fs_stack.h b/include/linux/fs_stack.h
> index bb516ce..fd4872e 100644
> --- a/include/linux/fs_stack.h
> +++ b/include/linux/fs_stack.h
> @@ -5,8 +5,272 @@ #define _LINUX_FS_STACK_H
> * filesystems; none of these functions require i_mutex to be held.
> */
>
> +#include <linux/namei.h>
> #include <linux/fs.h>
>
> +/* structs to maintain pointers to the lower VFS objects */
> +struct fsstack_sb_info {
> + union {
> + struct super_block *sb;
> + struct super_block **sbs;
> + };
> +};

Why are you defining all these structs that are just wrapping unions?

> +
> +struct fsstack_inode_info {
> + union {
> + struct inode *inode;
> + struct inode **inodes;
> + };
> +};
> +
> +struct fsstack_dentry_info {
> + union {
> + struct path path;
> + struct path *paths;
> + };
> +};
> +
> +struct fsstack_file_info {
> + union {
> + struct file *file;
> + struct file **files;
> + };
> +};
> +
> +/* DO NOT USE!
> + *
> + * The following structure is used during the container_of calls to allow
> + * for as generic as possible way of accessing fsstack_inode_info given a
> + * pointer to the inode.
> + */
> +struct __fsstack_inode_generic_info {
> + struct inode vfs_inode; /* vfs inode */
> + struct fsstack_inode_info info; /* fsstack inode info */
> +};
> +
> +/*
> + * Functions to get lower objects from an upper one.
> + *
> + * NOTE: The filesystem specific info structures (for dentries, superblocks
> + * and files) _must_ have the following layout:
> + *
> + * struct foo {
> + * struct fsstack_{dentry,sb,file}_info info;
> + * ...
> + * };
> + *
> + * Because of the usage of containers, the inode container structure _must_
> + * have the following layout:
> + *
> + * struct bar {
> + * struct inode vfs_inode;
> + * struct fsstack_inode_info info;
> + * ...
> + * };
> + */
> +static inline struct super_block *
> +__fsstack_lower_sb(struct super_block *sb, unsigned long branch_idx)
> +{
> + struct fsstack_sb_info *info = sb->s_fs_info;
> + return info->sbs[branch_idx];
> +}
> +
> +static inline struct super_block **
> +__fsstack_lower_sbs(struct super_block *sb)
> +{
> + struct fsstack_sb_info *info = sb->s_fs_info;
> + return info->sbs;
> +}
> +
> +static inline void
> +__fsstack_set_lower_sb(struct super_block *sb, unsigned long branch_idx,
> + struct super_block *lower_sb)
> +{
> + struct fsstack_sb_info *info = sb->s_fs_info;
> + info->sbs[branch_idx] = lower_sb;
> +}
> +
> +static inline void
> +__fsstack_set_lower_sbs(struct super_block *sb, struct super_block **lower_sbs)
> +{
> + struct fsstack_sb_info *info = sb->s_fs_info;
> + info->sbs = lower_sbs;
> +}
> +
> +static inline struct super_block *fsstack_lower_sb(struct super_block *sb)
> +{
> + struct fsstack_sb_info *info = sb->s_fs_info;
> + return info->sb;
> +}
> +
> +static inline void
> +fsstack_set_lower_sb(struct super_block *sb, struct super_block *lower_sb)
> +{
> + struct fsstack_sb_info *info = sb->s_fs_info;
> + info->sb = lower_sb;
> +}
> +
> +/* get the fs dependent data */
> +static inline void * fsstack_inode_data(struct inode *inode)
> +{
> + return &((struct __fsstack_inode_generic_info*) inode)->info;

Please make this wrap container_of() instead of rolling your own.
Also note that the naming convention for such a wrapper in almost all
other filesystems would be FSSTACK_I()

> +}
> +
> +static inline struct inode *
> +__fsstack_lower_inode(struct inode *inode, unsigned long branch_idx)
> +{
> + struct fsstack_inode_info *info = fsstack_inode_data(inode);
> +
> + return info->inodes[branch_idx];
> +}

What is the value of "functions" like the above? They appear just to
obfuscate the code. Unless your aim is to hide the internals of the
struct __fsstack_inode_generic_info (sort of futile, since you are
asking users to include that structure in their private inode structs)
then it is much more obvious to see what is going on when you write

inode = FSSTACK_I(inode)->inodes[branch];

rather than

inode = __fsstack_lower_inode(inode, branch);

> +
> +static inline struct inode **
> +__fsstack_lower_inodes(struct inode *inode)
> +{
> + struct fsstack_inode_info *info = fsstack_inode_data(inode);
> + return info->inodes;
> +}
> +
> +static inline void
> +__fsstack_set_lower_inode(struct inode *inode, unsigned long branch_idx,
> + struct inode *lower_inode)
> +{
> + struct fsstack_inode_info *info = fsstack_inode_data(inode);
> + info->inodes[branch_idx] = lower_inode;
> +}
> +
> +static inline void
> +__fsstack_set_lower_inodes(struct inode *inode, struct inode **lower_inodes)
> +{
> + struct fsstack_inode_info *info = fsstack_inode_data(inode);
> + info->inodes = lower_inodes;
> +}
> +
> +static inline struct inode *fsstack_lower_inode(struct inode *inode)
> +{
> + struct fsstack_inode_info *info = fsstack_inode_data(inode);
> + return info->inode;
> +}
> +
> +static inline void
> +fsstack_set_lower_inode(struct inode *inode, struct inode *lower_inode)
> +{
> + struct fsstack_inode_info *info = fsstack_inode_data(inode);
> + info->inode = lower_inode;
> +}
> +
> +static inline struct dentry *
> +__fsstack_lower_dentry(struct dentry *dentry, unsigned long branch_idx)
> +{
> + struct fsstack_dentry_info *info = dentry->d_fsdata;
> + return info->paths[branch_idx].dentry;
> +}
> +
> +static inline struct path *
> +__fsstack_lower_paths(struct dentry *dentry)
> +{
> + struct fsstack_dentry_info *info = dentry->d_fsdata;
> + return info->paths;
> +}
> +
> +static inline void
> +__fsstack_set_lower_dentry(struct dentry *dentry, unsigned long branch_idx,
> + struct dentry *lower_dentry)
> +{
> + struct fsstack_dentry_info *info = dentry->d_fsdata;
> + info->paths[branch_idx].dentry = lower_dentry;
> +}
> +
> +static inline void
> +__fsstack_set_lower_paths(struct dentry *dentry, struct path *lower_paths)
> +{
> + struct fsstack_dentry_info *info = dentry->d_fsdata;
> + info->paths = lower_paths;
> +}
> +
> +static inline struct dentry *fsstack_lower_dentry(struct dentry *dentry)
> +{
> + struct fsstack_dentry_info *info = dentry->d_fsdata;
> + return info->path.dentry;
> +}
> +
> +static inline void
> +fsstack_set_lower_dentry(struct dentry *dentry, struct dentry *lower_dentry)
> +{
> + struct fsstack_dentry_info *info = dentry->d_fsdata;
> + info->path.dentry = lower_dentry;
> +}
> +
> +static inline struct vfsmount *
> +__fsstack_lower_mnt(struct dentry *dentry, unsigned long branch_idx)
> +{
> + struct fsstack_dentry_info *info = dentry->d_fsdata;
> + return info->paths[branch_idx].mnt;
> +}
> +
> +static inline void
> +__fsstack_set_lower_mnt(struct dentry *dentry, unsigned long branch_idx,
> + struct vfsmount *lower_mnt)
> +{
> + struct fsstack_dentry_info *info = dentry->d_fsdata;
> + info->paths[branch_idx].mnt = lower_mnt;
> +}
> +
> +static inline struct vfsmount *fsstack_lower_mnt(struct dentry *dentry)
> +{
> + struct fsstack_dentry_info *info = dentry->d_fsdata;
> + return info->path.mnt;
> +}
> +
> +static inline void
> +fsstack_set_lower_mnt(struct dentry *dentry, struct vfsmount *lower_mnt)
> +{
> + struct fsstack_dentry_info *info = dentry->d_fsdata;
> + info->path.mnt = lower_mnt;
> +}
> +
> +static inline struct file *
> +__fsstack_lower_file(struct file *file, unsigned long branch_idx)
> +{
> + struct fsstack_file_info *info = file->private_data;
> + return info->files[branch_idx];
> +}
> +
> +static inline struct file **
> +__fsstack_lower_files(struct file *file)
> +{
> + struct fsstack_file_info *info = file->private_data;
> + return info->files;
> +}
> +
> +static inline void
> +__fsstack_set_lower_file(struct file *file, unsigned long branch_idx,
> + struct file *lower_file)
> +{
> + struct fsstack_file_info *info = file->private_data;
> + info->files[branch_idx] = lower_file;
> +}
> +
> +static inline void
> +__fsstack_set_lower_files(struct file *file, struct file **lower_files)
> +{
> + struct fsstack_file_info *info = file->private_data;
> + info->files = lower_files;
> +}
> +
> +static inline struct file *fsstack_lower_file(struct file *file)
> +{
> + struct fsstack_file_info *info = file->private_data;
> + return info->file;
> +}
> +
> +static inline void
> +fsstack_set_lower_file(struct file *file, struct file *lower_file)
> +{
> + struct fsstack_file_info *info = file->private_data;
> + info->file = lower_file;
> +}
> +
> /* externs for fs/stack.c */
> extern void fsstack_copy_attr_all(struct inode *dest, const struct inode *src,
> int (*get_nlinks)(struct inode *));
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2006-11-03 03:27:46

by Josef Sipek

[permalink] [raw]
Subject: Re: [PATCH 2/3] fsstack: Generic get/set lower object functions

On Thu, Nov 02, 2006 at 11:06:05AM -0500, Trond Myklebust wrote:
> On Wed, 2006-11-01 at 22:59 -0500, Josef Jeff Sipek wrote:
...
> > +/* structs to maintain pointers to the lower VFS objects */
> > +struct fsstack_sb_info {
> > + union {
> > + struct super_block *sb;
> > + struct super_block **sbs;
> > + };
> > +};
>
> Why are you defining all these structs that are just wrapping unions?

The reason for the union is simple...

1) if you have a linear stackable filesystem (e.g., ecryptfs), your objects
need to point to only one lower object (sb -> lower sb, etc.)

2) if you have a fanout stackable filesystem (e.g., unionfs), your objects
need to point to n lower objects

Since we don't want to hurt linear stacks by declaring arrays of pointers, I
think the best way is to have the lower pointer (e.g., *sb) in a union with
the lower double pointer (e.g., **sbs) - this works simply because linear
stacks will always

> > +/* get the fs dependent data */
> > +static inline void * fsstack_inode_data(struct inode *inode)
> > +{
> > + return &((struct __fsstack_inode_generic_info*) inode)->info;
>
> Please make this wrap container_of() instead of rolling your own.

Will do.

> Also note that the naming convention for such a wrapper in almost all
> other filesystems would be FSSTACK_I()

Very true, I'll change it to match.

I suppose the function to get the dentry private data should be called
FSSTACK_D() and for the superblock FSSTACK_SB() ?

> > +static inline struct inode *
> > +__fsstack_lower_inode(struct inode *inode, unsigned long branch_idx)
> > +{
> > + struct fsstack_inode_info *info = fsstack_inode_data(inode);
> > +
> > + return info->inodes[branch_idx];
> > +}
>
> What is the value of "functions" like the above? They appear just to
> obfuscate the code. Unless your aim is to hide the internals of the
> struct __fsstack_inode_generic_info (sort of futile, since you are
> asking users to include that structure in their private inode structs

Another idea is to move the fsstack_foo_info structure elsewhere...

For stackable filesystems it makes sense to have the pointers right in the
inode, but we don't want to penalize the rest if the filesystems. One
solution would be to have a special stackable_inode which contains the lower
inode pointers. This would still hide the details from the user...

> )
> then it is much more obvious to see what is going on when you write
>
> inode = FSSTACK_I(inode)->inodes[branch];
>
> rather than
>
> inode = __fsstack_lower_inode(inode, branch);

Point taken. You need to know what's going to anyway, so we might as well
make it painfully obvious.

Josef "Jeff" Sipek.

--
If I have trouble installing Linux, something is wrong. Very wrong.
- Linus Torvalds

2006-11-03 03:45:23

by Mark Williamson

[permalink] [raw]
Subject: Re: [PATCH 2/3] fsstack: Generic get/set lower object functions

> > Why are you defining all these structs that are just wrapping unions?
>
> The reason for the union is simple...

I think the query was more about why you'd need a struct which contains only
an anonymous union - why not just have a named union? Or do you anticipate
adding extra fields to the struct later?

I guess that having a union foo * rather than a struct foo * would be a bit
unconventional in the kernel. The named struct / anonymous union combo does
hide the union as merely an implementation detail, which is nice. Was this
your motivation?

Cheers,
Mark

> 1) if you have a linear stackable filesystem (e.g., ecryptfs), your objects
> need to point to only one lower object (sb -> lower sb, etc.)
>
> 2) if you have a fanout stackable filesystem (e.g., unionfs), your objects
> need to point to n lower objects
>
> Since we don't want to hurt linear stacks by declaring arrays of pointers,
> I think the best way is to have the lower pointer (e.g., *sb) in a union
> with the lower double pointer (e.g., **sbs) - this works simply because
> linear stacks will always
>
> > > +/* get the fs dependent data */
> > > +static inline void * fsstack_inode_data(struct inode *inode)
> > > +{
> > > + return &((struct __fsstack_inode_generic_info*) inode)->info;
> >
> > Please make this wrap container_of() instead of rolling your own.
>
> Will do.
>
> > Also note that the naming convention for such a wrapper in almost all
> > other filesystems would be FSSTACK_I()
>
> Very true, I'll change it to match.
>
> I suppose the function to get the dentry private data should be called
> FSSTACK_D() and for the superblock FSSTACK_SB() ?
>
> > > +static inline struct inode *
> > > +__fsstack_lower_inode(struct inode *inode, unsigned long branch_idx)
> > > +{
> > > + struct fsstack_inode_info *info = fsstack_inode_data(inode);
> > > +
> > > + return info->inodes[branch_idx];
> > > +}
> >
> > What is the value of "functions" like the above? They appear just to
> > obfuscate the code. Unless your aim is to hide the internals of the
> > struct __fsstack_inode_generic_info (sort of futile, since you are
> > asking users to include that structure in their private inode structs
>
> Another idea is to move the fsstack_foo_info structure elsewhere...
>
> For stackable filesystems it makes sense to have the pointers right in the
> inode, but we don't want to penalize the rest if the filesystems. One
> solution would be to have a special stackable_inode which contains the
> lower inode pointers. This would still hide the details from the user...
>
> > )
> > then it is much more obvious to see what is going on when you write
> >
> > inode = FSSTACK_I(inode)->inodes[branch];
> >
> > rather than
> >
> > inode = __fsstack_lower_inode(inode, branch);
>
> Point taken. You need to know what's going to anyway, so we might as well
> make it painfully obvious.
>
> Josef "Jeff" Sipek.

--
Dave: Just a question. What use is a unicyle with no seat? And no pedals!
Mark: To answer a question with a question: What use is a skateboard?
Dave: Skateboards have wheels.
Mark: My wheel has a wheel!

2006-11-03 03:52:06

by Josef Sipek

[permalink] [raw]
Subject: Re: [PATCH 2/3] fsstack: Generic get/set lower object functions

On Fri, Nov 03, 2006 at 03:45:50AM +0000, Mark Williamson wrote:
> > > Why are you defining all these structs that are just wrapping unions?
> >
> > The reason for the union is simple...
...
> I guess that having a union foo * rather than a struct foo * would be a bit
> unconventional in the kernel. The named struct / anonymous union combo does
> hide the union as merely an implementation detail, which is nice. Was this
> your motivation?

That's exactly it. Save space & hide the details.

Josef "Jeff" Sipek.

--
Once you have their hardware. Never give it back.
(The First Rule of Hardware Acquisition)

2006-11-03 06:25:28

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH 2/3] fsstack: Generic get/set lower object functions

On Thu, 2006-11-02 at 22:51 -0500, Josef Sipek wrote:
> On Fri, Nov 03, 2006 at 03:45:50AM +0000, Mark Williamson wrote:
> > > > Why are you defining all these structs that are just wrapping unions?
> > >
> > > The reason for the union is simple...
> ...
> > I guess that having a union foo * rather than a struct foo * would be a bit
> > unconventional in the kernel. The named struct / anonymous union combo does
> > hide the union as merely an implementation detail, which is nice. Was this
> > your motivation?
>
> That's exactly it. Save space & hide the details.

Why? What is so special about the details that you need to hide them?
This is a union that will always be part of a structure anyway.

Trond

2006-11-03 09:13:09

by Pekka Enberg

[permalink] [raw]
Subject: Re: [PATCH 2/3] fsstack: Generic get/set lower object functions

On 11/3/06, Trond Myklebust <[email protected]> wrote:
> Why? What is so special about the details that you need to hide them?
> This is a union that will always be part of a structure anyway.

Nothing. Josef, I think we should make them unions.

2006-11-03 16:50:07

by Jan Engelhardt

[permalink] [raw]
Subject: Re: [PATCH 2/3] fsstack: Generic get/set lower object functions


>> Why? What is so special about the details that you need to hide them?
>> This is a union that will always be part of a structure anyway.
>
> Nothing. Josef, I think we should make them unions.

In other words,

/* structs to maintain pointers to the lower VFS objects */
struct fsstack_sb_info {
union {
struct super_block *sb;
struct super_block **sbs;
};
};

should become:

union fsstack_sb_info {
struct super_block *sb;
struct super_block **sbs;
};


-`J'
--

2006-11-03 20:48:49

by Josef Sipek

[permalink] [raw]
Subject: Re: [PATCH 2/3] fsstack: Generic get/set lower object functions

On Thu, Nov 02, 2006 at 11:06:05AM -0500, Trond Myklebust wrote:
> On Wed, 2006-11-01 at 22:59 -0500, Josef Jeff Sipek wrote:
...
> > +static inline struct inode *
> > +__fsstack_lower_inode(struct inode *inode, unsigned long branch_idx)
> > +{
> > + struct fsstack_inode_info *info = fsstack_inode_data(inode);
> > +
> > + return info->inodes[branch_idx];
> > +}
>
> What is the value of "functions" like the above? They appear just to
> obfuscate the code. Unless your aim is to hide the internals of the
> struct __fsstack_inode_generic_info (sort of futile, since you are
> asking users to include that structure in their private inode structs)
> then it is much more obvious to see what is going on when you write
>
> inode = FSSTACK_I(inode)->inodes[branch];
>
> rather than
>
> inode = __fsstack_lower_inode(inode, branch);

I was thinking about this a bit, and it would seem that not having get/set
function pretty much kills the reson to have generic pointer structures at
all.

Would it make sense to change filesystems like ecryptfs to open-code all
these things instead of using _their own_ get/set functions (e.g.,
ecryptfs_inode_to_lower)?

Other posibility is to move the lower pointers into generic VFS objects in
some clever way (not to waste memory on regular filesystems) - this way, the
stackable filesystems can still share some parts.

Josef "Jeff" Sipek.

--
A computer without Microsoft is like chocolate cake without mustard.

2006-11-03 21:22:32

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH 2/3] fsstack: Generic get/set lower object functions

On Fri, 2006-11-03 at 15:47 -0500, Josef Sipek wrote:
> I was thinking about this a bit, and it would seem that not having get/set
> function pretty much kills the reson to have generic pointer structures at
> all.
>
> Would it make sense to change filesystems like ecryptfs to open-code all
> these things instead of using _their own_ get/set functions (e.g.,
> ecryptfs_inode_to_lower)?
>
> Other posibility is to move the lower pointers into generic VFS objects in
> some clever way (not to waste memory on regular filesystems) - this way, the
> stackable filesystems can still share some parts.

In order for this to be useful, you and the ecryptfs folks need to sit
down and figure out what functionality all these stackable filesystems
want to share.

For instance, there is nothing wrong with moving ecryptfs_d_revalidate()
into a common libstackfs.c if it turns out that several filesystems want
to do the same thing. This is particularly true if you find common
locking needs (as is the case with ecryptfs_do_create(), for instance)
since locks are a good example of something you never want to debug more
than once.

Given all the fun things that can happen with intents in the lower_level
filesystem lookup() code, then that is probably a prime candidate for
some nice helper functions to make everyone's lives easy too...

Cheers,
Trond