Allow block/genhd to notify user space about disk size changes
using a new helper disk_set_capacity(), which is a wrapper on top
of set_capacity(). disk_set_capacity() will only notify if
the current capacity or the target capacity is not zero.
Background:
As a part of a patch to allow sending the RESIZE event on disk capacity
change, Christoph ([email protected]) requested that the patch be made generic
and the hacks for virtio block and xen block devices be removed and
merged via a generic helper.
This series consists of 5 changes. The first one adds the basic
support for changing the size and notifying. The follow up patches
are per block subsystem changes. Other block drivers can add their
changes as necessary on top of this series.
Testing:
1. I did some basic testing with an NVME device, by resizing it in
the backend and ensured that udevd received the event.
NOTE: After these changes, the notification might happen before
revalidate disk, where as it occured later before.
Balbir Singh (5):
block/genhd: Notify udev about capacity change
drivers/block/virtio_blk.c: Convert to use disk_set_capacity
drivers/block/xen-blkfront.c: Convert to use disk_set_capacity
drivers/nvme/host/core.c: Convert to use disk_set_capacity
drivers/scsi/sd.c: Convert to use disk_set_capacity
block/genhd.c | 19 +++++++++++++++++++
drivers/block/virtio_blk.c | 4 +---
drivers/block/xen-blkfront.c | 5 +----
drivers/nvme/host/core.c | 2 +-
drivers/scsi/sd.c | 2 +-
include/linux/genhd.h | 1 +
6 files changed, 24 insertions(+), 9 deletions(-)
--
2.16.5
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 <[email protected]>
---
drivers/scsi/sd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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);
--
2.16.5
Allow block/genhd to notify user space (via udev) about disk size changes
using a new helper disk_set_capacity(), which is a wrapper on top
of set_capacity(). disk_set_capacity() will only notify via udev if
the current capacity or the target capacity is not zero.
Suggested-by: Christoph Hellwig <[email protected]>
Signed-off-by: Someswarudu Sangaraju <[email protected]>
Signed-off-by: Balbir Singh <[email protected]>
---
block/genhd.c | 19 +++++++++++++++++++
include/linux/genhd.h | 1 +
2 files changed, 20 insertions(+)
diff --git a/block/genhd.c b/block/genhd.c
index ff6268970ddc..94faec98607b 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -46,6 +46,25 @@ static void disk_add_events(struct gendisk *disk);
static void disk_del_events(struct gendisk *disk);
static void disk_release_events(struct gendisk *disk);
+/*
+ * Set disk capacity and notify if the size is not currently
+ * zero and will not be set to zero
+ */
+void disk_set_capacity(struct gendisk *disk, sector_t size)
+{
+ sector_t capacity = get_capacity(disk);
+
+ set_capacity(disk, size);
+ if (capacity != 0 && size != 0) {
+ char *envp[] = { "RESIZE=1", NULL };
+
+ kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp);
+ }
+}
+
+EXPORT_SYMBOL_GPL(disk_set_capacity);
+
+
void part_inc_in_flight(struct request_queue *q, struct hd_struct *part, int rw)
{
if (queue_is_mq(q))
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index a927829bb73a..f1a5ddcc781d 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -449,6 +449,7 @@ static inline int get_disk_ro(struct gendisk *disk)
extern void disk_block_events(struct gendisk *disk);
extern void disk_unblock_events(struct gendisk *disk);
extern void disk_flush_events(struct gendisk *disk, unsigned int mask);
+extern void disk_set_capacity(struct gendisk *disk, sector_t size);
extern unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask);
/* drivers/char/random.c */
--
2.16.5
block/genhd provides disk_set_capacity() for sending
RESIZE notifications via uevents. This notification is
newly added to NVME devices
Signed-off-by: Balbir Singh <[email protected]>
---
drivers/nvme/host/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 667f18f465be..cb214e914fc2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1808,7 +1808,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
ns->lba_shift > PAGE_SHIFT)
capacity = 0;
- set_capacity(disk, capacity);
+ disk_set_capacity(disk, capacity);
nvme_config_discard(disk, ns);
nvme_config_write_zeroes(disk, ns);
--
2.16.5
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.
On 01/01/2020 11:53 PM, Balbir Singh wrote:
> Allow block/genhd to notify user space (via udev) about disk size changes
> using a new helper disk_set_capacity(), which is a wrapper on top
> of set_capacity(). disk_set_capacity() will only notify via udev if
> the current capacity or the target capacity is not zero.
>
> Suggested-by: Christoph Hellwig<[email protected]>
> Signed-off-by: Someswarudu Sangaraju<[email protected]>
> Signed-off-by: Balbir Singh<[email protected]>
> ---
Since disk_set_capacity(() is on the same line as set_capacity()
we should follow the same convention, which is :-
1. Avoid exporting symbol.
2. Mark new function in-line.
Unless there is a very specific reason for breaking the pattern.
Why not this (totally untested but easy convey above comment)
on the top of this patch ?
# git diff
diff --git a/block/genhd.c b/block/genhd.c
index 94faec98607b..ff6268970ddc 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -46,25 +46,6 @@ static void disk_add_events(struct gendisk *disk);
static void disk_del_events(struct gendisk *disk);
static void disk_release_events(struct gendisk *disk);
-/*
- * Set disk capacity and notify if the size is not currently
- * zero and will not be set to zero
- */
-void disk_set_capacity(struct gendisk *disk, sector_t size)
-{
- sector_t capacity = get_capacity(disk);
-
- set_capacity(disk, size);
- if (capacity != 0 && size != 0) {
- char *envp[] = { "RESIZE=1", NULL };
-
- kobject_uevent_env(&disk_to_dev(disk)->kobj,
KOBJ_CHANGE, envp);
- }
-}
-
-EXPORT_SYMBOL_GPL(disk_set_capacity);
-
-
void part_inc_in_flight(struct request_queue *q, struct hd_struct
*part, int rw)
{
if (queue_is_mq(q))
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index d5e87d7cc357..5e595a28f893 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -469,6 +469,22 @@ static inline void set_capacity(struct gendisk
*disk, sector_t size)
disk->part0.nr_sects = size;
}
+/*
+ * Set disk capacity and notify if the size is not currently
+ * zero and will not be set to zero
+ */
+static inline void disk_set_capacity(struct gendisk *disk, sector_t size)
+{
+ sector_t capacity = get_capacity(disk);
+
+ set_capacity(disk, size);
+ if (capacity != 0 && size != 0) {
+ char *envp[] = { "RESIZE=1", NULL };
+
+ kobject_uevent_env(&disk_to_dev(disk)->kobj,
KOBJ_CHANGE, envp);
+ }
+}
+
#ifdef CONFIG_SOLARIS_X86_PARTITION
On Fri, 2020-01-03 at 06:16 +0000, Chaitanya Kulkarni wrote:
> On 01/01/2020 11:53 PM, Balbir Singh wrote:
> > Allow block/genhd to notify user space (via udev) about disk size changes
> > using a new helper disk_set_capacity(), which is a wrapper on top
> > of set_capacity(). disk_set_capacity() will only notify via udev if
> > the current capacity or the target capacity is not zero.
> >
> > Suggested-by: Christoph Hellwig<[email protected]>
> > Signed-off-by: Someswarudu Sangaraju<[email protected]>
> > Signed-off-by: Balbir Singh<[email protected]>
> > ---
>
> Since disk_set_capacity(() is on the same line as set_capacity()
> we should follow the same convention, which is :-
>
> 1. Avoid exporting symbol.
> 2. Mark new function in-line.
>
> Unless there is a very specific reason for breaking the pattern.
>
I don't see the benefit of marking it as inline. I expect this code to be
potentially expanded to provide callbacks into file system code for expansion
(something you brought up), marking it as inline might be a limitation.
I'd rather not clutter the headers with this code, but I am open to what the
maintainers think as well.
Balbir Singh.
Quick question here if user executes nvme ns-rescan /dev/nvme1
will following code result in triggering uevent(s) for
the namespace(s( for which there is no change in the size ?
If so is that an expected behavior ?
On 01/01/2020 11:54 PM, Balbir Singh wrote:
> block/genhd provides disk_set_capacity() for sending
> RESIZE notifications via uevents. This notification is
> newly added to NVME devices
>
> Signed-off-by: Balbir Singh <[email protected]>
> ---
> drivers/nvme/host/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 667f18f465be..cb214e914fc2 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1808,7 +1808,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
> ns->lba_shift > PAGE_SHIFT)
> capacity = 0;
>
> - set_capacity(disk, capacity);
> + disk_set_capacity(disk, capacity);
>
> nvme_config_discard(disk, ns);
> nvme_config_write_zeroes(disk, ns);
>
On 01/03/2020 08:44 PM, Singh, Balbir wrote:
>> >Since disk_set_capacity(() is on the same line as set_capacity()
>> >we should follow the same convention, which is :-
>> >
>> >1. Avoid exporting symbol.
>> >2. Mark new function in-line.
>> >
>> >Unless there is a very specific reason for breaking the pattern.
>> >
> I don't see the benefit of marking it as inline. I expect this code to be
> potentially expanded to provide callbacks into file system code for expansion
> (something you brought up), marking it as inline might be a limitation.
>
That can be done as a prep patch when framework is ready.
> I'd rather not clutter the headers with this code, but I am open to what the
> maintainers think as well.
>
Okay.
> Balbir Singh.
>
>
On Sat, 2020-01-04 at 22:27 +0000, Chaitanya Kulkarni wrote:
> Quick question here if user executes nvme ns-rescan /dev/nvme1
> will following code result in triggering uevent(s) for
> the namespace(s( for which there is no change in the size ?
>
> If so is that an expected behavior ?
>
My old code had a check to see if old_capacity != new_capacity as well.
I can redo those bits if needed.
The expected behaviour is not clear, but the functionality is not broken, user
space should be able to deal with a resize event where the previous capacity
== new capacity IMHO.
Balbir Singh.
> On 01/01/2020 11:54 PM, Balbir Singh wrote:
> > block/genhd provides disk_set_capacity() for sending
> > RESIZE notifications via uevents. This notification is
> > newly added to NVME devices
> >
> > Signed-off-by: Balbir Singh <[email protected]>
> > ---
> > drivers/nvme/host/core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 667f18f465be..cb214e914fc2 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -1808,7 +1808,7 @@ static void nvme_update_disk_info(struct gendisk
> > *disk,
> > ns->lba_shift > PAGE_SHIFT)
> > capacity = 0;
> >
> > - set_capacity(disk, capacity);
> > + disk_set_capacity(disk, capacity);
> >
> > nvme_config_discard(disk, ns);
> > nvme_config_write_zeroes(disk, ns);
> >
>
>
On 1/2/20 3:53 PM, Balbir Singh wrote:
> Allow block/genhd to notify user space about disk size changes
> using a new helper disk_set_capacity(), which is a wrapper on top
> of set_capacity(). disk_set_capacity() will only notify if
> the current capacity or the target capacity is not zero.
>
set_capacity_and_notify() may be a more straightforward name.
> Background:
>
> As a part of a patch to allow sending the RESIZE event on disk capacity
> change, Christoph ([email protected]) requested that the patch be made generic
> and the hacks for virtio block and xen block devices be removed and
> merged via a generic helper.
>
> This series consists of 5 changes. The first one adds the basic
> support for changing the size and notifying. The follow up patches
> are per block subsystem changes. Other block drivers can add their
> changes as necessary on top of this series.
>
> Testing:
> 1. I did some basic testing with an NVME device, by resizing it in
> the backend and ensured that udevd received the event.
>
> NOTE: After these changes, the notification might happen before
> revalidate disk, where as it occured later before.
>
It's better not to change original behavior.
How about something like:
+void set_capacity_and_notify(struct gendisk *disk, sector_t size, bool revalidate)
{
sector_t capacity = get_capacity(disk);
set_capacity(disk, size);
+ if (revalidate)
+ revalidate_disk(disk);
if (capacity != 0 && size != 0) {
char *envp[] = { "RESIZE=1", NULL };
kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp);
}
}
> Balbir Singh (5):
> block/genhd: Notify udev about capacity change
> drivers/block/virtio_blk.c: Convert to use disk_set_capacity
> drivers/block/xen-blkfront.c: Convert to use disk_set_capacity
> drivers/nvme/host/core.c: Convert to use disk_set_capacity
> drivers/scsi/sd.c: Convert to use disk_set_capacity
>
> block/genhd.c | 19 +++++++++++++++++++
> drivers/block/virtio_blk.c | 4 +---
> drivers/block/xen-blkfront.c | 5 +----
> drivers/nvme/host/core.c | 2 +-
> drivers/scsi/sd.c | 2 +-
> include/linux/genhd.h | 1 +
> 6 files changed, 24 insertions(+), 9 deletions(-)
>
On Mon, 2020-01-06 at 13:59 +0800, Bob Liu wrote:
> On 1/2/20 3:53 PM, Balbir Singh wrote:
> > Allow block/genhd to notify user space about disk size changes
> > using a new helper disk_set_capacity(), which is a wrapper on top
> > of set_capacity(). disk_set_capacity() will only notify if
> > the current capacity or the target capacity is not zero.
> >
>
> set_capacity_and_notify() may be a more straightforward name.
>
Yes, agreed.
> > Background:
> >
> > As a part of a patch to allow sending the RESIZE event on disk capacity
> > change, Christoph ([email protected]) requested that the patch be made generic
> > and the hacks for virtio block and xen block devices be removed and
> > merged via a generic helper.
> >
> > This series consists of 5 changes. The first one adds the basic
> > support for changing the size and notifying. The follow up patches
> > are per block subsystem changes. Other block drivers can add their
> > changes as necessary on top of this series.
> >
> > Testing:
> > 1. I did some basic testing with an NVME device, by resizing it in
> > the backend and ensured that udevd received the event.
> >
> > NOTE: After these changes, the notification might happen before
> > revalidate disk, where as it occured later before.
> >
>
> It's better not to change original behavior.
> How about something like:
>
> +void set_capacity_and_notify(struct gendisk *disk, sector_t size, bool
> revalidate)
> {
> sector_t capacity = get_capacity(disk);
>
> set_capacity(disk, size);
>
> + if (revalidate)
> + revalidate_disk(disk);
Do you see a concern with the notification going out before revalidate_disk?
I could keep the behaviour and come up with a suitable name
revalidate_disk_and_notify() (set_capacity is a part of the revalidation
process), or feel free to suggest a better name
Thanks,
Balbir Singh
On 1/6/20 4:47 PM, Singh, Balbir wrote:
> On Mon, 2020-01-06 at 13:59 +0800, Bob Liu wrote:
>> On 1/2/20 3:53 PM, Balbir Singh wrote:
>>> Allow block/genhd to notify user space about disk size changes
>>> using a new helper disk_set_capacity(), which is a wrapper on top
>>> of set_capacity(). disk_set_capacity() will only notify if
>>> the current capacity or the target capacity is not zero.
>>>
>>
>> set_capacity_and_notify() may be a more straightforward name.
>>
>
> Yes, agreed.
>
>>> Background:
>>>
>>> As a part of a patch to allow sending the RESIZE event on disk capacity
>>> change, Christoph ([email protected]) requested that the patch be made generic
>>> and the hacks for virtio block and xen block devices be removed and
>>> merged via a generic helper.
>>>
>>> This series consists of 5 changes. The first one adds the basic
>>> support for changing the size and notifying. The follow up patches
>>> are per block subsystem changes. Other block drivers can add their
>>> changes as necessary on top of this series.
>>>
>>> Testing:
>>> 1. I did some basic testing with an NVME device, by resizing it in
>>> the backend and ensured that udevd received the event.
>>>
>>> NOTE: After these changes, the notification might happen before
>>> revalidate disk, where as it occured later before.
>>>
>>
>> It's better not to change original behavior.
>> How about something like:
>>
>> +void set_capacity_and_notify(struct gendisk *disk, sector_t size, bool
>> revalidate)
>> {
>> sector_t capacity = get_capacity(disk);
>>
>> set_capacity(disk, size);
>>
>> + if (revalidate)
>> + revalidate_disk(disk);
>
> Do you see a concern with the notification going out before revalidate_disk?
Not aware any, but keep the original behavior is safer especial when not must change..
> I could keep the behaviour and come up with a suitable name
>
> revalidate_disk_and_notify() (set_capacity is a part of the revalidation
> process), or feel free to suggest a better name
>
Feel free to add my reviewed-by next version in this series.
Reviewed-by: Bob Liu <[email protected]>
> Thanks,
> Balbir Singh
>
Hi Balbir,
> Allow block/genhd to notify user space (via udev) about disk size
> changes using a new helper disk_set_capacity(), which is a wrapper on
> top of set_capacity(). disk_set_capacity() will only notify via udev
> if the current capacity or the target capacity is not zero.
I know set_capacity() is called all over the place making it a bit of a
pain to audit. Is that the reason you introduced a new function instead
of just emitting the event in set_capacity()?
--
Martin K. Petersen Oracle Linux Engineering
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.
--
Martin K. Petersen Oracle Linux Engineering
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.
--
Martin K. Petersen Oracle Linux Engineering
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.
On Mon, 2020-01-06 at 22:32 -0500, Martin K. Petersen wrote:
> Hi Balbir,
>
> > Allow block/genhd to notify user space (via udev) about disk size
> > changes using a new helper disk_set_capacity(), which is a wrapper on
> > top of set_capacity(). disk_set_capacity() will only notify via udev
> > if the current capacity or the target capacity is not zero.
>
> I know set_capacity() is called all over the place making it a bit of a
> pain to audit. Is that the reason you introduced a new function instead
> of just emitting the event in set_capacity()?
>
I did this to avoid having to enforce that set_capacity() implied a
notification. Largely to control the impact of the change by default.
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.
--
Martin K. Petersen Oracle Linux Engineering
Balbir,
> I did this to avoid having to enforce that set_capacity() implied a
> notification. Largely to control the impact of the change by default.
What I thought. I'm OK with set_capacity_and_notify(), btw.
--
Martin K. Petersen Oracle Linux Engineering
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.
--
Martin K. Petersen Oracle Linux Engineering
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 Tue, Jan 07, 2020 at 10:15:34PM -0500, Martin K. Petersen wrote:
>
> Balbir,
>
> > I did this to avoid having to enforce that set_capacity() implied a
> > notification. Largely to control the impact of the change by default.
>
> What I thought. I'm OK with set_capacity_and_notify(), btw.
To some extent it might make sense to always notify from set_capacity
and have a set_capacity_nonotify if we don't want to notify, as in
general we probably should notify unless we have a good reason not to.
On Fri, Jan 03, 2020 at 06:16:39AM +0000, Chaitanya Kulkarni wrote:
> Since disk_set_capacity(() is on the same line as set_capacity()
> we should follow the same convention, which is :-
>
> 1. Avoid exporting symbol.
> 2. Mark new function in-line.
>
> Unless there is a very specific reason for breaking the pattern.
Why would we mark it as inline? It isn't by any means in the fast
path, and there are no easy opportunities for constant propagation,
so the only thing that would do is increase the code size.
On Mon, Jan 06, 2020 at 12:46:26AM +0000, Singh, Balbir wrote:
> On Sat, 2020-01-04 at 22:27 +0000, Chaitanya Kulkarni wrote:
> > Quick question here if user executes nvme ns-rescan /dev/nvme1
> > will following code result in triggering uevent(s) for
> > the namespace(s( for which there is no change in the size ?
> >
> > If so is that an expected behavior ?
> >
>
> My old code had a check to see if old_capacity != new_capacity as well.
> I can redo those bits if needed.
>
> The expected behaviour is not clear, but the functionality is not broken, user
> space should be able to deal with a resize event where the previous capacity
> == new capacity IMHO.
I think it makes sense to not bother with a notification unless there
is an actual change.
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.
--
Martin K. Petersen Oracle Linux Engineering
Christoph,
>> The expected behaviour is not clear, but the functionality is not
>> broken, user space should be able to deal with a resize event where
>> the previous capacity == new capacity IMHO.
>
> I think it makes sense to not bother with a notification unless there
> is an actual change.
I agree.
--
Martin K. Petersen Oracle Linux Engineering
On Wed, 2020-01-08 at 22:33 -0500, Martin K. Petersen wrote:
> Christoph,
>
> > > The expected behaviour is not clear, but the functionality is not
> > > broken, user space should be able to deal with a resize event where
> > > the previous capacity == new capacity IMHO.
> >
> > I think it makes sense to not bother with a notification unless there
> > is an actual change.
>
> I agree.
>
Yes, absolutely.
On Wed, 2020-01-08 at 16:04 +0100, [email protected] wrote:
> On Mon, Jan 06, 2020 at 12:46:26AM +0000, Singh, Balbir wrote:
> > On Sat, 2020-01-04 at 22:27 +0000, Chaitanya Kulkarni wrote:
> > > Quick question here if user executes nvme ns-rescan /dev/nvme1
> > > will following code result in triggering uevent(s) for
> > > the namespace(s( for which there is no change in the size ?
> > >
> > > If so is that an expected behavior ?
> > >
> >
> > My old code had a check to see if old_capacity != new_capacity as well.
> > I can redo those bits if needed.
> >
> > The expected behaviour is not clear, but the functionality is not broken,
> > user
> > space should be able to deal with a resize event where the previous
> > capacity
> > == new capacity IMHO.
>
> I think it makes sense to not bother with a notification unless there
> is an actual change.
[Sorry for the delayed response, just back from LCA]
Agreed!
Balbir Singh
On Wed, 2020-01-08 at 16:04 +0100, [email protected] wrote:
> On Tue, Jan 07, 2020 at 10:15:34PM -0500, Martin K. Petersen wrote:
> >
> > Balbir,
> >
> > > I did this to avoid having to enforce that set_capacity() implied a
> > > notification. Largely to control the impact of the change by default.
> >
> > What I thought. I'm OK with set_capacity_and_notify(), btw.
>
> To some extent it might make sense to always notify from set_capacity
> and have a set_capacity_nonotify if we don't want to notify, as in
> general we probably should notify unless we have a good reason not to.
I am not sure if this is the right path, since with the new interface, we'll
now have revalidate disk bits built into the function (see the comments from
Bob earlier). I'd be happier converting interfaces a few at a time, A quick
grep found over 100 references and I am not even sure how many of them are
resizable without hotplug in/out on the fly. Hence, I limited myself to the
small subset, I could consider expanding it later based on the need and their
ability to resize on the fly
Balbir Singh.