Re: [PATCH v7 1/3] bio: limit bio max size
From: Changheun Lee <hidden>
Date: 2021-04-19 06:07:53
Also in:
lkml
quoted
@@ -167,6 +168,7 @@ void blk_queue_max_hw_sectors(struct request_queue *q, unsigned int max_hw_secto max_sectors = round_down(max_sectors, limits->logical_block_size >> SECTOR_SHIFT); limits->max_sectors = max_sectors; + limits->bio_max_bytes = max_sectors << SECTOR_SHIFT; q->backing_dev_info->io_pages = max_sectors >> (PAGE_SHIFT - 9); }Can the new shift operation overflow? If so, how about using check_shl_overflow()?
Actually, overflow might be not heppen in case of physical device. But I modified as below. feedback about this.
@@ -168,6 +169,9 @@ void blk_queue_max_hw_sectors(struct request_queue *q, unsigned int max_hw_secto limits->logical_block_size >> SECTOR_SHIFT); limits->max_sectors = max_sectors; + limits->bio_max_bytes = check_shl_overflow(max_sectors, SECTOR_SHIFT, + &limits->bio_max_bytes) ? UINT_MAX : max_sectors << SECTOR_SHIFT; + q->backing_dev_info->io_pages = max_sectors >> (PAGE_SHIFT - 9); } EXPORT_SYMBOL(blk_queue_max_hw_sectors);
quoted
quoted
quoted
@@ -538,6 +540,8 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b, { unsigned int top, bottom, alignment, ret = 0; + t->bio_max_bytes = min_not_zero(t->bio_max_bytes, b->bio_max_bytes); + t->max_sectors = min_not_zero(t->max_sectors, b->max_sectors); t->max_hw_sectors = min_not_zero(t->max_hw_sectors, b->max_hw_sectors); t->max_dev_sectors = min_not_zero(t->max_dev_sectors, b->max_dev_sectors);The above will limit bio_max_bytes for all stacked block devices, which is something we do not want. I propose to set t->bio_max_bytes to UINT_MAX in blk_stack_limits() and to let the stacked driver (e.g. dm-crypt) decide whether or not to lower that value.Actually, bio size should be limited in dm-crypt too. Because almost I/O from user space will be gone to dm-crypt first. I/O issue timing will be delayed if bio size is not limited in dm-crypt. Do you have any idea to decide whether takes lower bio max size, or not in the stacked driver? Add a flag to decide this in driver layer like before? Or insert code manually in each stacked driver if it is needed?There will be fewer stacked drivers for which the bio size has to be limited than for which the bio size has not to be limited. Hence the proposal to set t->bio_max_bytes to UINT_MAX in blk_stack_limits() and to let the stacked driver (e.g. dm-crypt) decide whether or not to lower that value.
I see what you said. I'll set t->bio_max_bytes to UINT_MAX in blk_stack_limits() as you mentioned.
quoted
diff --git a/include/linux/bio.h b/include/linux/bio.h index d0246c92a6e8..e5add63da3af 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h@@ -106,6 +106,8 @@ static inline void *bio_data(struct bio *bio) return NULL; } +extern unsigned int bio_max_size(struct bio *bio);You may want to define bio_max_size() as an inline function in bio.h such that no additional function calls are introduced in the hot path.
I tried, but it is not easy. because request_queue structure of blkdev.h should be referred in bio.h. I think it's not good to apply as a inline function. Thanks, Changheun Lee.