* [PATCH 1/2] staging: Reduce verbosity of log messages @ 2017-09-22 16:41 Richard Purdie 2017-09-22 16:41 ` [PATCH 2/2] pseudo: Add fastop reply fix Richard Purdie 2017-09-22 17:00 ` ✗ patchtest: failure for "staging: Reduce verbosity of l..." and 1 more Patchwork 0 siblings, 2 replies; 6+ messages in thread From: Richard Purdie @ 2017-09-22 16:41 UTC (permalink / raw) To: openembedded-core The staging changes were very verbose in their logging and whilst this is useful when staging issues occur, those thankfully seem rare now and we can tune down the logging to a sane level. This improves the readability of error messages from functions that fail. The code is still verbose when its replacing things in the sysroot. Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> --- meta/classes/staging.bbclass | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/meta/classes/staging.bbclass b/meta/classes/staging.bbclass index 6185aef..ea831e0 100644 --- a/meta/classes/staging.bbclass +++ b/meta/classes/staging.bbclass @@ -373,7 +373,8 @@ python extend_recipe_sysroot() { msgbuf.append("Following dependency on %s" % setscenedeps[datadep][0]) next = new - bb.note("\n".join(msgbuf)) + # This logging is too verbose for day to day use sadly + #bb.debug(2, "\n".join(msgbuf)) depdir = recipesysrootnative + "/installeddeps" bb.utils.mkdirhier(depdir) @@ -442,6 +443,8 @@ python extend_recipe_sysroot() { os.unlink(fl) os.unlink(fl + ".complete") + msg_exists = [] + msg_adding = [] for dep in configuredeps: c = setscenedeps[dep][0] if c not in installed: @@ -452,7 +455,7 @@ python extend_recipe_sysroot() { if os.path.exists(depdir + "/" + c): lnk = os.readlink(depdir + "/" + c) if lnk == c + "." + taskhash and os.path.exists(depdir + "/" + c + ".complete"): - bb.note("%s exists in sysroot, skipping" % c) + msg_exists.append(c) continue else: bb.note("%s exists in sysroot, but is stale (%s vs. %s), removing." % (c, lnk, c + "." + taskhash)) @@ -463,6 +466,8 @@ python extend_recipe_sysroot() { elif os.path.lexists(depdir + "/" + c): os.unlink(depdir + "/" + c) + msg_adding.append(c) + os.symlink(c + "." + taskhash, depdir + "/" + c) d2 = d @@ -559,6 +564,9 @@ python extend_recipe_sysroot() { continue staging_copyfile(l, targetdir, dest, postinsts, seendirs) + bb.note("Installed into sysroot: %s" % str(msg_adding)) + bb.note("Skipping as already exists in sysroot: %s" % str(msg_exists)) + for f in fixme: if f == '': staging_processfixme(fixme[f], recipesysroot, recipesysroot, recipesysrootnative, d) -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] pseudo: Add fastop reply fix 2017-09-22 16:41 [PATCH 1/2] staging: Reduce verbosity of log messages Richard Purdie @ 2017-09-22 16:41 ` Richard Purdie 2017-09-22 16:52 ` Seebs 2017-09-22 17:00 ` ✗ patchtest: failure for "staging: Reduce verbosity of l..." and 1 more Patchwork 1 sibling, 1 reply; 6+ messages in thread From: Richard Purdie @ 2017-09-22 16:41 UTC (permalink / raw) To: openembedded-core This changes the pseudo FASTOP functionality so that a reply to the operation is required. This means we then cannot lose data if a connection is closed. This in turn stops corruption if we run out of file handles and have to close connections. This tweaks the connection closure patch to update the comment there which is now outdated. Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> --- .../pseudo/files/fastopreply.patch | 74 ++++++++++++++++++++++ .../pseudo/files/toomanyfiles.patch | 7 +- meta/recipes-devtools/pseudo/pseudo_1.8.2.bb | 1 + 3 files changed, 77 insertions(+), 5 deletions(-) create mode 100644 meta/recipes-devtools/pseudo/files/fastopreply.patch diff --git a/meta/recipes-devtools/pseudo/files/fastopreply.patch b/meta/recipes-devtools/pseudo/files/fastopreply.patch new file mode 100644 index 0000000..c8e7b08 --- /dev/null +++ b/meta/recipes-devtools/pseudo/files/fastopreply.patch @@ -0,0 +1,74 @@ +Ensure FASTOP messages get an ACK reply so that the client can be sure the server +recieved them. This means if connections are terminated, data isn't lost. + +RP 2017/9/22 + +Index: pseudo-1.8.2/pseudo_client.c +=================================================================== +--- pseudo-1.8.2.orig/pseudo_client.c ++++ pseudo-1.8.2/pseudo_client.c +@@ -1331,21 +1331,19 @@ pseudo_client_request(pseudo_msg_t *msg, + * indicating a successful send. + */ + pseudo_debug(PDBGF_CLIENT | PDBGF_VERBOSE, "sent!\n"); +- if (msg->type != PSEUDO_MSG_FASTOP) { +- response = pseudo_msg_receive(connect_fd); +- if (!response) { +- pseudo_debug(PDBGF_CLIENT, "expected response did not occur; retrying\n"); ++ response = pseudo_msg_receive(connect_fd); ++ if (!response) { ++ pseudo_debug(PDBGF_CLIENT, "expected response did not occur; retrying\n"); ++ } else { ++ if (response->type != PSEUDO_MSG_ACK) { ++ pseudo_debug(PDBGF_CLIENT, "got non-ack response %d\n", response->type); ++ return 0; ++ } else if (msg->type != PSEUDO_MSG_FASTOP) { ++ pseudo_debug(PDBGF_CLIENT | PDBGF_VERBOSE, "got response type %d\n", response->type); ++ return response; + } else { +- if (response->type != PSEUDO_MSG_ACK) { +- pseudo_debug(PDBGF_CLIENT, "got non-ack response %d\n", response->type); +- return 0; +- } else { +- pseudo_debug(PDBGF_CLIENT | PDBGF_VERBOSE, "got response type %d\n", response->type); +- return response; +- } ++ return 0; + } +- } else { +- return 0; + } + } + pseudo_diag("pseudo: server connection persistently failed, aborting.\n"); +Index: pseudo-1.8.2/pseudo_server.c +=================================================================== +--- pseudo-1.8.2.orig/pseudo_server.c ++++ pseudo-1.8.2/pseudo_server.c +@@ -463,6 +463,11 @@ close_client(int client) { + --highest_client; + } + ++static pseudo_msg_t server_fastop_reply = { ++ .type = PSEUDO_MSG_ACK, ++ .op = OP_NONE, ++}; ++ + /* Actually process a request. + */ + static int +@@ -515,8 +520,14 @@ serve_client(int i) { + * pseudo_server_response. + */ + if (in->type != PSEUDO_MSG_SHUTDOWN) { +- if (in->type == PSEUDO_MSG_FASTOP) ++ if (in->type == PSEUDO_MSG_FASTOP) { + send_response = 0; ++ /* For fastops we reply now to say we got the data */ ++ if ((rc = pseudo_msg_send(clients[i].fd, &server_fastop_reply, 0, NULL)) != 0) { ++ pseudo_debug(PDBGF_SERVER, "failed to send fastop ack to client %d [%d]: %d (%s)\n", ++ i, (int) clients[i].pid, rc, strerror(errno)); ++ } ++ } + /* most messages don't need these, but xattr may */ + response_path = 0; + response_pathlen = -1; diff --git a/meta/recipes-devtools/pseudo/files/toomanyfiles.patch b/meta/recipes-devtools/pseudo/files/toomanyfiles.patch index 7319ab2..d014ad5 100644 --- a/meta/recipes-devtools/pseudo/files/toomanyfiles.patch +++ b/meta/recipes-devtools/pseudo/files/toomanyfiles.patch @@ -31,15 +31,12 @@ Index: pseudo-1.8.2/pseudo_server.c pseudo_debug(PDBGF_SERVER, "server loop started.\n"); if (listen_fd < 0) { -@@ -663,10 +665,18 @@ pseudo_server_loop(void) { +@@ -663,10 +665,15 @@ pseudo_server_loop(void) { message_time.tv_usec -= 1000000; ++message_time.tv_sec; } + } else if (hitmaxfiles) { -+ /* In theory there is a potential race here where if we close a client, -+ it may have sent us a fastop message which we don't act upon. -+ If we don't close a filehandle we'll loop indefinitely thought. -+ Only close one per loop iteration in the interests of caution */ ++ /* Only close one per loop iteration in the interests of caution */ + close_client(i); + hitmaxfiles = 0; } diff --git a/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb b/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb index 81853e9..73ef572 100644 --- a/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb +++ b/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb @@ -7,6 +7,7 @@ SRC_URI = "http://downloads.yoctoproject.org/releases/pseudo/${BPN}-${PV}.tar.bz file://moreretries.patch \ file://efe0be279901006f939cd357ccee47b651c786da.patch \ file://b6b68db896f9963558334aff7fca61adde4ec10f.patch \ + file://fastopreply.patch \ file://toomanyfiles.patch \ file://0001-Use-epoll-API-on-Linux.patch \ " -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] pseudo: Add fastop reply fix 2017-09-22 16:41 ` [PATCH 2/2] pseudo: Add fastop reply fix Richard Purdie @ 2017-09-22 16:52 ` Seebs 2017-09-22 17:12 ` Alexander Kanavin 0 siblings, 1 reply; 6+ messages in thread From: Seebs @ 2017-09-22 16:52 UTC (permalink / raw) To: Richard Purdie; +Cc: openembedded-core On Fri, 22 Sep 2017 17:41:17 +0100 Richard Purdie <richard.purdie@linuxfoundation.org> wrote: > This changes the pseudo FASTOP functionality so that a reply to the > operation is required. This means we then cannot lose data if a > connection is closed. This in turn stops corruption if we run out of > file handles and have to close connections. > > This tweaks the connection closure patch to update the comment there > which is now outdated. > > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> This looks reasonable to me. I did some testing with a very similar patch and concluded that, while it slowed performance slightly, it didn't slow it nearly as much as the pre-fastop behavior did. -s ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] pseudo: Add fastop reply fix 2017-09-22 16:52 ` Seebs @ 2017-09-22 17:12 ` Alexander Kanavin 2017-09-22 19:19 ` Otavio Salvador 0 siblings, 1 reply; 6+ messages in thread From: Alexander Kanavin @ 2017-09-22 17:12 UTC (permalink / raw) To: Seebs, Richard Purdie; +Cc: openembedded-core On 09/22/2017 07:52 PM, Seebs wrote: >> This changes the pseudo FASTOP functionality so that a reply to the >> operation is required. This means we then cannot lose data if a >> connection is closed. This in turn stops corruption if we run out of >> file handles and have to close connections. >> >> This tweaks the connection closure patch to update the comment there >> which is now outdated. >> >> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> > > This looks reasonable to me. I did some testing with a very similar > patch and concluded that, while it slowed performance slightly, it > didn't slow it nearly as much as the pre-fastop behavior did. Also, the issue this patch is fixing is not theoretical. Setting max fd limit to a low value (192) is causing errors on my machine: dpkg-deb: error: maintainer script 'postinst' has bad permissions 644 (must be >=0555 and <=0775) and with the patch they seem to go away. Alex ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] pseudo: Add fastop reply fix 2017-09-22 17:12 ` Alexander Kanavin @ 2017-09-22 19:19 ` Otavio Salvador 0 siblings, 0 replies; 6+ messages in thread From: Otavio Salvador @ 2017-09-22 19:19 UTC (permalink / raw) To: Alexander Kanavin; +Cc: Patches and discussions about the oe-core layer Richard, On Fri, Sep 22, 2017 at 2:12 PM, Alexander Kanavin <alexander.kanavin@linux.intel.com> wrote: > On 09/22/2017 07:52 PM, Seebs wrote: >>> >>> This changes the pseudo FASTOP functionality so that a reply to the >>> operation is required. This means we then cannot lose data if a >>> connection is closed. This in turn stops corruption if we run out of >>> file handles and have to close connections. >>> >>> This tweaks the connection closure patch to update the comment there >>> which is now outdated. >>> >>> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org> >> >> >> This looks reasonable to me. I did some testing with a very similar >> patch and concluded that, while it slowed performance slightly, it >> didn't slow it nearly as much as the pre-fastop behavior did. > > Also, the issue this patch is fixing is not theoretical. Setting max fd > limit to a low value (192) is causing errors on my machine: > > dpkg-deb: error: maintainer script 'postinst' has bad permissions 644 (must > be >=0555 and <=0775) > > and with the patch they seem to go away. When queueing it, please make sure to add the proper patch headers. -- Otavio Salvador O.S. Systems http://www.ossystems.com.br http://code.ossystems.com.br Mobile: +55 (53) 9981-7854 Mobile: +1 (347) 903-9750 ^ permalink raw reply [flat|nested] 6+ messages in thread
* ✗ patchtest: failure for "staging: Reduce verbosity of l..." and 1 more 2017-09-22 16:41 [PATCH 1/2] staging: Reduce verbosity of log messages Richard Purdie 2017-09-22 16:41 ` [PATCH 2/2] pseudo: Add fastop reply fix Richard Purdie @ 2017-09-22 17:00 ` Patchwork 1 sibling, 0 replies; 6+ messages in thread From: Patchwork @ 2017-09-22 17:00 UTC (permalink / raw) To: Richard Purdie; +Cc: openembedded-core == Series Details == Series: "staging: Reduce verbosity of l..." and 1 more Revision: 1 URL : https://patchwork.openembedded.org/series/9077/ State : failure == Summary == Thank you for submitting this patch series to OpenEmbedded Core. This is an automated response. Several tests have been executed on the proposed series by patchtest resulting in the following failures: * Issue Added patch file is missing Upstream-Status in the header [test_upstream_status_presence] Suggested fix Add Upstream-Status: <status> to the header of meta/recipes-devtools/pseudo/files/fastopreply.patch (possible values: Pending, Submitted, Accepted, Backport, Denied, Inappropriate) * Issue Series does not apply on top of target branch [test_series_merge_on_head] Suggested fix Rebase your series on top of targeted branch Targeted branch master (currently at acc5036a6b) * Issue A patch file has been added, but does not have a Signed-off-by tag [test_signed_off_by_presence] Suggested fix Sign off the added patch file (meta/recipes-devtools/pseudo/files/fastopreply.patch) If you believe any of these test results are incorrect, please reply to the mailing list (openembedded-core@lists.openembedded.org) raising your concerns. Otherwise we would appreciate you correcting the issues and submitting a new version of the patchset if applicable. Please ensure you add/increment the version number when sending the new version (i.e. [PATCH] -> [PATCH v2] -> [PATCH v3] -> ...). --- Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest Test suite: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-22 19:19 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-09-22 16:41 [PATCH 1/2] staging: Reduce verbosity of log messages Richard Purdie 2017-09-22 16:41 ` [PATCH 2/2] pseudo: Add fastop reply fix Richard Purdie 2017-09-22 16:52 ` Seebs 2017-09-22 17:12 ` Alexander Kanavin 2017-09-22 19:19 ` Otavio Salvador 2017-09-22 17:00 ` ✗ patchtest: failure for "staging: Reduce verbosity of l..." and 1 more Patchwork
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox