[PATCH 1/2] ext4: page-io: use 'unsigned int' to bare use of 'unsigned'

[PATCH 1/2] ext4: page-io: use 'unsigned int' to bare use of 'unsigned'

[Liu Peibao]

Fix warnings by checkpatch.

Signed-off-by: Liu Peibao <liupeibao@163.com>
---
 fs/ext4/page-io.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 14695e2b5042..fd55e11c8391 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -106,9 +106,9 @@ static void ext4_finish_bio(struct bio *bio)
 		struct page *page = bvec->bv_page;
 		struct page *bounce_page = NULL;
 		struct buffer_head *bh, *head;
-		unsigned bio_start = bvec->bv_offset;
-		unsigned bio_end = bio_start + bvec->bv_len;
-		unsigned under_io = 0;
+		unsigned int bio_start = bvec->bv_offset;
+		unsigned int bio_end = bio_start + bvec->bv_len;
+		unsigned int under_io = 0;
 		unsigned long flags;
 
 		if (fscrypt_is_bounce_page(page)) {
@@ -329,7 +329,7 @@ static void ext4_end_bio(struct bio *bio)
 	if (WARN_ONCE(!io_end, "io_end is NULL: %pg: sector %Lu len %u err %d\n",
 		      bio->bi_bdev,
 		      (long long) bio->bi_iter.bi_sector,
-		      (unsigned) bio_sectors(bio),
+		      (unsigned int) bio_sectors(bio),
 		      bio->bi_status)) {
 		ext4_finish_bio(bio);
 		bio_put(bio);
@@ -435,7 +435,7 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 {
 	struct page *bounce_page = NULL;
 	struct inode *inode = page->mapping->host;
-	unsigned block_start;
+	unsigned int block_start;
 	struct buffer_head *bh, *head;
 	int ret = 0;
 	int nr_submitted = 0;
-- 
2.20.1

[PATCH 2/2] ext4: rename temporary variables in ext4_finish_bio()

[Liu Peibao]

The temporary variable bio_start and bio_end are confusingly
misnamed. Rename them with bvec_ instead of bio_ to indicate
what they really are.

Signed-off-by: Liu Peibao <liupeibao@163.com>
---
 fs/ext4/page-io.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index fd55e11c8391..dd0a7f734d7f 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -106,8 +106,8 @@ static void ext4_finish_bio(struct bio *bio)
 		struct page *page = bvec->bv_page;
 		struct page *bounce_page = NULL;
 		struct buffer_head *bh, *head;
-		unsigned int bio_start = bvec->bv_offset;
-		unsigned int bio_end = bio_start + bvec->bv_len;
+		unsigned int bvec_start = bvec->bv_offset;
+		unsigned int bvec_end = bvec_start + bvec->bv_len;
 		unsigned int under_io = 0;
 		unsigned long flags;
 
@@ -127,8 +127,8 @@ static void ext4_finish_bio(struct bio *bio)
 		 */
 		spin_lock_irqsave(&head->b_uptodate_lock, flags);
 		do {
-			if (bh_offset(bh) < bio_start ||
-			    bh_offset(bh) + bh->b_size > bio_end) {
+			if (bh_offset(bh) < bvec_start ||
+			    bh_offset(bh) + bh->b_size > bvec_end) {
 				if (buffer_async_write(bh))
 					under_io++;
 				continue;
-- 
2.20.1

Re: [PATCH 1/2] ext4: page-io: use 'unsigned int' to bare use of 'unsigned'

[Theodore Ts'o]

On Wed, May 18, 2022 at 08:01:36PM +0800, Liu Peibao wrote:
> Fix warnings by checkpatch.
> 
> Signed-off-by: Liu Peibao <liupeibao@163.com>

Please don't send checkpatch-only patches.

Thanks,

						- Ted

Re: [PATCH 1/2] ext4: page-io: use 'unsigned int' to bare use of 'unsigned'

[Liu Peibao]

On 2022/6/16 22:49, Theodore Ts'o wrote:
> On Wed, May 18, 2022 at 08:01:36PM +0800, Liu Peibao wrote:
>> Fix warnings by checkpatch.
>>
>> Signed-off-by: Liu Peibao <liupeibao@163.com>
> 
> Please don't send checkpatch-only patches.
> 
> Thanks,
> 
> 						- Ted

Hi Ted,

Thanks for your reply. What I want do to is rename some temporary 
variables in the patch2 and when I make the patch, there are the 
checkpatch warnings. From the point of view "one patch do one thing", I 
split the modification into two patches. Thanks!

Best Regards,
Peibao

Re: [PATCH 1/2] ext4: page-io: use 'unsigned int' to bare use of 'unsigned'

[Theodore Ts'o]

On Sun, Jun 19, 2022 at 11:21:27AM +0800, Liu Peibao wrote:
> 
> Thanks for your reply. What I want do to is rename some temporary variables
> in the patch2 and when I make the patch, there are the checkpatch warnings.
> From the point of view "one patch do one thing", I split the modification
> into two patches. Thanks!

I didn't really see the poiont of renaming the temporary variables,
either.

In this particular case basically only used to avoid line lengths from
exceeding ~72 characters, and requiring a line wrap, and bio_start and
bio_end is used only in one place in the code block below.

Is it _really_ all that confusing whether they are named
bio_{start,end} instead of bvec_{start,end}?

If I was writing that code from scratch, I might have just used start
and end without any prefixes.  And as far as "only have a patch do one
thing at a time", this doesn't apply to checkpatch fixes.

The basic motivation behind "no checkpatch-only fixes" is that it
tends to introduce code churn which makes interpreting information
from "git blame" more difficult; and so therefore the costs exceed the
extremely marginal benefits of fixing most checkpatch complaints.  So
making a _patch_ be checkpatch clean, whether it's modifying existing
code or writing new code, is fine, since you're making a subtantive
change to the code, so this is as good a time as any to fix up tiny
nits such as checkpatch complaints.

But the idea behind "no unnecessary code churn since it ruins git
blame and could potentially induce future patch conflicts" also
applies to renaming variables.  The benefits are very minor, and they
don't outweigh the costs.

						- Ted

Re: [PATCH 1/2] ext4: page-io: use 'unsigned int' to bare use of 'unsigned'

[Liu Peibao]

On 6/20/22 2:18 AM, Theodore Ts'o wrote:
> On Sun, Jun 19, 2022 at 11:21:27AM +0800, Liu Peibao wrote:
>>
>> Thanks for your reply. What I want do to is rename some temporary variables
>> in the patch2 and when I make the patch, there are the checkpatch warnings.
>>  From the point of view "one patch do one thing", I split the modification
>> into two patches. Thanks!
> 
> I didn't really see the poiont of renaming the temporary variables,
> either.
> 
> In this particular case basically only used to avoid line lengths from
> exceeding ~72 characters, and requiring a line wrap, and bio_start and
> bio_end is used only in one place in the code block below.
> 
> Is it _really_ all that confusing whether they are named
> bio_{start,end} instead of bvec_{start,end}?
> 
> If I was writing that code from scratch, I might have just used start
> and end without any prefixes.  And as far as "only have a patch do one
> thing at a time", this doesn't apply to checkpatch fixes.
> 
> The basic motivation behind "no checkpatch-only fixes" is that it
> tends to introduce code churn which makes interpreting information
> from "git blame" more difficult; and so therefore the costs exceed the
> extremely marginal benefits of fixing most checkpatch complaints.  So
> making a _patch_ be checkpatch clean, whether it's modifying existing
> code or writing new code, is fine, since you're making a subtantive
> change to the code, so this is as good a time as any to fix up tiny
> nits such as checkpatch complaints.
> 
> But the idea behind "no unnecessary code churn since it ruins git
> blame and could potentially induce future patch conflicts" also
> applies to renaming variables.  The benefits are very minor, and they
> don't outweigh the costs.
> 
> 						- Ted
> 

Got it! Thanks for your detailed and comprehensive explanation!

Best Regards,
Peibao