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

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

[Kari Hurtta]

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;


-- 
          /"\                           |  Kari 
          \ /     ASCII Ribbon Campaign |    Hurtta
           X      Against HTML Mail     |
          / \                           |

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: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca