linux-kernel - Re: [PATCH RFC] block: fix bio merge checks when virt

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: <87shyg1jdx.fsf@vitty.brq.redhat.com>
Date:	Wed, 20 Apr 2016 15:48:10 +0200
From:	Vitaly Kuznetsov <vkuznets@...hat.com>
To:	Ming Lei <tom.leiming@...il.com>
Cc:	Keith Busch <keith.busch@...el.com>, linux-block@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Jens Axboe <axboe@...nel.dk>,
	Dan Williams <dan.j.williams@...el.com>,
	"Martin K. Petersen" <martin.petersen@...cle.com>,
	Sagi Grimberg <sagig@...lanox.com>,
	Mike Snitzer <snitzer@...hat.com>,
	"K. Y. Srinivasan" <kys@...rosoft.com>,
	Cathy Avery <cavery@...hat.com>
Subject: Re: [PATCH RFC] block: fix bio merge checks when virt_boundary is set

Ming Lei <tom.leiming@...il.com> writes:

> On Fri, Mar 18, 2016 at 10:59 AM, Ming Lei <tom.leiming@...il.com> wrote:
>> On Fri, Mar 18, 2016 at 12:39 AM, Keith Busch <keith.busch@...el.com> wrote:
>>> On Thu, Mar 17, 2016 at 12:20:28PM +0100, Vitaly Kuznetsov wrote:
>>>> Keith Busch <keith.busch@...el.com> writes:
>>>> > been combined. In any case, I think you can get what you're after just
>>>> > by moving the gap check after BIOVEC_PHYS_MERGABLE. Does the following
>>>> > look ok to you?
>>>> >
>>>>
>>>> Thanks, it does.
>>>
>>> Cool, thanks for confirming.
>>>
>>>> Will you send it or would you like me to do that with your Suggested-by?
>>>
>>> I'm not confident yet this doesn't break anything, particularly since
>>> we moved the gap check after the length check. Just wanted to confirm
>>> the concept addressed your concern, but still need to take a closer look
>>> and test before submitting.
>>
>> IMO, the change on blk_bio_segment_split() is correct, because actually it
>> is a sg gap and the check should have been done between segments
>> instead of bvecs. So it is reasonable to move the check just before populating
>> a new segment.
>
> Thinking of the 1st part change further, looks it is just correct in concept,
> but wrong from current implementation. Because of bios/reqs merge,
> blk_rq_map_sg() may end one segment in any bvec in theroy, so I guess
> that is why each non-1st bvec need the check to make sure no sg gap.
> Looks a very crazy limit, :-)
>
>>
>> But for the 2nd change in bio_will_gap(), which should fix Vitaly's problem, I
>> am still not sure if it is completely correct. bio_will_gap() is used
>> to check if two
>> bios may be merged. Suppose two bios are continues physically, the last bvec
>> in 1st bio and the first bvec in 2nd bio might not be in one same segment
>> because of segment size limit.
>
> How about the attached patch?
>

I just wanted to revive the discussion as the issue persists. I
re-tested your patch against 4.6-rc4 and it efficiently solves the
issue.

pre-patch:
# time mkfs.ntfs /dev/sdb1
Cluster size has been automatically set to 4096 bytes.
Initializing device with zeroes: 100% - Done.
Creating NTFS volume structures.
mkntfs completed successfully. Have a nice day.

real8m10.977s
user0m0.115s
sys0m12.672s

post-patch:
# time mkfs.ntfs /dev/sdb1
Cluster size has been automatically set to 4096 bytes.
Initializing device with zeroes: 100% - Done.
Creating NTFS volume structures.
mkntfs completed successfully. Have a nice day.

real0m42.430s
user0m0.171s
sys0m7.675s

Will you send this patch? Please let me know if I can further
assist. Thanks!

-- 
  Vitaly