Re: [Qemu-block] [PATCH v5 2/8] block: Add a mechanism for passing a block driver a block configuration

[Ari Sundholm]

On 06/29/2018 01:07 PM, Kevin Wolf wrote:
> Am 19.06.2018 um 15:43 hat Ari Sundholm geschrieben:
> > A block driver may need to know about the block configuration, most
> > critically the sector sizes, of a block backend for alignment purposes
> > or for some other reason.
>
> It makes sense to me that you need to know alignment constraints of the
> thing you are calling (i.e. your child nodes), but I don't really see
> why you would need to know anything about the parent nodes. I'm doubtful
> that this is a good thing to add.

I guess I should explain the exact use case here to show where we are 
coming from.

The use case is the new blklogwrites driver (introduced in this patch 
set), which we use to test file system implementations. It needs to log 
the writes to whatever guest block device is used (for instance, a 
-device scsi-hd). These block devices may have different kinds of 
logical sector sizes, as determined by their configuration (such as 
physical_block_size=4096,logical_block_size=512). In order to properly 
log the writes that hit such a block device, we need to do this at a 
granularity of its logical sector size for the log to be most useful. 
For this, we need to get this information from the direction of the parent.

This patch set proposes a mechanism for exposing that information to 
blklogwrites (and all other block drivers that may use this 
information), along with other information in the block configuration, 
to allow it to automatically adjust to it.

> I hope that the rest of the series will tell me why you are wanting this
> information, but if we keep it, it needs a better explanation in the
> commit message.
>
> > It doesn't seem like qemu has an existing
> > mechanism for a block backend to convey the required information to
> > the relevant block driver when it is being realized.
> >
> > The need for this mechanism rises from the fact that a drive may not
> > have a backend at the time it is created, as devices are created after
> > drives during qemu startup. Therefore, a driver requiring information
> > about the block configuration can get this information when a backend
> > is created for it at the earliest. The most natural place for this to
> > take place seems to be in the realization functions of the various
> > block device drivers, such as scsi-hd. The interface proposed here
> > allows the block driver to receive information about the block
> > configuration and the associated backend through a new callback.
> >
> > Signed-off-by: Ari Sundholm <address@hidden>
>
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 327e478..74c470e 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -465,6 +465,15 @@ struct BlockDriver {
> >       void (*bdrv_abort_perm_update)(BlockDriverState *bs);
> >   
> >       /**
> > +     * Called to inform the driver of the block configuration of the virtual
> > +     * block device.
> > +     *
> > +     * This function is called by a block device realization function if the
> > +     * device wants to inform the block driver of its block configuration.
> > +     */
> > +    void (*bdrv_apply_blkconf)(BlockDriverState *bs, BlockConf *conf);
>
> This interface can't really work. Block nodes (BlockDriverStates) can
> have an arbitrary number of parents, which can be attached and detached
> dynamically. This interface basically assumes that there is one device
> attached to the node, it will always stay attached and its configuration
> stays the same.
>
> Is this function supposed to be called only once? (That assumption
> wouldn't hold true.) If not, does a second call mean that a second
> parent was attached, or does it update the information of the existing
> parent?
>
> How is this information useful when one parent calls the function and
> provides BlockConf, but the other doesn't and the block driver doesn't
> know about this?

Yes, the interface needs work when you consider the general case. It 
should be considered a first approximation towards a proper one. As it 
is, it doesn't handle the removal of a block backend properly. Also, it 
can be cumbersome to manage the block configurations provided by the 
various parents and use them as constraints for the operation of the 
block driver. It is very general as it is, though, as block drivers can 
freely choose what to do with the block configuration.

I see you proposed dropping this interface entirely in your review of 
patch 8. I think having an interface like this can be valuable, but it 
seems that it requires more work, so I think I will move the definition 
of a proper interface to a separate patch set and opt for manual 
configuration of the log settings in blklogwrites, as you suggested.

> > diff --git a/block/io.c b/block/io.c
> > index b7beaee..3a06843 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -2932,3 +2932,25 @@ int coroutine_fn bdrv_co_copy_range(BdrvChild *src, 
> > uint64_t src_offset,
> >       bdrv_dec_in_flight(dst_bs);
> >       return ret;
> >   }
> > +
> > +void bdrv_apply_blkconf(BlockDriverState *bs, BlockConf *conf)
> > +{
> > +    BlockDriver *drv = bs->drv;
> > +
> > +    if (!drv) {
> > +        return;
> > +    }
> > +
> > +    if (drv->bdrv_apply_blkconf) {
> > +        drv->bdrv_apply_blkconf(bs, conf);
> > +        return;
> > +    }
> > +
> > +    if (bs->file && bs->file->bs) {
> > +        bdrv_apply_blkconf(bs->file->bs, conf);
> > +    }
> > +
> > +    if (bs->drv->supports_backing && backing_bs(bs)) {
> > +        bdrv_apply_blkconf(backing_bs(bs), conf);
> > +    }
> > +}
>
> You probably want to propagate to all children instead of singling out
> bs->file and bs->backing.

Oops, this was on my TODO list, but I apparently never did that change.

Thank you,
Ari Sundholm
address@hidden