On Sat, Aug 27, 2022 at 12:17:04AM +0800, Sam Li wrote: > +/* > + * Send a zone_management command. > + * op is the zone operation. > + * offset is the starting zone specified as a sector offset.
Does "sector offset" mean "byte offset from the start of the device" or
does it mean in 512B sector units? For consistency this should be in
bytes.
> + * len is the maximum number of sectors the command should operate on. It
> + * should be aligned with the zone sector size.
Please use bytes for consistency with QEMU's block layer APIs.
> @@ -3022,6 +3183,118 @@ static void raw_account_discard(BDRVRawState *s,
> uint64_t nbytes, int ret)
> }
> }
>
> +/*
> + * zone report - Get a zone block device's information in the form
> + * of an array of zone descriptors.
> + *
> + * @param bs: passing zone block device file descriptor
> + * @param zones: an array of zone descriptors to hold zone
> + * information on reply
> + * @param offset: offset can be any byte within the zone size.
This isn't an offset within a zone, it's an offset within the entire
device, so I think "zone size" is confusing here.
> + * @param len: (not sure yet.
Please remove this and document nr_zones instead.
> + * @return 0 on success, -1 on failure
> + */
> +static int coroutine_fn raw_co_zone_report(BlockDriverState *bs, int64_t
> offset,
> + unsigned int *nr_zones,
> + BlockZoneDescriptor *zones) {
> +#if defined(CONFIG_BLKZONED)
> + BDRVRawState *s = bs->opaque;
> + RawPosixAIOData acb;
> +
> + acb = (RawPosixAIOData) {
> + .bs = bs,
> + .aio_fildes = s->fd,
> + .aio_type = QEMU_AIO_ZONE_REPORT,
> + .aio_offset = offset,
> + .zone_report = {
> + .nr_zones = nr_zones,
> + .zones = zones,
> + },
> + };
> +
> + return raw_thread_pool_submit(bs, handle_aiocb_zone_report, &acb);
> +#else
> + return -ENOTSUP;
> +#endif
> +}
> +
> +/*
> + * zone management operations - Execute an operation on a zone
> + */
> +static int coroutine_fn raw_co_zone_mgmt(BlockDriverState *bs, BlockZoneOp
> op,
> + int64_t offset, int64_t len) {
> +#if defined(CONFIG_BLKZONED)
> + BDRVRawState *s = bs->opaque;
> + RawPosixAIOData acb;
> + int64_t zone_sector, zone_sector_mask;
> + const char *ioctl_name;
> + unsigned long zone_op;
> + int ret;
> +
> + struct stat st;
> + if (fstat(s->fd, &st) < 0) {
> + ret = -errno;
> + return ret;
> + }
st is not used and can be removed.
> + zone_sector = bs->bl.zone_sectors;
> + zone_sector_mask = zone_sector - 1;
> + if (offset & zone_sector_mask) {
> + error_report("sector offset %" PRId64 " is not aligned to zone size "
> + "%" PRId64 "", offset, zone_sector);
> + return -EINVAL;
> + }
> +
> + if (len & zone_sector_mask) {
> + error_report("number of sectors %" PRId64 " is not aligned to zone
> size"
> + " %" PRId64 "", len, zone_sector);
> + return -EINVAL;
> + }
> +
> + switch (op) {
> + case BLK_ZO_OPEN:
> + ioctl_name = "BLKOPENZONE";
> + zone_op = BLKOPENZONE;
> + break;
> + case BLK_ZO_CLOSE:
> + ioctl_name = "BLKCLOSEZONE";
> + zone_op = BLKCLOSEZONE;
> + break;
> + case BLK_ZO_FINISH:
> + ioctl_name = "BLKFINISHZONE";
> + zone_op = BLKFINISHZONE;
> + break;
> + case BLK_ZO_RESET:
> + ioctl_name = "BLKRESETZONE";
> + zone_op = BLKRESETZONE;
> + break;
> + default:
> + error_report("Invalid zone operation 0x%x", op);
> + return -EINVAL;
> + }
> +
> + acb = (RawPosixAIOData) {
> + .bs = bs,
> + .aio_fildes = s->fd,
> + .aio_type = QEMU_AIO_ZONE_MGMT,
> + .aio_offset = offset,
> + .aio_nbytes = len,
> + .zone_mgmt = {
> + .zone_op = zone_op,
> + },
> + };
> +
> + ret = raw_thread_pool_submit(bs, handle_aiocb_zone_mgmt, &acb);
> + if (ret != 0) {
> + error_report("ioctl %s failed %d", ioctl_name, errno);
> + return -errno;
ret contains a negative errno value. The errno variable is not used by
raw_thread_pool_submit().
I suggest simplifying it to:
return raw_thread_pool_submit(bs, handle_aiocb_zone_mgmt, &acb);
That's what most of the other raw_thread_pool_submit() callers.
signature.asc
Description: PGP signature
