Re: [2.4.13] [devfs 0.119 (20011009)] base.c: devfsd_ioctl

[Richard Gooch]

Kari Hurtta writes:
> 
> Can you say where I have wrong?
> 
> Code on devfsd_ioctl() [base.c]:
> 
>         if (fs_info->devfsd_task == NULL)
>         {
>             if ( !spin_trylock (&lock) ) return -EBUSY;
>             fs_info->devfsd_task = current;
>             spin_unlock (&lock);
> 
> To me that spinlock looks like it is useless.
> Either
> 
> 	1) If it is mean that lock protects two CPUs settting
>             fs_info->devfsd_task when another is set it,
> 	    then test about fs_info->devfsd_task == NULL
> 	    should be inside of locked code
> or     2) this is protected with some other lock as comment
>           perhaps indicates:
> 
>         /*  Ensure only one reader has access to the queue. This scheme will
>             work even if the global kernel lock were to be removed, because it
>             doesn't matter who gets in first, as long as only one gets it
>         */
>         if (fs_info->devfsd_task == NULL)
>         {
>             if ( !spin_trylock (&lock) ) return -EBUSY;
> 
> Should this be:
> 
> --- fs/devfs/base.c.old	Thu Oct 11 09:23:24 2001
> +++ fs/devfs/base.c	Sun Oct 28 14:59:03 2001
> @@ -3227,6 +3227,11 @@
>  	if (fs_info->devfsd_task == NULL)
>  	{
>  	    if ( !spin_trylock (&lock) ) return -EBUSY;
> +	    if (fs_info->devfsd_task != NULL) {
> +	         /* We lost race ... */
> +	         spin_unlock (&lock);
> +		 return -EBUSY;
> +	    }
>  	    fs_info->devfsd_task = current;
>  	    spin_unlock (&lock);
>  	    fs_info->devfsd_file = file;

Damn! You're right. I broke that when I simplified the anti-race code
back in June and replaced the #ifdef CONFIG_SMP bits with plain
spinlocks. Unfortunately I didn't put back in a critcal line.
Cut-and-paste error without the paste :-(

OK, I've updated my both my trees. I'll feed this to Linus in my next
release.

				Regards,

					Richard....
Permanent: [e-mail]
Current:   [e-mail]