[PATCH] xfs: remove the unused BBMASK macro

[PATCH] xfs: remove the unused BBMASK macro

[xiakaixu1987]

From: Kaixu Xia <kaixuxia@tencent.com>

There are no callers of the BBMASK macro, so remove it.

Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
 fs/xfs/libxfs/xfs_fs.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
index 2a2e3cfd94f0..8fd1e20f0d73 100644
--- a/fs/xfs/libxfs/xfs_fs.h
+++ b/fs/xfs/libxfs/xfs_fs.h
@@ -847,7 +847,6 @@ struct xfs_scrub_metadata {
  */
 #define BBSHIFT		9
 #define BBSIZE		(1<<BBSHIFT)
-#define BBMASK		(BBSIZE-1)
 #define BTOBB(bytes)	(((__u64)(bytes) + BBSIZE - 1) >> BBSHIFT)
 #define BTOBBT(bytes)	((__u64)(bytes) >> BBSHIFT)
 #define BBTOB(bbs)	((bbs) << BBSHIFT)
-- 
2.20.0

[PATCH] xfs: use the SECTOR_SHIFT macro instead of the magic number

[xiakaixu1987]

From: Kaixu Xia <kaixuxia@tencent.com>

We use the SECTOR_SHIFT macro to define the sector size shift, so maybe
it is more reasonable to use it than the magic number 9.

Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
 fs/xfs/xfs_bmap_util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index f2a8a0e75e1f..9f02c1824205 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -63,8 +63,8 @@ xfs_zero_extent(
 	sector_t		block = XFS_BB_TO_FSBT(mp, sector);
 
 	return blkdev_issue_zeroout(target->bt_bdev,
-		block << (mp->m_super->s_blocksize_bits - 9),
-		count_fsb << (mp->m_super->s_blocksize_bits - 9),
+		block << (mp->m_super->s_blocksize_bits - SECTOR_SHIFT),
+		count_fsb << (mp->m_super->s_blocksize_bits - SECTOR_SHIFT),
 		GFP_NOFS, 0);
 }
 
-- 
2.20.0

Re: [PATCH] xfs: use the SECTOR_SHIFT macro instead of the magic number

[Eric Sandeen]

On 10/19/20 4:47 AM, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> We use the SECTOR_SHIFT macro to define the sector size shift, so maybe
> it is more reasonable to use it than the magic number 9.
> 
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>

Hm ...  SECTOR_SHIFT is a block layer #define, really,
and blkdev_issue_zeroout is a block layer interface I guess.

We also have our own BBSHIFT in XFS which is used elsewhere, though.

And FWIW, /many/ other fs/* manipulations still use the "- 9" today when
converting s_blocksize_bits to sectors.  *shrug* this seems like something
that should change tree-wide, if it's to be changed at all.

-Eric

> ---
>  fs/xfs/xfs_bmap_util.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index f2a8a0e75e1f..9f02c1824205 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -63,8 +63,8 @@ xfs_zero_extent(
>  	sector_t		block = XFS_BB_TO_FSBT(mp, sector);
>  
>  	return blkdev_issue_zeroout(target->bt_bdev,
> -		block << (mp->m_super->s_blocksize_bits - 9),
> -		count_fsb << (mp->m_super->s_blocksize_bits - 9),
> +		block << (mp->m_super->s_blocksize_bits - SECTOR_SHIFT),
> +		count_fsb << (mp->m_super->s_blocksize_bits - SECTOR_SHIFT),
>  		GFP_NOFS, 0);
>  }
>  
> 

Re: [PATCH] xfs: use the SECTOR_SHIFT macro instead of the magic number

[kaixuxia]

On 2020/10/20 0:32, Eric Sandeen wrote:
> On 10/19/20 4:47 AM, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> We use the SECTOR_SHIFT macro to define the sector size shift, so maybe
>> it is more reasonable to use it than the magic number 9.
>>
>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> 
> Hm ...  SECTOR_SHIFT is a block layer #define, really,
> and blkdev_issue_zeroout is a block layer interface I guess.
> 
> We also have our own BBSHIFT in XFS which is used elsewhere, though.
> 
> And FWIW, /many/ other fs/* manipulations still use the "- 9" today when
> converting s_blocksize_bits to sectors.  *shrug* this seems like something
> that should change tree-wide, if it's to be changed at all.
> 

Yeah, I think the magic number 9 is insecure, maybe a patchset is needed to
change them :)

Thanks,
Kaixu

> -Eric
> 
>> ---
>>  fs/xfs/xfs_bmap_util.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index f2a8a0e75e1f..9f02c1824205 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -63,8 +63,8 @@ xfs_zero_extent(
>>  	sector_t		block = XFS_BB_TO_FSBT(mp, sector);
>>  
>>  	return blkdev_issue_zeroout(target->bt_bdev,
>> -		block << (mp->m_super->s_blocksize_bits - 9),
>> -		count_fsb << (mp->m_super->s_blocksize_bits - 9),
>> +		block << (mp->m_super->s_blocksize_bits - SECTOR_SHIFT),
>> +		count_fsb << (mp->m_super->s_blocksize_bits - SECTOR_SHIFT),
>>  		GFP_NOFS, 0);
>>  }
>>  
>>

-- 
kaixuxia

Re: [PATCH] xfs: remove the unused BBMASK macro

[Eric Sandeen]

On 10/19/20 4:47 AM, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> There are no callers of the BBMASK macro, so remove it.
> 
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> ---
>  fs/xfs/libxfs/xfs_fs.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index 2a2e3cfd94f0..8fd1e20f0d73 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -847,7 +847,6 @@ struct xfs_scrub_metadata {
>   */
>  #define BBSHIFT		9
>  #define BBSIZE		(1<<BBSHIFT)
> -#define BBMASK		(BBSIZE-1)
>  #define BTOBB(bytes)	(((__u64)(bytes) + BBSIZE - 1) >> BBSHIFT)
>  #define BTOBBT(bytes)	((__u64)(bytes) >> BBSHIFT)
>  #define BBTOB(bbs)	((bbs) << BBSHIFT)


This header is shared with userspace, and the macro is used there,
though only once.

This header is also shipped as part of the "install-dev" fileset, and
defines a public API, though I don't think BBMSK is actually used
in any userspace interface.

-Eric

Re: [PATCH] xfs: remove the unused BBMASK macro

[Darrick J. Wong]

On Mon, Oct 19, 2020 at 08:54:28AM -0500, Eric Sandeen wrote:
> On 10/19/20 4:47 AM, xiakaixu1987@gmail.com wrote:
> > From: Kaixu Xia <kaixuxia@tencent.com>
> > 
> > There are no callers of the BBMASK macro, so remove it.
> > 
> > Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> > ---
> >  fs/xfs/libxfs/xfs_fs.h | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > index 2a2e3cfd94f0..8fd1e20f0d73 100644
> > --- a/fs/xfs/libxfs/xfs_fs.h
> > +++ b/fs/xfs/libxfs/xfs_fs.h
> > @@ -847,7 +847,6 @@ struct xfs_scrub_metadata {
> >   */
> >  #define BBSHIFT		9
> >  #define BBSIZE		(1<<BBSHIFT)
> > -#define BBMASK		(BBSIZE-1)
> >  #define BTOBB(bytes)	(((__u64)(bytes) + BBSIZE - 1) >> BBSHIFT)
> >  #define BTOBBT(bytes)	((__u64)(bytes) >> BBSHIFT)
> >  #define BBTOB(bbs)	((bbs) << BBSHIFT)
> 
> 
> This header is shared with userspace, and the macro is used there,
> though only once.
> 
> This header is also shipped as part of the "install-dev" fileset, and
> defines a public API, though I don't think BBMSK is actually used
> in any userspace interface.

$ grep BBMASK /usr/include/
/usr/include/xfs/xfs_fs.h:868:#define BBMASK            (BBSIZE-1)

This ships in a user-visible header file, so it can only be removed by
going through the deprecation process.

--D

> 
> -Eric

Re: [PATCH] xfs: remove the unused BBMASK macro

[Christoph Hellwig]

On Mon, Oct 19, 2020 at 09:08:02AM -0700, Darrick J. Wong wrote:
> $ grep BBMASK /usr/include/
> /usr/include/xfs/xfs_fs.h:868:#define BBMASK            (BBSIZE-1)
> 
> This ships in a user-visible header file, so it can only be removed by
> going through the deprecation process.

I don't think we had such a strong process before.  Not that I'm going
to complain much.

Re: [PATCH] xfs: remove the unused BBMASK macro

[kaixuxia]

On 2020/10/19 21:54, Eric Sandeen wrote:
> On 10/19/20 4:47 AM, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> There are no callers of the BBMASK macro, so remove it.
>>
>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
>> ---
>>  fs/xfs/libxfs/xfs_fs.h | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
>> index 2a2e3cfd94f0..8fd1e20f0d73 100644
>> --- a/fs/xfs/libxfs/xfs_fs.h
>> +++ b/fs/xfs/libxfs/xfs_fs.h
>> @@ -847,7 +847,6 @@ struct xfs_scrub_metadata {
>>   */
>>  #define BBSHIFT		9
>>  #define BBSIZE		(1<<BBSHIFT)
>> -#define BBMASK		(BBSIZE-1)
>>  #define BTOBB(bytes)	(((__u64)(bytes) + BBSIZE - 1) >> BBSHIFT)
>>  #define BTOBBT(bytes)	((__u64)(bytes) >> BBSHIFT)
>>  #define BBTOB(bbs)	((bbs) << BBSHIFT)
> 
> 
> This header is shared with userspace, and the macro is used there,
> though only once.
> 
> This header is also shipped as part of the "install-dev" fileset, and
> defines a public API, though I don't think BBMSK is actually used
> in any userspace interface.

Right...I didn't consider this situation, will drop this patch.

Thanks,
Kaixu
> 
> -Eric
> 

-- 
kaixuxia

Re: [PATCH] xfs: remove the unused BBMASK macro

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>