[PATCH] block: make blk_queue_zone_sectors() return sector_t type

[PATCH] block: make blk_queue_zone_sectors() return sector_t type

[Dan Carpenter]

My static checker complains about this line from dmz_get_zoned_device()

	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);

The problem is that "aligned_capacity" and "dev->capacity" are sector_t
type (which is a u64) but blk_queue_zone_sectors(q) returns a u32 so the
higher 32 bits in aligned_capacity are always cleared to zero.

Declaring blk_queue_zone_sectors() as a sector_t addresses this warning
and it feels intuitive based on the function name.  I updated
bdev_zone_sectors() as well just to be consistent.

Fixes: 114e025968b5 ("dm zoned: ignore last smaller runt zone")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Please review this one extra carefully.  I'm not positive it's correct.

 include/linux/blkdev.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 4b85dc066264..1785a7f506be 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -670,7 +670,7 @@ static inline bool blk_queue_is_zoned(struct request_queue *q)
 	}
 }
 
-static inline unsigned int blk_queue_zone_sectors(struct request_queue *q)
+static inline sector_t blk_queue_zone_sectors(struct request_queue *q)
 {
 	return blk_queue_is_zoned(q) ? q->limits.chunk_sectors : 0;
 }
@@ -1419,7 +1419,7 @@ static inline bool bdev_is_zoned(struct block_device *bdev)
 	return false;
 }
 
-static inline unsigned int bdev_zone_sectors(struct block_device *bdev)
+static inline sector_t bdev_zone_sectors(struct block_device *bdev)
 {
 	struct request_queue *q = bdev_get_queue(bdev);
 
-- 
2.17.1

Re: [PATCH] block: make blk_queue_zone_sectors() return sector_t type

[Damien Le Moal]

On 2019/04/10 15:10, Dan Carpenter wrote:
> My static checker complains about this line from dmz_get_zoned_device()
> 
> 	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);
> 
> The problem is that "aligned_capacity" and "dev->capacity" are sector_t
> type (which is a u64) but blk_queue_zone_sectors(q) returns a u32 so the
> higher 32 bits in aligned_capacity are always cleared to zero.
> 
> Declaring blk_queue_zone_sectors() as a sector_t addresses this warning
> and it feels intuitive based on the function name.  I updated
> bdev_zone_sectors() as well just to be consistent.
> 
> Fixes: 114e025968b5 ("dm zoned: ignore last smaller runt zone")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Please review this one extra carefully.  I'm not positive it's correct.
> 
>  include/linux/blkdev.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 4b85dc066264..1785a7f506be 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -670,7 +670,7 @@ static inline bool blk_queue_is_zoned(struct request_queue *q)
>  	}
>  }
>  
> -static inline unsigned int blk_queue_zone_sectors(struct request_queue *q)
> +static inline sector_t blk_queue_zone_sectors(struct request_queue *q)
>  {
>  	return blk_queue_is_zoned(q) ? q->limits.chunk_sectors : 0;
>  }
> @@ -1419,7 +1419,7 @@ static inline bool bdev_is_zoned(struct block_device *bdev)
>  	return false;
>  }
>  
> -static inline unsigned int bdev_zone_sectors(struct block_device *bdev)
> +static inline sector_t bdev_zone_sectors(struct block_device *bdev)
>  {
>  	struct request_queue *q = bdev_get_queue(bdev);

Indeed, using sector_t instead of unsigned int is more intuitive and changing to
sector_t matches the 64bits zone length in struct blk_zone too, so no problem on
this front.

However, q->limits.chunk_sectors is an unsigned int, which is why I used that
type, to reflect the fact that even though the API allows values larger than 4G
sectors, the handling through the queue limits truncates that to at most 4G-1
sectors. Furthermore, the unsigned int type is used wherever
blk_queue_zone_sectors() and bdev_zone_sectors() are called (block/ioctl.c,
block/blk-zoned.c, f2fs, etc). That needs in depth checking.

I would rather keep the unsigned int since the queue limits will give only 32
bit values anyway. What about a cast for the aligned capacity line ? Something like:

aligned_capacity = dev->capacity & ~((u64)blk_queue_zone_sectors(q) - 1);

Would that make your checker happy ?


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] block: make blk_queue_zone_sectors() return sector_t type

[Dan Carpenter]

