[01/10] block: introduce blk_is_valid_logical_block_size
diff mbox series

Message ID 20200721105239.8270-2-mlevitsk@redhat.com
State New
Headers show
Series
  • RFC: move logical block size checking to the block core
Related show

Commit Message

Maxim Levitsky July 21, 2020, 10:52 a.m. UTC
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(+)

Comments

Damien Le Moal July 21, 2020, 11:05 a.m. UTC | #1
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);
>
Maxim Levitsky July 21, 2020, 11:09 a.m. UTC | #2
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);
> > 
> 
>
Damien Le Moal July 21, 2020, 11:31 a.m. UTC | #3
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);
>>>
>>
>>
> 
> 
>
Christoph Hellwig July 21, 2020, 3:13 p.m. UTC | #4
> +/**
> + * 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.
Maxim Levitsky July 22, 2020, 8:50 a.m. UTC | #5
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

Patch
diff mbox series

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