Re: [PATCH 2/2] revoke: change revoke_table to fileset and revoke_details
On Thu, 3 May 2007, Andrew Morton wrote: > Well that's the "locking" protocol then: each instance of this structure is > only ever touched by a single thread, yes? Yes. Each do_revoke() call creates a new instance. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] revoke: change revoke_table to fileset and revoke_details
On Thu, 3 May 2007, Andrew Morton wrote: Well that's the locking protocol then: each instance of this structure is only ever touched by a single thread, yes? Yes. Each do_revoke() call creates a new instance. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] revoke: change revoke_table to fileset and revoke_details
On Thu, 3 May 2007 23:32:28 +0300 (EEST)
Pekka J Enberg <[EMAIL PROTECTED]> wrote:
> On Thu, 3 May 2007, Andrew Morton wrote:
> > > +/**
> > > + * fileset - an array of file pointers.
> > > + * @files:the array of file pointers
> > > + * @nr: number of elements in the array
> > > + * @end: index to next unused file pointer
> > > + */
> > > +struct fileset {
> > > + struct file **files;
> > > + unsigned long nr;
> > > + unsigned long end;
> > > +};
> >
> > What's the locking protocol for all this?
>
> What do you mean? There is no concurrent access going on here.
Well that's the "locking" protocol then: each instance of this structure is
only ever touched by a single thread, yes?
> On Thu, 3 May 2007, Andrew Morton wrote:
> > > +static void free_fset(struct fileset *fset)
> > > +{
> > > + int i;
> > > +
> > > + for (i = fset->end; i < fset->nr; i++)
> > > + fput(fset->files[i]);
> > > +
> > > + kfree(fset->files);
> > > + kfree(fset);
> > > +}
> >
> > Confused. Shouldn't it be
> >
> > for (i = 0; i < fset->end; i++)
>
> No. The fset->end is an index to the first _unused_ file pointer. All
> entries before that are in use by revoked file descriptors so we don't
> want to fput() them.
>
OK.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] revoke: change revoke_table to fileset and revoke_details
On Thu, 3 May 2007, Andrew Morton wrote:
> > +/**
> > + * fileset - an array of file pointers.
> > + * @files:the array of file pointers
> > + * @nr: number of elements in the array
> > + * @end: index to next unused file pointer
> > + */
> > +struct fileset {
> > + struct file **files;
> > + unsigned long nr;
> > + unsigned long end;
> > +};
>
> What's the locking protocol for all this?
What do you mean? There is no concurrent access going on here.
On Thu, 3 May 2007, Andrew Morton wrote:
> > +static void free_fset(struct fileset *fset)
> > +{
> > + int i;
> > +
> > + for (i = fset->end; i < fset->nr; i++)
> > + fput(fset->files[i]);
> > +
> > + kfree(fset->files);
> > + kfree(fset);
> > +}
>
> Confused. Shouldn't it be
>
> for (i = 0; i < fset->end; i++)
No. The fset->end is an index to the first _unused_ file pointer. All
entries before that are in use by revoked file descriptors so we don't
want to fput() them.
Pekka
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] revoke: change revoke_table to fileset and revoke_details
On Thu, 3 May 2007 17:53:07 +0300 (EEST)
Pekka J Enberg <[EMAIL PROTECTED]> wrote:
> From: Pekka Enberg <[EMAIL PROTECTED]>
>
> The revoke_table struct is overloaded because it serves two purposes:
> it manages the pre-allocated set of files and tracks the revoke
> operation so that we know where to start restore if the operation
> fails. This splits file set management to separate struct fileset and
> renames struct revoke_table to revoke_details.
>
> Signed-off-by: Pekka Enberg <[EMAIL PROTECTED]>
> ---
> fs/revoke.c | 171
> +++-
> 1 file changed, 101 insertions(+), 70 deletions(-)
>
> Index: 26-mm/fs/revoke.c
> ===
> --- 26-mm.orig/fs/revoke.c2007-05-03 17:10:56.0 +0300
> +++ 26-mm/fs/revoke.c 2007-05-03 17:14:49.0 +0300
> @@ -18,19 +18,71 @@ * Copyright (C) 2006-2007 Pekka Enberg
> #include
> #include
>
> -/*
> - * This is used for pre-allocating an array of file pointers so that we don't
> - * have to do memory allocation under tasklist_lock.
> +/**
> + * fileset - an array of file pointers.
> + * @files:the array of file pointers
> + * @nr: number of elements in the array
> + * @end: index to next unused file pointer
> + */
> +struct fileset {
> + struct file **files;
> + unsigned long nr;
> + unsigned long end;
> +};
What's the locking protocol for all this?
> +static void free_fset(struct fileset *fset)
> +{
> + int i;
> +
> + for (i = fset->end; i < fset->nr; i++)
> + fput(fset->files[i]);
> +
> + kfree(fset->files);
> + kfree(fset);
> +}
Confused. Shouldn't it be
for (i = 0; i < fset->end; i++)
?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] revoke: change revoke_table to fileset and revoke_details
From: Pekka Enberg <[EMAIL PROTECTED]>
The revoke_table struct is overloaded because it serves two purposes:
it manages the pre-allocated set of files and tracks the revoke
operation so that we know where to start restore if the operation
fails. This splits file set management to separate struct fileset and
renames struct revoke_table to revoke_details.
Signed-off-by: Pekka Enberg <[EMAIL PROTECTED]>
---
fs/revoke.c | 171 +++-
1 file changed, 101 insertions(+), 70 deletions(-)
Index: 26-mm/fs/revoke.c
===
--- 26-mm.orig/fs/revoke.c 2007-05-03 17:10:56.0 +0300
+++ 26-mm/fs/revoke.c 2007-05-03 17:14:49.0 +0300
@@ -18,19 +18,71 @@ * Copyright (C) 2006-2007 Pekka Enberg
#include
#include
-/*
- * This is used for pre-allocating an array of file pointers so that we don't
- * have to do memory allocation under tasklist_lock.
+/**
+ * fileset - an array of file pointers.
+ * @files:the array of file pointers
+ * @nr: number of elements in the array
+ * @end: index to next unused file pointer
+ */
+struct fileset {
+ struct file **files;
+ unsigned long nr;
+ unsigned long end;
+};
+
+/**
+ * revoke_details - details of the revoke operation
+ * @inode:invalidate open file descriptors of this inode
+ * @fset: set of files that point to a revoked inode
+ * @restore_start:index to the first file pointer that is currently in
+ *use by a file descriptor but the real file has not
+ *been revoked
*/
-struct revoke_table {
- struct file **files;
- unsigned long size;
- unsigned long end;
- unsigned long restore_start;
+struct revoke_details {
+ struct fileset *fset;
+ unsigned long restore_start;
};
static struct kmem_cache *revokefs_inode_cache;
+static inline bool fset_is_full(struct fileset *set)
+{
+ return set->nr == set->end;
+}
+
+static inline struct file *fset_get_filp(struct fileset *set)
+{
+ return set->files[set->end++];
+}
+
+static struct fileset *alloc_fset(unsigned long size)
+{
+ struct fileset *fset;
+
+ fset = kzalloc(sizeof *fset, GFP_KERNEL);
+ if (!fset)
+ return NULL;
+
+ fset->files = kcalloc(size, sizeof(struct file *), GFP_KERNEL);
+ if (!fset->files) {
+ kfree(fset);
+ return NULL;
+ }
+ fset->nr = size;
+ return fset;
+}
+
+static void free_fset(struct fileset *fset)
+{
+ int i;
+
+ for (i = fset->end; i < fset->nr; i++)
+ fput(fset->files[i]);
+
+ kfree(fset->files);
+ kfree(fset);
+}
+
/*
* Revoked file descriptors point to inodes in the revokefs filesystem.
*/
@@ -84,22 +136,12 @@ static inline bool can_revoke_file(struc
return file->f_dentry->d_inode == inode;
}
-static inline bool revoke_table_is_full(struct revoke_table *table)
-{
- return table->end == table->size;
-}
-
-static inline struct file *revoke_table_get(struct revoke_table *table)
-{
- return table->files[table->end++];
-}
-
/*
* LOCKING: task_lock(owner)
*/
static int revoke_fds(struct task_struct *owner,
struct inode *inode,
- struct file *to_exclude, struct revoke_table *table)
+ struct file *to_exclude, struct fileset *fset)
{
struct files_struct *files;
struct fdtable *fdt;
@@ -127,12 +169,12 @@ for (fd = 0; fd < fdt->max_fds; fd++) {
goto failed;
}
- if (revoke_table_is_full(table)) {
+ if (fset_is_full(fset)) {
err = -ENOMEM;
goto failed;
}
- new_filp = revoke_table_get(table);
+ new_filp = fset_get_filp(fset);
/*
* Replace original struct file pointer with a pointer to
@@ -208,17 +250,17 @@ if (ret < 0) {
return err;
}
-static int revoke_break_cow(struct revoke_table *table, struct inode *inode,
+static int revoke_break_cow(struct fileset *fset, struct inode *inode,
struct file *to_exclude)
{
unsigned long i;
int err = 0;
- for (i = 0; i < table->end; i++) {
+ for (i = 0; i < fset->end; i++) {
struct revokefs_inode_info *info;
struct file *this;
- this = table->files[i];
+ this = fset->files[i];
info = revokefs_i(this->f_dentry->d_inode);
err = __revoke_break_cow(info->owner, inode, to_exclude);
@@ -399,32 +441,32 @@ static void restore_file(struct revokefs
info->owner = NULL; /* To avoid double-restore. */
}
-static void restore_files(struct revoke_table *table)
+static
[PATCH 2/2] revoke: change revoke_table to fileset and revoke_details
From: Pekka Enberg [EMAIL PROTECTED]
The revoke_table struct is overloaded because it serves two purposes:
it manages the pre-allocated set of files and tracks the revoke
operation so that we know where to start restore if the operation
fails. This splits file set management to separate struct fileset and
renames struct revoke_table to revoke_details.
Signed-off-by: Pekka Enberg [EMAIL PROTECTED]
---
fs/revoke.c | 171 +++-
1 file changed, 101 insertions(+), 70 deletions(-)
Index: 26-mm/fs/revoke.c
===
--- 26-mm.orig/fs/revoke.c 2007-05-03 17:10:56.0 +0300
+++ 26-mm/fs/revoke.c 2007-05-03 17:14:49.0 +0300
@@ -18,19 +18,71 @@ * Copyright (C) 2006-2007 Pekka Enberg
#include linux/revoked_fs_i.h
#include linux/syscalls.h
-/*
- * This is used for pre-allocating an array of file pointers so that we don't
- * have to do memory allocation under tasklist_lock.
+/**
+ * fileset - an array of file pointers.
+ * @files:the array of file pointers
+ * @nr: number of elements in the array
+ * @end: index to next unused file pointer
+ */
+struct fileset {
+ struct file **files;
+ unsigned long nr;
+ unsigned long end;
+};
+
+/**
+ * revoke_details - details of the revoke operation
+ * @inode:invalidate open file descriptors of this inode
+ * @fset: set of files that point to a revoked inode
+ * @restore_start:index to the first file pointer that is currently in
+ *use by a file descriptor but the real file has not
+ *been revoked
*/
-struct revoke_table {
- struct file **files;
- unsigned long size;
- unsigned long end;
- unsigned long restore_start;
+struct revoke_details {
+ struct fileset *fset;
+ unsigned long restore_start;
};
static struct kmem_cache *revokefs_inode_cache;
+static inline bool fset_is_full(struct fileset *set)
+{
+ return set-nr == set-end;
+}
+
+static inline struct file *fset_get_filp(struct fileset *set)
+{
+ return set-files[set-end++];
+}
+
+static struct fileset *alloc_fset(unsigned long size)
+{
+ struct fileset *fset;
+
+ fset = kzalloc(sizeof *fset, GFP_KERNEL);
+ if (!fset)
+ return NULL;
+
+ fset-files = kcalloc(size, sizeof(struct file *), GFP_KERNEL);
+ if (!fset-files) {
+ kfree(fset);
+ return NULL;
+ }
+ fset-nr = size;
+ return fset;
+}
+
+static void free_fset(struct fileset *fset)
+{
+ int i;
+
+ for (i = fset-end; i fset-nr; i++)
+ fput(fset-files[i]);
+
+ kfree(fset-files);
+ kfree(fset);
+}
+
/*
* Revoked file descriptors point to inodes in the revokefs filesystem.
*/
@@ -84,22 +136,12 @@ static inline bool can_revoke_file(struc
return file-f_dentry-d_inode == inode;
}
-static inline bool revoke_table_is_full(struct revoke_table *table)
-{
- return table-end == table-size;
-}
-
-static inline struct file *revoke_table_get(struct revoke_table *table)
-{
- return table-files[table-end++];
-}
-
/*
* LOCKING: task_lock(owner)
*/
static int revoke_fds(struct task_struct *owner,
struct inode *inode,
- struct file *to_exclude, struct revoke_table *table)
+ struct file *to_exclude, struct fileset *fset)
{
struct files_struct *files;
struct fdtable *fdt;
@@ -127,12 +169,12 @@ for (fd = 0; fd fdt-max_fds; fd++) {
goto failed;
}
- if (revoke_table_is_full(table)) {
+ if (fset_is_full(fset)) {
err = -ENOMEM;
goto failed;
}
- new_filp = revoke_table_get(table);
+ new_filp = fset_get_filp(fset);
/*
* Replace original struct file pointer with a pointer to
@@ -208,17 +250,17 @@ if (ret 0) {
return err;
}
-static int revoke_break_cow(struct revoke_table *table, struct inode *inode,
+static int revoke_break_cow(struct fileset *fset, struct inode *inode,
struct file *to_exclude)
{
unsigned long i;
int err = 0;
- for (i = 0; i table-end; i++) {
+ for (i = 0; i fset-end; i++) {
struct revokefs_inode_info *info;
struct file *this;
- this = table-files[i];
+ this = fset-files[i];
info = revokefs_i(this-f_dentry-d_inode);
err = __revoke_break_cow(info-owner, inode, to_exclude);
@@ -399,32 +441,32 @@ static void restore_file(struct revokefs
info-owner = NULL; /* To avoid double-restore. */
}
-static void restore_files(struct revoke_table *table)
+static
Re: [PATCH 2/2] revoke: change revoke_table to fileset and revoke_details
On Thu, 3 May 2007 17:53:07 +0300 (EEST)
Pekka J Enberg [EMAIL PROTECTED] wrote:
From: Pekka Enberg [EMAIL PROTECTED]
The revoke_table struct is overloaded because it serves two purposes:
it manages the pre-allocated set of files and tracks the revoke
operation so that we know where to start restore if the operation
fails. This splits file set management to separate struct fileset and
renames struct revoke_table to revoke_details.
Signed-off-by: Pekka Enberg [EMAIL PROTECTED]
---
fs/revoke.c | 171
+++-
1 file changed, 101 insertions(+), 70 deletions(-)
Index: 26-mm/fs/revoke.c
===
--- 26-mm.orig/fs/revoke.c2007-05-03 17:10:56.0 +0300
+++ 26-mm/fs/revoke.c 2007-05-03 17:14:49.0 +0300
@@ -18,19 +18,71 @@ * Copyright (C) 2006-2007 Pekka Enberg
#include linux/revoked_fs_i.h
#include linux/syscalls.h
-/*
- * This is used for pre-allocating an array of file pointers so that we don't
- * have to do memory allocation under tasklist_lock.
+/**
+ * fileset - an array of file pointers.
+ * @files:the array of file pointers
+ * @nr: number of elements in the array
+ * @end: index to next unused file pointer
+ */
+struct fileset {
+ struct file **files;
+ unsigned long nr;
+ unsigned long end;
+};
What's the locking protocol for all this?
+static void free_fset(struct fileset *fset)
+{
+ int i;
+
+ for (i = fset-end; i fset-nr; i++)
+ fput(fset-files[i]);
+
+ kfree(fset-files);
+ kfree(fset);
+}
Confused. Shouldn't it be
for (i = 0; i fset-end; i++)
?
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] revoke: change revoke_table to fileset and revoke_details
On Thu, 3 May 2007, Andrew Morton wrote:
+/**
+ * fileset - an array of file pointers.
+ * @files:the array of file pointers
+ * @nr: number of elements in the array
+ * @end: index to next unused file pointer
+ */
+struct fileset {
+ struct file **files;
+ unsigned long nr;
+ unsigned long end;
+};
What's the locking protocol for all this?
What do you mean? There is no concurrent access going on here.
On Thu, 3 May 2007, Andrew Morton wrote:
+static void free_fset(struct fileset *fset)
+{
+ int i;
+
+ for (i = fset-end; i fset-nr; i++)
+ fput(fset-files[i]);
+
+ kfree(fset-files);
+ kfree(fset);
+}
Confused. Shouldn't it be
for (i = 0; i fset-end; i++)
No. The fset-end is an index to the first _unused_ file pointer. All
entries before that are in use by revoked file descriptors so we don't
want to fput() them.
Pekka
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] revoke: change revoke_table to fileset and revoke_details
On Thu, 3 May 2007 23:32:28 +0300 (EEST)
Pekka J Enberg [EMAIL PROTECTED] wrote:
On Thu, 3 May 2007, Andrew Morton wrote:
+/**
+ * fileset - an array of file pointers.
+ * @files:the array of file pointers
+ * @nr: number of elements in the array
+ * @end: index to next unused file pointer
+ */
+struct fileset {
+ struct file **files;
+ unsigned long nr;
+ unsigned long end;
+};
What's the locking protocol for all this?
What do you mean? There is no concurrent access going on here.
Well that's the locking protocol then: each instance of this structure is
only ever touched by a single thread, yes?
On Thu, 3 May 2007, Andrew Morton wrote:
+static void free_fset(struct fileset *fset)
+{
+ int i;
+
+ for (i = fset-end; i fset-nr; i++)
+ fput(fset-files[i]);
+
+ kfree(fset-files);
+ kfree(fset);
+}
Confused. Shouldn't it be
for (i = 0; i fset-end; i++)
No. The fset-end is an index to the first _unused_ file pointer. All
entries before that are in use by revoked file descriptors so we don't
want to fput() them.
OK.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
