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 >> 'sectors-count'). > > 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 with quorum: https://lists.gnu.org/archive/html/qemu-devel/2012-08/msg02817.html Berto
