[PATCH 0/6] md: Fine-tuning for some function implementations

[PATCH 0/6] md: Fine-tuning for some function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 2 May 2017 16:12:34 +0200

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (6):
  Replace seven seq_printf() calls by seq_putc()
  Replace 17 seq_printf() calls by seq_puts()
  Adjust four function calls together with a variable assignment
  Use seq_puts() in faulty_status()
  Adjust six function calls together with a variable assignment in faulty_status()
  Add some spaces for better code readability

 drivers/md/faulty.c | 48 ++++++++++++++++++++++----------------
 drivers/md/md.c     | 66 +++++++++++++++++++++++++++++------------------------
 2 files changed, 64 insertions(+), 50 deletions(-)

-- 
2.12.2

[PATCH 1/6] md: Replace seven seq_printf() calls by seq_putc()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 2 May 2017 14:01:17 +0200

A few single characters should be put into a sequence.
Thus use the corresponding function "seq_putc".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/md.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 82f798be964f..7f0a35ee192a 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -7498,7 +7498,7 @@ static void status_unused(struct seq_file *seq)
 	if (!i)
 		seq_printf(seq, "<none>");
 
-	seq_printf(seq, "\n");
+	seq_putc(seq, '\n');
 }
 
 static int status_resync(struct seq_file *seq, struct mddev *mddev)
@@ -7552,12 +7552,13 @@ static int status_resync(struct seq_file *seq, struct mddev *mddev)
 	per_milli = res;
 	{
 		int i, x = per_milli/50, y = 20-x;
-		seq_printf(seq, "[");
+
+		seq_putc(seq, '[');
 		for (i = 0; i < x; i++)
-			seq_printf(seq, "=");
-		seq_printf(seq, ">");
+			seq_putc(seq, '=');
+		seq_putc(seq, '>');
 		for (i = 0; i < y; i++)
-			seq_printf(seq, ".");
+			seq_putc(seq, '.');
 		seq_printf(seq, "] ");
 	}
 	seq_printf(seq, " %s =%3u.%u%% (%llu/%llu)",
@@ -7678,7 +7679,7 @@ static int md_seq_show(struct seq_file *seq, void *v)
 			seq_printf(seq, "[%s] ", pers->name);
 
 		spin_unlock(&pers_lock);
-		seq_printf(seq, "\n");
+		seq_putc(seq, '\n');
 		seq->poll_event = atomic_read(&md_event_count);
 		return 0;
 	}
@@ -7754,8 +7755,7 @@ static int md_seq_show(struct seq_file *seq, void *v)
 			seq_printf(seq, "\n       ");
 
 		bitmap_status(seq, mddev->bitmap);
-
-		seq_printf(seq, "\n");
+		seq_putc(seq, '\n');
 	}
 	spin_unlock(&mddev->lock);
 
-- 
2.12.2

[PATCH 2/6] md: Replace 17 seq_printf() calls by seq_puts()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 2 May 2017 14:22:45 +0200

Strings which did not contain data format specifications should be put
into a sequence. Thus use the corresponding function "seq_puts".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/md.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 7f0a35ee192a..65233a91e5e2 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -7487,7 +7487,7 @@ static void status_unused(struct seq_file *seq)
 	int i = 0;
 	struct md_rdev *rdev;
 
-	seq_printf(seq, "unused devices: ");
+	seq_puts(seq, "unused devices: ");
 
 	list_for_each_entry(rdev, &pending_raid_disks, same_set) {
 		char b[BDEVNAME_SIZE];
@@ -7496,7 +7496,7 @@ static void status_unused(struct seq_file *seq)
 			      bdevname(rdev->bdev,b));
 	}
 	if (!i)
-		seq_printf(seq, "<none>");
+		seq_puts(seq, "<none>");
 
 	seq_putc(seq, '\n');
 }
@@ -7525,13 +7525,13 @@ static int status_resync(struct seq_file *seq, struct mddev *mddev)
 
 	if (resync == 0) {
 		if (mddev->recovery_cp < MaxSector) {
-			seq_printf(seq, "\tresync=PENDING");
+			seq_puts(seq, "\tresync=PENDING");
 			return 1;
 		}
 		return 0;
 	}
 	if (resync < 3) {
-		seq_printf(seq, "\tresync=DELAYED");
+		seq_puts(seq, "\tresync=DELAYED");
 		return 1;
 	}
 
@@ -7559,7 +7559,7 @@ static int status_resync(struct seq_file *seq, struct mddev *mddev)
 		seq_putc(seq, '>');
 		for (i = 0; i < y; i++)
 			seq_putc(seq, '.');
