On Tue, Aug 20, 2019 at 10:08:38PM +0000, Verma, Vishal L wrote:
> On Wed, 2019-08-21 at 07:44 +1000, Dave Chinner wrote:
> >
> > However, the case here is that:
> >
> > > > > > i.e. page offset len sector
> > > > > > 00000000a77f0146 768 3328 0x7d0048
> > > > > > 000000006ceca91e 0 768 0x7d004e
> >
> > The second page added to the bvec is actually offset alignedr. Hence
> > the check would do nothing on the first page because the bvec array
> > is empty (so goes into a new bvec anyway), and the check on the
> > second page would do nothing an it would merge with first because
> > the offset is aligned correctly. In both cases, the length of the
> > segment is not aligned, so that needs to be checked, too.
> >
> > IOWs, I think the check needs to be in bio_add_page, it needs to
> > check both the offset and length for alignment, and it needs to grab
> > the alignment from queue_dma_alignment(), not use a hard coded value
> > of 511.
> >
> So something like this?
>
> diff --git a/block/bio.c b/block/bio.c
> index 299a0e7651ec..80f449d23e5a 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -822,8 +822,12 @@ EXPORT_SYMBOL_GPL(__bio_add_page);
> int bio_add_page(struct bio *bio, struct page *page,
> unsigned int len, unsigned int offset)
> {
> + struct request_queue *q = bio->bi_disk->queue;
> bool same_page = false;
>
> + if (offset & queue_dma_alignment(q) || len & queue_dma_alignment(q))
> + return 0;
bio->bi_disk or bio->bi_disk->queue may not be available in bio_add_page().
And 'len' has to be aligned with logical block size of the disk on which fs is
mounted, which info is obviously available for fs.
So the following code is really buggy:
xfs_rw_bdev():
+ struct page *page = kmem_to_page(data);
+ unsigned int off = offset_in_page(data);
+ unsigned int len = min_t(unsigned, left, PAGE_SIZE - off);
+
+ while (bio_add_page(bio, page, len, off) != len) {
Thanks,
Ming
