Re: [PATCH] virtio-blk: emit udev event when device is resized

Re: [PATCH] virtio-blk: emit udev event when device is resized

[Greg KH]

On Fri, Feb 22, 2013 at 10:14:49AM +1030, Rusty Russell wrote:
> Milos Vyletel <milos.vyletel@sde.cz> writes:
> 
> > When virtio-blk device is resized from host (using block_resize from QEMU) emit
> > KOBJ_CHANGE uevent to notify guest about such change. This allows user to have
> > custom udev rules which would take whatever action if such event occurs. As a
> > proof of concept I've created simple udev rule that automatically resize
> > filesystem on virtio-blk device.
> >
> > ACTION="change", KERNEL="vd*", \
> >         ENV{RESIZE}="1", \
> >         ENV{ID_FS_TYPE}="ext[3-4]", \
> >         RUN+="/sbin/resize2fs /dev/%k"
> > ACTION="change", KERNEL="vd*", \
> >         ENV{RESIZE}="1", \
> >         ENV{ID_FS_TYPE}="LVM2_member", \
> >         RUN+="/sbin/pvresize /dev/%k"
> 
> This looks fine to me, but I like to check with Greg before adding udev
> callouts.... Greg?

Hm, I thought we were frowning apon running binaries from udev rules
these days, especially ones that might have big consequences (like
resizing a disk image) like this.

Kay, am I right?

We already emit KOBJECT_CHANGE events when block devices change, from
within the block core code.  Why is the patch below needed instead of
using these events that are already generated?  How are virtio block
devices special?

> BTW, if this is good, it's good for stable IMHO.

What bug does it fix?

thanks,

greg k-h

> Cheers,
> Rusty.
> 
> > Signed-off-by: Milos Vyletel <milos.vyletel@sde.cz>
> > ---
> >  drivers/block/virtio_blk.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 8ad21a2..5990382 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -539,6 +539,8 @@ static void virtblk_config_changed_work(struct work_struct *work)
> >  	struct virtio_device *vdev = vblk->vdev;
> >  	struct request_queue *q = vblk->disk->queue;
> >  	char cap_str_2[10], cap_str_10[10];
> > +	char event[] = "RESIZE=1";
> > +	char *envp[] = { event, NULL };
> >  	u64 capacity, size;
> >  
> >  	mutex_lock(&vblk->config_lock);
> > @@ -568,6 +570,7 @@ static void virtblk_config_changed_work(struct work_struct *work)
> >  
> >  	set_capacity(vblk->disk, capacity);
> >  	revalidate_disk(vblk->disk);
> > +	kobject_uevent_env(&disk_to_dev(vblk->disk)->kobj, KOBJ_CHANGE, envp);
> >  done:
> >  	mutex_unlock(&vblk->config_lock);
> >  }
> > -- 
> > 1.7.1

Re: [PATCH] virtio-blk: emit udev event when device is resized

[Kay Sievers]

On Mon, Feb 25, 2013 at 11:12 PM, Greg KH <greg@kroah.com> wrote:

> Hm, I thought we were frowning apon running binaries from udev rules
> these days, especially ones that might have big consequences (like
> resizing a disk image) like this.
>
> Kay, am I right?

We removed most of them from the default setups, yes. But there is
nothing wrong if people want to ship that in some package or as custom
rules.

It looks fine to me, we would just not add such things to the default
set of of rules these days.

> We already emit KOBJECT_CHANGE events when block devices change, from
> within the block core code.  Why is the patch below needed instead of
> using these events that are already generated?  How are virtio block
> devices special?

I think we only do that for dm and md and a couple of special cases
like loop and read-only settings.

Kay

Re: [PATCH] virtio-blk: emit udev event when device is resized

[Greg KH]

