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

PATCH In review Qu Wenruo 4 participants 6 messages Nov 21, 2017 Btrfs file system

lore

Share & tools

lore

Qu Wenruo

author

2017-11-21 05:55 UTC #0 lore raw

PATCH 1/1 btrfs: extent-tree: Use round up to replace align macro

To save reader seconds before checking if it's rounding up or down.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/extent-tree.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

fs/btrfs/extent-tree.c +5 -5

index 309a109069f1..8ab18e25cb91 100644
@@ -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 >

Signature · 1 line

2.15.0

Nikolay Borisov

2017-11-21 07:33 UTC #1 lore raw

Quoted 12 lines · Qu Wenruo

To save reader seconds before checking if it's rounding up or down.
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/extent-tree.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
fs/btrfs/extent-tree.c +0 -0
index 309a109069f1..8ab18e25cb91 100644

fs/btrfs/extent-tree.c static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len, · line 2057

Quoted 5 lines

fs/btrfs/extent-tree.c +1 -1

@@ -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.

Quoted 40 lines

fs/btrfs/extent-tree.c +4 -4

 	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 >

Qu Wenruo

author

2017-11-21 08:07 UTC #2 lore raw

Quoted 10 lines · Nikolay Borisov

Quoted 7 lines · Qu Wenruo
To save reader seconds before checking if it's rounding up or down.
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/extent-tree.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Quoted 14 lines

fs/btrfs/extent-tree.c static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len, · line 2057
Quoted 10 lines
fs/btrfs/extent-tree.c +1 -1
index 309a109069f1..8ab18e25cb91 100644
@@ -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

Quoted 41 lines

fs/btrfs/extent-tree.c +4 -4

 	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

2017-11-21 13:49 UTC #3 lore raw

Quoted 13 lines · Qu Wenruo

Quoted 11 lines · Nikolay Borisov
fs/btrfs/extent-tree.c static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len, · line 2057
Quoted 6 lines · Qu Wenruo
fs/btrfs/extent-tree.c +1 -1
@@ -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.

Quoted 4 lines

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.

Quoted 2 lines

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.

David Sterba

2017-11-21 13:38 UTC #4 lore raw

In reply to Qu Wenruo

Quoted 1 line · Qu Wenruo

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.

Qu Wenruo

2017-11-21 14:36 UTC #5 lore raw

Quoted 5 lines · David Sterba

Quoted 1 line · Qu Wenruo
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

Quoted 5 lines

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