On Fri, Jun 04, 2021 at 06:38:45AM +0000, Long Li wrote:
> > Subject: Re: [PATCH] block: return the correct bvec when checking for gaps
> >
> > Hello Long,
> >
> > On Thu, Jun 03, 2021 at 03:34:31PM -0700, longli@xxxxxxxxxxxxxxxxx wrote:
> > > From: Long Li <longli@xxxxxxxxxxxxx>
> > >
> > > After commit 07173c3ec276 ("block: enable multipage bvecs"), a bvec
> > > can have multiple pages. But bio_will_gap() still assumes one page
> > > bvec while checking for merging. This causes data corruption on
> > > drivers relying on the correct merging on virt_boundary_mask.
> >
> > Can you explain the data corruption a bit?
> >
> > IMO, either single page bvec or multipage bvec should be fine, because
> > bio_will_gap() just checks if the last bvec of prev bio and the 1st bvec of next
> > bio can be merged.
>
> Hi Ming,
>
> When bio_will_gap() calls into biovec_phys_mergeable (), seg_boundary_mask (queue_segment_boundary()) is used to test if the two bio_vecs can be merged. This test can succeed if only the 1st page in bvec is used, but at the same time it can fail if all the pages in bvec are used. In other words, if the pages in bvec go across the seg_boundary_mask, the test can potentially succeed if only the 1st page is tested, but can fail if all the pages are tested.
>
> Later, when SCSI builds the SG list from BIOs (that calls into __blk_bios_map_sg), __blk_segment_map_sg_merge() calls biovec_phys_mergeable() doing the same test . This time it may fail if the pages in bvec go across the seg_boundary_mask (but tested okay in bio_will_gap() earlier, so those two BIOs were merged). If __blk_segment_map_sg_merge() fails, we end up with a broken SG list for drivers assuming the SG list not having offsets in intermediate pages.
>
OK, the reason is that both bio_will_gap() and __blk_segment_map_sg_merge()
have to use same approach to check if two bvecs from two bios can be
mergeable.
Now __blk_segment_map_sg_merge() won't merge the 1st bvec of next bio into
previous bio if the 1st bvec of next bio crosses segment boundary, so bio_will_gap()
has to take same way to check if the two bvecs can be merged.
Please add the segment boundary and map SG list story in commit log,
then the patch looks fine.
Thanks,
Ming