On Wed, Apr 10, 2019 at 07:20:27AM +0000, Damien Le Moal wrote:
> On 2019/04/10 15:10, Dan Carpenter wrote:
> > My static checker complains about this line from dmz_get_zoned_device()
> > 
> > 	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);
> > 
> > The problem is that "aligned_capacity" and "dev->capacity" are sector_t
> > type (which is a u64) but blk_queue_zone_sectors(q) returns a u32 so the
> > higher 32 bits in aligned_capacity are always cleared to zero.
> > 
> > Declaring blk_queue_zone_sectors() as a sector_t addresses this warning
> > and it feels intuitive based on the function name.  I updated
> > bdev_zone_sectors() as well just to be consistent.
> > 
> > Fixes: 114e025968b5 ("dm zoned: ignore last smaller runt zone")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > Please review this one extra carefully.  I'm not positive it's correct.
> > 
> >  include/linux/blkdev.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index 4b85dc066264..1785a7f506be 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -670,7 +670,7 @@ static inline bool blk_queue_is_zoned(struct request_queue *q)
> >  	}
> >  }
> >  
> > -static inline unsigned int blk_queue_zone_sectors(struct request_queue *q)
> > +static inline sector_t blk_queue_zone_sectors(struct request_queue *q)
> >  {
> >  	return blk_queue_is_zoned(q) ? q->limits.chunk_sectors : 0;
> >  }
> > @@ -1419,7 +1419,7 @@ static inline bool bdev_is_zoned(struct block_device *bdev)
> >  	return false;
> >  }
> >  
> > -static inline unsigned int bdev_zone_sectors(struct block_device *bdev)
> > +static inline sector_t bdev_zone_sectors(struct block_device *bdev)
> >  {
> >  	struct request_queue *q = bdev_get_queue(bdev);
> 
> Indeed, using sector_t instead of unsigned int is more intuitive and changing to
> sector_t matches the 64bits zone length in struct blk_zone too, so no problem on
> this front.
> 
> However, q->limits.chunk_sectors is an unsigned int, which is why I used that
> type, to reflect the fact that even though the API allows values larger than 4G
> sectors, the handling through the queue limits truncates that to at most 4G-1
> sectors. Furthermore, the unsigned int type is used wherever
> blk_queue_zone_sectors() and bdev_zone_sectors() are called (block/ioctl.c,
> block/blk-zoned.c, f2fs, etc). That needs in depth checking.
> 
> I would rather keep the unsigned int since the queue limits will give only 32
> bit values anyway. What about a cast for the aligned capacity line ? Something like:
> 
> aligned_capacity = dev->capacity & ~((u64)blk_queue_zone_sectors(q) - 1);
> 
> Would that make your checker happy ?
> 

Yeah.  Or a sector_t cast.

	aligned_capacity = dev->capacity & ~((sector_t)blk_queue_zone_sectors(q) - 1);

regards,
dan carpenter

[PATCH v2] dm zoned: Silence a static checker warning

[Dan Carpenter]

My static checker complains about this line from dmz_get_zoned_device()

	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);

The problem is that "aligned_capacity" and "dev->capacity" are sector_t
type (which is a u64) but blk_queue_zone_sectors(q) returns a u32 so the
higher 32 bits in "aligned_capacity" are always cleared to zero.  This
patch adds a cast to u64 to address this issue.

Fixes: 114e025968b5 ("dm zoned: ignore last smaller runt zone")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: In v1 I changed blk_queue_zone_sectors() to return a sector_t type,
but in v2 I just add a cast.  The v2 fix would end up going through
different maintainers and reviewers so the CC list has grown...

Original discussion: https://marc.info/?l=kernel-janitors&m\x155487663405737&w=2

 drivers/md/dm-zoned-target.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
index 8865c1709e16..b6cb44fa946d 100644
--- a/drivers/md/dm-zoned-target.c
+++ b/drivers/md/dm-zoned-target.c
@@ -643,7 +643,8 @@ static int dmz_get_zoned_device(struct dm_target *ti, char *path)
 
 	q = bdev_get_queue(dev->bdev);
 	dev->capacity = i_size_read(dev->bdev->bd_inode) >> SECTOR_SHIFT;
