nfs/write: Use common error handling code in nfs_lock_and_join_requests()

From: Markus Elfring <[email protected]>
Date: Tue, 7 Nov 2017 08:51:00 +0100

Add a jump target so that a bit of exception handling can be better reused at the end of this function.

This issue was detected by using the Coccinelle software.

---

v1 - Request for comments:
I can offer another bit of information for a software development discussion. 💭

The affected source file can be compiled for the processor architecture “x86_64� by a tool like “GCC 6.4.1+r251631-1.3� from the software distribution “openSUSE Tumbleweed� with the following command example.

my_cc=/usr/bin/gcc-6 \
&& my_module=fs/nfs/write.o \
&& for XYZ in 0 s 3; do echo " _____ $XYZ _____" \
&& my_extra="-O$XYZ" \
&& git checkout next-20171102 \
&& make -j4 CC="${my_cc}" HOSTCC="${my_cc}" EXTRA_CFLAGS="${my_extra}" allmodconfig "${my_module}" \
&& size "${my_module}" \
&& git checkout ':/^nfs/write: Use common error handling code in nfs_lock_and_join_requests' \
&& make -j4 CC="${my_cc}" HOSTCC="${my_cc}" EXTRA_CFLAGS="${my_extra}" allmodconfig "${my_module}" \
&& size "${my_module}"; done

🔮 Do you find the following differences worth for further clarification?

╔���������╤������╗ ║ setting │ text ║
╠���������╪������╣

║   O0    │ ???  ║   Compilation failure?
║   Os    │ -17  ║
║   O3    │ -16  ║

╚���������╧�������

 fs/nfs/write.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

index babebbccae2a..5b5f464f6f2a 100644
@@ -487,10 +487,8 @@ nfs_lock_and_join_requests(struct page *page)
 	}
 
 	ret = nfs_page_group_lock(head);
-	if (ret < 0) {
-		nfs_unlock_and_release_request(head);
-		return ERR_PTR(ret);
-	}
+	if (ret < 0)
+		goto release_request;
 
 	/* lock each request in the page group */
 	total_bytes = head->wb_bytes;

@@ -515,8 +513,7 @@ nfs_lock_and_join_requests(struct page *page)
 			if (ret < 0) {
 				nfs_unroll_locks(inode, head, subreq);
 				nfs_release_request(subreq);
-				nfs_unlock_and_release_request(head);
-				return ERR_PTR(ret);
+				goto release_request;
 			}
 		}
 		/*

@@ -532,8 +529,8 @@ nfs_lock_and_join_requests(struct page *page)
 			nfs_page_group_unlock(head);
 			nfs_unroll_locks(inode, head, subreq);
 			nfs_unlock_and_release_request(subreq);
-			nfs_unlock_and_release_request(head);
-			return ERR_PTR(-EIO);
+			ret = -EIO;
+			goto release_request;
 		}
 	}
 

@@ -576,6 +573,10 @@ nfs_lock_and_join_requests(struct page *page)
 	/* still holds ref on head from nfs_page_find_head_request
 	 * and still has lock on head from lock loop */
 	return head;
+
+release_request:
+	nfs_unlock_and_release_request(head);
+	return ERR_PTR(ret);
 }
 
 static void nfs_write_error_remove_page(struct nfs_page *req)

2.15.0

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

Hello,

I came along some software modules where I suggested source code adjustments.

Example:
nfs/write: Use common error handling code in nfs_lock_and_join_requests()

https://lkml.org/lkml/2017/11/7/599
https://patchwork.kernel.org/patch/10047013/
https://lkml.kernel.org/r/<[email protected]>

I would like to check corresponding build results then without extra optimisation applied by the compiler.
But I got surprised by error messages for a command like the following.

elfring@Sonne:~/Projekte/Linux/next-patched> my_cc=/usr/bin/gcc-7 && LANG=C make -j4 CC="${my_cc}" HOSTCC="${my_cc}" EXTRA_CFLAGS='-O0' allmodconfig fs/nfs/write.o …
In file included from ./include/linux/compiler.h:58:0,

                 from ./include/uapi/linux/stddef.h:1,
                 from ./include/linux/stddef.h:4,
                 from ./include/uapi/linux/posix_types.h:4,
                 from ./include/uapi/linux/types.h:13,
                 from ./include/linux/types.h:5,
                 from fs/nfs/write.c:9:

