| Message ID | 20200721105239.8270-2-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: move logical block size checking to the block core | expand |
On 2020/07/21 19:53, Maxim Levitsky wrote: > Kernel block layer has never supported logical block > sizes less that SECTOR_SIZE nor larger that PAGE_SIZE. > > Some drivers have runtime configurable block size, > so it makes sense to have common helper for that. ...common helper to check the validity of the logical block size set by the driver. ("for that" does not refer to a clear action) > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > block/blk-settings.c | 18 ++++++++++++++++++ > include/linux/blkdev.h | 1 + > 2 files changed, 19 insertions(+) > > diff --git a/block/blk-settings.c b/block/blk-settings.c > index 9a2c23cd97007..3c4ef0d00c2bc 100644 > --- a/block/blk-settings.c > +++ b/block/blk-settings.c > @@ -311,6 +311,21 @@ void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size) > } > EXPORT_SYMBOL(blk_queue_max_segment_size); > > + > +/** > + * blk_check_logical_block_size - check if logical block size is supported > + * by the kernel > + * @size: the logical block size, in bytes > + * > + * Description: > + * This function checks if the block layers supports given block size > + **/ > +bool blk_is_valid_logical_block_size(unsigned int size) > +{ > + return size >= SECTOR_SIZE && size <= PAGE_SIZE && !is_power_of_2(size); > +} > +EXPORT_SYMBOL(blk_is_valid_logical_block_size); > + > /** > * blk_queue_logical_block_size - set logical block size for the queue > * @q: the request queue for the device > @@ -323,6 +338,8 @@ EXPORT_SYMBOL(blk_queue_max_segment_size); > **/ > void blk_queue_logical_block_size(struct request_queue *q, unsigned int size) > { > + WARN_ON(!blk_is_valid_logical_block_size(size)); > + > q->limits.logical_block_size = size; > > if (q->limits.physical_block_size < size) > @@ -330,6 +347,7 @@ void blk_queue_logical_block_size(struct request_queue *q, unsigned int size) > > if (q->limits.io_min < q->limits.physical_block_size) > q->limits.io_min = q->limits.physical_block_size; > + white line change. > } > EXPORT_SYMBOL(blk_queue_logical_block_size); > > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h > index 57241417ff2f8..2ed3151397e41 100644 > --- a/include/linux/blkdev.h > +++ b/include/linux/blkdev.h > @@ -1099,6 +1099,7 @@ extern void blk_queue_max_write_same_sectors(struct request_queue *q, > unsigned int max_write_same_sectors); > extern void blk_queue_max_write_zeroes_sectors(struct request_queue *q, > unsigned int max_write_same_sectors); > +extern bool blk_is_valid_logical_block_size(unsigned int size); > extern void blk_queue_logical_block_size(struct request_queue *, unsigned int); > extern void blk_queue_max_zone_append_sectors(struct request_queue *q, > unsigned int max_zone_append_sectors); >
On Tue, 2020-07-21 at 11:05 +0000, Damien Le Moal wrote: > On 2020/07/21 19:53, Maxim Levitsky wrote: > > Kernel block layer has never supported logical block > > sizes less that SECTOR_SIZE nor larger that PAGE_SIZE. > > > > Some drivers have runtime configurable block size, > > so it makes sense to have common helper for that. > > ...common helper to check the validity of the logical block size set by the driver. Agree, will fix. > > ("for that" does not refer to a clear action) > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > block/blk-settings.c | 18 ++++++++++++++++++ > > include/linux/blkdev.h | 1 + > > 2 files changed, 19 insertions(+) > > > > diff --git a/block/blk-settings.c b/block/blk-settings.c > > index 9a2c23cd97007..3c4ef0d00c2bc 100644 > > --- a/block/blk-settings.c > > +++ b/block/blk-settings.c > > @@ -311,6 +311,21 @@ void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size) > > } > > EXPORT_SYMBOL(blk_queue_max_segment_size); > > > > + > > +/** > > + * blk_check_logical_block_size - check if logical block size is supported > > + * by the kernel > > + * @size: the logical block size, in bytes > > + * > > + * Description: > > + * This function checks if the block layers supports given block size > > + **/ > > +bool blk_is_valid_logical_block_size(unsigned int size) > > +{ > > + return size >= SECTOR_SIZE && size <= PAGE_SIZE && !is_power_of_2(size); Note here a typo, made in last minute change which I didn't test. It should be without '!' Best regards, Maxim Levitsky > > +} > > +EXPORT_SYMBOL(blk_is_valid_logical_block_size); > > + > > /** > > * blk_queue_logical_block_size - set logical block size for the queue > > * @q: the request queue for the device > > @@ -323,6 +338,8 @@ EXPORT_SYMBOL(blk_queue_max_segment_size); > > **/ > > void blk_queue_logical_block_size(struct request_queue *q, unsigned int size) > > { > > + WARN_ON(!blk_is_valid_logical_block_size(size)); > > + > > q->limits.logical_block_size = size; > > > > if (q->limits.physical_block_size < size) > > @@ -330,6 +347,7 @@ void blk_queue_logical_block_size(struct request_queue *q, unsigned int size) > > > > if (q->limits.io_min < q->limits.physical_block_size) > > q->limits.io_min = q->limits.physical_block_size; > > + > > white line change. > > > } > > EXPORT_SYMBOL(blk_queue_logical_block_size); > > > > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h > > index 57241417ff2f8..2ed3151397e41 100644 > > --- a/include/linux/blkdev.h > > +++ b/include/linux/blkdev.h > > @@ -1099,6 +1099,7 @@ extern void blk_queue_max_write_same_sectors(struct request_queue *q, > > unsigned int max_write_same_sectors); > > extern void blk_queue_max_write_zeroes_sectors(struct request_queue *q, > > unsigned int max_write_same_sectors); > > +extern bool blk_is_valid_logical_block_size(unsigned int size); > > extern void blk_queue_logical_block_size(struct request_queue *, unsigned int); > > extern void blk_queue_max_zone_append_sectors(struct request_queue *q, > > unsigned int max_zone_append_sectors); > > > >
On 2020/07/21 20:09, Maxim Levitsky wrote: > On Tue, 2020-07-21 at 11:05 +0000, Damien Le Moal wrote: >> On 2020/07/21 19:53, Maxim Levitsky wrote: >>> Kernel block layer has never supported logical block >>> sizes less that SECTOR_SIZE nor larger that PAGE_SIZE. >>> >>> Some drivers have runtime configurable block size, >>> so it makes sense to have common helper for that. >> >> ...common helper to check the validity of the logical block size set by the driver. > Agree, will fix. >> >> ("for that" does not refer to a clear action) >> >>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> >>> --- >>> block/blk-settings.c | 18 ++++++++++++++++++ >>> include/linux/blkdev.h | 1 + >>> 2 files changed, 19 insertions(+) >>> >>> diff --git a/block/blk-settings.c b/block/blk-settings.c >>> index 9a2c23cd97007..3c4ef0d00c2bc 100644 >>> --- a/block/blk-settings.c >>> +++ b/block/blk-settings.c >>> @@ -311,6 +311,21 @@ void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size) >>> } >>> EXPORT_SYMBOL(blk_queue_max_segment_size); >>> >>> + >>> +/** >>> + * blk_check_logical_block_size - check if logical block size is supported >>> + * by the kernel >>> + * @size: the logical block size, in bytes >>> + * >>> + * Description: >>> + * This function checks if the block layers supports given block size >>> + **/ >>> +bool blk_is_valid_logical_block_size(unsigned int size) >>> +{ >>> + return size >= SECTOR_SIZE && size <= PAGE_SIZE && !is_power_of_2(size); > Note here a typo, made in last minute change which I didn't test. > It should be without '!' Oh ! Indeed. I missed that. > > Best regards, > Maxim Levitsky > >>> +} >>> +EXPORT_SYMBOL(blk_is_valid_logical_block_size); >>> + >>> /** >>> * blk_queue_logical_block_size - set logical block size for the queue >>> * @q: the request queue for the device >>> @@ -323,6 +338,8 @@ EXPORT_SYMBOL(blk_queue_max_segment_size); >>> **/ >>> void blk_queue_logical_block_size(struct request_queue *q, unsigned int size) >>> { >>> + WARN_ON(!blk_is_valid_logical_block_size(size)); >>> + >>> q->limits.logical_block_size = size; >>> >>> if (q->limits.physical_block_size < size) >>> @@ -330,6 +347,7 @@ void blk_queue_logical_block_size(struct request_queue *q, unsigned int size) >>> >>> if (q->limits.io_min < q->limits.physical_block_size) >>> q->limits.io_min = q->limits.physical_block_size; >>> + >> >> white line change. >> >>> } >>> EXPORT_SYMBOL(blk_queue_logical_block_size); >>> >>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h >>> index 57241417ff2f8..2ed3151397e41 100644 >>> --- a/include/linux/blkdev.h >>> +++ b/include/linux/blkdev.h >>> @@ -1099,6 +1099,7 @@ extern void blk_queue_max_write_same_sectors(struct request_queue *q, >>> unsigned int max_write_same_sectors); >>> extern void blk_queue_max_write_zeroes_sectors(struct request_queue *q, >>> unsigned int max_write_same_sectors); >>> +extern bool blk_is_valid_logical_block_size(unsigned int size); >>> extern void blk_queue_logical_block_size(struct request_queue *, unsigned int); >>> extern void blk_queue_max_zone_append_sectors(struct request_queue *q, >>> unsigned int max_zone_append_sectors); >>> >> >> > > >
> +/** > + * blk_check_logical_block_size - check if logical block size is supported > + * by the kernel > + * @size: the logical block size, in bytes > + * > + * Description: > + * This function checks if the block layers supports given block size > + **/ > +bool blk_is_valid_logical_block_size(unsigned int size) > +{ > + return size >= SECTOR_SIZE && size <= PAGE_SIZE && !is_power_of_2(size); Shouldn't this be a ... && is_power_of_2(size)? > if (q->limits.io_min < q->limits.physical_block_size) > q->limits.io_min = q->limits.physical_block_size; > + > } This adds a pointless empty line. > +extern bool blk_is_valid_logical_block_size(unsigned int size); No need for externs on function declarations.
On Tue, 2020-07-21 at 17:13 +0200, Christoph Hellwig wrote: > > +/** > > + * blk_check_logical_block_size - check if logical block size is > > supported > > + * by the kernel > > + * @size: the logical block size, in bytes > > + * > > + * Description: > > + * This function checks if the block layers supports given block > > size > > + **/ > > +bool blk_is_valid_logical_block_size(unsigned int size) > > +{ > > + return size >= SECTOR_SIZE && size <= PAGE_SIZE && > > !is_power_of_2(size); > > Shouldn't this be a ... && is_power_of_2(size)? Yep. I noticed that few minutes after I sent the patches. > > > if (q->limits.io_min < q->limits.physical_block_size) > > q->limits.io_min = q->limits.physical_block_size; > > + > > } > > This adds a pointless empty line. Will fix. > > > +extern bool blk_is_valid_logical_block_size(unsigned int size); > > No need for externs on function declarations. I also think so, but I followed the style of all existing function prototypes in this file. Most of them have 'extern'. Thanks for the review! Best regards, maxim Levitsky
diff --git a/block/blk-settings.c b/block/blk-settings.c index 9a2c23cd97007..3c4ef0d00c2bc 100644 --- a/block/blk-settings.c +++ b/block/blk-settings.c @@ -311,6 +311,21 @@ void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size) } EXPORT_SYMBOL(blk_queue_max_segment_size); + +/** + * blk_check_logical_block_size - check if logical block size is supported + * by the kernel + * @size: the logical block size, in bytes + * + * Description: + * This function checks if the block layers supports given block size + **/ +bool blk_is_valid_logical_block_size(unsigned int size) +{ + return size >= SECTOR_SIZE && size <= PAGE_SIZE && !is_power_of_2(size); +} +EXPORT_SYMBOL(blk_is_valid_logical_block_size); + /** * blk_queue_logical_block_size - set logical block size for the queue * @q: the request queue for the device @@ -323,6 +338,8 @@ EXPORT_SYMBOL(blk_queue_max_segment_size); **/ void blk_queue_logical_block_size(struct request_queue *q, unsigned int size) { + WARN_ON(!blk_is_valid_logical_block_size(size)); + q->limits.logical_block_size = size; if (q->limits.physical_block_size < size) @@ -330,6 +347,7 @@ void blk_queue_logical_block_size(struct request_queue *q, unsigned int size) if (q->limits.io_min < q->limits.physical_block_size) q->limits.io_min = q->limits.physical_block_size; + } EXPORT_SYMBOL(blk_queue_logical_block_size); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 57241417ff2f8..2ed3151397e41 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -1099,6 +1099,7 @@ extern void blk_queue_max_write_same_sectors(struct request_queue *q, unsigned int max_write_same_sectors); extern void blk_queue_max_write_zeroes_sectors(struct request_queue *q, unsigned int max_write_same_sectors); +extern bool blk_is_valid_logical_block_size(unsigned int size); extern void blk_queue_logical_block_size(struct request_queue *, unsigned int); extern void blk_queue_max_zone_append_sectors(struct request_queue *q, unsigned int max_zone_append_sectors);
Kernel block layer has never supported logical block sizes less that SECTOR_SIZE nor larger that PAGE_SIZE. Some drivers have runtime configurable block size, so it makes sense to have common helper for that. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- block/blk-settings.c | 18 ++++++++++++++++++ include/linux/blkdev.h | 1 + 2 files changed, 19 insertions(+)