[PATCH RFC v1] nfs/write: Use common error handling code in nfs_lock_and_join_requests()

[PATCH RFC v1] nfs/write: Use common error handling code in nfs_lock_and_join_requests()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

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(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index babebbccae2a..5b5f464f6f2a 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -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

Difficulties for compilation without extra optimisation

[SF Markus Elfring]

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/<7f072f78-eef4-6d87-d233-cee71dac5a32@users.sourceforge.net>

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

Re: Difficulties for compilation without extra optimisation

[Trond Myklebust]

On Sun, 2017-12-03 at 15:15 +0100, SF Markus Elfring wrote:
> 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/<7f072f78-eef4-6d87-d233-cee71dac5a32@users
> .sourceforge.net>;
> 
> 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?

I'm not seeing anything obviously wrong with the NFS use of tracepoints
there, and the warning suggests rather that gcc has an issue with the
inlined assembly code in jump_label.h.

Ccing Peter Zijlstra (who appears to have been the last person to touch
that assembly code) and Steven Rostedt.

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com

Re: Difficulties for compilation without extra optimisation

[Steven Rostedt]

On Sun, 3 Dec 2017 15:17:32 +0000
Trond Myklebust <trondmy@primarydata.com> wrote:

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

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

Re: Difficulties for compilation without extra optimisation

[SF Markus Elfring]

> Why would you compile the kernel without optimization?

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


> There's many places in the kernel that WILL NOT BUILD without optimization.

I did not really know this detail so far.

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


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

Thanks for your information.

Can the software areas distinguished where such special handling matters?

Regards,
Markus

Re: Difficulties for compilation without extra optimisation

[Steven Rostedt]

On Sun, 3 Dec 2017 22:56:51 +0100
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> Can the software areas distinguished where such special handling matters?

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

-- Steve

Re: Difficulties for compilation without extra optimisation

[SF Markus Elfring]

> Why would you compile the kernel without optimization?

Can another reason be occasionally still relevant?

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


> There's many places in the kernel that WILL NOT BUILD without optimization.

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


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

* 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

Re: Difficulties for compilation without extra optimisation

[Steven Rostedt]

On Mon, 4 Dec 2017 10:00:54 +0100
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> > Why would you compile the kernel without optimization?  
> 
> Can another reason be occasionally still relevant?

No.

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

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.

> 
> 
> > There's many places in the kernel that WILL NOT BUILD without optimization.  
> 
> Would you like to keep the software situation in this way?

Yes.

> 
> 
> > 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.  
> 
> * Can this goal be also achieved without the addition of “broken code”?

No.

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

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

Re: Difficulties for compilation without extra optimisation

[SF Markus Elfring]

>> Can the software areas distinguished where such special handling matters?
> 
> No idea.

I would like to point another example out.


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

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/<034c0760-b8af-0fe2-e7a0-81199ff31931@users.sourceforge.net>

Regards,
Markus

Re: Difficulties for compilation without extra optimisation

[SF Markus Elfring]

>> Will the compilation be a bit quicker when extra data processing
>> could be omitted?
> 
> Why would you care more about the time it takes to compile the kernel,
> than the time it takes for executing it?

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


> Benchmarks are all about performance of a running kernel,

This is generally reasonable.


> nobody compares benchmarks of the time it takes to compile it.

I guess that the situation can be occasionally different there.


> Sure, we like to make the compile times quicker

Good to know …


> (heck, I wrote "make localmodconfig" for just that purpose),

Thanks.


> but we never favor compiler time over execution time.

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


> In fact, if we can improve the execution performance by sacrificing compile time,
> we are happy to do that.

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


>>> 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.  
>>
>> * Can this goal be also achieved without the addition of “broken code”?
> 
> No.

Will any other contributors take another look?


>> * How do you think about to improve the error handling there?
> 
> It works just fine as is.

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


> Errors that can be detected at build time are 100 times better
> than detecting them at execution time.

I agree to such a general view.

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

Regards,
Markus