Re: [bug?] blk_queue_may_bounce() has the comparison max_low_pfn and max_pfn wrong way

[Al Viro <viro@zeniv.linux.org.uk>]

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... ;-/