Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750998AbbLUFKX (ORCPT ); Mon, 21 Dec 2015 00:10:23 -0500 Received: from mail-ig0-f171.google.com ([209.85.213.171]:32987 "EHLO mail-ig0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750761AbbLUFKU (ORCPT ); Mon, 21 Dec 2015 00:10:20 -0500 MIME-Version: 1.0 In-Reply-To: <20151221042613.GA27493@htj.duckdns.org> References: <20151221042613.GA27493@htj.duckdns.org> Date: Sun, 20 Dec 2015 21:10:19 -0800 X-Google-Sender-Auth: MjrUn1LF2nl-aqXmR80vonNGBg0 Message-ID: Subject: Re: IO errors after "block: remove bio_get_nr_vecs()" From: Linus Torvalds To: Tejun Heo Cc: Kent Overstreet , Christoph Hellwig , Ming Lin , Jens Axboe , "Artem S. Tashkinov" , Steven Whitehouse , IDE-ML , Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2609 Lines: 71 On Sun, Dec 20, 2015 at 8:26 PM, Tejun Heo wrote: > > I wonder whether ahci is screwing up command / sg table setup in a way > that e.g. if there are too many segments the sg table overflows into > the neighboring one which is now being exposed by upper layer being > fixed to send down larger commands. Looking into it. That would explain the Corrupted low memory at c0001000 ... that Artem also saw. Anyway, it would be lovely to have some verification in the ATA routines that the passed-on IO actually h9onors the limits it set. Could you add a WARN_ON_ONCE(check_io_limits())" or similar, and maybe we could catch whatever causes the overflow red-handed? On a totally separate issue: Just looking at some of the merging code, and I have to say that it strikes me as insane. This in particular: #define __BIO_SEG_BOUNDARY(addr1, addr2, mask) \ (((addr1) | (mask)) == (((addr2) - 1) | (mask))) #define BIOVEC_SEG_BOUNDARY(q, b1, b2) \ __BIO_SEG_BOUNDARY(bvec_to_phys((b1)), bvec_to_phys((b2)) + (b2)->bv_len, queue_segment_boundary((q))) seems just *stupid*. Why does it do that "bvec_to_phys((b2)) + (b2)->bv_len -1" on the second bvec? That's the :"physical address of the last byte of the second bvec". I understand the "round both addresses up by the mask, and we want to make sure that they are in the same segment" part. But since an individual bvec had better be fully inside one segment (since we split at bvec boundaries anyway, so if ). why do all that crap anyway? The end address doesn't matter, you could just use the beginning. So remove the "-1" and remove the "+bv_len". At which it would become just #define __BIO_SEG_BOUNDARY(addr1, addr2, mask) \ ((addr1) | (mask) == (addr2)|(mask)) #define BIOVEC_SEG_BOUNDARY(q, b1, b2) \ __BIO_SEG_BOUNDARY(bvec_to_phys((b1)), bvec_to_phys((b2)), queue_segment_boundary((q))) which seems simpler and more understandable. "Are the beginning addresses in within the same segment" Or are there ever bv_len == 0 things at the boundary that we want to merge. Because then the "-1+bv_len" case migth make sense. Anyway, that shouldn't change the end result in any way, so that doesn't all *matter*, but it worries me when things look more complicated than I think they should be. Is there something I'm missing? Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/