./arch/x86/include/asm/jump_label.h: In function ‘trace_nfs_writeback_page_enter’:
./include/linux/compiler-gcc.h:275:38: warning: asm operand 0 probably doesn’t match constraints
#define asm_volatile_goto(x...) do { asm goto(x); asm (""); } while (0)
…

How do you think about to improve this software situation anyhow?

Regards,
Markus

T24gU3VuLCAyMDE3LTEyLTAzIGF0IDE1OjE1ICswMTAwLCBTRiBNYXJrdXMgRWxmcmluZyB3cm90 ZToNCj4gSGVsbG8sDQo+IA0KPiBJIGNhbWUgYWxvbmcgc29tZSBzb2Z0d2FyZSBtb2R1bGVzIHdo ZXJlIEkgc3VnZ2VzdGVkIHNvdXJjZSBjb2RlDQo+IGFkanVzdG1lbnRzLg0KPiANCj4gRXhhbXBs ZToNCj4gbmZzL3dyaXRlOiBVc2UgY29tbW9uIGVycm9yIGhhbmRsaW5nIGNvZGUgaW4NCj4gbmZz X2xvY2tfYW5kX2pvaW5fcmVxdWVzdHMoKQ0KPiANCj4gaHR0cHM6Ly9sa21sLm9yZy9sa21sLzIw MTcvMTEvNy81OTkNCj4gaHR0cHM6Ly9wYXRjaHdvcmsua2VybmVsLm9yZy9wYXRjaC8xMDA0NzAx My8NCj4gaHR0cHM6Ly9sa21sLmtlcm5lbC5vcmcvci88N2YwNzJmNzgtZWVmNC02ZDg3LWQyMzMt Y2VlNzFkYWM1YTMyQHVzZXJzDQo+IC5zb3VyY2Vmb3JnZS5uZXQ+Ow0KPiANCj4gSSB3b3VsZCBs aWtlIHRvIGNoZWNrIGNvcnJlc3BvbmRpbmcgYnVpbGQgcmVzdWx0cyB0aGVuIHdpdGhvdXQgZXh0 cmENCj4gb3B0aW1pc2F0aW9uIGFwcGxpZWQgYnkgdGhlIGNvbXBpbGVyLg0KPiBCdXQgSSBnb3Qg c3VycHJpc2VkIGJ5IGVycm9yIG1lc3NhZ2VzIGZvciBhIGNvbW1hbmQgbGlrZSB0aGUNCj4gZm9s bG93aW5nLg0KPiANCj4gZWxmcmluZ0BTb25uZTp+L1Byb2pla3RlL0xpbnV4L25leHQtcGF0Y2hl ZD4gbXlfY2M9L3Vzci9iaW4vZ2NjLTcgJiYNCj4gTEFORz1DIG1ha2UgLWo0IENDPSIke215X2Nj fSIgSE9TVENDPSIke215X2NjfSIgRVhUUkFfQ0ZMQUdTPSctTzAnDQo+IGFsbG1vZGNvbmZpZyBm cy9uZnMvd3JpdGUubw0KPiDigKYNCj4gSW4gZmlsZSBpbmNsdWRlZCBmcm9tIC4vaW5jbHVkZS9s aW51eC9jb21waWxlci5oOjU4OjAsDQo+ICAgICAgICAgICAgICAgICAgZnJvbSAuL2luY2x1ZGUv dWFwaS9saW51eC9zdGRkZWYuaDoxLA0KPiAgICAgICAgICAgICAgICAgIGZyb20gLi9pbmNsdWRl L2xpbnV4L3N0ZGRlZi5oOjQsDQo+ICAgICAgICAgICAgICAgICAgZnJvbSAuL2luY2x1ZGUvdWFw aS9saW51eC9wb3NpeF90eXBlcy5oOjQsDQo+ICAgICAgICAgICAgICAgICAgZnJvbSAuL2luY2x1 ZGUvdWFwaS9saW51eC90eXBlcy5oOjEzLA0KPiAgICAgICAgICAgICAgICAgIGZyb20gLi9pbmNs dWRlL2xpbnV4L3R5cGVzLmg6NSwNCj4gICAgICAgICAgICAgICAgICBmcm9tIGZzL25mcy93cml0 ZS5jOjk6DQo+IC4vYXJjaC94ODYvaW5jbHVkZS9hc20vanVtcF9sYWJlbC5oOiBJbiBmdW5jdGlv bg0KPiDigJh0cmFjZV9uZnNfd3JpdGViYWNrX3BhZ2VfZW50ZXLigJk6DQo+IC4vaW5jbHVkZS9s aW51eC9jb21waWxlci1nY2MuaDoyNzU6Mzg6IHdhcm5pbmc6IGFzbSBvcGVyYW5kIDANCj4gcHJv YmFibHkgZG9lc27igJl0IG1hdGNoIGNvbnN0cmFpbnRzDQo+ICAjZGVmaW5lIGFzbV92b2xhdGls ZV9nb3RvKHguLi4pIGRvIHsgYXNtIGdvdG8oeCk7IGFzbSAoIiIpOyB9IHdoaWxlDQo+ICgwKQ0K PiDigKYNCj4gDQo+IA0KPiBIb3cgZG8geW91IHRoaW5rIGFib3V0IHRvIGltcHJvdmUgdGhpcyBz b2Z0d2FyZSBzaXR1YXRpb24gYW55aG93Pw0KDQpJJ20gbm90IHNlZWluZyBhbnl0aGluZyBvYnZp b3VzbHkgd3Jvbmcgd2l0aCB0aGUgTkZTIHVzZSBvZiB0cmFjZXBvaW50cw0KdGhlcmUsIGFuZCB0 aGUgd2FybmluZyBzdWdnZXN0cyByYXRoZXIgdGhhdCBnY2MgaGFzIGFuIGlzc3VlIHdpdGggdGhl DQppbmxpbmVkIGFzc2VtYmx5IGNvZGUgaW4ganVtcF9sYWJlbC5oLg0KDQpDY2luZyBQZXRlciBa aWpsc3RyYSAod2hvIGFwcGVhcnMgdG8gaGF2ZSBiZWVuIHRoZSBsYXN0IHBlcnNvbiB0byB0b3Vj aA0KdGhhdCBhc3NlbWJseSBjb2RlKSBhbmQgU3RldmVuIFJvc3RlZHQuDQoNCi0tIA0KVHJvbmQg TXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXIsIFByaW1hcnlEYXRhDQp0cm9u ZC5teWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tDQo

