RE: [PATCH] block: return the correct bvec when checking for gaps

[Long Li]

> Subject: Re: [PATCH] block: return the correct bvec when checking for gaps
> 
> 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.

Sure, I will send v2.

Long


> 
> 
> Thanks,
> Ming