[Cbe-oss-dev] [PATCH] fix state_mutex leaks

[Luke Browning]

On Thu, 2008-01-17 at 14:51 +0100, Christoph Hellwig wrote:
> Fix various state_mutex leaks.  The worst one was introduced by the
> interrutible state_mutex conversion but there've been a few before
> aswell.   Notably spufs_wait now returns without the state_mutex held
> when returning an error, which actually cleans up some code aswell.
> 
> 
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> 

I like this approach a lot better.  You mispelled bookkeeping
(bookkepping) in a couple of places and you have a bug below.

> Index: powerpc/arch/powerpc/platforms/cell/spufs/file.c
> ===================================================================
> --- powerpc.orig/arch/powerpc/platforms/cell/spufs/file.c	2008-01-14 18:11:42.000000000 +0100
> +++ powerpc/arch/powerpc/platforms/cell/spufs/file.c	2008-01-17 14:49:01.000000000 +0100
> @@ -357,6 +357,7 @@ static unsigned long spufs_ps_nopfn(stru
>  {
>  	struct spu_context *ctx = vma->vm_file->private_data;
>  	unsigned long area, offset = address - vma->vm_start;
> +	int ret = 0;
> 
>  	offset += vma->vm_pgoff << PAGE_SHIFT;
>  	if (offset >= ps_size)
> @@ -375,14 +376,15 @@ static unsigned long spufs_ps_nopfn(stru
> 
>  	if (ctx->state == SPU_STATE_SAVED) {
>  		up_read(&current->mm->mmap_sem);
> -		spufs_wait(ctx->run_wq, ctx->state == SPU_STATE_RUNNABLE);
> +		ret = spufs_wait(ctx->run_wq, ctx->state == SPU_STATE_RUNNABLE);
>  		down_read(&current->mm->mmap_sem);
>  	} else {
>  		area = ctx->spu->problem_phys + ps_offs;
>  		vm_insert_pfn(vma, address, (area + offset) >> PAGE_SHIFT);
>  	}
> 
> -	spu_release(ctx);
> +	if (!ret)
> +		spu_release(ctx);
>  	return NOPFN_REFAULT;
>  }
> 
> @@ -749,23 +751,25 @@ static ssize_t spufs_ibox_read(struct fi
> 
>  	count = spu_acquire(ctx);
>  	if (count)
> -		return count;
> +		goto out;
> 
>  	/* wait only for the first element */
>  	count = 0;
>  	if (file->f_flags & O_NONBLOCK) {
> -		if (!spu_ibox_read(ctx, &ibox_data))
> +		if (!spu_ibox_read(ctx, &ibox_data)) {
>  			count = -EAGAIN;
> +			goto out_unlock;
> +		}
>  	} else {
>  		count = spufs_wait(ctx->ibox_wq, spu_ibox_read(ctx, &ibox_data));
> +		if (count)
> +			goto out;
>  	}
> -	if (count)
> -		goto out;
> 
>  	/* if we can't write at all, return -EFAULT */
>  	count = __put_user(ibox_data, udata);
>  	if (count)
> -		goto out;
> +		goto out_unlock;
> 
>  	for (count = 4, udata++; (count + 4) <= len; count += 4, udata++) {
>  		int ret;
> @@ -782,9 +786,9 @@ static ssize_t spufs_ibox_read(struct fi
>  			break;
>  	}
> 
> -out:
> +out_unlock:
>  	spu_release(ctx);
> -
> +out:
>  	return count;
>  }
> 
> @@ -899,7 +903,7 @@ static ssize_t spufs_wbox_write(struct f
> 
>  	count = spu_acquire(ctx);
>  	if (count)
> -		return count;
> +		goto out;
> 
>  	/*
>  	 * make sure we can at least write one element, by waiting
> @@ -907,14 +911,16 @@ static ssize_t spufs_wbox_write(struct f
>  	 */
>  	count = 0;
>  	if (file->f_flags & O_NONBLOCK) {
> -		if (!spu_wbox_write(ctx, wbox_data))
> +		if (!spu_wbox_write(ctx, wbox_data)) {
>  			count = -EAGAIN;
> +			goto out_unlock;
> +		}
>  	} else {
>  		count = spufs_wait(ctx->wbox_wq, spu_wbox_write(ctx, wbox_data));
> +		if (count)
> +			goto out;
>  	}
> 
> -	if (count)
> -		goto out;
> 
>  	/* write as much as possible */
>  	for (count = 4, udata++; (count + 4) <= len; count += 4, udata++) {
> @@ -928,8 +934,9 @@ static ssize_t spufs_wbox_write(struct f
>  			break;
>  	}
> 
> -out:
> +out_unlock:
>  	spu_release(ctx);
> +out:
>  	return count;
>  }
> 
> @@ -1589,15 +1596,15 @@ static ssize_t spufs_mfc_read(struct fil
>  		else
>  			/* XXX(hch): shouldn't we clear ret here? */
>  			ctx->tagwait &= ~status;
> +		spu_release(ctx);
>  	} else {
>  		ret = spufs_wait(ctx->mfc_wq,
>  			   spufs_read_mfc_tagstatus(ctx, &status));
> +		if (ret)
> +			goto out;
>  	}
>  	spu_release(ctx);

You have two spu_releases() in a row here.

Cheers, Luke