Message-ID: <CACGkMEtT-UeLk6Hv1AFXk_yhiVxx-5tWknqs=OYg8+cAGDmd9w@mail.gmail.com>
Date: Wed, 10 Jul 2024 15:28:31 +0800
From: Jason Wang <jasowang@...hat.com>
To: Stefano Garzarella <sgarzare@...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 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).

Thanks

>
> Thanks,
> Stefano
>

Powered by blists - more mailing lists