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]

Message-ID: <20120920233632.GC5519@google.com>
Date:	Thu, 20 Sep 2012 16:36:32 -0700
From:	Kent Overstreet <koverstreet@...gle.com>
To:	Tejun Heo <tj@...nel.org>
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()

On Thu, Sep 20, 2012 at 04:20:00PM -0700, Tejun Heo wrote:
> Hello,
> 
> On Mon, Sep 10, 2012 at 05:22:14PM -0700, Kent Overstreet wrote:
> >  static void req_bio_endio(struct request *rq, struct bio *bio,
> >  			  unsigned int nbytes, int error)
> >  {
> > +	/*
> > +	 * XXX: bio_endio() does this. only need this because of the weird
> > +	 * flush seq thing.
> > +	 */
> >  	if (error)
> >  		clear_bit(BIO_UPTODATE, &bio->bi_flags);
> >  	else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
> >  		error = -EIO;
> 
> Isn't this also necessary to record errors on partial completions?

Ah yeah, you're right. Meant to delete that comment anyways.

> 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 :)

> 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?
--
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/