On Mon, Feb 25, 2013 at 11:39:52PM +0100, Kay Sievers wrote:
> On Mon, Feb 25, 2013 at 11:12 PM, Greg KH <greg@kroah.com> wrote:
> 
> > Hm, I thought we were frowning apon running binaries from udev rules
> > these days, especially ones that might have big consequences (like
> > resizing a disk image) like this.
> >
> > Kay, am I right?
> 
> We removed most of them from the default setups, yes. But there is
> nothing wrong if people want to ship that in some package or as custom
> rules.
> 
> It looks fine to me, we would just not add such things to the default
> set of of rules these days.
> 
> > We already emit KOBJECT_CHANGE events when block devices change, from
> > within the block core code.  Why is the patch below needed instead of
> > using these events that are already generated?  How are virtio block
> > devices special?
> 
> I think we only do that for dm and md and a couple of special cases
> like loop and read-only settings.

What about when we repartition a block device?  I've seen the events
happen then.

Anyway, if you are ok with this, no objection from my side then Rusty.

thanks,

greg k-h

Re: [PATCH] virtio-blk: emit udev event when device is resized

[Kay Sievers]

On Mon, Feb 25, 2013 at 11:43 PM, Greg KH <greg@kroah.com> wrote:
> On Mon, Feb 25, 2013 at 11:39:52PM +0100, Kay Sievers wrote:
>> On Mon, Feb 25, 2013 at 11:12 PM, Greg KH <greg@kroah.com> wrote:
>>
>> > Hm, I thought we were frowning apon running binaries from udev rules
>> > these days, especially ones that might have big consequences (like
>> > resizing a disk image) like this.
>> >
>> > Kay, am I right?
>>
>> We removed most of them from the default setups, yes. But there is
>> nothing wrong if people want to ship that in some package or as custom
>> rules.
>>
>> It looks fine to me, we would just not add such things to the default
>> set of of rules these days.
>>
>> > We already emit KOBJECT_CHANGE events when block devices change, from
>> > within the block core code.  Why is the patch below needed instead of
>> > using these events that are already generated?  How are virtio block
>> > devices special?
>>
>> I think we only do that for dm and md and a couple of special cases
>> like loop and read-only settings.
>
> What about when we repartition a block device?  I've seen the events
> happen then.

Right, from the common block code we send events for removable media
changes like cdroms, sd cards, when a device is switched to read-only,
and when we re-scan a partition table like on re-partitioning. Most of
the other events are block subsystem-specific like this one. For
things like device-mapper they are used pretty heavily.

> Anyway, if you are ok with this, no objection from my side then Rusty.

Looks fine to me, it should not do any harm if there are not heavy
programs hooked up -- which is nothing the kernel could fix if people
do that. :)

Kay

Re: [PATCH] virtio-blk: emit udev event when device is resized

[Milos Vyletel]

On Feb 25, 2013, at 5:39 PM, Kay Sievers <kay@vrfy.org> wrote:

> On Mon, Feb 25, 2013 at 11:12 PM, Greg KH <greg@kroah.com> wrote:
> 
>> Hm, I thought we were frowning apon running binaries from udev rules
>> these days, especially ones that might have big consequences (like
>> resizing a disk image) like this.
>> 
>> Kay, am I right?
> 
> We removed most of them from the default setups, yes. But there is
> nothing wrong if people want to ship that in some package or as custom
> rules.
> 
> It looks fine to me, we would just not add such things to the default
> set of of rules these days.

That was not my intention. I just wanted to demonstrate that event like this
could be useful in cases like filesystem resize. We have a need for automatic
resize by our customers so we will ship our custom udev rules. It's perfectly
understandable that default udev rules will not have this.

Milos

> 
>> We already emit KOBJECT_CHANGE events when block devices change, from
>> within the block core code.  Why is the patch below needed instead of
>> using these events that are already generated?  How are virtio block
>> devices special?
> 
> I think we only do that for dm and md and a couple of special cases
> like loop and read-only settings.
> 
> Kay

Re: [PATCH] virtio-blk: emit udev event when device is resized

[Milos Vyletel]