-	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);
+	aligned_capacity = dev->capacity &
+				~((u64)blk_queue_zone_sectors(q) - 1);
 	if (ti->begin ||
 	    ((ti->len != dev->capacity) && (ti->len != aligned_capacity))) {
 		ti->error = "Partial mapping not supported";
-- 
2.17.1

Re: [PATCH v2] dm zoned: Silence a static checker warning

[Damien Le Moal]

On 2019/04/10 16:48, Dan Carpenter wrote:
> My static checker complains about this line from dmz_get_zoned_device()
> 
> 	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);
> 
> The problem is that "aligned_capacity" and "dev->capacity" are sector_t
> type (which is a u64) but blk_queue_zone_sectors(q) returns a u32 so the
> higher 32 bits in "aligned_capacity" are always cleared to zero.  This
> patch adds a cast to u64 to address this issue.
> 
> Fixes: 114e025968b5 ("dm zoned: ignore last smaller runt zone")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: In v1 I changed blk_queue_zone_sectors() to return a sector_t type,
> but in v2 I just add a cast.  The v2 fix would end up going through
> different maintainers and reviewers so the CC list has grown...
> 
> Original discussion: https://marc.info/?l=kernel-janitors&m=155487663405737&w=2
> 
>  drivers/md/dm-zoned-target.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
> index 8865c1709e16..b6cb44fa946d 100644
> --- a/drivers/md/dm-zoned-target.c
> +++ b/drivers/md/dm-zoned-target.c
> @@ -643,7 +643,8 @@ static int dmz_get_zoned_device(struct dm_target *ti, char *path)
>  
>  	q = bdev_get_queue(dev->bdev);
>  	dev->capacity = i_size_read(dev->bdev->bd_inode) >> SECTOR_SHIFT;
> -	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);
> +	aligned_capacity = dev->capacity &
> +				~((u64)blk_queue_zone_sectors(q) - 1);

sector_t is an u64 only if CONFIG_LBDAF is defined (I think this option is going
away though). Otherwise it is an unsigned long which would be u32 on 32 bits
arch. Not a problem in terms of arithmetic, but why not cast to sector_t directly ?

>  	if (ti->begin ||
>  	    ((ti->len != dev->capacity) && (ti->len != aligned_capacity))) {
>  		ti->error = "Partial mapping not supported";
> 
-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] dm zoned: Silence a static checker warning

[Dan Carpenter]

On Wed, Apr 10, 2019 at 07:56:14AM +0000, Damien Le Moal wrote:
> On 2019/04/10 16:48, Dan Carpenter wrote:
> > My static checker complains about this line from dmz_get_zoned_device()
> > 
> > 	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);
> > 
> > The problem is that "aligned_capacity" and "dev->capacity" are sector_t
> > type (which is a u64) but blk_queue_zone_sectors(q) returns a u32 so the
> > higher 32 bits in "aligned_capacity" are always cleared to zero.  This
> > patch adds a cast to u64 to address this issue.
> > 
> > Fixes: 114e025968b5 ("dm zoned: ignore last smaller runt zone")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > v2: In v1 I changed blk_queue_zone_sectors() to return a sector_t type,
> > but in v2 I just add a cast.  The v2 fix would end up going through
> > different maintainers and reviewers so the CC list has grown...
> > 
> > Original discussion: https://marc.info/?l=kernel-janitors&m\x155487663405737&w=2
> > 
> >  drivers/md/dm-zoned-target.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
> > index 8865c1709e16..b6cb44fa946d 100644
> > --- a/drivers/md/dm-zoned-target.c
> > +++ b/drivers/md/dm-zoned-target.c
> > @@ -643,7 +643,8 @@ static int dmz_get_zoned_device(struct dm_target *ti, char *path)
> >  
> >  	q = bdev_get_queue(dev->bdev);
> >  	dev->capacity = i_size_read(dev->bdev->bd_inode) >> SECTOR_SHIFT;
> > -	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);
> > +	aligned_capacity = dev->capacity &
> > +				~((u64)blk_queue_zone_sectors(q) - 1);
> 
> sector_t is an u64 only if CONFIG_LBDAF is defined (I think this option is going
> away though). Otherwise it is an unsigned long which would be u32 on 32 bits
> arch. Not a problem in terms of arithmetic, but why not cast to sector_t directly ?
> 

I would have prefered to do that but I didn't have strong feelings
either way.  I am always slight annoyed when people don't just copy and
paste my code when I tell them how to fix a patch so I decided to go
with your version...  :P

regards,
dan carpenter

Re: [dm-devel] [PATCH v2] dm zoned: Silence a static checker warning

[Damien Le Moal]

