Re: nbd: add locking to nbd_ioctl

[Andrew Morton <akpm@linux-foundation.org>]

On Mon, 26 Jan 2009 18:31:33 +0100
Pavel Machek <pavel@suse.cz> wrote:

> 
> The code was written to rely on big kernel lock to protect it from
> races. It mostly works when interface is not abused.
> 
> So this uses tx_lock to protect data structures from concurrent use
> between ioctl and worker threads.
> 
> Next step will be moving from ioctl to unlocked_ioctl. 

I hope I got the latest version of this...

The patch adds new trailing whitespace.  Either a) you wanted to add
new trailing whitespace or b) you're not running checkpatch.  Either
way: bad Pavel!

>
> ...
>
> +	case NBD_SET_SOCK:
> +		{
> +			struct file *file;
> +			if (lo->file)
> +				return -EBUSY;
> +			file = fget(arg);
> +			if (file) {
> +				struct inode *inode = file->f_path.dentry->d_inode;
> +				if (S_ISSOCK(inode->i_mode)) {
> +					lo->file = file;
> +					lo->sock = SOCKET_I(inode);
> +					if (max_part > 0)
> +						bdev->bd_invalidated = 1;
> +					return 0;
> +				} else {
> +					fput(file);
> +				}
>  			}
>  		}
> -		return error;
> +		return -EINVAL;

This isn't terribly readable - the -EINVAL is in fact specifically a
response to receiving a !S_ISSOCK fd, but this is presented in a
rather obfuscated way.

>  	case NBD_SET_BLKSIZE:
>  		lo->blksize = arg;
>  		lo->bytesize &= ~(lo->blksize-1);
> @@ -634,47 +626,65 @@ static int nbd_ioctl(struct block_device
>  		set_blocksize(bdev, lo->blksize);
>  		set_capacity(lo->disk, lo->bytesize >> 9);
>  		return 0;
> +
>  	case NBD_SET_SIZE:
>  		lo->bytesize = arg & ~(lo->blksize-1);
>  		bdev->bd_inode->i_size = lo->bytesize;
>  		set_blocksize(bdev, lo->blksize);
>  		set_capacity(lo->disk, lo->bytesize >> 9);
>  		return 0;
> +
>  	case NBD_SET_TIMEOUT:
>  		lo->xmit_timeout = arg * HZ;
>  		return 0;
> +
>  	case NBD_SET_SIZE_BLOCKS:
>  		lo->bytesize = ((u64) arg) * lo->blksize;
>  		bdev->bd_inode->i_size = lo->bytesize;
>  		set_blocksize(bdev, lo->blksize);
>  		set_capacity(lo->disk, lo->bytesize >> 9);
>  		return 0;
> +
>  	case NBD_DO_IT:
> -		if (lo->pid)
> -			return -EBUSY;
> -		if (!lo->file)
> -			return -EINVAL;
> -		thread = kthread_create(nbd_thread, lo, lo->disk->disk_name);
> -		if (IS_ERR(thread))
> -			return PTR_ERR(thread);
> -		wake_up_process(thread);
> -		error = nbd_do_it(lo);
> -		kthread_stop(thread);
> -		if (error)
> -			return error;
> -		sock_shutdown(lo, 1);
> -		file = lo->file;
> -		lo->file = NULL;
> -		nbd_clear_que(lo);
> -		printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
> -		if (file)
> -			fput(file);
> -		lo->bytesize = 0;
> -		bdev->bd_inode->i_size = 0;
> -		set_capacity(lo->disk, 0);
> -		if (max_part > 0)
> -			ioctl_by_bdev(bdev, BLKRRPART, 0);
> -		return lo->harderror;
> +		{
> +			struct task_struct *thread;
> +			struct file *file;
> +			int error;
> +
> +			if (lo->pid)
> +				return -EBUSY;
> +			if (!lo->file)
> +				return -EINVAL;
> +
> +			mutex_unlock(&lo->tx_lock);
> +
> +			thread = kthread_create(nbd_thread, lo, lo->disk->disk_name);
> +			if (IS_ERR(thread)) {
> +				mutex_lock(&lo->tx_lock);
> +				return PTR_ERR(thread);
> +			}
> +			wake_up_process(thread);

We could/should use kthread_run() here.

> +			error = nbd_do_it(lo);
> +			kthread_stop(thread);
> +
> +			mutex_lock(&lo->tx_lock);
> +			if (error)
> +				return error;
> +			sock_shutdown(lo, 0);
> +			file = lo->file;
> +			lo->file = NULL;
> +			nbd_clear_que(lo);
> +			printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);

I don't get it - why are we warning about what appears to be an
expected operation?

> +			if (file)
> +				fput(file);
> +			lo->bytesize = 0;
> +			bdev->bd_inode->i_size = 0;
> +			set_capacity(lo->disk, 0);
> +			if (max_part > 0)
> +				ioctl_by_bdev(bdev, BLKRRPART, 0);
> +			return lo->harderror;
> +		}
> +