On Feb 25, 2013, at 5:12 PM, Greg KH <greg@kroah.com> wrote:

> On Fri, Feb 22, 2013 at 10:14:49AM +1030, Rusty Russell wrote:
>> Milos Vyletel <milos.vyletel@sde.cz> writes:
>> 
>>> When virtio-blk device is resized from host (using block_resize from QEMU) emit
>>> KOBJ_CHANGE uevent to notify guest about such change. This allows user to have
>>> custom udev rules which would take whatever action if such event occurs. As a
>>> proof of concept I've created simple udev rule that automatically resize
>>> filesystem on virtio-blk device.
>>> 
>>> ACTION="change", KERNEL="vd*", \
>>>        ENV{RESIZE}="1", \
>>>        ENV{ID_FS_TYPE}="ext[3-4]", \
>>>        RUN+="/sbin/resize2fs /dev/%k"
>>> ACTION="change", KERNEL="vd*", \
>>>        ENV{RESIZE}="1", \
>>>        ENV{ID_FS_TYPE}="LVM2_member", \
>>>        RUN+="/sbin/pvresize /dev/%k"
>> 
>> This looks fine to me, but I like to check with Greg before adding udev
>> callouts.... Greg?
> 
> Hm, I thought we were frowning apon running binaries from udev rules
> these days, especially ones that might have big consequences (like
> resizing a disk image) like this.
> 
> Kay, am I right?
> 
> We already emit KOBJECT_CHANGE events when block devices change, from
> within the block core code.  Why is the patch below needed instead of
> using these events that are already generated?  How are virtio block
> devices special?
> 
>> BTW, if this is good, it's good for stable IMHO.
> 
> What bug does it fix?
> 

It is not really a bug but it definitely is useful enhancement to have in stable too. I
can imagine lots of people can benefit from this.

Milos

> thanks,
> 
> greg k-h
> 
>> Cheers,
>> Rusty.
>> 
>>> Signed-off-by: Milos Vyletel <milos.vyletel@sde.cz>
>>> ---
>>> drivers/block/virtio_blk.c |    3 +++
>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>> 
>>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>>> index 8ad21a2..5990382 100644
>>> --- a/drivers/block/virtio_blk.c
>>> +++ b/drivers/block/virtio_blk.c
>>> @@ -539,6 +539,8 @@ static void virtblk_config_changed_work(struct work_struct *work)
>>> 	struct virtio_device *vdev = vblk->vdev;
>>> 	struct request_queue *q = vblk->disk->queue;
>>> 	char cap_str_2[10], cap_str_10[10];
>>> +	char event[] = "RESIZE=1";
>>> +	char *envp[] = { event, NULL };
>>> 	u64 capacity, size;
>>> 
>>> 	mutex_lock(&vblk->config_lock);
>>> @@ -568,6 +570,7 @@ static void virtblk_config_changed_work(struct work_struct *work)
>>> 
>>> 	set_capacity(vblk->disk, capacity);
>>> 	revalidate_disk(vblk->disk);
>>> +	kobject_uevent_env(&disk_to_dev(vblk->disk)->kobj, KOBJ_CHANGE, envp);
>>> done:
>>> 	mutex_unlock(&vblk->config_lock);
>>> }
>>> -- 
>>> 1.7.1

Re: [PATCH] virtio-blk: emit udev event when device is resized

[Rusty Russell]

