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

Powered by blists - more mailing lists