-		seq_printf(seq, "] ");
+		seq_puts(seq, "] ");
 	}
 	seq_printf(seq, " %s =%3u.%u%% (%llu/%llu)",
 		   (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery)?
@@ -7673,7 +7673,8 @@ static int md_seq_show(struct seq_file *seq, void *v)
 
 	if (v == (void*)1) {
 		struct md_personality *pers;
-		seq_printf(seq, "Personalities : ");
+
+		seq_puts(seq, "Personalities : ");
 		spin_lock(&pers_lock);
 		list_for_each_entry(pers, &pers_list, list)
 			seq_printf(seq, "[%s] ", pers->name);
@@ -7694,9 +7695,9 @@ static int md_seq_show(struct seq_file *seq, void *v)
 						mddev->pers ? "" : "in");
 		if (mddev->pers) {
 			if (mddev->ro==1)
-				seq_printf(seq, " (read-only)");
+				seq_puts(seq, " (read-only)");
 			if (mddev->ro==2)
-				seq_printf(seq, " (auto-read-only)");
+				seq_puts(seq, " (auto-read-only)");
 			seq_printf(seq, " %s", mddev->pers->name);
 		}
 
@@ -7707,17 +7708,17 @@ static int md_seq_show(struct seq_file *seq, void *v)
 			seq_printf(seq, " %s[%d]",
 				bdevname(rdev->bdev,b), rdev->desc_nr);
 			if (test_bit(WriteMostly, &rdev->flags))
-				seq_printf(seq, "(W)");
+				seq_puts(seq, "(W)");
 			if (test_bit(Journal, &rdev->flags))
-				seq_printf(seq, "(J)");
+				seq_puts(seq, "(J)");
 			if (test_bit(Faulty, &rdev->flags)) {
-				seq_printf(seq, "(F)");
+				seq_puts(seq, "(F)");
 				continue;
 			}
 			if (rdev->raid_disk < 0)
-				seq_printf(seq, "(S)"); /* spare */
+				seq_puts(seq, "(S)"); /* spare */
 			if (test_bit(Replacement, &rdev->flags))
-				seq_printf(seq, "(R)");
+				seq_puts(seq, "(R)");
 			sectors += rdev->sectors;
 		}
 		rcu_read_unlock();
@@ -7742,17 +7743,18 @@ static int md_seq_show(struct seq_file *seq, void *v)
 			seq_printf(seq, " super external:%s",
 				   mddev->metadata_type);
 		else
-			seq_printf(seq, " super non-persistent");
+			seq_puts(seq, " super non-persistent");
 
 		if (mddev->pers) {
 			mddev->pers->status(seq, mddev);
-			seq_printf(seq, "\n      ");
+			seq_puts(seq, "\n      ");
 			if (mddev->pers->sync_request) {
 				if (status_resync(seq, mddev))
-					seq_printf(seq, "\n      ");
+					seq_puts(seq, "\n      ");
 			}
-		} else
-			seq_printf(seq, "\n       ");
+		} else {
+			seq_puts(seq, "\n       ");
+		}
 
 		bitmap_status(seq, mddev->bitmap);
 		seq_putc(seq, '\n');
-- 
2.12.2

[PATCH 3/6] md: Adjust four function calls together with a variable assignment

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 2 May 2017 15:07:21 +0200

The script "checkpatch.pl" pointed information out like the following.

ERROR: do not use assignment in if condition

Thus fix affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/md.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 65233a91e5e2..60580095a5de 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2153,7 +2153,8 @@ static int bind_rdev_to_array(struct md_rdev *rdev, struct mddev *mddev)
 	rdev->mddev = mddev;
 	pr_debug("md: bind<%s>\n", b);
 
-	if ((err = kobject_add(&rdev->kobj, &mddev->kobj, "dev-%s", b)))
+	err = kobject_add(&rdev->kobj, &mddev->kobj, "dev-%s", b);
+	if (err)
 		goto fail;
 
 	ko = &part_to_dev(rdev->bdev->bd_part)->kobj;
@@ -7306,7 +7307,8 @@ static int md_open(struct block_device *bdev, fmode_t mode)
 	}
 	BUG_ON(mddev != bdev->bd_disk->private_data);
 
-	if ((err = mutex_lock_interruptible(&mddev->open_mutex)))
+	err = mutex_lock_interruptible(&mddev->open_mutex);
+	if (err)
 		goto out;
 
 	if (test_bit(MD_CLOSING, &mddev->flags)) {
@@ -8927,10 +8929,12 @@ static int __init md_init(void)
 	if (!md_misc_wq)
 		goto err_misc_wq;
 
-	if ((ret = register_blkdev(MD_MAJOR, "md")) < 0)
+	ret = register_blkdev(MD_MAJOR, "md");
+	if (ret < 0)
 		goto err_md;
 
-	if ((ret = register_blkdev(0, "mdp")) < 0)
+	ret = register_blkdev(0, "mdp");
+	if (ret < 0)
 		goto err_mdp;
 	mdp_major = ret;
 
-- 
2.12.2

[PATCH 4/6] md: Use seq_puts() in faulty_status()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 2 May 2017 15:20:30 +0200

A string which did not contain a data format specification should be put
into a sequence. Thus use the corresponding function "seq_puts".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/faulty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/faulty.c b/drivers/md/faulty.c
index b0536cfd8e17..f5536c91be5c 100644
--- a/drivers/md/faulty.c
+++ b/drivers/md/faulty.c
@@ -253,7 +253,7 @@ static void faulty_status(struct seq_file *seq, struct mddev *mddev)
 			   n, conf->period[ReadFixable]);
 
 	if ((n=atomic_read(&conf->counters[WriteAll])) != 0)
-		seq_printf(seq, " WriteAll");
+		seq_puts(seq, " WriteAll");
 
 	seq_printf(seq, " nfaults=%d", conf->nfaults);
 }
-- 
2.12.2

[PATCH 5/6] md: Adjust six function calls together with a variable assignment in faulty_status()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 2 May 2017 15:35:35 +0200

The script "checkpatch.pl" pointed information out like the following.

ERROR: do not use assignment in if condition

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/faulty.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/md/faulty.c b/drivers/md/faulty.c
index f5536c91be5c..2573009b1265 100644
--- a/drivers/md/faulty.c
+++ b/drivers/md/faulty.c
@@ -231,28 +231,33 @@ static void faulty_status(struct seq_file *seq, struct mddev *mddev)
 	struct faulty_conf *conf = mddev->private;
 	int n;
 
-	if ((n=atomic_read(&conf->counters[WriteTransient])) != 0)
+	n = atomic_read(&conf->counters[WriteTransient]);
+	if (n != 0)
 		seq_printf(seq, " WriteTransient=%d(%d)",
 			   n, conf->period[WriteTransient]);
 
-	if ((n=atomic_read(&conf->counters[ReadTransient])) != 0)
+	n = atomic_read(&conf->counters[ReadTransient]);
+	if (n != 0)
 		seq_printf(seq, " ReadTransient=%d(%d)",
 			   n, conf->period[ReadTransient]);
 
-	if ((n=atomic_read(&conf->counters[WritePersistent])) != 0)
+	n = atomic_read(&conf->counters[WritePersistent]);
+	if (n != 0)
 		seq_printf(seq, " WritePersistent=%d(%d)",
 			   n, conf->period[WritePersistent]);
 
-	if ((n=atomic_read(&conf->counters[ReadPersistent])) != 0)
+	n = atomic_read(&conf->counters[ReadPersistent]);
+	if (n != 0)
 		seq_printf(seq, " ReadPersistent=%d(%d)",
 			   n, conf->period[ReadPersistent]);
 
-
-	if ((n=atomic_read(&conf->counters[ReadFixable])) != 0)
+	n = atomic_read(&conf->counters[ReadFixable]);
+	if (n != 0)
 		seq_printf(seq, " ReadFixable=%d(%d)",
 			   n, conf->period[ReadFixable]);
 
-	if ((n=atomic_read(&conf->counters[WriteAll])) != 0)
+	n = atomic_read(&conf->counters[WriteAll]);
+	if (n != 0)
 		seq_puts(seq, " WriteAll");
 
 	seq_printf(seq, " nfaults=%d", conf->nfaults);
-- 
2.12.2

[PATCH 6/6] md: Add some spaces for better code readability

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 2 May 2017 16:00:45 +0200

