From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=40547 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1OEovI-0007Xz-1p
	for qemu-devel@nongnu.org; Wed, 19 May 2010 15:27:21 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69)
	(envelope-from <mjt@tls.msk.ru>) id 1OEov0-000286-Kr
	for qemu-devel@nongnu.org; Wed, 19 May 2010 15:27:09 -0400
Received: from isrv.corpit.ru ([81.13.33.159]:37273)
	by eggs.gnu.org with esmtp (Exim 4.69)
	(envelope-from <mjt@tls.msk.ru>) id 1OEov0-00027n-BB
	for qemu-devel@nongnu.org; Wed, 19 May 2010 15:27:02 -0400
Message-ID: <4BF43B82.9000806@msgid.tls.msk.ru>
Date: Wed, 19 May 2010 23:26:58 +0400
From: Michael Tokarev <mjt@tls.msk.ru>
MIME-Version: 1.0
References: <20100519185309.GA27591@lst.de>
In-Reply-To: <20100519185309.GA27591@lst.de>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: [Qemu-devel] Re: [PATCH] block: fix sector comparism in
	multiwrite_req_compare
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Christoph Hellwig <hch@lst.de>
Cc: Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org, Avi Kivity <avi@redhat.com>

19.05.2010 22:53, Christoph Hellwig wrote:
> The difference between the start sectors of two requests can be larger
> than the size of the "int" type, which can lead to a not correctly
> sorted multiwrite array and thus spurious I/O errors and filesystem
> corruption due to incorrect request merges.
>
> So instead of doing the cute sector arithmetics trick spell out the
> exact comparisms.
>
> Spotted by Kevin Wolf based on a testcase from Michael Tokarev.
>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
>
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c	2010-05-19 17:08:24.970255636 +0200
> +++ qemu/block.c	2010-05-19 17:17:34.227006021 +0200
> @@ -1933,7 +1933,19 @@ static void multiwrite_cb(void *opaque,
>
>   static int multiwrite_req_compare(const void *a, const void *b)
>   {
> -    return (((BlockRequest*) a)->sector - ((BlockRequest*) b)->sector);
> +    const BlockRequest *req1 = a, *req2 = b;
> +
> +    /*
> +     * Note that we can't simply subtract req2->sector from req1->sector
> +     * here as that could overflow the return value.
> +     */
> +    if (req1->sector>  req2->sector) {
> +        return 1;
> +    } else if (req1->sector<  req2->sector) {
> +        return -1;
> +    } else {
> +        return 0;
> +    }
>   }

Fantastic.

I ran numerous mkfs'es of my 1.5Tb image, -- every single
run before resulted in at least one "fun" comparison there.

There is NO MORE OVERLAPPING REQUESTS.  At least I can't
trigger one.

Fantastic!

Thanks guys!

Now the question is if the previous patch by Avi is actually
worth to apply -- I mean this one:
  [Qemu-devel] [PATCH +stable] block: don't attempt to merge overlapping requests

But it looks like all 3 should be applied:

  [Qemu-devel] [PATCH] block: fix sector comparism in multiwrite_req_compare
  [Qemu-devel] [PATCH +stable] block: don't attempt to merge overlapping requests
  [Qemu-devel] [PATCH] virtio-blk: fix barrier support

..to -stable.

I wonder how many similar "funny" cases are still around... ;)

/mjt