On Mon, Jun 01, 2009 at 02:50:28PM -0700, Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
>
> In the implementation of revoke it is desirable to find all of the
> files we want to operation on. Currently tty's and mark_files_ro
> use the file_list for this, and this patch generalizes the file
> list so it can be used more efficiently.
>
> This patch starts by introducing struct file_list making the file
> list a first class object. file_list_lock and file_list_unlock
> are modified to take this object, making it clear which file_list
> we intended to lock.
>
> file_move is transformed into file_list_add taking a file_list and not
> allowing the movement of one file to another. __dentry_open
> is modified to support this by only adding normal files in open,
> special files have always been ignored when walking the file_list.
> __dentry_open skipping special files allows __ptmx_open and __tty_open
> to safely call file_add as they are adding the file to the file_list
> for the first time.
>
> file_kill has been renamed file_list_del to make it clear what it is
> doing and to keep from confusing it with a more revoke like operation.
>
> put_filp has been modified to not take file_list_del as we are never
> on a file_list when put_filp is called.
>
> fs_may_remount_ro and mark_files_ro have been modified to walk the
> inode list to find all of the inodes and then to walk the file list
> on those inodes. It can be a slightly longer walk as we frequently
> cache inodes that we do not have open but the overall complexity
> should be about the same,
Well not really. I have a couple of orders of magnitude more cached
inodes than open files here.
> these are slow path functions, and it
> gives us much greater flexibility overall.
Define flexibility. Walking the sb's file list and checking for
equality with the inode in question gives the same functionality,
just different performance profile.
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -699,6 +699,11 @@ static inline int mapping_writably_mapped(struct address_space *mapping)
> return mapping->i_mmap_writable != 0;
> }
>
> +struct file_list {
> + spinlock_t lock;
> + struct list_head list;
> +};
> +
> /*
> * Use sequence counter to get consistent i_size on 32-bit processors.
> */
> @@ -764,6 +769,7 @@ struct inode {
> struct list_head inotify_watches; /* watches on this inode */
> struct mutex inotify_mutex; /* protects the watches list */
> #endif
> + struct file_list i_files;
>
> unsigned long i_state;
> unsigned long dirtied_when; /* jiffies of first dirtying */
> @@ -934,9 +940,15 @@ struct file {
> unsigned long f_mnt_write_state;
> #endif
> };
> -extern spinlock_t files_lock;
> -#define file_list_lock() spin_lock(&files_lock);
> -#define file_list_unlock() spin_unlock(&files_lock);
> +
> +static inline void file_list_lock(struct file_list *files)
> +{
> + spin_lock(&files->lock);
> +}
> +static inline void file_list_unlock(struct file_list *files)
> +{
> + spin_unlock(&files->lock);
> +}
I don't really like this. It's just a list head. Get rid of
all these wrappers and crap I'd say. In fact, starting with my
patch to unexport files_lock and remove these wrappers would
be reasonable, wouldn't it?
Increasing the size of the struct inode by 24 bytes hurts.
Even when you decrapify it and can reuse i_lock or something,
then it is still 16 bytes on 64-bit.
I haven't looked through all the patches... but this is to
speed up a slowpath operation, isn't it? Or does revoke
need to be especially performant?
So this patch is purely a perofrmance improvement? Then I think
it needs to be justified with numbers and the downsides (bloating
struct inode in particulra) to be changelogged.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
