linux-kernel - Re: [PATCH v2 03/26] block: Refactor blk_update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 20 Sep 2012 16:41:33 -0700
From:	Tejun Heo <tj@...nel.org>
To:	Kent Overstreet <koverstreet@...gle.com>
Cc:	linux-bcache@...r.kernel.org, linux-kernel@...r.kernel.org,
	dm-devel@...hat.com, axboe@...nel.dk, neilb@...e.de,
	Vivek Goyal <vgoyal@...hat.com>
Subject: Re: [PATCH v2 03/26] block: Refactor blk_update_request()

Hey,

On Thu, Sep 20, 2012 at 04:36:32PM -0700, Kent Overstreet wrote:
> > Other than that, I definitely like this.  It would be nice to note
> > that the custom partial bio advancing in blk_update_request() is
> > replaced with multiple calls to req_bio_endio().  I don't think it has
> > any meaningful performance implications.  It's just nice to future
> > readers of the commit.
> 
> The number of calls to req_bio_endio() isn't changing...
> blk_update_request() called it for partial completions before. It's just
> where the bio itself is updated that's getting shuffled around.
>
> Or did you mean that bio_advance() is getting called on every bio
> instead of the custom advancing in blk_update_request() before? That is
> different, yeah - it's now always looping over the iovec, not just for
> partial completions.
> 
> Yeah, I will note that in the commit message, in case Jens sees a
> performance regression from it :)

I don't think there's any performance implication.  It's just nice to
explain how the complexity went away.  If for nothing else, to point
out how silly the original code was. :)

> > Also, it would be really nice if you can verify this actually works
> > with partial blk_update_request().  sector update bug in the previous
> > patch scares me a bit.  Implementing some debug hacks in the
> > completion path might be the easiest way to verify.  A subtle bug here
> > could be pretty painful.
> 
> Any suggestions on how to trigger partial updates?

ide along with many legacy drivers do it.  Any SCSI driver including
libata only does full completion.  I don't know.  Even just trying to
call the function and comparing before & after with the original code
would be good.  I'd like to see at least some form of verification
because the manifested bugs could be extremely nasty and difficult to
track down.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/