Jaunty iscsitarget

[Andy Whitcroft]

On Fri, Dec 19, 2008 at 04:12:52PM -0700, Tim Gardner wrote:
> I upgraded the iscsitarget to 0.4.17 using sources from
> http://iscsitarget.sourceforge.net/
> 
> Attached is the patch I wrote to get that version to compile. Since I've
> not done much in this part of the kernel, would someone have a look and
> tell me if I've done it right?
> 
> rtg
> -- 
> Tim Gardner tim.gardner at canonical.com

> --- iscsitarget-0.4.17/kernel/block-io.c	2008-06-26 08:59:00.000000000 -0600
> +++ tmp/kernel/block-io.c	2008-12-19 15:52:16.000000000 -0700
> @@ -21,6 +21,7 @@
>  struct blockio_data {
>  	char *path;
>  	struct block_device *bdev;
> +	fmode_t fmode;
>  };
>  
>  struct tio_work {
> @@ -154,19 +155,21 @@
>  {
>  	struct blockio_data *bio_data = volume->private;
>  	struct block_device *bdev;
> -	int flags = LUReadonly(volume) ? MS_RDONLY : 0;

The old code here seems to ask for a read-only open if the LUReadonly()
is true on the volume ...

>  	int err = 0;
>  
> +	bio_data->fmode = FMODE_READ|FMODE_WRITE;

... but here you you alway ask for ready-write.  But in the new
open_bdev_exclusive() we have the below code fragment.  To my reading
this will trigger and EACCES failure if you ask for write on a read-only
device?  With the new code it seems that a read-only mount of a
write-protected USB stick would fail even though it is safe:

        error = -EACCES;
        if ((mode & FMODE_WRITE) && bdev_read_only(bdev))
                goto blkdev_put;

I wonder why you do not do:

	bio_data->fmode = LUReadonly(volume) ? FMODE_READ :
					FMODE_READ|FMODE_WRITE;


>  	bio_data->path = kstrdup(path, GFP_KERNEL);
>  	if (!bio_data->path)
>  		return -ENOMEM;
>  
> -	bdev = open_bdev_excl(path, flags, THIS_MODULE);
> +	bdev = open_bdev_exclusive(path, bio_data->fmode, THIS_MODULE);
>  	if (IS_ERR(bdev)) {
>  		err = PTR_ERR(bdev);
>  		eprintk("Can't open device %s, error %d\n", path, err);
>  		bio_data->bdev = NULL;
>  	} else {

I am assuming if you did the fmode change that this would then not be
needed?

> +		if (LUReadonly(volume))
> +			set_device_ro(bdev,1);
>  		bio_data->bdev = bdev;
>  		fsync_bdev(bio_data->bdev);
>  	}
> @@ -325,7 +328,7 @@
>  	struct blockio_data *bio_data = volume->private;
>  
>  	if (bio_data->bdev)
> -		close_bdev_excl(bio_data->bdev);
> +		close_bdev_exclusive(bio_data->bdev,bio_data->fmode);
>  	kfree(bio_data->path);
>  
>  	kfree(volume->private);

-apw