Re: svn commit: r299371 - in head: sbin/camcontrol sys/cam sys/cam/scsi

[Alan Somers]

On Wed, May 18, 2016 at 3:56 AM, Edward Tomasz Napierała 
wrote:

> On 0517T1158, Warner Losh wrote:
> > On Tue, May 10, 2016 at 11:33 AM, Edward Tomasz Napierala <
> tr...@freebsd.org
> > > wrote:
> >
> > > On 0510T1020, Alan Somers wrote:
> > > > On Tue, May 10, 2016 at 9:46 AM, Edward Tomasz Napierala <
> > > tr...@freebsd.org>
> > > > wrote:
> > > >
> > > > > Author: trasz
> > > > > Date: Tue May 10 15:46:33 2016
> > > > > New Revision: 299371
> > > > > URL: https://svnweb.freebsd.org/changeset/base/299371
> > > > >
> > > > > Log:
> > > > >   Add "camcontrol reprobe" subcommand, and implement it for da(4).
> > > > >   This makes it possible to manually force updating capacity data
> > > > >   after the disk got resized. Without it it might be neccessary to
> > > > >   reboot before FreeBSD notices updated disk size under eg VMWare.
> > > > >
> > > > >   Discussed with:   imp@
> > > > >   MFC after:1 month
> > > > >   Sponsored by: The FreeBSD Foundation
> > > > >   Differential Revision:https://reviews.freebsd.org/D6108
> > > > >
> > > > > Modified:
> > > > >   head/sbin/camcontrol/camcontrol.8
> > > > >   head/sbin/camcontrol/camcontrol.c
> > > > >   head/sys/cam/cam_ccb.h
> > > > >   head/sys/cam/cam_xpt.c
> > > > >   head/sys/cam/scsi/scsi_da.c
> > > > >
> > > > >
> > > >
> > > > I too have been annoyed that "camcontrol rescan" won't update
> capacity
> > > > data.  But could we solve the problem by simply adding logic to
> > > "camcontrol
> > > > rescan" instead of adding an entirely new command?  Would a user ever
> > > want
> > > > to rescan a device without reprobing it too?
> > >
> > > Two reasons.  First, I want to be able to pass the device name (like
> > > 'da0') and not the CAM path (like 1:0:0) for usability reasons - it
> seems
> > > easy to figure out the latter from the former, using "camcontrol
> devlist",
> > > but it suddenly becomes complicated when you try to explain it in a man
> > > page.
> >
> >
> > You can look up one or the other. fwdownload uses the daX name.
>
> Indeed.  But it would mean fixing "camcontrol rescan" first.
>
> > > Second - I don't understand the "camcontrol rescan" logic well
> > > enough, and "camcontrol rescan all" sometimes fails for me anyway,
> > > in a way I'm not sure how to debug.
> > >
> >
> > That's a cop-out. CAM is hard, but if you aren't willing to figure itout,
> > adding hacks that the other CAM maintainers have to cope with doesn't
> > help.
>
> That's true.  However, this hack is pretty non-intrusive - it only adds
> a trivial amount of code, and the "reprobe" command could be replaced
> with a simple alias to "rescan" if someone steps up to reimplement it.
>
> > Also, to be honest I'm not sure those two are actually that related.
> > > Rescanning is about discovering new devices on the bus.  "Reprobe"
> > > is about updating... well, mostly updating the capacity.  The former
> > > requires enumerating the bus using a mechanism built into XPT; the
> > > latter is just notifying the periph driver (in this case da(4)) that
> > > it needs to query the capacity and call disk_resize(4).
> > >
> >
> > The two are very related. Now we have two stupid paths in CAM instead of
> > one.
>
> We have two clearly separated code paths, doing completely different
> things - one scanning the bus, and only notifying periph drivers if
> new device is discovered, and the other one to notify existing periph
> driver instances, without scanning anything.  I just don't see how
> entangling them with each other would improve things.
>
> > and you didn't do ada like I asked.
>
> As I said in review, the ada(4) driver seems to lack resizing
> capability.  It doesn't contain a call to disk_resize(9).  It's been
> a few years since I've added resizing to da(4), but it took quite
> some time to make sure it interfaces with existing code in exactly
> the right way.  I just don't have time for this kind of side quest
> right now.  And I'm not even sure how to test it.  With da(4) it
> was easy - I've just added LUN resizing to CTL.
>


You can test ada(4) resize by using "camcontrol hpa".  Most SATA disks
allow you to reduce the disk's capacity through the hpa command.



