[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 7/8] quorum: Implement .bdrv_co_preadv/pwrit
Re: [Qemu-devel] [RFC PATCH 7/8] quorum: Implement .bdrv_co_preadv/pwritev()
Thu, 17 Nov 2016 16:30:30 +0100
Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu)
On Fri 11 Nov 2016 10:58:57 AM CET, Kevin Wolf wrote:
>> > Note that the QMP events emitted by the driver are an external API
>> > that we were careless enough to define as sector based. The offset
>> > and length of requests reported in events are rounded down
>> > therefore.
>> Would it be better to round offset down and length up? A length of 0
>> looks odd.
> To be honest, I don't know what these events are used for and what the
> most useful rounding would be. Maybe Berto can tell?
What makes sense if we don't want to break the API is to round as Eric
suggests. Or, more specifically:
sector-num = ROUND_SECTOR_DOWN(offset)
sectors-count = ROUND_SECTOR_UP(offset + length) - sector-num
>> Should we add more fields to the two affected events (QUORUM_FAILURE
>> and QUORUM_REPORT_BAD)? We have to keep the existing fields for
>> back-compat, but we could add new fields that give byte-based
>> locations for management apps smart enough to use the new instead of
>> the old (particularly since the old fields are named 'sector-num' and
> If there is a user for the new fields, I can do that.
I wouldn't do it until we have a use case.
>> > @@ -462,8 +461,8 @@ static void GCC_FMT_ATTR(2, 3) quorum_err(QuorumAIOCB
>> > *acb,
>> > va_list ap;
>> > va_start(ap, fmt);
>> > - fprintf(stderr, "quorum: sector_num=%" PRId64 " nb_sectors=%d ",
>> > - acb->sector_num, acb->nb_sectors);
>> > + fprintf(stderr, "quorum: offset=%" PRIu64 " bytes=%" PRIu64 " ",
>> > + acb->offset, acb->bytes);
>> Might be worth a separate patch to get rid of fprintf and use correct
>> error reporting. But not the work for this patch.
> What would correct error reporting be in this case? A QMP event?
> Because other than that and stderr, I don't think we have any channels
> for error messages for I/O requests. We could use error_report(), but
> it would be effectively the same thing as fprintf().
I'm not sure what was the original use case of this, but the
functionality should be equivalente to the blkverify driver.
I think it should actually be possible to replace blkverify completely
Re: [Qemu-devel] [RFC PATCH 0/8] quorum: Implement .bdrv_co_preadv/pwritev(), Paolo Bonzini, 2016/11/11