Re: [PATCH] blk-merge: fix blk_recount_segments

[Ming Lei]

On Wed, Sep 3, 2014 at 1:01 AM, Jeff Moyer <jmoyer@xxxxxxxxxx> wrote:
> Jens Axboe <axboe@xxxxxxxxx> writes:
>
>> On 09/02/2014 09:02 AM, Ming Lei wrote:
>>> QUEUE_FLAG_NO_SG_MERGE is set at default for blk-mq devices,
>>> so bio->bi_phys_segment computed may be bigger than
>>> queue_max_segments(q) for blk-mq devices, then drivers will
>>> fail to handle the case, for example, BUG_ON() in
>>> virtio_queue_rq() can be triggerd for virtio-blk:
>>>
>>>      https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1359146
>>>
>>> This patch fixes the issue by ignoring the QUEUE_FLAG_NO_SG_MERGE
>>> flag if the computed bio->bi_phys_segment is bigger than
>>> queue_max_segments(q), and the regression is caused by commit
>>> 05f1dd53152173(block: add queue flag for disabling SG merging).
>>>
>>> Reported-by: Kick In <pierre-andre.morey@xxxxxxxxxxxxx>
>>> Tested-by: Chris J Arges <chris.j.arges@xxxxxxxxxxxxx>
>>> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxxxxx>
>>
>> Thanks Ming, this looks nice and clean. Will apply for 3.17.
>
> This sounds an awful lot like the bug I fixed in dm:

Anyway block should respect max segments constraint even though
QUEUE_FLAG_NO_SG_MERGE is set, that is what this patch does.

>
> commit 200612ec33e555a356eebc717630b866ae2b694f
> Author: Jeff Moyer <jmoyer@xxxxxxxxxx>
> Date:   Fri Aug 8 11:03:41 2014 -0400
>
>     dm table: propagate QUEUE_FLAG_NO_SG_MERGE
>
>     Commit 05f1dd5 ("block: add queue flag for disabling SG merging")
>     introduced a new queue flag: QUEUE_FLAG_NO_SG_MERGE.  This gets set by
>     default in blk_mq_init_queue for mq-enabled devices.  The effect of
>     the flag is to bypass the SG segment merging.  Instead, the
>     bio->bi_vcnt is used as the number of hardware segments.
>
>     With a device mapper target on top of a device with
>     QUEUE_FLAG_NO_SG_MERGE set, we can end up sending down more segments
>     than a driver is prepared to handle.  I ran into this when backporting
>     the virtio_blk mq support.  It triggerred this BUG_ON, in
>     virtio_queue_rq:
>
>             BUG_ON(req->nr_phys_segments + 2 > vblk->sg_elems);
>
>     ...
>
> Is bcache a stacking driver in this case?  Should it not inherit this
> flag, just as DM now does?

IMO your patch might make things complicated because:

- max segment constraint is HW related thing, so DM/bcache shouldn't
have touched related flag

- it isn't good to let both DM and underlying device do SG merge or not
do SG merge, performance may hurt if both do SG merge.

So why not let driver and block-core handle it? If driver has max segment
constraint requirement, block core just meets its requirement like this patch.

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