[PATCH 2/2] revoke: change revoke_table to fileset and revoke_details

[PATCH 2/2] revoke: change revoke_table to fileset and revoke_details

[Pekka J Enberg]

From: Pekka Enberg <penberg@cs.helsinki.fi>

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 <penberg@cs.helsinki.fi>
---
 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.000000000 +0300
+++ 26-mm/fs/revoke.c	2007-05-03 17:14:49.000000000 +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 void restore_files(struct revoke_details *details)
 {
 	unsigned long i;
 
-	for (i = table->restore_start; i < table->end; i++) {
+	for (i = details->restore_start; i < details->fset->end; i++) {
 		struct revokefs_inode_info *info;
 		struct file *filp;
 
-		filp = table->files[i];
+		filp = details->fset->files[i];
 		info = revokefs_i(filp->f_dentry->d_inode);
 
 		restore_file(info);
 	}
 }
 
-static int revoke_files(struct revoke_table *table)
+static int revoke_files(struct revoke_details *details)
 {
 	unsigned long i;
 	int err = 0;
 
-	for (i = 0; i < table->end; i++) {
+	for (i = 0; i < details->fset->end; i++) {
 		struct revokefs_inode_info *info;
 		struct file *this, *filp;
 		struct inode *inode;
 
-		this = table->files[i];
+		this = details->fset->files[i];
 		inode = this->f_dentry->d_inode;
 		info = revokefs_i(inode);
 
@@ -432,7 +474,7 @@ 	for (i = 0; i < table->end; i++) {
 		 * Increase count before attempting to close file as
 		 * an partially closed file can no longer be restored.
 		 */
-		table->restore_start++;
+		details->restore_start++;
 		filp = info->file;
 		err = filp->f_op->revoke(filp, inode->i_mapping);
 		put_task_struct(info->owner);
@@ -481,50 +523,31 @@ 		for (fd = 0; fd < fdt->max_fds; fd++) 
 	return nr_fds;
 }
 
-static void free_revoke_table(struct revoke_table *table)
-{
-	int i;
-
-	for (i = table->end; i < table->size; i++)
-		fput(table->files[i]);
-
-	kfree(table->files);
-	kfree(table);
-}
-
-static struct revoke_table *__alloc_revoke_table(unsigned long size)
+static struct fileset *__alloc_revoke_fset(unsigned long size)
 {
-	struct revoke_table *table;
+	struct fileset *fset;
 	int i;
 
-	table = kzalloc(sizeof *table, GFP_KERNEL);
-	if (!table)
-		return NULL;
-
-	table->size = size;
-	table->files = kcalloc(size, sizeof(struct file *), GFP_KERNEL);
-	if (!table->files) {
-		kfree(table);
+	fset = alloc_fset(size);
+	if (!fset)
 		return NULL;
-	}
 
-	for (i = 0; i < table->size; i++) {
+	for (i = 0; i < fset->nr; i++) {
 		struct file *filp;
 
 		filp = get_revoked_file();
 		if (!filp)
 			goto err;
 
-		table->files[i] = filp;
+		fset->files[i] = filp;
 	}
-	return table;
+	return fset;
   err:
-	free_revoke_table(table);
+	free_fset(fset);
 	return NULL;
 }
 
-static struct revoke_table *alloc_revoke_table(struct inode *inode,
-					       struct file *to_exclude)
+static struct fileset *alloc_revoke_fset(struct inode *inode, struct file *to_exclude)
 {
 	unsigned long nr_fds;
 
@@ -532,12 +555,13 @@ static struct revoke_table *alloc_revoke
 	nr_fds = inode_fds(inode, to_exclude);
 	read_unlock(&tasklist_lock);
 
-	return __alloc_revoke_table(nr_fds);
+	return __alloc_revoke_fset(nr_fds);
 }
 
 static int do_revoke(struct inode *inode, struct file *to_exclude)
 {
-	struct revoke_table *table = NULL;
+	struct revoke_details details;
+	struct fileset *fset = NULL;
 	struct task_struct *g, *p;
 	int err = 0;
 
@@ -552,8 +576,12 @@ 	int err = 0;
 		goto out;
 	}
 
-	table = alloc_revoke_table(inode, to_exclude);
-	if (!table) {
+	/*
+	 * Pre-allocate memory because the first pass is done under
+	 * tasklist_lock.
+	 */
+	fset = alloc_revoke_fset(inode, to_exclude);
+	if (!fset) {
 		err = -ENOMEM;
 		goto out;
 	}
@@ -563,9 +591,9 @@ 	int err = 0;
 	/*
 	 * If someone forked while we were allocating memory, try again.
 	 */
-	if (inode_fds(inode, to_exclude) > table->size) {
+	if (inode_fds(inode, to_exclude) > fset->nr) {
 		read_unlock(&tasklist_lock);
-		free_revoke_table(table);
+		free_fset(fset);
 		goto retry;
 	}
 
@@ -574,7 +602,7 @@ 	int err = 0;
 	 * new operations on them.
 	 */
 	do_each_thread(g, p) {
-		err = revoke_fds(p, inode, to_exclude, table);
+		err = revoke_fds(p, inode, to_exclude, fset);
 		if (err)
 			goto exit_loop;
 	}
@@ -593,24 +621,27 @@ 	int err = 0;
 	/*
  	 * Break COW for private mappings.
  	 */
-	err = revoke_break_cow(table, inode, to_exclude);
+	err = revoke_break_cow(fset, inode, to_exclude);
 	if (err)
 		goto out_restore;
 
+	details.fset = fset;
+	details.restore_start = 0;
+
 	/*
 	 * Now, revoke the files for good.
 	 */
-	err = revoke_files(table);
+	err = revoke_files(&details);
 	if (err)
 		goto out_restore;
 
   out_free_table:
-	free_revoke_table(table);
+	free_fset(fset);
   out:
 	return err;
 
   out_restore:
-	restore_files(table);
+	restore_files(&details);
 	goto out_free_table;
 }
 

Re: [PATCH 2/2] revoke: change revoke_table to fileset and revoke_details

[Andrew Morton]

On Thu, 3 May 2007 17:53:07 +0300 (EEST)
Pekka J Enberg <penberg@cs.helsinki.fi> wrote:

> From: Pekka Enberg <penberg@cs.helsinki.fi>
> 
> 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 <penberg@cs.helsinki.fi>
> ---
>  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.000000000 +0300
> +++ 26-mm/fs/revoke.c	2007-05-03 17:14:49.000000000 +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++)

?

Re: [PATCH 2/2] revoke: change revoke_table to fileset and revoke_details

[Pekka J Enberg]

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

Re: [PATCH 2/2] revoke: change revoke_table to fileset and revoke_details

[Andrew Morton]

On Thu, 3 May 2007 23:32:28 +0300 (EEST)
Pekka J Enberg <penberg@cs.helsinki.fi> 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.

Re: [PATCH 2/2] revoke: change revoke_table to fileset and revoke_details

[Pekka J Enberg]

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.