| Message ID | 20200102075315.22652-6-sblbir@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for block disk resize notification | expand |
On 01/01/2020 11:53 PM, Balbir Singh wrote: > block/genhd provides disk_set_capacity() for sending > RESIZE notifications via uevents. This notification is > newly added to scsi sd. nit :- The above commit messages in this series are looking odd from the ones we have in the tree and is it possible to fold it in two lines ? [Can be done at the time of applying series.]
On Thu, 2020-01-02 at 22:21 +0000, Chaitanya Kulkarni wrote: > On 01/01/2020 11:53 PM, Balbir Singh wrote: > > block/genhd provides disk_set_capacity() for sending > > RESIZE notifications via uevents. This notification is > > newly added to scsi sd. > > nit :- > > The above commit messages in this series are looking odd from > the ones we have in the tree and is it possible to fold it in > two lines ? > > [Can be done at the time of applying series.] > Sounds good! I'll request the maintainer or repost if needed Balbir Singh.
Balbir, > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 5afb0046b12a..1a3be30b6b78 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -3184,7 +3184,7 @@ static int sd_revalidate_disk(struct gendisk *disk) > > sdkp->first_scan = 0; > > - set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity)); > + disk_set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity)); > sd_config_write_same(sdkp); > kfree(buffer); We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device capacity changes. However, this event does not automatically cause revalidation.
On Mon, 2020-01-06 at 22:48 -0500, Martin K. Petersen wrote: > Balbir, > > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > > index 5afb0046b12a..1a3be30b6b78 100644 > > --- a/drivers/scsi/sd.c > > +++ b/drivers/scsi/sd.c > > @@ -3184,7 +3184,7 @@ static int sd_revalidate_disk(struct gendisk > > *disk) > > > > sdkp->first_scan = 0; > > > > - set_capacity(disk, logical_to_sectors(sdp, sdkp- > > >capacity)); > > + disk_set_capacity(disk, logical_to_sectors(sdp, sdkp- > > >capacity)); > > sd_config_write_same(sdkp); > > kfree(buffer); > > We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device > capacity changes. However, this event does not automatically cause > revalidation. Which I seem to remember was a deliberate choice: some change capacities occur because the path goes passive and default values get installed. James
James, >> We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device >> capacity changes. However, this event does not automatically cause >> revalidation. > > Which I seem to remember was a deliberate choice: some change > capacities occur because the path goes passive and default values get > installed. Yep, it's very tricky territory.
On Mon, 2020-01-06 at 23:39 -0500, Martin K. Petersen wrote: > James, > > > > We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device > > > capacity changes. However, this event does not automatically cause > > > revalidation. > > > > Which I seem to remember was a deliberate choice: some change > > capacities occur because the path goes passive and default values get > > installed. > > Yep, it's very tricky territory. > Yes, there are some storage arrays that refuse a READ CAPACITY command in certain ALUA states so you can't get the new capacity anyway. It might be nice to improve this, though, there are some cases now where we set the capacity to zero when we revalidate and can't get the value. -Ewan
On Mon, 2020-01-06 at 22:48 -0500, Martin K. Petersen wrote: > Balbir, > > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > > index 5afb0046b12a..1a3be30b6b78 100644 > > --- a/drivers/scsi/sd.c > > +++ b/drivers/scsi/sd.c > > @@ -3184,7 +3184,7 @@ static int sd_revalidate_disk(struct gendisk *disk) > > > > sdkp->first_scan = 0; > > > > - set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity)); > > + disk_set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity)); > > sd_config_write_same(sdkp); > > kfree(buffer); > > We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device > capacity changes. However, this event does not automatically cause > revalidation. > The proposed idea is to not reinforce revalidation, unless explictly specified (in the thread before Bob Liu had suggestions). The goal is to notify user space of changes via RESIZE. SCSI sd can opt out of this IOW, I can remove this if you feel SDEV_EVT_CAPACITY_CHANGE_REPORTED is sufficient for current use cases. Balbir Singh.
Ewan, > Yes, there are some storage arrays that refuse a READ CAPACITY > command in certain ALUA states so you can't get the new capacity > anyway. Yep. And some devices will temporarily return a capacity of 0xFFFFFFFF... If we were to trigger a filesystem resize, the results would be disastrous. > It might be nice to improve this, though, there are some cases now > where we set the capacity to zero when we revalidate and can't get the > value. If you have a test case, let's fix it.
Balbir, >> We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device >> capacity changes. However, this event does not automatically cause >> revalidation. > > The proposed idea is to not reinforce revalidation, unless explictly > specified (in the thread before Bob Liu had suggestions). The goal is > to notify user space of changes via RESIZE. SCSI sd can opt out of > this IOW, I can remove this if you feel > SDEV_EVT_CAPACITY_CHANGE_REPORTED is sufficient for current use cases. I have no particular objection to the code change. I was just observing that in the context of sd.c, RESIZE=1 is more of a "your request to resize was successful" notification due to the requirement of an explicit userland action in case a device reports a capacity change.
On Tue, 2020-01-07 at 22:15 -0500, Martin K. Petersen wrote: > Balbir, > > > > We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device > > > capacity changes. However, this event does not automatically cause > > > revalidation. > > > > The proposed idea is to not reinforce revalidation, unless explictly > > specified (in the thread before Bob Liu had suggestions). The goal is > > to notify user space of changes via RESIZE. SCSI sd can opt out of > > this IOW, I can remove this if you feel > > SDEV_EVT_CAPACITY_CHANGE_REPORTED is sufficient for current use cases. > > I have no particular objection to the code change. I was just observing > that in the context of sd.c, RESIZE=1 is more of a "your request to > resize was successful" notification due to the requirement of an > explicit userland action in case a device reports a capacity change. > That is true, yes I agree with your observation. Balbir Singh.
On Mon, Jan 06, 2020 at 10:48:30PM -0500, Martin K. Petersen wrote: > > Balbir, > > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > > index 5afb0046b12a..1a3be30b6b78 100644 > > --- a/drivers/scsi/sd.c > > +++ b/drivers/scsi/sd.c > > @@ -3184,7 +3184,7 @@ static int sd_revalidate_disk(struct gendisk *disk) > > > > sdkp->first_scan = 0; > > > > - set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity)); > > + disk_set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity)); > > sd_config_write_same(sdkp); > > kfree(buffer); > > We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device > capacity changes. However, this event does not automatically cause > revalidation. Who is looking at these sdev specific events? (And who is looking at the virtio/xenblk ones?) It makes a lot of sense to have one event supported by all devices. But don't ask me which one would be the best..
On Tue, 2020-01-07 at 21:59 -0500, Martin K. Petersen wrote: > Ewan, > > > Yes, there are some storage arrays that refuse a READ CAPACITY > > command in certain ALUA states so you can't get the new capacity > > anyway. > > Yep. And some devices will temporarily return a capacity of > 0xFFFFFFFF... If we were to trigger a filesystem resize, the results > would be disastrous. > > > It might be nice to improve this, though, there are some cases now > > where we set the capacity to zero when we revalidate and can't get the > > value. > > If you have a test case, let's fix it. > This happens with NVMe fabric devices, I thought. I'll check.
On Tue, 2020-01-07 at 22:15 -0500, Martin K. Petersen wrote: > Balbir, > > > > We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device > > > capacity changes. However, this event does not automatically cause > > > revalidation. > > > > The proposed idea is to not reinforce revalidation, unless explictly > > specified (in the thread before Bob Liu had suggestions). The goal is > > to notify user space of changes via RESIZE. SCSI sd can opt out of > > this IOW, I can remove this if you feel > > SDEV_EVT_CAPACITY_CHANGE_REPORTED is sufficient for current use cases. Remember that this event is generated because of a Unit Attention from the device. We are only passing on this indication to udev. It basically allows automation without having to scrape the log file. We don't proactively look. e.g. in the case of SCSI unless you have commands being sent to the device to return the UA status you won't hear about it. > > I have no particular objection to the code change. I was just observing > that in the context of sd.c, RESIZE=1 is more of a "your request to > resize was successful" notification due to the requirement of an > explicit userland action in case a device reports a capacity change. >
Christoph, >> We already emit an SDEV_EVT_CAPACITY_CHANGE_REPORTED event if device >> capacity changes. However, this event does not automatically cause >> revalidation. > > Who is looking at these sdev specific events? (And who is looking > at the virtio/xenblk ones?) It makes a lot of sense to have one event > supported by all devices. But don't ask me which one would be the best.. Users typically have site-specific udev rules or similar. There is no standard entity handling these events. Sadly. I'm all for standardizing on RESIZE=1. However, we can't really get rid of the emitting existing SDEV* event without breaking existing setups.
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 5afb0046b12a..1a3be30b6b78 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -3184,7 +3184,7 @@ static int sd_revalidate_disk(struct gendisk *disk) sdkp->first_scan = 0; - set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity)); + disk_set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity)); sd_config_write_same(sdkp); kfree(buffer);
block/genhd provides disk_set_capacity() for sending RESIZE notifications via uevents. This notification is newly added to scsi sd. Signed-off-by: Balbir Singh <sblbir@amazon.com> --- drivers/scsi/sd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)