btrfs: extent-tree: Use round up to replace align macro
diff mbox

Message ID 20171121055445.13972-1-wqu@suse.com
State New
Headers show

Commit Message

Qu WenRuo Nov. 21, 2017, 5:54 a.m. UTC
To save reader seconds before checking if it's rounding up or down.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent-tree.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Nikolay Borisov Nov. 21, 2017, 7:33 a.m. UTC | #1
On 21.11.2017 07:54, Qu Wenruo wrote:
> To save reader seconds before checking if it's rounding up or down.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/extent-tree.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 309a109069f1..8ab18e25cb91 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2054,7 +2054,7 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
>  {
>  	int j, ret = 0;
>  	u64 bytes_left, end;
> -	u64 aligned_start = ALIGN(start, 1 << 9);
> +	u64 aligned_start = round_up(start, 1 << 9);

nit: 1 << 9 == SZ_512 or essentially sector size however looking at the
defines there is SECTOR_SIZE defined in ide.h which we do not include.
Or just use 512 no need to obscure it behind a shift.

>  
>  	if (WARN_ON(start != aligned_start)) {
>  		len -= aligned_start - start;
> @@ -4266,7 +4266,7 @@ int btrfs_alloc_data_chunk_ondemand(struct btrfs_inode *inode, u64 bytes)
>  	int have_pinned_space;
>  
>  	/* make sure bytes are sectorsize aligned */
> -	bytes = ALIGN(bytes, fs_info->sectorsize);
> +	bytes = round_up(bytes, fs_info->sectorsize);
>  
>  	if (btrfs_is_free_space_inode(inode)) {
>  		need_commit = 0;
> @@ -6080,7 +6080,7 @@ int btrfs_delalloc_reserve_metadata(struct btrfs_inode *inode, u64 num_bytes)
>  	if (delalloc_lock)
>  		mutex_lock(&inode->delalloc_mutex);
>  
> -	num_bytes = ALIGN(num_bytes, fs_info->sectorsize);
> +	num_bytes = round_up(num_bytes, fs_info->sectorsize);
>  
>  	/* Add our new extents and calculate the new rsv size. */
>  	spin_lock(&inode->lock);
> @@ -6135,7 +6135,7 @@ void btrfs_delalloc_release_metadata(struct btrfs_inode *inode, u64 num_bytes)
>  {
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->vfs_inode.i_sb);
>  
> -	num_bytes = ALIGN(num_bytes, fs_info->sectorsize);
> +	num_bytes = round_up(num_bytes, fs_info->sectorsize);
>  	spin_lock(&inode->lock);
>  	inode->csum_bytes -= num_bytes;
>  	btrfs_calculate_inode_block_rsv_size(fs_info, inode);
> @@ -7818,7 +7818,7 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info,
>  			goto loop;
>  		}
>  checks:
> -		search_start = ALIGN(offset, fs_info->stripesize);
> +		search_start = round_up(offset, fs_info->stripesize);
>  
>  		/* move on to the next group */
>  		if (search_start + num_bytes >
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Qu WenRuo Nov. 21, 2017, 8:06 a.m. UTC | #2
On 2017年11月21日 15:33, Nikolay Borisov wrote:
> 
> 
> On 21.11.2017 07:54, Qu Wenruo wrote:
>> To save reader seconds before checking if it's rounding up or down.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/extent-tree.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 309a109069f1..8ab18e25cb91 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -2054,7 +2054,7 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
>>  {
>>  	int j, ret = 0;
>>  	u64 bytes_left, end;
>> -	u64 aligned_start = ALIGN(start, 1 << 9);
>> +	u64 aligned_start = round_up(start, 1 << 9);
> 
> nit: 1 << 9 == SZ_512 or essentially sector size however looking at the
> defines there is SECTOR_SIZE defined in ide.h which we do not include.
> Or just use 512 no need to obscure it behind a shift.

I also noticed that while I don't have any good idea which macro should
be used.

That SECTOR_SIZE defined in ide.h doesn't sound good to me.
And all other filesystems are using their own definition, without some
definition in fs/bio level.

Thanks,
Qu

> 
>>  
>>  	if (WARN_ON(start != aligned_start)) {
>>  		len -= aligned_start - start;
>> @@ -4266,7 +4266,7 @@ int btrfs_alloc_data_chunk_ondemand(struct btrfs_inode *inode, u64 bytes)
>>  	int have_pinned_space;
>>  
>>  	/* make sure bytes are sectorsize aligned */
>> -	bytes = ALIGN(bytes, fs_info->sectorsize);
>> +	bytes = round_up(bytes, fs_info->sectorsize);
>>  
>>  	if (btrfs_is_free_space_inode(inode)) {
>>  		need_commit = 0;
>> @@ -6080,7 +6080,7 @@ int btrfs_delalloc_reserve_metadata(struct btrfs_inode *inode, u64 num_bytes)
>>  	if (delalloc_lock)
>>  		mutex_lock(&inode->delalloc_mutex);
>>  
>> -	num_bytes = ALIGN(num_bytes, fs_info->sectorsize);
>> +	num_bytes = round_up(num_bytes, fs_info->sectorsize);
>>  
>>  	/* Add our new extents and calculate the new rsv size. */
>>  	spin_lock(&inode->lock);
>> @@ -6135,7 +6135,7 @@ void btrfs_delalloc_release_metadata(struct btrfs_inode *inode, u64 num_bytes)
>>  {
>>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->vfs_inode.i_sb);
>>  
>> -	num_bytes = ALIGN(num_bytes, fs_info->sectorsize);
>> +	num_bytes = round_up(num_bytes, fs_info->sectorsize);
>>  	spin_lock(&inode->lock);
>>  	inode->csum_bytes -= num_bytes;
>>  	btrfs_calculate_inode_block_rsv_size(fs_info, inode);
>> @@ -7818,7 +7818,7 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info,
>>  			goto loop;
>>  		}
>>  checks:
>> -		search_start = ALIGN(offset, fs_info->stripesize);
>> +		search_start = round_up(offset, fs_info->stripesize);
>>  
>>  		/* move on to the next group */
>>  		if (search_start + num_bytes >
>>
David Sterba Nov. 21, 2017, 1:36 p.m. UTC | #3
On Tue, Nov 21, 2017 at 01:54:45PM +0800, Qu Wenruo wrote:
> To save reader seconds before checking if it's rounding up or down.

Agreed, this is more clear. Please switch all instances of ALIGN to
round_up, there's not much point doing it file by file.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba Nov. 21, 2017, 1:47 p.m. UTC | #4
On Tue, Nov 21, 2017 at 04:06:54PM +0800, Qu Wenruo wrote:
> On 2017年11月21日 15:33, Nikolay Borisov wrote:
> > On 21.11.2017 07:54, Qu Wenruo wrote:
> >> @@ -2054,7 +2054,7 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
> >>  {
> >>  	int j, ret = 0;
> >>  	u64 bytes_left, end;
> >> -	u64 aligned_start = ALIGN(start, 1 << 9);
> >> +	u64 aligned_start = round_up(start, 1 << 9);
> > 
> > nit: 1 << 9 == SZ_512 or essentially sector size however looking at the
> > defines there is SECTOR_SIZE defined in ide.h which we do not include.
> > Or just use 512 no need to obscure it behind a shift.
> 
> I also noticed that while I don't have any good idea which macro should
> be used.
> 
> That SECTOR_SIZE defined in ide.h doesn't sound good to me.

Heh, no we won't include ide.h just to get the macro definition.

> And all other filesystems are using their own definition, without some
> definition in fs/bio level.

That would be useful, as the sectorsize == 512 is part of the bio ABI.

I'd like to convert all the >> 9 shifts to *512 or /512, but this relies
on the compiler to convert namely the division to shifts where we use
u64.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Qu Wenruo Nov. 21, 2017, 2:36 p.m. UTC | #5
On 2017年11月21日 21:36, David Sterba wrote:
> On Tue, Nov 21, 2017 at 01:54:45PM +0800, Qu Wenruo wrote:
>> To save reader seconds before checking if it's rounding up or down.
> 
> Agreed, this is more clear. Please switch all instances of ALIGN to
> round_up, there's not much point doing it file by file.

OK.

I just encountered this when digging space reservation related code.

Converting all won't be a problem.

Thanks,
Qu

> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Patch
diff mbox

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 309a109069f1..8ab18e25cb91 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2054,7 +2054,7 @@  static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
 {
 	int j, ret = 0;
 	u64 bytes_left, end;
-	u64 aligned_start = ALIGN(start, 1 << 9);
+	u64 aligned_start = round_up(start, 1 << 9);
 
 	if (WARN_ON(start != aligned_start)) {
 		len -= aligned_start - start;
@@ -4266,7 +4266,7 @@  int btrfs_alloc_data_chunk_ondemand(struct btrfs_inode *inode, u64 bytes)
 	int have_pinned_space;
 
 	/* make sure bytes are sectorsize aligned */
-	bytes = ALIGN(bytes, fs_info->sectorsize);
+	bytes = round_up(bytes, fs_info->sectorsize);
 
 	if (btrfs_is_free_space_inode(inode)) {
 		need_commit = 0;
@@ -6080,7 +6080,7 @@  int btrfs_delalloc_reserve_metadata(struct btrfs_inode *inode, u64 num_bytes)
 	if (delalloc_lock)
 		mutex_lock(&inode->delalloc_mutex);
 
-	num_bytes = ALIGN(num_bytes, fs_info->sectorsize);
+	num_bytes = round_up(num_bytes, fs_info->sectorsize);
 
 	/* Add our new extents and calculate the new rsv size. */
 	spin_lock(&inode->lock);
@@ -6135,7 +6135,7 @@  void btrfs_delalloc_release_metadata(struct btrfs_inode *inode, u64 num_bytes)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->vfs_inode.i_sb);
 
-	num_bytes = ALIGN(num_bytes, fs_info->sectorsize);
+	num_bytes = round_up(num_bytes, fs_info->sectorsize);
 	spin_lock(&inode->lock);
 	inode->csum_bytes -= num_bytes;
 	btrfs_calculate_inode_block_rsv_size(fs_info, inode);
@@ -7818,7 +7818,7 @@  static noinline int find_free_extent(struct btrfs_fs_info *fs_info,
 			goto loop;
 		}
 checks:
-		search_start = ALIGN(offset, fs_info->stripesize);
+		search_start = round_up(offset, fs_info->stripesize);
 
 		/* move on to the next group */
 		if (search_start + num_bytes >