>
> > Not happy with this at all, but not asking for a back out.
>
> Thanks.
>
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r299371 - in head: sbin/camcontrol sys/cam sys/cam/scsi

[Edward Tomasz Napierała]

On 0517T1158, Warner Losh wrote:
> On Tue, May 10, 2016 at 11:33 AM, Edward Tomasz Napierala  > wrote:
> 
> > On 0510T1020, Alan Somers wrote:
> > > On Tue, May 10, 2016 at 9:46 AM, Edward Tomasz Napierala <
> > tr...@freebsd.org>
> > > wrote:
> > >
> > > > Author: trasz
> > > > Date: Tue May 10 15:46:33 2016
> > > > New Revision: 299371
> > > > URL: https://svnweb.freebsd.org/changeset/base/299371
> > > >
> > > > Log:
> > > >   Add "camcontrol reprobe" subcommand, and implement it for da(4).
> > > >   This makes it possible to manually force updating capacity data
> > > >   after the disk got resized. Without it it might be neccessary to
> > > >   reboot before FreeBSD notices updated disk size under eg VMWare.
> > > >
> > > >   Discussed with:   imp@
> > > >   MFC after:1 month
> > > >   Sponsored by: The FreeBSD Foundation
> > > >   Differential Revision:https://reviews.freebsd.org/D6108
> > > >
> > > > Modified:
> > > >   head/sbin/camcontrol/camcontrol.8
> > > >   head/sbin/camcontrol/camcontrol.c
> > > >   head/sys/cam/cam_ccb.h
> > > >   head/sys/cam/cam_xpt.c
> > > >   head/sys/cam/scsi/scsi_da.c
> > > >
> > > >
> > >
> > > I too have been annoyed that "camcontrol rescan" won't update capacity
> > > data.  But could we solve the problem by simply adding logic to
> > "camcontrol
> > > rescan" instead of adding an entirely new command?  Would a user ever
> > want
> > > to rescan a device without reprobing it too?
> >
> > Two reasons.  First, I want to be able to pass the device name (like
> > 'da0') and not the CAM path (like 1:0:0) for usability reasons - it seems
> > easy to figure out the latter from the former, using "camcontrol devlist",
> > but it suddenly becomes complicated when you try to explain it in a man
> > page.
> 
> 
> You can look up one or the other. fwdownload uses the daX name.

Indeed.  But it would mean fixing "camcontrol rescan" first.

> > Second - I don't understand the "camcontrol rescan" logic well
> > enough, and "camcontrol rescan all" sometimes fails for me anyway,
> > in a way I'm not sure how to debug.
> >
> 
> That's a cop-out. CAM is hard, but if you aren't willing to figure itout,
> adding hacks that the other CAM maintainers have to cope with doesn't
> help.

That's true.  However, this hack is pretty non-intrusive - it only adds
a trivial amount of code, and the "reprobe" command could be replaced
with a simple alias to "rescan" if someone steps up to reimplement it.

> Also, to be honest I'm not sure those two are actually that related.
> > Rescanning is about discovering new devices on the bus.  "Reprobe"
> > is about updating... well, mostly updating the capacity.  The former
> > requires enumerating the bus using a mechanism built into XPT; the
> > latter is just notifying the periph driver (in this case da(4)) that
> > it needs to query the capacity and call disk_resize(4).
> >
> 
> The two are very related. Now we have two stupid paths in CAM instead of
> one.

We have two clearly separated code paths, doing completely different
things - one scanning the bus, and only notifying periph drivers if
new device is discovered, and the other one to notify existing periph
driver instances, without scanning anything.  I just don't see how
entangling them with each other would improve things.

> and you didn't do ada like I asked.

As I said in review, the ada(4) driver seems to lack resizing
capability.  It doesn't contain a call to disk_resize(9).  It's been
a few years since I've added resizing to da(4), but it took quite 
some time to make sure it interfaces with existing code in exactly
the right way.  I just don't have time for this kind of side quest
right now.  And I'm not even sure how to test it.  With da(4) it
was easy - I've just added LUN resizing to CTL.

> Not happy with this at all, but not asking for a back out.

Thanks.

___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r299371 - in head: sbin/camcontrol sys/cam sys/cam/scsi

[Warner Losh]

On Tue, May 10, 2016 at 11:33 AM, Edward Tomasz Napierala  wrote:

