Re: [PATCH 1/2] ublk_drv: store device parameters

[Ming Lei <ming.lei@redhat.com>]

On Tue, Jul 26, 2022 at 02:32:24PM +0200, Christoph Hellwig wrote:
> On Mon, Jul 25, 2022 at 03:06:50PM +0800, Ming Lei wrote:
> > There could be more parameters than the two types(), such as segments,
> > zoned, ..., also in future, feature related parameters can be added
> > in this way too, and most of them are optional.
> 
> Yes.  But just having a struct that grows is much cleaner and simpler
> than those indirections.  e.g something like this patch on top of this

But most of fields in ublk_params aren't required for one device,
sending them all isn't friendly from both userspace and kernel side.
Especially inside kernel, a big chunk buffer is allocated for
the structure, but only few fields are useful for one device. There can
be lots of ublk device, and total wasted memory can't be ignored.

If we group parameters, it is easier to extend by adding new parameter
type. One ublk device usually just uses several parameter types.
And the xarray implementation is simple enough too, which is just one
array, but really sparsed.

Adding one parameter type just needs to add two callbacks, so pretty easy
to review.

So I really suggest to keep the parameter type and xarray
implementation, and will post V2 after ublksrv side is updated.

> series.  With this new fields can just be added to the end of
> struct ublk_params.  Old kernels will ignore them, but due to the copy
> back of the parsed structure userspace can detect that if it cares:

There has to be one length field in header, otherwise ublk driver can't
know the exact length for copying back.

With parameter type approach, it is natural to handle new/old app and
old/new kernel, since kernel will fail set parameter command simply
if one parameter type isn't supported.


Thanks, 
Ming