Re: [ANNOUNCE] libblkio v0.1.0 preview release

[Kevin Wolf]

Am 30.04.2021 um 17:49 hat Stefan Hajnoczi geschrieben:
> On Thu, Apr 29, 2021 at 05:51:16PM +0200, Kevin Wolf wrote:
> > Am 29.04.2021 um 16:05 hat Stefan Hajnoczi geschrieben:
> > > Hi,
> > > A preview release of libblkio, a library for high-performance block I/O,
> > > is now available:
> > > 
> > >   https://gitlab.com/libblkio/libblkio/-/tree/v0.1.0
> > > 
> > > Applications are increasingly integrating high-performance I/O
> > > interfaces such as Linux io_uring, userspace device drivers, and
> > > vhost-user device support. The effort required to add each of these
> > > low-level interfaces into an application is relatively high. libblkio
> > > provides a single API for efficiently accessing block devices and
> > > eliminates the need to write custom code for each one.
> > > 
> > > The library is not yet ready to use and currently lacks many features.
> > > In fact, I hope this news doesn't spread too far yet because libblkio is
> > > at a very early stage, but feedback from the QEMU community is necessary
> > > at this time.
> > 
> > I'm a bit worried about the configuration interface. This looks an awful
> > lot like plain QOM properties, which have proven to result in both very
> > verbose (not to say messy) and rather error prone implementations.
> > 
> > You have to write setters/getters for every property, even if it usually
> > ends up doing the same thing, storing the value somewhere. Worse, these
> > getters and setters have to work in very different circumstances, namely
> > initialisation where you usually have to store the value away so that it
> > can be checked for consistency with other properties in .realize() or
> > .complete(), and property updates during runtime. Often enough, we
> > forget the latter and create bugs. If we don't create bugs, we usually
> > start with 'if (realized)' and have two completely different code paths.
> > Another common bug in QOM objects is forgetting to check if mandatory
> > properties were actually set.
> >
> > Did you already consider these problems and decided to go this way
> > anyway, or is this more of an accidental design? And if the former, what
> > were the reasons that made it appealing?
> 
> That's true. Here is the code to reject accesses when the instance is
> not initialized:
> 
>   self.must_be_initialized()?;
> 
> It's very concise but you still need to remember to add it.

Yes, I think the problem isn't the length (in QOM it's commonly four
lines because we don't inline it like that, but same complexity really),
but remembering to have it there.

> The overall reasons for choosing the properties API were:
> 
> 1. It keeps the library's API very minimal (just generic getters/setters
>    for primitive types). It minimizes ABI compatibility issues because
>    there are no configuration structs or functions exported by the
>    library.
> 
> 2. It's trivial to have a string setter/getter that automatically
>    converts to the primitive type representation, so application config
>    file or command-line values can easily be set.
> 
>    This is kind of the inverse of what you're saying. If the library
>    offers dedicated interfaces for each configuration value then the
>    library doesn't need getters/setters for each one but all
>    applications need special-purpose code for each configuration value.
> 
> That said, this is exactly why I published the preview release. If
> someone has a better way to do this or the feedback is just that this is
> bad style, then I'm happy to change it.

I can see the advantages in this. Maybe the problem that I'm seeing is
more about implementing drivers in the Rust code rather than the
external interface in C.

I've been playing a bit with this and maybe a good part of it can be
abstracted away (maybe a bit like qdev properties abstract the
complexity of QOM properties away). If I get somewhere with this, I can
send patches.

There is one more thing I'm wondering right now: Why do we have separate
states for connecting to the backend (created) and configuring it
(initialized)? The property setters check for the right state, but they
don't really do anything that wouldn't be possible in the other state.
A state machine exposed as two boolean rather than a tristate enum feels
a bit awkward, too, but even more so if two states could possibly be
enough.

The reason why I'm asking is that in order to address my points, it
would be best to have separate property accessors for each state, and
two pairs of accessors would make property declarations more managable
than three pairs.

> > Alternatives in QEMU are qdev properties (which are internally QOM
> > properties, but provide default implementations and are at least
> > automatically read-only after realize, avoiding that whole class of
> > bugs) and QAPI.
> > If this was QEMU code, I would of course go for QAPI, but a library is
> > something different and adding the code generator would probably be a
> > bit too much anyway. But the idea in the resulting code would be dealing
> > with native structs instead of a bunch of function calls. This would
> > probably be the least error prone way for the implementation, but of
> > course, it would make binary compatibility a bit harder when adding new
> > properties.
> 
> An alternative I considered was the typestate and builder patterns:
> 
>   /* Create a new io_uring driver in the uninitialized state */
>   struct blkio_iou_uninit *blkio_new_io_uring(void);
> 
>   /* Uninitialized state property setters */
>   int blkio_iou_uninit_set_path(struct blkio_iou_uninit *u,
>                                 const char *path);
>   int blkio_iou_uninit_set_direct(struct blkio_iou_uninit *u,
>                                   bool o_direct);
>   ...
> 
>   /* Transition to initialized state. Frees u on success. */
>   struct blkio_iou_init *blkio_iou_init(struct blkio_iou_uninit *u);
> 
>   /* Initialized state property setters/getters */
>   int blkio_iou_init_get_capacity(struct blkio_iou_init *i,
>                                   uint64_t *capacity);
>   ...
> 
>   /* Transition to started state. Frees i on success. */
>   struct blkio_iou_started *blkio_iou_start(struct blkio_iou_init *i);
> 
>   ...
> 
>   /* Transition back to initialized state. Frees s on success. */
>   struct blkio_iou_init *blkio_iou_stop(struct blkio_iou_started *s);
> 
> On the plus side:
> 
> - No state checks are needed because an API won't even exist if it's
>   unavailable in a given state (uninitialized/initialized/started).
> 
> - State structs come with pre-initialized default values, so the caller
>   only needs to set non-default values. For example O_DIRECT is false by
>   default and callers happy with that don't need to set the property.
> 
> - ABI compatibility is easy since the state structs are opaque (their
>   size is not defined) and new properties can be added at any time.
> 
> On the minus side:
> 
> - Completely static. Hard to introspect and requires a dedicated call
>   site for each property (applications cannot simply assign a property
>   string given to them on the command-line). This means every single
>   property must be explicitly coded into every application :(.

How are you going to deal with this for QEMU integration, by the way?
Put all the properties that we know into the QAPI schema and then some
way of passing key/value pairs for the rest?

> - So many functions! This makes understanding the API harder.
> 
> - Very verbose. The function and type names get long and there is a lot
>   of repetition in the API.

I think it wouldn't be too bad if all drivers exposed the same
properties, but you're explicitly expecting driver-specific properties.
If drivers add an external APIs that just fail for other drivers, it
would indeed make understanding the API much harder.

We could consider a mix where you would first create a configuration
object, then use the generic property functions to set options for it
and finally have a separate blkio_initialize() function where you turn
that config into a struct blkio that is needed to actually do I/O (and
also supports generic property functions for runtime option updates).

I'm not sure it provides much except making the state machine more
prominent than just two random bool properties.

Kevin

signature.asc
Description: PGP signature