Re: [pnfs] [PATCH 05/28] pnfsblock: expose scsi interface

Re: [pnfs] [PATCH 05/28] pnfsblock: expose scsi interface

[Benny Halevy]

Cc'ing scsi mailing list (and Christoph, please speak up now,
or forever hold your peace ;-)

On Mar. 11, 2008, 21:31 +0200, Fred Isaman <iisaman@citi.umich.edu> wrote:
> Signed-off-by: Fred Isaman <iisaman@citi.umich.edu>
> ---
>  drivers/scsi/hosts.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 24271a8..64f2847 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -48,10 +48,11 @@ static void scsi_host_cls_release(struct class_device *class_dev)
>  	put_device(&class_to_shost(class_dev)->shost_gendev);
>  }
>  
> -static struct class shost_class = {
> +struct class shost_class = {
>  	.name		= "scsi_host",
>  	.release	= scsi_host_cls_release,
>  };
> +EXPORT_SYMBOL(shost_class);
>  
>  /**
>   *	scsi_host_set_state - Take the given host through the host


This calls for a layering violation.

To fill-in more context, here's an excerpt from the next patch,
showing how you use shost_class to scan all scsi disks:

+/* Walk the list of scsi_devices. Add disks that can be opened and claimed
+ * to the device list
+ */
+static int
+nfs4_blk_add_scsi_disk(struct Scsi_Host *shost,
+		       int index, struct list_head *dlist)
+{
+	static char *claim_ptr = "I belong to pnfs block driver";
+	struct block_device *bdev;
+	struct gendisk *gd;
+	struct scsi_device *sdev;
+	unsigned int major, minor, ret = 0;
+	dev_t dev;
+
+	dprintk("%s enter \n", __func__);
+	if (index >= MAX_VOLS) {
+		dprintk("%s MAX_VOLS hit\n", __func__);
+		return -ENOSPC;
+	}
+	dprintk("%s 1 \n", __func__);
+	index--;
+	shost_for_each_device(sdev, shost) {
+		dprintk("%s 2\n", __func__);
+		/* Need to do this check before bumping index */
+		if (sdev->type != TYPE_DISK)
+			continue;
+		dprintk("%s 3 index %d \n", __func__, index);
+		if (++index >= MAX_VOLS) {
+			scsi_device_put(sdev);
+			break;
+		}
+		major = (!(index >> 4) ? SCSI_DISK0_MAJOR :
+			 SCSI_DISK1_MAJOR-1 + (index  >> 4));
+		minor =  ((index << 4) & 255);
+
+		dprintk("%s SCSI device %d:%d \n", __func__, major, minor);
+
+		dev = MKDEV(major, minor);
+		bdev = nfs4_blkdev_get(dev);
+		if (!bdev) {
+			dprintk("%s: failed to open device %d:%d\n",
+					__func__, major, minor);
+			continue;
+		}
+		gd = bdev->bd_disk;
+
+		dprintk("%s 4\n", __func__);
+
+		if (bd_claim(bdev, claim_ptr)) {
+			dprintk("%s: failed to claim device %d:%d\n",
+				__func__, gd->major, gd->first_minor);
+			blkdev_put(bdev);
+			continue;
+		}
+
+		ret = alloc_add_disk(bdev, dlist);
+		if (ret < 0)
+			goto out_err;
+		dprintk("%s ADDED DEVICE capacity %ld, bd_block_size %d\n",
+					__func__,
+					(unsigned long)gd->capacity,
+					bdev->bd_block_size);
+
+	}
+	index++;
+	dprintk("%s returns index %d \n", __func__, index);
+	return index;
+
+out_err:
+	dprintk("%s Can't add disk to list. ERROR: %d\n", __func__, ret);
+	nfs4_blkdev_put(bdev);
+	return ret;
+}
+
+/*
+ * Create a temporary list of all SCSI disks host can see, and that have not
+ * yet been claimed.
+ * shost_class: list of all registered scsi_hosts
+ * returns -errno on error, and #of devices found on success.
+ * XXX Loosely emulate scsi_host_lookup from scsi/host.c
+*/
+int nfs4_blk_create_scsi_disk_list(struct list_head *dlist)
+{
+	struct class *class = &shost_class;
+	struct class_device *cdev;
+	struct Scsi_Host *shost;
+	int ret = 0, index = 0;
+
+	dprintk("%s enter\n", __func__);
+
+	down(&class->sem);
+	list_for_each_entry(cdev, &class->children, node) {
+		dprintk("%s 1\n", __func__);
+		shost = class_to_shost(cdev);
+		ret = nfs4_blk_add_scsi_disk(shost, index, dlist);
+		dprintk("%s 2 ret %d\n", __func__, ret);
+		if (ret < 0)
+			goto out;
+		index = ret;
+	}
+out:
+	up(&class->sem);
+	return ret;
+}

My question is how should a proper API between the scsi layer and
the block layout driver look like?

Can you list your requirements, e.g.:
- scanning all available devices,

- discovering new devices on the fly

- getting notified for new devices?

Benny

Re: [pnfs] [PATCH 05/28] pnfsblock: expose scsi interface

[Christoph Hellwig]

On Wed, Mar 12, 2008 at 06:33:34PM +0200, Benny Halevy wrote:
> This calls for a layering violation.
> 
> To fill-in more context, here's an excerpt from the next patch,
> showing how you use shost_class to scan all scsi disks:

Yes, absolutely.  No one outside of few places in the core scsi code
should ever iterate over the scsi disks.

> My question is how should a proper API between the scsi layer and
> the block layout driver look like?
> 
> Can you list your requirements, e.g.:
> - scanning all available devices,
> 
> - discovering new devices on the fly
> 
> - getting notified for new devices?

Neither.  pnfs shouldn't open block devices from kernelspace at all,
but do it's disovery in userspace.

Or even better this whole block layout driver crap should go away
completely and the people who have designed it beaten up until they
aren't recognizable anymore.

Re: [pnfs] [PATCH 05/28] pnfsblock: expose scsi interface

[Fredric Isaman]

On Wed, 12 Mar 2008, Jason Glasgow wrote:

> Christoph,
>
> Putting device discovery in user space a valid alternative implementation,
> and we will pursue that model.
>
> -Jason
>
>
> On Wed, Mar 12, 2008 at 12:43 PM, Christoph Hellwig <hch@infradead.org>
> wrote:
>
>> On Wed, Mar 12, 2008 at 06:33:34PM +0200, Benny Halevy wrote:
>>> This calls for a layering violation.
>>>
>>> To fill-in more context, here's an excerpt from the next patch,
>>> showing how you use shost_class to scan all scsi disks:
>>

The referenced code is an adhoc hack that does what we need it to for now. 
We would *love* to be able to just call in to some scsi function that fits 
our needs

>> Yes, absolutely.  No one outside of few places in the core scsi code
>> should ever iterate over the scsi disks.
>>

Is this for locking reasons?

>>> My question is how should a proper API between the scsi layer and
>>> the block layout driver look like?
>>>
>>> Can you list your requirements, e.g.:
>>> - scanning all available devices,
>>>
>>> - discovering new devices on the fly
>>>
>>> - getting notified for new devices?
>>
>> Neither.  pnfs shouldn't open block devices from kernelspace at all,
>> but do it's disovery in userspace.
>>


Basically, we are given a disk signature, (an array of offset, byte
sequence pairs) and want to match it against some visible disk (ignoring
partitions), or know that it is not currently visible.

Thus our requirement are:

Scan all available block devices (though all available SCSI devices is a
workable substitute).

Notification of new devices would be a helpful in optimizing rescans for 
unmapped disk signatures.


As Jason mentions, we will pursue doing this in userspace.

 	Fred

Re: [pnfs] [PATCH 05/28] pnfsblock: expose scsi interface

[Boaz Harrosh]

On Wed, Mar 12 2008 at 20:10 +0200, Fredric Isaman <iisaman@citi.umich.edu> wrote:
> 
> On Wed, 12 Mar 2008, Jason Glasgow wrote:
> 
>> Christoph,
>>
>> Putting device discovery in user space a valid alternative implementation,
>> and we will pursue that model.
>>
>> -Jason
>>
>>
>> On Wed, Mar 12, 2008 at 12:43 PM, Christoph Hellwig <hch@infradead.org>
>> wrote:
>>
>>> On Wed, Mar 12, 2008 at 06:33:34PM +0200, Benny Halevy wrote:
>>>> This calls for a layering violation.
>>>>
>>>> To fill-in more context, here's an excerpt from the next patch,
>>>> showing how you use shost_class to scan all scsi disks:
> 
> The referenced code is an adhoc hack that does what we need it to for now. 
> We would *love* to be able to just call in to some scsi function that fits 
> our needs
> 
>>> Yes, absolutely.  No one outside of few places in the core scsi code
>>> should ever iterate over the scsi disks.
>>>
> 
> Is this for locking reasons?
> 
>>>> My question is how should a proper API between the scsi layer and
>>>> the block layout driver look like?
>>>>
>>>> Can you list your requirements, e.g.:
>>>> - scanning all available devices,
>>>>
>>>> - discovering new devices on the fly
>>>>
>>>> - getting notified for new devices?
>>> Neither.  pnfs shouldn't open block devices from kernelspace at all,
>>> but do it's disovery in userspace.
>>>
> 
> 
> Basically, we are given a disk signature, (an array of offset, byte
> sequence pairs) and want to match it against some visible disk (ignoring
> partitions), or know that it is not currently visible.
> 
> Thus our requirement are:
> 
> Scan all available block devices (though all available SCSI devices is a
> workable substitute).
> 
> Notification of new devices would be a helpful in optimizing rescans for 
> unmapped disk signatures.
> 
> 
> As Jason mentions, we will pursue doing this in userspace.
> 
>  	Fred
> 
> 
OK I found:
#ifndef __i386__
#undef CONFIG_SCSI_OMIT_FLASHPOINT
#define CONFIG_SCSI_OMIT_FLASHPOINT
#endif

try patch below and report
---
BusLogic: Remove total bullshit

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
git-diff --stat -p
 drivers/scsi/BusLogic.h |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/BusLogic.h b/drivers/scsi/BusLogic.h
index bfbfb5c..0f0d7f3 100644
--- a/drivers/scsi/BusLogic.h
+++ b/drivers/scsi/BusLogic.h
@@ -38,11 +38,6 @@
   CONFIG_PCI set.
 */
 
-#ifndef __i386__
-#undef CONFIG_SCSI_OMIT_FLASHPOINT
-#define CONFIG_SCSI_OMIT_FLASHPOINT
-#endif
-
 #ifndef CONFIG_PCI
 #undef CONFIG_SCSI_OMIT_FLASHPOINT
 #define CONFIG_SCSI_OMIT_FLASHPOINT

Re: [pnfs] [PATCH 05/28] pnfsblock: expose scsi interface

[Christoph Hellwig]

On Wed, Mar 12, 2008 at 02:10:03PM -0400, Fredric Isaman wrote:
>>> Yes, absolutely.  No one outside of few places in the core scsi code
>>> should ever iterate over the scsi disks.
>>>
>
> Is this for locking reasons?

It's for abstraction and data hiding reasons.  The existance of such
a list is an implementation detail.   Any inkernel or external user
should use the block device API to access block devices.  Even the
implementation using the scsi layer is an implementation detail.

> Basically, we are given a disk signature, (an array of offset, byte
> sequence pairs) and want to match it against some visible disk (ignoring
> partitions), or know that it is not currently visible.
>
> Thus our requirement are:
>
> Scan all available block devices (though all available SCSI devices is a
> workable substitute).
>
> Notification of new devices would be a helpful in optimizing rescans for 
> unmapped disk signatures.
>
>
> As Jason mentions, we will pursue doing this in userspace.

Try looking into the libvolume_id and libblkid libraries which solve
exactly this problem already, and it's a quite complicated one to start
with.

Re: [pnfs] [PATCH 05/28] pnfsblock: expose scsi interface

[Fredric Isaman]

On Wed, 12 Mar 2008, Christoph Hellwig wrote:

> On Wed, Mar 12, 2008 at 02:10:03PM -0400, Fredric Isaman wrote:
>>>> Yes, absolutely.  No one outside of few places in the core scsi code
>>>> should ever iterate over the scsi disks.
>>>>
>>
>> Is this for locking reasons?
>
> It's for abstraction and data hiding reasons.  The existance of such
> a list is an implementation detail.   Any inkernel or external user
> should use the block device API to access block devices.  Even the
> implementation using the scsi layer is an implementation detail.
>
>> Basically, we are given a disk signature, (an array of offset, byte
>> sequence pairs) and want to match it against some visible disk (ignoring
>> partitions), or know that it is not currently visible.
>>
>> Thus our requirement are:
>>
>> Scan all available block devices (though all available SCSI devices is a
>> workable substitute).
>>
>> Notification of new devices would be a helpful in optimizing rescans for
>> unmapped disk signatures.
>>
>>
>> As Jason mentions, we will pursue doing this in userspace.
>
> Try looking into the libvolume_id and libblkid libraries which solve
> exactly this problem already, and it's a quite complicated one to start
> with.
>
>

OK.  Thanks for the pointers.

 	Fred

Re: [pnfs] [PATCH 05/28] pnfsblock: expose scsi interface

[Jeff Garzik]

Christoph Hellwig wrote:
> On Wed, Mar 12, 2008 at 06:33:34PM +0200, Benny Halevy wrote:
>> This calls for a layering violation.
>>
>> To fill-in more context, here's an excerpt from the next patch,
>> showing how you use shost_class to scan all scsi disks:
> 
> Yes, absolutely.  No one outside of few places in the core scsi code
> should ever iterate over the scsi disks.
> 
>> My question is how should a proper API between the scsi layer and
>> the block layout driver look like?
>>
>> Can you list your requirements, e.g.:
>> - scanning all available devices,
>>
>> - discovering new devices on the fly
>>
>> - getting notified for new devices?
> 
> Neither.  pnfs shouldn't open block devices from kernelspace at all,
> but do it's disovery in userspace.
> 
> Or even better this whole block layout driver crap should go away
> completely and the people who have designed it beaten up until they
> aren't recognizable anymore.

The fact that NFSv4.1 requires SCSI is stupid crap, too.

The _design_ is a layering violation.  I say we just say "no".

NFS _clients_ should not need to know deep device-level details about 
how to store their data, and should not require SCSI in order to store data.

	Jeff