> On 0510T1020, Alan Somers wrote:
> > On Tue, May 10, 2016 at 9:46 AM, Edward Tomasz Napierala <
> tr...@freebsd.org>
> > wrote:
> >
> > > Author: trasz
> > > Date: Tue May 10 15:46:33 2016
> > > New Revision: 299371
> > > URL: https://svnweb.freebsd.org/changeset/base/299371
> > >
> > > Log:
> > >   Add "camcontrol reprobe" subcommand, and implement it for da(4).
> > >   This makes it possible to manually force updating capacity data
> > >   after the disk got resized. Without it it might be neccessary to
> > >   reboot before FreeBSD notices updated disk size under eg VMWare.
> > >
> > >   Discussed with:   imp@
> > >   MFC after:1 month
> > >   Sponsored by: The FreeBSD Foundation
> > >   Differential Revision:https://reviews.freebsd.org/D6108
> > >
> > > Modified:
> > >   head/sbin/camcontrol/camcontrol.8
> > >   head/sbin/camcontrol/camcontrol.c
> > >   head/sys/cam/cam_ccb.h
> > >   head/sys/cam/cam_xpt.c
> > >   head/sys/cam/scsi/scsi_da.c
> > >
> > >
> >
> > I too have been annoyed that "camcontrol rescan" won't update capacity
> > data.  But could we solve the problem by simply adding logic to
> "camcontrol
> > rescan" instead of adding an entirely new command?  Would a user ever
> want
> > to rescan a device without reprobing it too?
>
> Two reasons.  First, I want to be able to pass the device name (like
> 'da0') and not the CAM path (like 1:0:0) for usability reasons - it seems
> easy to figure out the latter from the former, using "camcontrol devlist",
> but it suddenly becomes complicated when you try to explain it in a man
> page.


You can look up one or the other. fwdownload uses the daX name.


> Second - I don't understand the "camcontrol rescan" logic well
> enough, and "camcontrol rescan all" sometimes fails for me anyway,
> in a way I'm not sure how to debug.
>

That's a cop-out. CAM is hard, but if you aren't willing to figure itout,
adding hacks that the other CAM maintainers have to cope with doesn't
help.

Also, to be honest I'm not sure those two are actually that related.
> Rescanning is about discovering new devices on the bus.  "Reprobe"
> is about updating... well, mostly updating the capacity.  The former
> requires enumerating the bus using a mechanism built into XPT; the
> latter is just notifying the periph driver (in this case da(4)) that
> it needs to query the capacity and call disk_resize(4).
>

The two are very related. Now we have two stupid paths in CAM instead of
one.

and you didn't do ada like I asked.

Not happy with this at all, but not asking for a back out.

Warner
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r299371 - in head: sbin/camcontrol sys/cam sys/cam/scsi

[Edward Tomasz Napierala]

On 0510T1546, Edward Tomasz Napierala wrote:
> Author: trasz
> Date: Tue May 10 15:46:33 2016
> New Revision: 299371
> URL: https://svnweb.freebsd.org/changeset/base/299371
> 
> Log:
>   Add "camcontrol reprobe" subcommand, and implement it for da(4).
>   This makes it possible to manually force updating capacity data
>   after the disk got resized. Without it it might be neccessary to
>   reboot before FreeBSD notices updated disk size under eg VMWare.

Turns out there was an earlier patch, written by Matthew Grooms
, that provided the same functionality,
and was submitted in PR 204901.  While I don't remember seeing
the actual diff (I'd just commit it if I had), and the code looks
different, I do remember discussing this, and Matthew's analysis
of the problem was crucial for getting this done.  The code also
works in a pretty much same way.  Great minds think alike, I
guess :-)

(I should learn to check the PR database before writing code,
though.  Sorry for that.)

___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r299371 - in head: sbin/camcontrol sys/cam sys/cam/scsi

[Edward Tomasz Napierala]

On 0510T1020, Alan Somers wrote:
> On Tue, May 10, 2016 at 9:46 AM, Edward Tomasz Napierala 
> wrote:
> 
> > Author: trasz
> > Date: Tue May 10 15:46:33 2016
> > New Revision: 299371
> > URL: https://svnweb.freebsd.org/changeset/base/299371
> >
> > Log:
> >   Add "camcontrol reprobe" subcommand, and implement it for da(4).
> >   This makes it possible to manually force updating capacity data
> >   after the disk got resized. Without it it might be neccessary to
> >   reboot before FreeBSD notices updated disk size under eg VMWare.
> >
> >   Discussed with:   imp@
> >   MFC after:1 month
> >   Sponsored by: The FreeBSD Foundation
> >   Differential Revision:https://reviews.freebsd.org/D6108
> >
> > Modified:
> >   head/sbin/camcontrol/camcontrol.8
> >   head/sbin/camcontrol/camcontrol.c
> >   head/sys/cam/cam_ccb.h
> >   head/sys/cam/cam_xpt.c
> >   head/sys/cam/scsi/scsi_da.c
> >
> >
> 
> I too have been annoyed that "camcontrol rescan" won't update capacity
> data.  But could we solve the problem by simply adding logic to "camcontrol
> rescan" instead of adding an entirely new command?  Would a user ever want
> to rescan a device without reprobing it too?