On 2019/04/10 17:03, Dan Carpenter wrote:
> On Wed, Apr 10, 2019 at 07:56:14AM +0000, Damien Le Moal wrote:
>> On 2019/04/10 16:48, Dan Carpenter wrote:
>>> My static checker complains about this line from dmz_get_zoned_device()
>>>
>>> 	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);
>>>
>>> The problem is that "aligned_capacity" and "dev->capacity" are sector_t
>>> type (which is a u64) but blk_queue_zone_sectors(q) returns a u32 so the
>>> higher 32 bits in "aligned_capacity" are always cleared to zero.  This
>>> patch adds a cast to u64 to address this issue.
>>>
>>> Fixes: 114e025968b5 ("dm zoned: ignore last smaller runt zone")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> ---
>>> v2: In v1 I changed blk_queue_zone_sectors() to return a sector_t type,
>>> but in v2 I just add a cast.  The v2 fix would end up going through
>>> different maintainers and reviewers so the CC list has grown...
>>>
>>> Original discussion: https://marc.info/?l=kernel-janitors&m=155487663405737&w=2
>>>
>>>  drivers/md/dm-zoned-target.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
>>> index 8865c1709e16..b6cb44fa946d 100644
>>> --- a/drivers/md/dm-zoned-target.c
>>> +++ b/drivers/md/dm-zoned-target.c
>>> @@ -643,7 +643,8 @@ static int dmz_get_zoned_device(struct dm_target *ti, char *path)
>>>  
>>>  	q = bdev_get_queue(dev->bdev);
>>>  	dev->capacity = i_size_read(dev->bdev->bd_inode) >> SECTOR_SHIFT;
>>> -	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);
>>> +	aligned_capacity = dev->capacity &
>>> +				~((u64)blk_queue_zone_sectors(q) - 1);
>>
>> sector_t is an u64 only if CONFIG_LBDAF is defined (I think this option is going
>> away though). Otherwise it is an unsigned long which would be u32 on 32 bits
>> arch. Not a problem in terms of arithmetic, but why not cast to sector_t directly ?
>>
> 
> I would have prefered to do that but I didn't have strong feelings
> either way.  I am always slight annoyed when people don't just copy and
> paste my code when I tell them how to fix a patch so I decided to go
> with your version...  :P

I proposed u64 cast without checking sector_t definition.
After doing that, I liked your sector_t cast more than my own misinformed
proposal :)

-- 
Damien Le Moal
Western Digital Research

[PATCH v3] dm zoned: Silence a static checker warning

[Dan Carpenter]

My static checker complains about this line from dmz_get_zoned_device()

	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);

The problem is that "aligned_capacity" and "dev->capacity" are sector_t
type (which is a u64 under most configs) but blk_queue_zone_sectors(q)
returns a u32 so the higher 32 bits in aligned_capacity are cleared to
zero.  This patch adds a cast to address the issue.

Fixes: 114e025968b5 ("dm zoned: ignore last smaller runt zone")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: The v1 patch declared blk_queue_zone_sectors() as sector_t but the
    v2 cast added a cast to u64.
v3: Cast it to sector_t instead

 drivers/md/dm-zoned-target.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
index 8865c1709e16..51d029bbb740 100644
--- a/drivers/md/dm-zoned-target.c
+++ b/drivers/md/dm-zoned-target.c
@@ -643,7 +643,8 @@ static int dmz_get_zoned_device(struct dm_target *ti, char *path)
 
 	q = bdev_get_queue(dev->bdev);
 	dev->capacity = i_size_read(dev->bdev->bd_inode) >> SECTOR_SHIFT;
-	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);
+	aligned_capacity = dev->capacity &
+				~((sector_t)blk_queue_zone_sectors(q) - 1);
 	if (ti->begin ||
 	    ((ti->len != dev->capacity) && (ti->len != aligned_capacity))) {
 		ti->error = "Partial mapping not supported";
-- 
2.17.1

Re: [PATCH v3] dm zoned: Silence a static checker warning

[Damien Le Moal]

On 2019/04/10 17:13, Dan Carpenter wrote:
> My static checker complains about this line from dmz_get_zoned_device()
> 
> 	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);
> 
> The problem is that "aligned_capacity" and "dev->capacity" are sector_t
> type (which is a u64 under most configs) but blk_queue_zone_sectors(q)
> returns a u32 so the higher 32 bits in aligned_capacity are cleared to
> zero.  This patch adds a cast to address the issue.
> 
> Fixes: 114e025968b5 ("dm zoned: ignore last smaller runt zone")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: The v1 patch declared blk_queue_zone_sectors() as sector_t but the
>     v2 cast added a cast to u64.
> v3: Cast it to sector_t instead
> 
>  drivers/md/dm-zoned-target.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
> index 8865c1709e16..51d029bbb740 100644
> --- a/drivers/md/dm-zoned-target.c
> +++ b/drivers/md/dm-zoned-target.c
> @@ -643,7 +643,8 @@ static int dmz_get_zoned_device(struct dm_target *ti, char *path)
>  
>  	q = bdev_get_queue(dev->bdev);
>  	dev->capacity = i_size_read(dev->bdev->bd_inode) >> SECTOR_SHIFT;
> -	aligned_capacity = dev->capacity & ~(blk_queue_zone_sectors(q) - 1);
> +	aligned_capacity = dev->capacity &
> +				~((sector_t)blk_queue_zone_sectors(q) - 1);
>  	if (ti->begin ||
>  	    ((ti->len != dev->capacity) && (ti->len != aligned_capacity))) {
>  		ti->error = "Partial mapping not supported";
> 

Thanks for fixing this !

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research