[Qemu-devel] Re: [PATCH] block: fix sector comparism in multiwrite_req_compare

[Michael Tokarev <mjt@tls.msk.ru>]

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