Message-ID: <glbjelgjjsebultcvx6hbywo6dz43dxnbnvydlaxgaohwovng4@7plje3qmqvdi>
Date: Wed, 10 Jul 2024 09:36:09 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: Cindy Lu <lulu@...hat.com>, "Michael S. Tsirkin" <mst@...hat.com>, 
	virtualization@...ts.linux.dev, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, 
	linux-kernel@...r.kernel.org, Eugenio Pérez <eperezma@...hat.com>
Subject: Re: [PATCH] vdpa_sim_blk: add `capacity` module parameter

On Wed, Jul 10, 2024 at 03:28:31PM GMT, Jason Wang wrote:
>On Wed, Jul 10, 2024 at 3:19 PM Stefano Garzarella <sgarzare@...hat.com> wrote:
>>
>> On Wed, Jul 10, 2024 at 11:08:48AM GMT, Jason Wang wrote:
>> >On Tue, Jul 9, 2024 at 8:41 PM Stefano Garzarella <sgarzare@...hat.com> wrote:
>> >>
>> >> On Tue, Jul 09, 2024 at 10:56:16AM GMT, Jason Wang wrote:
>> >> >On Mon, Jul 8, 2024 at 4:15 PM Stefano Garzarella <sgarzare@...hat.com> wrote:
>> >> >>
>> >> >> Hi Cindy, Jason,
>> >> >>
>> >> >> On Mon, Jul 08, 2024 at 03:59:34PM GMT, Jason Wang wrote:
>> >> >> >On Mon, Jul 8, 2024 at 3:06 PM Cindy Lu <lulu@...hat.com> wrote:
>> >> >> >>
>> >> >> >> On Fri, 5 Jul 2024 at 20:42, Stefano Garzarella <sgarzare@...hat.com> wrote:
>> >> >> >> >
>> >> >> >> > On Fri, Jul 05, 2024 at 07:30:51AM GMT, Michael S. Tsirkin wrote:
>> >> >> >> > >On Fri, Jul 05, 2024 at 01:28:21PM +0200, Stefano Garzarella wrote:
>> >> >> >> > >> The vDPA block simulator always allocated a 128 MiB ram-disk, but some
>> >> >> >> > >> filesystems (e.g. XFS) may require larger minimum sizes (see
>> >> >> >> > >> https://issues.redhat.com/browse/RHEL-45951).
>> >> >> >> > >>
>> >> >> >> > >> So to allow us to test these filesystems, let's add a module parameter
>> >> >> >> > >> to control the size of the simulated virtio-blk devices.
>> >> >> >> > >> The value is mapped directly to the `capacity` field of the virtio-blk
>> >> >> >> > >> configuration space, so it must be expressed in sector numbers of 512
>> >> >> >> > >> bytes.
>> >> >> >> > >>
>> >> >> >> > >> The default value (0x40000) is the same as the previous value, so the
>> >> >> >> > >> behavior without setting `capacity` remains unchanged.
>> >> >> >> > >>
>> >> >> >> > >> Before this patch or with this patch without setting `capacity`:
>> >> >> >> > >>   $ modprobe vdpa-sim-blk
>> >> >> >> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
>> >> >> >> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
>> >> >> >> > >>   virtio_blk virtio6: [vdb] 262144 512-byte logical blocks (134 MB/128 MiB)
>> >> >> >> > >>
>> >> >> >> > >> After this patch:
>> >> >> >> > >>   $ modprobe vdpa-sim-blk capacity=614400
>> >> >> >> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
>> >> >> >> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
>> >> >> >> > >>   virtio_blk virtio6: [vdb] 614400 512-byte logical blocks (315 MB/300 MiB)
>> >> >> >> > >>
>> >> >> >> > >> Signed-off-by: Stefano Garzarella <sgarzare@...hat.com>
>> >> >> >> > >
>> >> >> >> > >What a hack. Cindy was working on adding control over config
>> >> >> >> > >space, why can't that be used?
>> >> >> >> >
>> >> >> >> > If it can be used easily with virtio-blk device too, it will be great.
>> >> >> >> > @Cindy do you plan to support that changes for a virtio-blk device too?
>> >> >> >> >
>> >> >> >> Hi Stefano
>> >> >> >> I plan to add support to change the vdpa device's configuration after
>> >> >> >> it is created.
>> >> >> >
>> >> >> >I think for Stefano's case, we can just implement it via provisioning
>> >> >> >parameters?
>> >> >>
>> >> >> Yep, I think we don't need to change it after creation, but specifying
>> >> >> while creating should be enough.
>> >> >>
>> >> >> So, IIUC we can already do it, implementing something similar to
>> >> >> vdpasim_net_setup_config() to call during vdpasim_blk_dev_add(), right?
>> >> >
>> >> >Right.
>> >> >
>> >> >>
>> >> >> What about when we have `shared_backend` set to true for the
>> >> >> vdpa_sim_blk.ko? In this case the backend is supposed to be shared
>> >> >> between all the devices to test live migration.
>> >> >
>> >> >This seems to be another topic.
>> >>
>> >> Yep, but really related. I think we need to handle that case when
>> >> supporting the `capacity` setting.
>> >
>> >Ok, so if I was not wrong, the goal is to test migration.
>>
>> Sorry, I was not clear, I try to rephrase:
>> vdpa_sim_blk already supports a module parameter called `shared_backend`
>> introduced mainly to test live migration on the same host. When that
>> parameter is on, all the created devices share the same backend and so
>> we can easily do migration from one to another.
>>
>> With that parameter on or off, the device is always 128 MB, but now
>> that's a problem for testing, because it looks like XFS requires at
>> least 300 MB: https://issues.redhat.com/browse/RHEL-45951
>>
>> That's why I sent this patch.
>>
>> When `shared_backend` is off (default), using the provisioning
>> parameters seems feasible to me, but when it's on, how do I deal with
>> it?
>>
>> Being a simulator, we can maybe make it so that only the first device
>> can change the size for example, or that all devices control the size,
>> but then we would have to handle the size change at runtime, which I
>> think is feasible, but it requires some work to send a notification of
>> configuration change, etc.
>
>Can we mandate the size parameter to be exactly the same as the first
>vDPA block simulator?
>
>>
>> >
>> >>
>> >> >
>> >> >>
>> >> >> Maybe we can just change the size of the shared ramdisk to be reflected
>> >> >> to all devices.
>> >> >>
>> >> >> Suggestions?
>> >> >
>> >> >Could we specify the path to tmpfs or others during provisioning
>> >> >instead?  It seems more general (but more work).
>> >>
>> >> Then it would almost become a real device, no longer just a simulator.
>> >> It's enough work, though, as you said, but at that point we'd just have
>> >> to specify the backend file to use for the device.
>> >>
>> >> In that case what API would we need to use to allow the user to set the
>> >> backend file?
>> >
>> >Yes, I think we can allow some vendor specific provisioning parameters.
>> >
>> >But not sure it's an overkill for the use case here. If others are
>> >happy with the shared_backed. I'm fine.
>>
>> Yeah, maybe it's overkill and I don't have much time these days :-(
>>
>> I think the easiest way is to merge this patch, but I understand that a
>> module parameter is not very beautiful
>>
>> I'll try to see if I can implement provisioning parameters for
>> vdpa_sim_blk. Allowing capacity to be set only to the first device if
>> `shared_backend` is on.
>>
>> WDYT?
>
>Something like this.
>
>When there's no block simulator, allow an arbitrary capacity. When
>there is one, fail the creation when the capacity doesn't match. (when
>'shared_backend' is on).

Yeah, makes sense to me! I'll do it!

Thanks for the help,
Stefano

Powered by blists - more mailing lists