[RFC] scsi-block: Deprecate rotation_rate
diff mbox series

Message ID 20180917083138.3948-1-famz@redhat.com
State New
Headers show
Series
  • [RFC] scsi-block: Deprecate rotation_rate
Related show

Commit Message

Fam Zheng Sept. 17, 2018, 8:31 a.m. UTC
This option is added together with scsi-disk but is never honoured,
becuase we don't emulate the VPD page for scsi-block. We could intercept
and inject the user specified value like for max xfer len, but it's
probably not helpful since the intent of 070f80095ad was for random
entropy aspects, not for performance. If emulated rotation rate is
desired, scsi-hd is more suitable.

Signed-off-by: Fam Zheng <famz@redhat.com>

---

RFC to discuss about if we want to keep the option. Another possibility
is, naturally, to actually write code to make use of the option.
---
 hw/scsi/scsi-disk.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Thomas Huth Sept. 17, 2018, 8:55 a.m. UTC | #1
On 2018-09-17 10:31, Fam Zheng wrote:
> This option is added together with scsi-disk but is never honoured,
> becuase we don't emulate the VPD page for scsi-block. We could intercept
> and inject the user specified value like for max xfer len, but it's
> probably not helpful since the intent of 070f80095ad was for random
> entropy aspects, not for performance. If emulated rotation rate is
> desired, scsi-hd is more suitable.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> ---
> 
> RFC to discuss about if we want to keep the option. Another possibility
> is, naturally, to actually write code to make use of the option.

As far as I've understood, scsi-disk is considered to be legacy anyway
and scsi-cd or scsi-hd should rather always be used instead. Thus
another idea that was mentioned in the past already is: Should we maybe
rather deprecate the whole "scsi-disk" device? Or deprecate
"media=cdrom" and make this an alias to "scsi-hd" instead?

Independent of that discussion, I think your patch is fine, it does
certainly not make sense to implement this for a legacy device anymore.
But please also add a deprecation note to qemu-deprecated.texi to mark
it as deprecated "officially".

 Thomas
Fam Zheng Sept. 17, 2018, 9:03 a.m. UTC | #2
On Mon, 09/17 10:55, Thomas Huth wrote:
> On 2018-09-17 10:31, Fam Zheng wrote:
> > This option is added together with scsi-disk but is never honoured,
> > becuase we don't emulate the VPD page for scsi-block. We could intercept
> > and inject the user specified value like for max xfer len, but it's
> > probably not helpful since the intent of 070f80095ad was for random
> > entropy aspects, not for performance. If emulated rotation rate is
> > desired, scsi-hd is more suitable.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > 
> > ---
> > 
> > RFC to discuss about if we want to keep the option. Another possibility
> > is, naturally, to actually write code to make use of the option.
> 
> As far as I've understood, scsi-disk is considered to be legacy anyway
> and scsi-cd or scsi-hd should rather always be used instead. Thus
> another idea that was mentioned in the past already is: Should we maybe
> rather deprecate the whole "scsi-disk" device? Or deprecate
> "media=cdrom" and make this an alias to "scsi-hd" instead?
> 
> Independent of that discussion, I think your patch is fine, it does
> certainly not make sense to implement this for a legacy device anymore.
> But please also add a deprecation note to qemu-deprecated.texi to mark
> it as deprecated "officially".

Ah, that is not what I meant. Sorry for the confusion. I should have typed
scsi-hd in the beginning of the commit message.  Scsi-block, which this patch
applies to, is for scsi command passthrough and is different from both scsi-hd
and scsi-disk.

Fam
Paolo Bonzini Sept. 17, 2018, 4 p.m. UTC | #3
On 17/09/2018 10:31, Fam Zheng wrote:
> This option is added together with scsi-disk but is never honoured,
> becuase we don't emulate the VPD page for scsi-block. We could intercept
> and inject the user specified value like for max xfer len, but it's
> probably not helpful since the intent of 070f80095ad was for random
> entropy aspects, not for performance. If emulated rotation rate is
> desired, scsi-hd is more suitable.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> ---
> 
> RFC to discuss about if we want to keep the option. Another possibility
> is, naturally, to actually write code to make use of the option.

Sounds good.  I queued the patch.

Paolo

Patch
diff mbox series

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 5ae7baa082..c43163cef4 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2610,6 +2610,12 @@  static void scsi_block_realize(SCSIDevice *dev, Error **errp)
         return;
     }
 
+    if (s->rotation_rate) {
+        error_report_once("rotation_rate is specified for scsi-block but is "
+                          "not implemented. This option is deprecated and will "
+                          "be removed in a future version");
+    }
+
     /* check we are using a driver managing SG_IO (version 3 and after) */
     rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
     if (rc < 0) {