On 2022/09/11 17:00, Sam Li wrote:
[...]
>>> @@ -1604,6 +1629,12 @@ static ssize_t
>>> handle_aiocb_rw_linear(RawPosixAIOData *aiocb, char *buf)
>>> (const char *)buf + offset,
>>> aiocb->aio_nbytes - offset,
>>> aiocb->aio_offset + offset);
>>> + } else if (aiocb->aio_type == QEMU_AIO_ZONE_APPEND) {
>>> + uint64_t wp = aiocb->aio_offset;
>>
>> This variable is not necessary.
>>
>>> + len = pwrite(aiocb->aio_fildes,
>>> + (const char *)buf + offset,
>>> + aiocb->aio_nbytes - offset,
>>> + wp + offset);
>>> } else {
>>> len = pread(aiocb->aio_fildes,
>>> buf + offset,
>>> @@ -1638,7 +1669,6 @@ static int handle_aiocb_rw(void *opaque)
>>> RawPosixAIOData *aiocb = opaque;
>>> ssize_t nbytes;
>>> char *buf;
>>> -
>>
>> whiteline change.
>>
>>> if (!(aiocb->aio_type & QEMU_AIO_MISALIGNED)) {
>>> /*
>>> * If there is just a single buffer, and it is properly aligned
>>> @@ -1692,7 +1722,7 @@ static int handle_aiocb_rw(void *opaque)
>>> }
>>>
>>> nbytes = handle_aiocb_rw_linear(aiocb, buf);
>>> - if (!(aiocb->aio_type & QEMU_AIO_WRITE)) {
>>> + if (!(aiocb->aio_type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND))) {
>>> char *p = buf;
>>> size_t count = aiocb->aio_nbytes, copy;
>>> int i;
>>> @@ -1713,6 +1743,25 @@ static int handle_aiocb_rw(void *opaque)
>>>
>>> out:
>>> if (nbytes == aiocb->aio_nbytes) {
>>> +#if defined(CONFIG_BLKZONED)
>>> + if (aiocb->aio_type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND)) {
>>> + BlockZoneDescriptor *zone = aiocb->io.zone;
>>> + int64_t nr_sectors = aiocb->aio_nbytes / 512;
>>> + if (zone) {
>>> + qemu_mutex_init(&zone->lock);
>>> + if (zone->type == BLK_ZT_SWR) {
>>> + qemu_mutex_lock(&zone->lock);
>>> + zone->wp += nr_sectors;
>>> + qemu_mutex_unlock(&zone->lock);
>>> + }
>>> + qemu_mutex_destroy(&zone->lock);
>>
>> This is weird. you init the mutex, lock/unlock it and destroy it. So it has
>> absolutely no meaning at all.
>
> I was thinking that init every lock for all the zones or init the lock
> for the zone that needed it. The confusion I have here is the cost of
> initializing/destroying the lock.
A mutex needs to be initialized before it is used and should not be
re-initialized, ever, until it is not needed anymore. That is, in this case,
since the mutex protects a zone wp tracking entry, it should be initialized when
the array of zone wp is allocated & initialized with a zone report, and the
mutex destroyed when that same array is freed.
The cost of initializing & destroying a mutex is low. And since that is not done
in the hot IO path, you do not need to worry about it.
[...]
>>> +static int coroutine_fn raw_co_zone_append(BlockDriverState *bs,
>>> + int64_t *offset,
>>> + QEMUIOVector *qiov,
>>> + BdrvRequestFlags flags) {
>>> +#if defined(CONFIG_BLKZONED)
>>> + BDRVRawState *s = bs->opaque;
>>> + int64_t zone_sector = bs->bl.zone_sectors;
>>> + int64_t zone_sector_mask = zone_sector - 1;
>>> + int64_t iov_len = 0;
>>> + int64_t len = 0;
>>> + RawPosixAIOData acb;
>>> +
>>> + if (*offset & zone_sector_mask) {
>>> + error_report("offset %" PRId64 " is not aligned to zone size "
>>> + "%" PRId64 "", *offset, zone_sector);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + int64_t lbsz = bs->bl.logical_block_size;> + int64_t lbsz_mask =
>>> lbsz - 1;
>>> + for (int i = 0; i < qiov->niov; i++) {
>>> + iov_len = qiov->iov[i].iov_len;
>>> + if (iov_len & lbsz_mask) {
>>> + error_report("len of IOVector[%d] %" PRId64 " is not aligned to
>>> block "
>>> + "size %" PRId64 "", i, iov_len, lbsz);
>>> + return -EINVAL;
>>> + }
>>
>> This alignment check should be against the device write granularity, not the
>> logical block size. The write granularity will always be equal to the device
>> physical block size, which may or may not be equal to the device logical
>> block
>> size. E.g. a 512e SMR disk has a 512B logical block size but a 4096B physical
>> block size. And the ZBC & ZAC specifications mandate that all write be
>> aligned
>> to the physical block size.
>
> I see. I'll change it to physical block size.
I would use a filed called "write_granularity" since the virtio specs will
introduce that anyway. This zone granularity is going to be indeed equal to the
physical block size of the host device for now.
[...]
>>> /* removable device specific */
>>> bool (*bdrv_is_inserted)(BlockDriverState *bs);
>>> @@ -854,6 +857,12 @@ typedef struct BlockLimits {
>>>
>>> /* maximum number of active zones */
>>> int64_t max_active_zones;
>>> +
>>> + /* array of zones in the zoned block device. Only tracks write
>>> pointer's
>>> + * location of each zone as a helper for zone_append API */
>>> + BlockZoneDescriptor *zones;
>>
>> This is a lot of memory for only tracking the wp... Why not reduce this to an
>> array of int64 values, for the wp only ? As you may need the zone type too
>> (conventional vs sequential), you can use the most significant bit (a zone wp
>> value will never use all 64 bits !).
>>
>> Or device another zone structure with zone type, zone wp and mutex only, so
>> smaller than the regular zone report structure.
>
> I was just trying to reuse do_zone_report. It is better to only track
> the wp only. Then a new struct and smaller zone_report should be
> introduced for it.
Yes, this will use less memory, which is always good.
--
Damien Le Moal
Western Digital Research