Two reasons.  First, I want to be able to pass the device name (like
'da0') and not the CAM path (like 1:0:0) for usability reasons - it seems
easy to figure out the latter from the former, using "camcontrol devlist",
but it suddenly becomes complicated when you try to explain it in a man
page.  Second - I don't understand the "camcontrol rescan" logic well
enough, and "camcontrol rescan all" sometimes fails for me anyway,
in a way I'm not sure how to debug.

Also, to be honest I'm not sure those two are actually that related.
Rescanning is about discovering new devices on the bus.  "Reprobe"
is about updating... well, mostly updating the capacity.  The former
requires enumerating the bus using a mechanism built into XPT; the
latter is just notifying the periph driver (in this case da(4)) that
it needs to query the capacity and call disk_resize(4).

___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r299371 - in head: sbin/camcontrol sys/cam sys/cam/scsi

[Alan Somers]

On Tue, May 10, 2016 at 9:46 AM, Edward Tomasz Napierala 
wrote:

> Author: trasz
> Date: Tue May 10 15:46:33 2016
> New Revision: 299371
> URL: https://svnweb.freebsd.org/changeset/base/299371
>
> Log:
>   Add "camcontrol reprobe" subcommand, and implement it for da(4).
>   This makes it possible to manually force updating capacity data
>   after the disk got resized. Without it it might be neccessary to
>   reboot before FreeBSD notices updated disk size under eg VMWare.
>
>   Discussed with:   imp@
>   MFC after:1 month
>   Sponsored by: The FreeBSD Foundation
>   Differential Revision:https://reviews.freebsd.org/D6108
>
> Modified:
>   head/sbin/camcontrol/camcontrol.8
>   head/sbin/camcontrol/camcontrol.c
>   head/sys/cam/cam_ccb.h
>   head/sys/cam/cam_xpt.c
>   head/sys/cam/scsi/scsi_da.c
>
>

I too have been annoyed that "camcontrol rescan" won't update capacity
data.  But could we solve the problem by simply adding logic to "camcontrol
rescan" instead of adding an entirely new command?  Would a user ever want
to rescan a device without reprobing it too?

-Alan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

svn commit: r299371 - in head: sbin/camcontrol sys/cam sys/cam/scsi

[Edward Tomasz Napierala]

Author: trasz
Date: Tue May 10 15:46:33 2016
New Revision: 299371
URL: https://svnweb.freebsd.org/changeset/base/299371

Log:
  Add "camcontrol reprobe" subcommand, and implement it for da(4).
  This makes it possible to manually force updating capacity data
  after the disk got resized. Without it it might be neccessary to
  reboot before FreeBSD notices updated disk size under eg VMWare.
  
  Discussed with:   imp@
  MFC after:1 month
  Sponsored by: The FreeBSD Foundation
  Differential Revision:https://reviews.freebsd.org/D6108

Modified:
  head/sbin/camcontrol/camcontrol.8
  head/sbin/camcontrol/camcontrol.c
  head/sys/cam/cam_ccb.h
  head/sys/cam/cam_xpt.c
  head/sys/cam/scsi/scsi_da.c

Modified: head/sbin/camcontrol/camcontrol.8
==
--- head/sbin/camcontrol/camcontrol.8   Tue May 10 15:45:59 2016
(r299370)
+++ head/sbin/camcontrol/camcontrol.8   Tue May 10 15:46:33 2016
(r299371)
@@ -27,7 +27,7 @@
 .\"
 .\" $FreeBSD$
 .\"
-.Dd August 6, 2015
+.Dd April 26, 2016
 .Dt CAMCONTROL 8
 .Os
 .Sh NAME
@@ -98,6 +98,9 @@
 .Op device id
 .Op generic args
 .Nm
+.Ic reprobe
+.Op device id
+.Nm
 .Ic rescan
 .Aq all | bus Ns Op :target:lun
 .Nm
