linux-kernel - Re: [PATCH 2/5] [PATCH 2/5] revert: "blk-mq: blk-mq should free bios in pass through case"

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] [day] [month] [year] [list]

Message-ID: <20131006150247.GA15624@infradead.org>
Date:	Sun, 6 Oct 2013 08:02:47 -0700
From:	Christoph Hellwig <hch@...radead.org>
To:	Mike Christie <michaelc@...wisc.edu>
Cc:	Jens Axboe <axboe@...nel.dk>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/5] [PATCH 2/5] revert: "blk-mq: blk-mq should free bios
 in pass through case"

On Sat, Oct 05, 2013 at 03:20:05PM -0500, Mike Christie wrote:
> The functions take in requests as the argument, but they end up
> operating on bios too. The scsi layer does
> scsi_io_completion->scsi_end_request-> blk_end_request ->
> blk_end_bidi_request -> blk_update_bidi_request -> blk_update_request.
> That function will then complete bios on the request passed in. It does
> not matter if the request is a REQ_TYPE_FS or REQ_TYPE_BLOCK_PC.
> 
> With my patch I was trying to make the block layer do the same for mq
> REQ_TYPE_BLOCK_PC requests inserted into the queue with
> blk_execute_rq_nowait (Nick's patch had support for something like that)
> by having the block mq layer call blk_mq_finish_request instead of
> making the code that calls blk_execute_rq_nowait do it.

Ok, I get the point now.  Didn't see that issue because I've only been
testing the non-bio REQ_TYPE_BLOCK_PC case as exposed by the virtio-blk
serial attribute so far.

> > That beeing said the old ones all require the caller to free the
> > request, and complicate that with the useless refcounting that my patch
> > 3 removes.  Take a look at the other patches how all the calling
> > conventions can be nicely unified.
> 
> I agree. I like them. I am saying though it could be better because even
> with your patches the rq->end_io functions need to have the mq_ops check
> like the flush_end_io does. If my patch worked as intended we would have
> your improvements and we would not need the rq->end_io functions to have
> that check and call blk_mq_finish_request because the blk mq layer was
> doing it for them.
> 
> That is all I am saying. I would like to be able to remove that check
> and blk_mq_finish_request call from the rq->end_io callouts.

I've found the bug that caused the regression with your patch, it's that
blk-mq incorrectly completes bios midway through a flush sequence, while
the old path didn't.

I'll send out a series soon that fixes this and re-reverts your patch,
although I split that re-revert into two patches in addition to my new
fix, given that I think your mixing up of the blk_mq_finish_request /
blk_mq_free_spit plus the confusing description made it really hard to
grasp, but I'll leave it to Jens how he wants to apply it to his tree
and make it look after the next rebase.
--
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/