On 11/11/2016 03:58 AM, Kevin Wolf wrote: >> 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.
Libvirt is not using quorums yet, but would definitely prefer to use byte-based information rather than sector based (especially since the documentation isn't specific on how much a sector is; a quorum built on top of disks with 4k sectors may be weird to interpret if you don't know that qemu is hard-coded to 512-byte sectors) >>> qapi_event_send_quorum_report_bad(type, !!msg, msg, node_name, >>> - sector_num, nb_sectors, >>> &error_abort); >>> + offset / BDRV_SECTOR_SIZE, >>> + bytes / BDRV_SECTOR_SIZE, >>> &error_abort); >> >> Rounding the offset down makes sense, but rounding the bytes down can >> lead to weird messages. Blindly rounding it up to a sector boundary can >> also be wrong (example: writing 2 bytes at offset 511 really affects >> 1024 bytes when you consider that two sectors had to undergo >> read-modify-write). Don't we have a helper routine for determining the >> end boundary when we have to convert an offset and length to a courser >> alignment? > > Hm, I would have to check the header files. I don't know one off the top > of my head. If you find it, let me know. > I guess I was thinking of something like io.c:bdrv_round_sectors_to_clusters(), but didn't readily find a counterpart for rounding bytes to sectors or request_alignment. Don't know if it would help to have one, or not. >>> @@ -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(). Hmm, you're probably right that a QMP event would be best, if anything is needed at all. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