@@ -518,6 +521,12 @@ are not specified).
 Print out the last logical block or the size of the device only, and omit
 the blocksize.
 .El
+.Pp
+Note that this command only displays the information, it does not update
+the kernel data structures.
+Use the
+.Nm 
+reprobe subcommand to do that.
 .It Ic start
 Send the SCSI Start/Stop Unit (0x1B) command to the given device with the
 start bit set.
@@ -539,6 +548,12 @@ The user
 may specify a scan of all busses, a single bus, or a lun.
 Scanning all luns
 on a target is not supported.
+.It Ic reprobe
+Tell the kernel to refresh the information about the device and
+notify the upper layer,
+.Xr GEOM 4 .
+This includes sending the SCSI READ CAPACITY command and updating
+the disk size visible to the rest of the system.
 .It Ic reset
 Tell the kernel to reset all busses in the system (with the
 .Ar all

Modified: head/sbin/camcontrol/camcontrol.c
==
--- head/sbin/camcontrol/camcontrol.c   Tue May 10 15:45:59 2016
(r299370)
+++ head/sbin/camcontrol/camcontrol.c   Tue May 10 15:46:33 2016
(r299371)
@@ -100,7 +100,8 @@ typedef enum {
CAM_CMD_APM = 0x0021,
CAM_CMD_AAM = 0x0022,
CAM_CMD_ATTRIB  = 0x0023,
-   CAM_CMD_OPCODES = 0x0024
+   CAM_CMD_OPCODES = 0x0024,
+   CAM_CMD_REPROBE = 0x0025
 } cam_cmdmask;
 
 typedef enum {
@@ -190,6 +191,7 @@ static struct camcontrol_opts option_tab
{"eject", CAM_CMD_STARTSTOP, CAM_ARG_EJECT, NULL},
{"reportluns", CAM_CMD_REPORTLUNS, CAM_ARG_NONE, "clr:"},
{"readcapacity", CAM_CMD_READCAP, CAM_ARG_NONE, "bhHNqs"},
+   {"reprobe", CAM_CMD_REPROBE, CAM_ARG_NONE, NULL},
 #endif /* MINIMALISTIC */
{"rescan", CAM_CMD_RESCAN, CAM_ARG_NONE, NULL},
{"reset", CAM_CMD_RESET, CAM_ARG_NONE, NULL},
@@ -328,6 +330,7 @@ static int scsiprintopcodes(struct cam_d
 static int scsiopcodes(struct cam_device *device, int argc, char **argv,
   char *combinedopt, int retry_count, int timeout,
   int verbose);
+static int scsireprobe(struct cam_device *device);
 
 #endif /* MINIMALISTIC */
 #ifndef min
@@ -8660,6 +8663,42 @@ bailout:
 
 #endif /* MINIMALISTIC */
 
+static int
+scsireprobe(struct cam_device *device)
+{
+   union ccb *ccb;
+   int retval = 0;
+
+   ccb = cam_getccb(device);
+
+   if (ccb == NULL) {
+   warnx("%s: error allocating ccb", __func__);
+   return (1);
+   }
+
+   bzero(&(>ccb_h)[1],
+ sizeof(struct ccb_scsiio) - sizeof(struct ccb_hdr));
+
+   ccb->ccb_h.func_code = XPT_REPROBE_LUN;
+
+   if (cam_send_ccb(device, ccb) < 0) {
+   warn("error sending XPT_REPROBE_LUN CCB");
+   retval = 1;
+   goto bailout;
+   }
+
+   if ((ccb->ccb_h.status & CAM_STATUS_MASK) != CAM_REQ_CMP) {
+   cam_error_print(device, ccb, CAM_ESF_ALL, CAM_EPF_ALL, stderr);
+   retval = 1;
+   goto bailout;
+   }
+
+bailout:
+   cam_freeccb(ccb);
+
+   return (retval);
+}
+
 void
 usage(int printlong)
 {
@@ -8679,6 +8718,7 @@ usage(int printlong)
 "camcontrol stop   [dev_id][generic args]\n"
 "camcontrol load   [dev_id][generic args]\n"
 "camcontrol eject  [dev_id][generic args]\n"
+"camcontrol reprobe[dev_id][generic args]\n"
 #endif /* MINIMALISTIC */
 "camcontrol rescan \n"
 "camcontrol reset  \n"
@@ -8751,6 +8791,7 @@ usage(int printlong)
 "stopsend a Stop Unit command to the device\n"
 "load