Re: [PATCH] vdpa_sim_blk: add `capacity` module parameter

[Stefano Garzarella]

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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > >> >> >>
> > >> >> >> On Fri, 5 Jul 2024 at 20:42, Stefano Garzarella <sgarzare@xxxxxxxxxx> 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@xxxxxxxxxx>
> > >> >> >> > >
> > >> >> >> > >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