Use space characters and blank lines at some source code places
according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/faulty.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/md/faulty.c b/drivers/md/faulty.c
index 2573009b1265..434c3643c9c3 100644
--- a/drivers/md/faulty.c
+++ b/drivers/md/faulty.c
@@ -110,19 +110,20 @@ static int check_sector(struct faulty_conf *conf, sector_t start, sector_t end,
 {
 	/* If we find a ReadFixable sector, we fix it ... */
 	int i;
-	for (i=0; i<conf->nfaults; i++)
+
+	for (i = 0; i < conf->nfaults; i++)
 		if (conf->faults[i] >= start &&
 		    conf->faults[i] < end) {
 			/* found it ... */
 			switch (conf->modes[i] * 2 + dir) {
-			case WritePersistent*2+WRITE: return 1;
-			case ReadPersistent*2+READ: return 1;
-			case ReadFixable*2+READ: return 1;
-			case ReadFixable*2+WRITE:
+			case WritePersistent * 2 + WRITE: return 1;
+			case ReadPersistent * 2 + READ: return 1;
+			case ReadFixable * 2 + READ: return 1;
+			case ReadFixable * 2 + WRITE:
 				conf->modes[i] = NoPersist;
 				return 0;
-			case AllPersist*2+READ:
-			case AllPersist*2+WRITE: return 1;
+			case AllPersist * 2 + READ:
+			case AllPersist * 2 + WRITE: return 1;
 			default:
 				return 0;
 			}
@@ -134,9 +135,10 @@ static void add_sector(struct faulty_conf *conf, sector_t start, int mode)
 {
 	int i;
 	int n = conf->nfaults;
-	for (i=0; i<conf->nfaults; i++)
+
+	for (i = 0; i < conf->nfaults; i++)
 		if (conf->faults[i] == start) {
-			switch(mode) {
+			switch (mode) {
 			case NoPersist: conf->modes[i] = mode; return;
 			case WritePersistent:
 				if (conf->modes[i] == ReadPersistent ||
@@ -167,7 +169,7 @@ static void add_sector(struct faulty_conf *conf, sector_t start, int mode)
 	conf->faults[n] = start;
 	conf->modes[n] = mode;
 	if (conf->nfaults == n)
-		conf->nfaults = n+1;
+		conf->nfaults = n + 1;
 }
 
 static void faulty_make_request(struct mddev *mddev, struct bio *bio)
@@ -278,7 +280,8 @@ static int faulty_reshape(struct mddev *mddev)
 		conf->nfaults = 0;
 	else if (mode == ClearErrors) {
 		int i;
-		for (i=0 ; i < Modes ; i++) {
+
+		for (i = 0; i < Modes; i++) {
 			conf->period[i] = 0;
 			atomic_set(&conf->counters[i], 0);
 		}
@@ -317,7 +320,7 @@ static int faulty_run(struct mddev *mddev)
 	if (!conf)
 		return -ENOMEM;
 
-	for (i=0; i<Modes; i++) {
+	for (i = 0; i < Modes; i++) {
 		atomic_set(&conf->counters[i], 0);
 		conf->period[i] = 0;
 	}
-- 
2.12.2

Re: [PATCH 0/6] md: Fine-tuning for some function implementations

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

On Tue, May 02 2017, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 2 May 2017 16:12:34 +0200
>
> Some update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (6):
>   Replace seven seq_printf() calls by seq_putc()
>   Replace 17 seq_printf() calls by seq_puts()

Why does anyone care whether printf or putc/puts is used?  Really it
doesn't matter *at* *all*.
I don't object to the patch but if it would up to me I probably wouldn't
bother applying it it either.
Sometimes I just want to "print" something and I don't want to care
whether it is a constant string or a single-byte constant string, or
something more general.
I see these changes as worse than white-space fixes.

NeilBrown


>   Adjust four function calls together with a variable assignment
>   Use seq_puts() in faulty_status()
>   Adjust six function calls together with a variable assignment in faulty_status()
>   Add some spaces for better code readability
>
>  drivers/md/faulty.c | 48 ++++++++++++++++++++++----------------
>  drivers/md/md.c     | 66 +++++++++++++++++++++++++++++------------------------
>  2 files changed, 64 insertions(+), 50 deletions(-)
>
> -- 
> 2.12.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 0/6] md: Fine-tuning for some function implementations

[Nikola Pajkovsky]

NeilBrown <neilb@suse.com> writes:

> On Tue, May 02 2017, SF Markus Elfring wrote:
>
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Tue, 2 May 2017 16:12:34 +0200
>>
>> Some update suggestions were taken into account
>> from static source code analysis.
>>
>> Markus Elfring (6):
>>   Replace seven seq_printf() calls by seq_putc()
>>   Replace 17 seq_printf() calls by seq_puts()
>
> Why does anyone care whether printf or putc/puts is used?  Really it
> doesn't matter *at* *all*.

It could matter, but this patchset does not state it all. Does not
have any perf tests and so on.

f7a5f132b447 ("proc: faster /proc/*/status")
68ba0326b4e1 ("proc: much faster /proc/vmstat")

> I don't object to the patch but if it would up to me I probably wouldn't
> bother applying it it either.
> Sometimes I just want to "print" something and I don't want to care
> whether it is a constant string or a single-byte constant string, or
> something more general.
> I see these changes as worse than white-space fixes.
>
> NeilBrown
>
>
>>   Adjust four function calls together with a variable assignment
>>   Use seq_puts() in faulty_status()
>>   Adjust six function calls together with a variable assignment in faulty_status()
>>   Add some spaces for better code readability
>>
>>  drivers/md/faulty.c | 48 ++++++++++++++++++++++----------------
>>  drivers/md/md.c     | 66 +++++++++++++++++++++++++++++------------------------
>>  2 files changed, 64 insertions(+), 50 deletions(-)
>>
>> -- 
>> 2.12.2


-- 
Nikola