diff mbox series

[pseudo] do not abort on hard link path mismatch

Message ID 20220425144924.1033647-1-cam@myfastmail.com
State New
Headers show
Series [pseudo] do not abort on hard link path mismatch | expand

Commit Message

C. Andy Martin April 25, 2022, 2:49 p.m. UTC 
For hard-links there are many potential races where the path may not
yet exist in the database but we find an inode match on the linked
file. For instance, if one sub-process performs the link and another
one soon after opens the link, it is a race as to which operation's
message is received first by the pseudo server. Therefore, do not
abort on hard links, instead go back to master-branch behavior
(verbose file debug) for such cases.

See commit 691a2304c8c29a3b7d5977908ef82aaf188f5ba0 for original
nerf of the path mismatch messages for hard links.
---
 pseudo.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

Comments

Seebs April 25, 2022, 3:44 p.m. UTC | #1 
On Mon, 25 Apr 2022 10:49:24 -0400
"C. Andy Martin" <cam@myfastmail.com> wrote:

> -					msg->result = RESULT_ABORT;
> -					goto op_exit;
> +					if (msg->nlink == 1) {
> +						msg->result =
> RESULT_ABORT;
> +						goto op_exit;
> +					}

I'm distrustful of this because I feel like, in the case where we have
apparently-stale information in the database, we really should be coming
back with "we don't have valid database information", not yielding
what's probably incorrect/old data.

I'm also concerned a bit because nlink is never 1 for a directory...

-s
C. Andy Martin April 25, 2022, 6:01 p.m. UTC | #2 
On Mon, Apr 25, 2022, at 11:44 AM, Seebs wrote:
> On Mon, 25 Apr 2022 10:49:24 -0400
> "C. Andy Martin" <cam@myfastmail.com> wrote:
>
>> -					msg->result = RESULT_ABORT;
>> -					goto op_exit;
>> +					if (msg->nlink == 1) {
>> +						msg->result =
>> RESULT_ABORT;
>> +						goto op_exit;
>> +					}
>
> I'm distrustful of this because I feel like, in the case where we have
> apparently-stale information in the database, we really should be coming
> back with "we don't have valid database information", not yielding
> what's probably incorrect/old data.
>
> I'm also concerned a bit because nlink is never 1 for a directory...
>
> -s

I agree this is not the ideal solution. However, I was basing this compromise on this commit:

commit 691a2304c8c29a3b7d5977908ef82aaf188f5ba0
Author: Seebs <seebs@seebs.net>
Date:   Fri Apr 13 14:37:34 2018 -0500

    Less chatty debugging
    
    Stop producing the path mismatch messages for things with multiple
    links. We could probably make this smarter, but for now this is
    sufficient to reduce the spam.
    
    pseudo_debug(0, ...) now produces a message, as a special case.
    
    Signed-off-by: Seebs <seebs@seebs.net>

The issue is w/out the change, my builds occasionally abort due to races
-Andy
diff mbox series

Patch

diff --git a/pseudo.c b/pseudo.c
index 528fe1b..cb03987 100644
--- a/pseudo.c
+++ b/pseudo.c
@@ -696,15 +696,31 @@  pseudo_op(pseudo_msg_t *msg, const char *program, const char *tag, char **respon
 					pseudo_debug(PDBGF_FILE, "inode mismatch for '%s' -- old one was marked for deletion.\n",
 						msg->path);
 				} else {
-					pseudo_diag("path mismatch [%d link%s]: ino %llu db '%s' req '%s'.\n",
-						msg->nlink,
-						msg->nlink == 1 ? "" : "s",
-						(unsigned long long) msg_header.ino,
-						path_by_ino ? path_by_ino : "no path",
-						msg->path);
+					if (msg->nlink > 1) {
+						/* For hard-links there are many potential races
+						 * where the path may not yet exist in the
+						 * database but we find an inode match on the
+						 * linked file. */
+						int flags = PDBGF_FILE | PDBGF_VERBOSE;
+						pseudo_debug(flags, "path mismatch [%d link%s]: ino %llu db '%s' req '%s'.\n",
+							msg->nlink,
+							msg->nlink == 1 ? "" : "s",
+							(unsigned long long) msg_header.ino,
+							path_by_ino ? path_by_ino : "no path",
+							msg->path);
+					} else {
+						pseudo_diag("path mismatch [%d link%s]: ino %llu db '%s' req '%s'.\n",
+							msg->nlink,
+							msg->nlink == 1 ? "" : "s",
+							(unsigned long long) msg_header.ino,
+							path_by_ino ? path_by_ino : "no path",
+							msg->path);
+					}
 					found_ino = 0;
-					msg->result = RESULT_ABORT;
-					goto op_exit;
+					if (msg->nlink == 1) {
+						msg->result = RESULT_ABORT;
+						goto op_exit;
+					}
 				}
 			}
 		} else {