Re: [RFC v3 2/5] qemu-io: add zoned block device operations.

[Sam Li]

Damien Le Moal <damien.lemoal@opensource.wdc.com> 于2022年6月28日周二 17:11写道：
>
> On 6/28/22 16:56, Stefan Hajnoczi wrote:
> > On Mon, Jun 27, 2022 at 08:19:14AM +0800, Sam Li wrote:
> >> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> >> index 2f0d8ac25a..3f2592b9f5 100644
> >> --- a/qemu-io-cmds.c
> >> +++ b/qemu-io-cmds.c
> >> @@ -1706,6 +1706,122 @@ static const cmdinfo_t flush_cmd = {
> >>      .oneline    = "flush all in-core file state to disk",
> >>  };
> >>
> >> +static int zone_report_f(BlockBackend *blk, int argc, char **argv)
> >> +{
> >> +    int ret;
> >> +    int64_t offset, len, nr_zones;
> >> +    int i = 0;
> >> +
> >> +    ++optind;
> >> +    offset = cvtnum(argv[optind]);
> >> +    ++optind;
> >> +    len = cvtnum(argv[optind]);
> >> +    ++optind;
> >> +    nr_zones = cvtnum(argv[optind]);
> >> +
> >> +    g_autofree BlockZoneDescriptor *zones = g_new(BlockZoneDescriptor, 
> >> nr_zones);
> >> +    ret = blk_zone_report(blk, offset, len, &nr_zones, zones);
> >> +    while (i < nr_zones) {
> >
> > Does blk_zone_report() set nr_zones to 0 on failure or do we need to
> > check if (ret < 0) here?
>
> ret = 0 means "no zone reported" which happen only if nr_zones is 0 or the
> start offset is past the end of the disk capacity. ret < 0 would mean that
> a report zone operation was actually attempted and failed (EIO, ENOMEM etc).
>
> >
> >> +        fprintf(stdout, "start: 0x%lx, len 0x%lx, cap 0x%lx, wptr 0x%lx, "
> >
> > The rest of the source file uses printf() instead of fprintf(stdout,
> > ...). That's usually preferred because it's shorter.
> >
> >> +                        "zcond:%u, [type: %u]\n",
> >
> > Please use PRIx64 instead of lx format specifiers for portability. On
> > 32-bit hosts lx is 32-bit, not 64-bit. You can grep QEMU's code for
> > examples of PRIx64.
> >
> >> +                zones[i].start, zones[i].length, zones[i].cap, 
> >> zones[i].wp,
> >> +                zones[i].cond, zones[i].type);
> >> +        ++i;
> >> +    }
> >
> > A for loop is more idiomatic:
> >
> >   for (int i = 0; i < nr_zones; i++) {
> >       ...
> >   }
> >
> >> +    return ret;
> >> +}
> >> +
> >> +static const cmdinfo_t zone_report_cmd = {
> >> +        .name = "zone_report",
> >> +        .altname = "f",
> >> +        .cfunc = zone_report_f,
> >> +        .argmin = 3,
> >> +        .argmax = 3,
> >> +        .args = "offset [offset..] len [len..] number [num..]",
> >
> > The arguments are "offset len number". This command does not accept
> > optional offset/len/num arguments.
>
> The arguments should be offset + len OR offset + number of zones. Having
> the 3 of them does not make sense to me. The interface would then be:
>
> (1) offset + len -> report all zones in the block range [offset .. offset
> + len - 1]
>
> (2) offset + number of zones -> report at most "number of zones" from the
> zone containing the block at "offset".
>
> (2) matches the semantic used at the device command level. So I prefer to
> approach (1).
Yes, I'll remove the len argument then.

>
> >
> >> +        .oneline = "report a number of zones",
> >
> > Maybe "report zone information".
> >
> >> +};
> >> +
> >> +static int zone_open_f(BlockBackend *blk, int argc, char **argv)
> >> +{
> >> +    int64_t offset, len;
> >> +    ++optind;
> >> +    offset = cvtnum(argv[optind]);
> >> +    ++optind;
> >> +    len = cvtnum(argv[optind]);
> >> +    return blk_zone_mgmt(blk, zone_open, offset, len);
> >
> > Where is the error reported? When I look at read_f() I see:
> >
> >     if (ret < 0) {
> >         printf("read failed: %s\n", strerror(-ret));
> >
> > I think something similar is needed because qemu-io.c does not print an
> > error message for us. The same is true for the other commands defined in
> > this patch.
> >
> >> +}
> >> +
> >> +static const cmdinfo_t zone_open_cmd = {
> >> +        .name = "zone_open",
> >> +        .altname = "f",
> >> +        .cfunc = zone_open_f,
> >> +        .argmin = 2,
> >> +        .argmax = 2,
> >> +        .args = "offset [offset..] len [len..]",
> >
> > There are no optional offset/len args. The same is true for the other
> > commands below.
>
>
> --
> Damien Le Moal
> Western Digital Research