Milos Vyletel <milos.vyletel@sde.cz> writes:
> On Feb 25, 2013, at 5:12 PM, Greg KH <greg@kroah.com> wrote:
>
>> On Fri, Feb 22, 2013 at 10:14:49AM +1030, Rusty Russell wrote:
>>> Milos Vyletel <milos.vyletel@sde.cz> writes:
>>> 
>>>> When virtio-blk device is resized from host (using block_resize from QEMU) emit
>>>> KOBJ_CHANGE uevent to notify guest about such change. This allows user to have
>>>> custom udev rules which would take whatever action if such event occurs. As a
>>>> proof of concept I've created simple udev rule that automatically resize
>>>> filesystem on virtio-blk device.
>>>> 
>>>> ACTION="change", KERNEL="vd*", \
>>>>        ENV{RESIZE}="1", \
>>>>        ENV{ID_FS_TYPE}="ext[3-4]", \
>>>>        RUN+="/sbin/resize2fs /dev/%k"
>>>> ACTION="change", KERNEL="vd*", \
>>>>        ENV{RESIZE}="1", \
>>>>        ENV{ID_FS_TYPE}="LVM2_member", \
>>>>        RUN+="/sbin/pvresize /dev/%k"
>>> 
>>> This looks fine to me, but I like to check with Greg before adding udev
>>> callouts.... Greg?
>> 
>> Hm, I thought we were frowning apon running binaries from udev rules
>> these days, especially ones that might have big consequences (like
>> resizing a disk image) like this.
>> 
>> Kay, am I right?
>> 
>> We already emit KOBJECT_CHANGE events when block devices change, from
>> within the block core code.  Why is the patch below needed instead of
>> using these events that are already generated?  How are virtio block
>> devices special?
>> 
>>> BTW, if this is good, it's good for stable IMHO.
>> 
>> What bug does it fix?
>> 
>
> It is not really a bug but it definitely is useful enhancement to have in stable too. I
> can imagine lots of people can benefit from this.

But that applies to almost any enhancement :)

It will go in *next* merge window, not this one.

Thanks,
Rusty.

Re: [PATCH] virtio-blk: emit udev event when device is resized

[Milos Vyletel]

----- Original Message -----
> Milos Vyletel <milos.vyletel@sde.cz> writes:
> > On Feb 25, 2013, at 5:12 PM, Greg KH <greg@kroah.com> wrote:
> >
> >> On Fri, Feb 22, 2013 at 10:14:49AM +1030, Rusty Russell wrote:
> >>> Milos Vyletel <milos.vyletel@sde.cz> writes:
> >>> 
> >>>> When virtio-blk device is resized from host (using block_resize from
> >>>> QEMU) emit
> >>>> KOBJ_CHANGE uevent to notify guest about such change. This allows user
> >>>> to have
> >>>> custom udev rules which would take whatever action if such event occurs.
> >>>> As a
> >>>> proof of concept I've created simple udev rule that automatically resize
> >>>> filesystem on virtio-blk device.
> >>>> 
> >>>> ACTION="change", KERNEL="vd*", \
> >>>>        ENV{RESIZE}="1", \
> >>>>        ENV{ID_FS_TYPE}="ext[3-4]", \
> >>>>        RUN+="/sbin/resize2fs /dev/%k"
> >>>> ACTION="change", KERNEL="vd*", \
> >>>>        ENV{RESIZE}="1", \
> >>>>        ENV{ID_FS_TYPE}="LVM2_member", \
> >>>>        RUN+="/sbin/pvresize /dev/%k"
> >>> 
> >>> This looks fine to me, but I like to check with Greg before adding udev
> >>> callouts.... Greg?
> >> 
> >> Hm, I thought we were frowning apon running binaries from udev rules
> >> these days, especially ones that might have big consequences (like
> >> resizing a disk image) like this.
> >> 
> >> Kay, am I right?
> >> 
> >> We already emit KOBJECT_CHANGE events when block devices change, from
> >> within the block core code.  Why is the patch below needed instead of
> >> using these events that are already generated?  How are virtio block
> >> devices special?
> >> 
> >>> BTW, if this is good, it's good for stable IMHO.
> >> 
> >> What bug does it fix?
> >> 
> >
> > It is not really a bug but it definitely is useful enhancement to have in
> > stable too. I
> > can imagine lots of people can benefit from this.
> 
> But that applies to almost any enhancement :)
> 

Good point :) 

> It will go in *next* merge window, not this one.
> 

Cool, thanks.

Milos