On Fri, Oct 28, 2022 at 12:51:00PM -0600, Jens Axboe wrote:
> On 10/28/22 12:02 PM, Al Viro wrote:
> > We have this:
> >
> > static inline bool blk_queue_may_bounce(struct request_queue *q)
> > {
> > return IS_ENABLED(CONFIG_BOUNCE) &&
> > q->limits.bounce == BLK_BOUNCE_HIGH &&
> > max_low_pfn >= max_pfn;
> > }
> >
> > static inline struct bio *blk_queue_bounce(struct bio *bio,
> > struct request_queue *q)
> > {
> > if (unlikely(blk_queue_may_bounce(q) && bio_has_data(bio)))
> > return __blk_queue_bounce(bio, q);
> > return bio;
> > }
> >
> > Now, the last term in expression in blk_queue_may_bounce() is
> > true only on the boxen where max_pfn is no greater than max_low_pfn.
> >
> > Unless I'm very confused, that's the boxen where we don't *have*
> > any highmem pages.
> >
> > What's more, consider this:
> >
> > static __init int init_emergency_pool(void)
> > {
> > int ret;
> >
> > #ifndef CONFIG_MEMORY_HOTPLUG
> > if (max_pfn <= max_low_pfn)
> > return 0;
> > #endif
> >
> > ret = mempool_init_page_pool(&page_pool, POOL_SIZE, 0);
> > BUG_ON(ret);
> > pr_info("pool size: %d pages\n", POOL_SIZE);
> >
> > init_bounce_bioset();
> > return 0;
> > }
> >
> > On the same boxen (assuming we've not hotplug) page_pool won't be set up
> > at all, so we wouldn't be able to bounce any highmem page if we ever
> > ran into one.
> >
> > AFAICS, this condition is backwards - it should be
> >
> > static inline bool blk_queue_may_bounce(struct request_queue *q)
> > {
> > return IS_ENABLED(CONFIG_BOUNCE) &&
> > q->limits.bounce == BLK_BOUNCE_HIGH &&
> > max_low_pfn < max_pfn;
> > }
> >
> > What am I missing here?
>
> I don't think you're missing anything, the case we need it for is when
> max_pfn > max_low_pfn. I wonder when this got botched? Because some
> of those statements date back probably about 20 years when we started
> allowing highmem pages to do IO.
9bb33f24abbd "block: refactor the bounce buffering code" about a year ago.
But fixing the test alone is not going to be enough - just a quick look
through __blk_queue_bounce() catches an obvious bug on write (we copy
the part of highmem page we covered by bio into the beginning of the
bounce page - and leave ->bv_offset as-is) as well as a possible bug in
->bi_status handling (if we really can run into bio_split() there).
I wonder if we ought to simply add "depends on BROKEN" for CONFIG_BOUNCE... ;-/
