Re: [PATCH 08/21] block/backup: stricter backup_calculate_cluster_size()

[Vladimir Sementsov-Ogievskiy]

17.05.2021 19:57, Max Reitz wrote:
> On 17.05.21 08:44, Vladimir Sementsov-Ogievskiy wrote:
> > No reason to tolerate bdrv_get_info() errors except for ENOTSUP. Let's
> > just error-out, it's simpler and safer.
>
> Hm, doesn’t look that much simpler to me.  Not sure how much safer it is, 
> because the point was that in the target_does_cow case, we would like a cluster 
> size hint, but it isn’t necessary.  So if we don’t get one, regardless of the 
> reason, we use the default cluster size.  I don’t know why ENOTSUP should be 
> treated in a special way there.
>
> So I don’t know.

I'm probably OK to drop this for now and don't care. Still, I can share what 
brings me to this:

First I thought that cluster size should be easily available for any driver:

protocol drivers and not-backing-supporting format drivers can set it to 1 or to 
request_alignment, if they don't have a "cluster" in mind.

backing-supporting format drivers should of course provide actual cluster size

And I decided to just add bs->cluster_size variable, set on driver open, to 
simplify the whole thing and make it clean. Then, most this detect-cluster-size 
function would be just dropped.

But it occurs, that there is one driver, that has a good and rather tricky 
reason for ENOTSUP: vmdk can have several extents with different cluster size..

So I give up refactored, and finished with this one patch. It can be simply 
dropped, I am not really a fan of it..

> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   block/backup.c | 14 +++++---------
> >   1 file changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/block/backup.c b/block/backup.c
> > index fe685e411b..fe7a1f1e37 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -367,7 +367,10 @@ static int64_t 
> > backup_calculate_cluster_size(BlockDriverState *target,
> >        * targets with a backing file, try to avoid COW if possible.
> >        */
> >       ret = bdrv_get_info(target, &bdi);
> > -    if (ret == -ENOTSUP && !target_does_cow) {
> > +    if (ret < 0 && ret != -ENOTSUP) {
> > +        error_setg_errno(errp, -ret, "Failed to get target info");
> > +        return ret;
> > +    } else if (ret == -ENOTSUP && !target_does_cow) {
> >           /* Cluster size is not defined */
> >           warn_report("The target block device doesn't provide "
> >                       "information about the block size and it doesn't have a "
> > @@ -376,14 +379,7 @@ static int64_t 
> > backup_calculate_cluster_size(BlockDriverState *target,
> >                       "this default, the backup may be unusable",
> >                       BACKUP_CLUSTER_SIZE_DEFAULT);
> >           return BACKUP_CLUSTER_SIZE_DEFAULT;
> > -    } else if (ret < 0 && !target_does_cow) {
> > -        error_setg_errno(errp, -ret,
> > -            "Couldn't determine the cluster size of the target image, "
> > -            "which has no backing file");
> > -        error_append_hint(errp,
> > -            "Aborting, since this may create an unusable destination image\n");
> > -        return ret;
> > -    } else if (ret < 0 && target_does_cow) {
> > +    } else if (ret == -ENOTSUP && target_does_cow) {
> >           /* Not fatal; just trudge on ahead. */
> >           return BACKUP_CLUSTER_SIZE_DEFAULT;
> >       }


--
Best regards,
Vladimir