From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49990) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c7Oe9-0000Z5-Cx for qemu-devel@nongnu.org; Thu, 17 Nov 2016 10:30:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c7Oe8-0005L4-GE for qemu-devel@nongnu.org; Thu, 17 Nov 2016 10:30:41 -0500 From: Alberto Garcia In-Reply-To: <20161111095857.GA7533@noname.redhat.com> References: <1478798349-28983-1-git-send-email-kwolf@redhat.com> <1478798349-28983-8-git-send-email-kwolf@redhat.com> <6386e9e0-3317-3d68-9ff9-d33b0a87213a@redhat.com> <20161111095857.GA7533@noname.redhat.com> Date: Thu, 17 Nov 2016 16:30:30 +0100 Message-ID: MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [RFC PATCH 7/8] quorum: Implement .bdrv_co_preadv/pwritev() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , Eric Blake Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, mreitz@redhat.com 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