Why would you compile the kernel without optimization? There's many places in the kernel that WILL NOT BUILD without optimization. In fact, we do a lot of tricks to make sure that things work the way we expect it to, because we add broken code that only gets compiled out when gcc optimizes the code the way we expect it to be, and the kernel build will break otherwise.

-- Steve

I would like to see how big an effect finally is in such a build configuration after specific source code adjustments.

I did not really know this detail so far.

I noticed that the optimised build variants worked during my test comparisons.

Thanks for your information.

Can the software areas distinguished where such special handling matters?

Regards,
Markus

No idea. That's something you are going to have to figure out on your own.

-- Steve

I would like to point another example out.

How do you think about information from an other clarification request for the topic “caif: Use common error handling code in cfdgml_receive()”?

https://lkml.org/lkml/2017/11/7/486
https://patchwork.kernel.org/patch/10046849/
https://lkml.kernel.org/r/<[email protected]>

Regards,
Markus

Can another reason be occasionally still relevant?

Will the compilation be a bit quicker when extra data processing could be omitted?

Would you like to keep the software situation in this way?

* Can this goal be also achieved without the addition of “broken code”?

* How do you think about to improve the error handling there?

Regards,
Markus

No.

Why would you care more about the time it takes to compile the kernel, than the time it takes for executing it? Benchmarks are all about performance of a running kernel, nobody compares benchmarks of the time it takes to compile it. Sure, we like to make the compile times quicker (heck, I wrote "make localmodconfig" for just that purpose), but we never favor compiler time over execution time. In fact, if we can improve the execution performance by sacrificing compile time, we are happy to do that.

Yes.

No.

It works just fine as is. Errors that can be detected at build time are 100 times better than detecting them at execution time.

-- Steve

I am also interested in the evolution of compilation time frames.

This is generally reasonable.

I guess that the situation can be occasionally different there.

Good to know …

Thanks.

I imagine that the speed expectations could be adjusted during software development, couldn't they?

I guess that you would like to consider some constraints there.

Will any other contributors take another look?

I hope that further software improvements can be achieved also for this use case.

I agree to such a general view.

Will an other (or no) error message be more appropriate?

Regards,
Markus