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;
> +
> if (!__bio_try_merge_page(bio, page, len, offset, &same_page)) {
> if (bio_full(bio, len))
> return 0;
>
> I tried this, but the 'mount' just hangs - which looks like it might be
> due to xfs_rw_bdev() doing:
>
> while (bio_add_page(bio, page, len, off) != len) {
That's the return of zero that causes the loop to make no progress.
i.e. a return of 0 means "won't fit in bio, allocate a new bio
and try again". It's not an error return, so always returning zero
will eventually chew up all your memory allocating bios it
doesn't use, because submit_bio() doesn't return errors on chained
bios until the final bio in the chain is completed.
Add a bio_add_page_checked() function that does exactly the same
this as bio_add_page(), but add the
if (WARN_ON_ONCE((offset | len) & queue_dma_alignment(q)))
return -EIO;
to it and change the xfs code to:
while ((len = bio_add_page_checked(bio, page, len, off)) != len) {
if (len < 0) {
/*
* submit the bio to wait on the rest of the
* chain to complete, then return an error.
* This is a really shitty failure on write, as we
* will have just done a partial write and
* effectively corrupted something on disk.
*/
submit_bio_wait(bio);
return len;
}
....
}
We probably should change all the XFS calls to bio_add_page to
bio_add_page_checked() while we are at it, because we have the
same alignment problem through xfs_buf.c and, potentially, on iclogs
via xfs_log.c as iclogs are allocated with kmem_alloc_large(), too.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
