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.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
