Re: [PATCH v3 3/4] cxlflash: Superpipe support

[Manoj Kumar <manoj@linux.vnet.ibm.com>]

Brian: Thanks for your review. See comments inline, below.

- Manoj Kumar

On 8/6/2015 3:46 PM, Brian King wrote:
>>    * cxlflash_queuecommand() - sends a mid-layer request
>>    * @host:	SCSI host associated with device.
>>    * @scp:	SCSI command to send.
>> @@ -512,6 +535,13 @@ static int cxlflash_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *scp)
>>   			     get_unaligned_be32(&((u32 *)scp->cmnd)[2]),
>>   			     get_unaligned_be32(&((u32 *)scp->cmnd)[3]));
>>
>> +	/* Fail all read/write commands when in operating superpipe mode */
>> +	if (scp->device->hostdata && is_scsi_read_write(scp)) {
>> +		pr_debug_ratelimited("%s: LUN being used in superpipe mode. "
>> +				     "Operation not allowed!\n", __func__);
>> +		goto error;
>> +	}
>
> Not sure I like this. A couple of concerns:
>
> 1. Any process that comes along and issues a read to the device will result in I/O errors getting logged
>     by the layers above you. The general expectation is that if reads or writes are failing on a block device,
>     then something is wrong and the user should know about it. A user innocently running "fdisk -l", for example,
>     to list all disk partitions on the system, would see errors getting logged for every disk configured for
>     superpipe mode.
> 2. How will users know that devices are in superpipe mode? The only indication is the driver specific sysfs attribute
>     that no existing tooling will be checking. GUI tools to do disk partitioning and such will see these devices
>     and present them to the user with the expectation that something can be done with them.
> 3. If this is a multipath device, you'll have a dm device sitting on top of this and potentially have multipathd doing health
>     checking periodically and these devices will show up in multipath -ll output.
>
> It seems to me like the cleanest option would be, when switching into superpipe mode, for the user code doing
> this to unbind the sd driver from the relevant scsi device. This will prevent any reads or writes from being
> issued to the LUN from the block layer, since there will no longer be any way to do this. It will also prevent
> these devices from showing up in GUIs and menus where we don't want them to.
>
> So, I'd say, kill this snooping of the op code and failing I/O in superpipe mode, and solve this in userspace.

The intent of this snooping and failing I/O was to prevent an accidental 
over-write of the LUN in superpipe mode by some other user unaware of 
our sysfs attribute. This was an additional precautionary measure to 
unbinding the sd driver from the relevant scsi device. We can fall back 
on to that primary mechanism.


>> +	gli->max_lba = swab64(*((u64 *)&cmd_buf[0]));
>> +	gli->blk_len = swab32(*((u32 *)&cmd_buf[8]));
>
> This doesn't look right. How does this work on big endian? I think you want to be using
> be64_to_cpu and be32_to_cpu here.
>

Good catch. Will rectify in the v4 patch.