blk_queue_may_bounce() has the comparison max_low_pfn and max_pfn wrong way

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?

Quoted 55 lines · Al Viro

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;
}

Quoted 1 line

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.

Quoted 56 lines · Jens Axboe

Quoted 55 lines · Al Viro

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;
}

Quoted 6 lines

Quoted 1 line

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

completion (in NULL_Q_BIO case, at least). What happens if request gets split and split-off part finishes first with an error? AFAICS, its ->bi_status will be copied to parent (original bio, the one that covers the tail). Now the IO on the original bio is over as well and we hit drivers/block/null_blk/main.c:end_cmd(). Suppose this part succeeds; won't we end up overwriting ->bi_status with zero and assuming that the entire thing had succeeded, despite the (now lost) error on the split-off part?

Quoted 8 lines · Al Viro

completion (in NULL_Q_BIO case, at least). What happens if request gets split and split-off part finishes first with an error? AFAICS, its ->bi_status will be copied to parent (original bio, the one that covers the tail). Now the IO on the original bio is over as well and we hit drivers/block/null_blk/main.c:end_cmd(). Suppose this part succeeds; won't we end up overwriting ->bi_status with zero and assuming that the entire thing had succeeded, despite the (now lost) error on the split-off part?

As a rule of thumb drives should never set bi_status to 0, so null_blk here has a bug. What is missing everywhere is proper memory barriers, though.

static inline void set_bio_status(struct bio *bio, blk_status_t status)

{
	if (unlikely(status))
		cmpxchg(&bio->bi_status, 0, status);
}

with e.g.

        if (bio->bi_status && !dio->bio.bi_status)
		dio->bio.bi_status = bio->bi_status;
in blkdev_bio_end_io() replaced with
	set_bio_status(&dio->bio, bio->bi_status);

perhaps?

That would probably do for almost all users, but... what about e.g. drivers/md/raid1.c:fix_sync_read_error()? Looks like it really intends non-zero -> zero change; I'm not familiar enough with the guts of that sucker to tell if it is guaranteed to get no propagation from another bio...

I wonder if we ought to simply add "depends on BROKEN" for CONFIG_BOUNCE... ;-/

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?

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.