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/