Re: [PATCH] block: use static bio_set for bio_split() calls

["Edmund Nadolski (Microsoft)" <edmund.f.nadolski@gmail.com>]

On 4/18/19 8:02 AM, Hannes Reinecke wrote:
> On 4/18/19 4:34 PM, Ming Lei wrote:
>> Hi Hannes,
>> 
>> On Thu, Apr 18, 2019 at 04:06:32PM +0200, Hannes Reinecke wrote:
>>> When calling blk_queue_split() it will be using the per-queue
>>> bioset to allocate the split bio from. However, blk_steal_bios()
>>> might move the bio to another queue, _and_ the original queue
>>> might be removed completely (nvme is especially prone to do so).
>> 
>> Could you explain a bit how the original queue is removed in case
>> that blk_steal_bios() is involved?
>> 
> It's not blk_steal_bios() which removes the queue. What happens is:
> 
> - bio returns with error
> - blk_steal_bios() moves bio over to a different queue
> - nvme_reset_ctrl() is called
> - Error detection finds that the original device is gone
> - nvme calls nvme_remove_ns()
> - nvme_remove_ns() removes the original request queue alongside the
> bio_set from which the bvecs have been allocated from.
> - 'stolen' bio is completed
> - 'stolen' bio calls bio_endio()
> - bio_endio() calls mempool_free() on the bvecs, referencing the mempool
> from the original queue
> - crash
> 
>>> That leaves the bvecs of the split bio with a missing / destroyed
>>> mempool, and a really fun crash in bio_endio().
>> 
>> per-queue bioset is used originally for avoiding deadlock, are you
>> sure the static bioset is safe?
>> 
> If that turns out be be an issue we could be having a per 'ns_head'
> bio_set for allocating the split bio from.
> But the main point is that we cannot use the bioset from the request
> queue as the queue (and the bioset) might be removed during the lifetime
> of the bio.

Does this mean that as-is, this is safe because nvme (currently) is the 
only consumer of blk_steal_bios()?

(Sorry if this is a lame question, I'm in the process of learning this 
code.)

TIA,
Ed