Re: [PATCH 2.6.13.1] Patch for invisible threads

[Bill Davidsen <davidsen@tmr.com>]

Sripathi Kodi wrote:
> Linus Torvalds wrote:
> 
>> I don't think this is wrong per se, but you shouldn't take the 
>> tasklist lock normally. You're better off just doing
> 
> 
> Linus,
> 
> I incarporated the path that doesn't hold tasklist lock unnecessarily. 
> The patch is below. This seems to work without any problems for me.
> 
> If the decision is to remove ->permission, I can send a small patch I 
> have that removes .permission entry from proc_task_inode_operations. 
> Either way fixes the problem I found.

Let me say that this solution, and any other which loops through all 
threads of a task, isn't going to scale well. I don't have a magic O(1) 
solution, if it were easy someone would have done that instead of the 
while loop, just noting that a clever solution would be a win on servers.

> 
> Thanks and regards,
> Sripathi.
> 
> Signed-off-by: Sripathi Kodi <sripathik@in.ibm.com>
> 
> --- linux-2.6.13.1-orig/fs/proc/base.c    2005-09-14 03:46:22.000000000 
> -0500
> +++ linux-2.6.13.1/fs/proc/base.c    2005-09-14 03:48:35.000000000 -0500
> @@ -275,11 +275,33 @@ static int proc_root_link(struct inode *
>  {
>      struct fs_struct *fs;
>      int result = -ENOENT;
> -    task_lock(proc_task(inode));
> -    fs = proc_task(inode)->fs;
> -    if(fs)
> +    struct task_struct *leader = proc_task(inode);
> +
> +    task_lock(leader);
> +    fs = leader->fs;
> +    if (fs) {
>          atomic_inc(&fs->count);
> -    task_unlock(proc_task(inode));
> +        task_unlock(leader);
> +    } else {
> +        /* Try to get fs from sub-threads */
> +        task_unlock(leader);
> +        struct task_struct *task = leader;
> +        read_lock(&tasklist_lock);
> +        if (pid_alive(task)) {
> +            while ((task = next_thread(task)) != leader) {
> +                task_lock(task);
> +                fs = task->fs;
> +                if (fs) {
> +                    atomic_inc(&fs->count);
> +                    task_unlock(task);
> +                    break;
> +                }
> +                task_unlock(task);
> +            }
> +        }
> +        read_unlock(&tasklist_lock);
> +    }
> +
>      if (fs) {
>          read_lock(&fs->lock);
>          *mnt = mntget(fs->rootmnt);
>