Hey Samuel, Yeah we don't really need to do it, i was doing it for readability. (in my mind these functions are getting too large and maybe it would be worth splitting them into smaller functions...but that is a separate thing).
But yeah assert works as well, and it has no impact on performance. here is the updated patch, where assert_backtrace is used instead of an explicit if statement. Let me know. Thanks a lot, Milos On Thu, Feb 12, 2026 at 3:39 PM Samuel Thibault <[email protected]> wrote: > Hello, > > Milos Nikic, le jeu. 12 févr. 2026 06:30:45 -0800, a ecrit: > > We don't always undo the link increases in error cases. > > Added logic the undo the changes to link count if an > > error occurred in those cases. > > --- > > libdiskfs/dir-rename.c | 12 ++++++++++-- > > libdiskfs/dir-renamed.c | 29 +++++++++++++++++++++++++++-- > > 2 files changed, 37 insertions(+), 4 deletions(-) > > > > diff --git a/libdiskfs/dir-rename.c b/libdiskfs/dir-rename.c > > index c8bb0dad..7953132b 100644 > > --- a/libdiskfs/dir-rename.c > > +++ b/libdiskfs/dir-rename.c > > @@ -188,12 +188,20 @@ diskfs_S_dir_rename (struct protid *fromcred, > > diskfs_node_update (tdp, 1); > > > > pthread_mutex_unlock (&tdp->lock); > > - pthread_mutex_unlock (&fnp->lock); > > if (err) > > { > > - diskfs_nrele (fnp); > > + if (fnp->dn_stat.st_nlink > 0) > > Do we really need to check these? We have increased it just above, and > haven't released the mutex. We're actually the owner of that reference, > it'd rather be an assert. > > > + { > > + fnp->dn_stat.st_nlink--; > > + fnp->dn_set_ctime = 1; > > + if (diskfs_synchronous) > > + diskfs_node_update (fnp, 1); > > + } > > + diskfs_drop_dirstat (tdp, ds); > > + diskfs_nput (fnp); > > return err; > > } > > + pthread_mutex_unlock (&fnp->lock); > > > > /* We now hold no locks */ > > > > diff --git a/libdiskfs/dir-renamed.c b/libdiskfs/dir-renamed.c > > index 1f3f3de4..532ff998 100644 > > --- a/libdiskfs/dir-renamed.c > > +++ b/libdiskfs/dir-renamed.c > > @@ -159,6 +159,13 @@ diskfs_rename_dir (struct node *fdp, struct node > *fnp, const char *fromname, > > assert_backtrace (err != ENOENT); > > if (err) > > { > > + if (tdp->dn_stat.st_nlink > 0) > > + { > > + tdp->dn_stat.st_nlink--; > > + tdp->dn_set_ctime = 1; > > + if (diskfs_synchronous) > > + diskfs_node_update (tdp, 1); > > + } > > diskfs_drop_dirstat (fnp, tmpds); > > goto out; > > } > > @@ -168,7 +175,16 @@ diskfs_rename_dir (struct node *fdp, struct node > *fnp, const char *fromname, > > if (diskfs_synchronous) > > diskfs_file_update (fnp, 1); > > if (err) > > - goto out; > > + { > > + if (tdp->dn_stat.st_nlink > 0) > > + { > > + tdp->dn_stat.st_nlink--; > > + tdp->dn_set_ctime = 1; > > + if (diskfs_synchronous) > > + diskfs_node_update (tdp, 1); > > + } > > + goto out; > > + } > > > > fdp->dn_stat.st_nlink--; > > fdp->dn_set_ctime = 1; > > @@ -213,7 +229,16 @@ diskfs_rename_dir (struct node *fdp, struct node > *fnp, const char *fromname, > > } > > > > if (err) > > - goto out; > > + { > > + if (fnp->dn_stat.st_nlink > 0) > > + { > > + fnp->dn_stat.st_nlink--; > > + fnp->dn_set_ctime = 1; > > + if (diskfs_synchronous) > > + diskfs_node_update (fnp, 1); > > + } > > + goto out; > > + } > > > > /* 4: Remove the entry in fdp. */ > > ds = buf; > > -- > > 2.52.0 > > > > -- > Samuel > "...Unix, MS-DOS, and Windows NT (also known as the Good, the Bad, and > the Ugly)." > (By Matt Welsh) >
From 253032a144192aa34fb456c072c7e30ec96a9662 Mon Sep 17 00:00:00 2001 From: Milos Nikic <[email protected]> Date: Thu, 12 Feb 2026 06:06:16 -0800 Subject: [PATCH] libdiskfs: fix cleanups in dir-rename and dir-renamed We don't always undo the link increases in error cases. Added logic the undo the changes to link count if an error occurred in those cases. --- libdiskfs/dir-rename.c | 10 ++++++++-- libdiskfs/dir-renamed.c | 23 +++++++++++++++++++++-- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/libdiskfs/dir-rename.c b/libdiskfs/dir-rename.c index c8bb0dad..6b4e9a7d 100644 --- a/libdiskfs/dir-rename.c +++ b/libdiskfs/dir-rename.c @@ -188,12 +188,18 @@ diskfs_S_dir_rename (struct protid *fromcred, diskfs_node_update (tdp, 1); pthread_mutex_unlock (&tdp->lock); - pthread_mutex_unlock (&fnp->lock); if (err) { - diskfs_nrele (fnp); + assert_backtrace (fnp->dn_stat.st_nlink > 0); + fnp->dn_stat.st_nlink--; + fnp->dn_set_ctime = 1; + if (diskfs_synchronous) + diskfs_node_update (fnp, 1); + diskfs_drop_dirstat (tdp, ds); + diskfs_nput (fnp); return err; } + pthread_mutex_unlock (&fnp->lock); /* We now hold no locks */ diff --git a/libdiskfs/dir-renamed.c b/libdiskfs/dir-renamed.c index 1f3f3de4..a68dc07c 100644 --- a/libdiskfs/dir-renamed.c +++ b/libdiskfs/dir-renamed.c @@ -159,6 +159,11 @@ diskfs_rename_dir (struct node *fdp, struct node *fnp, const char *fromname, assert_backtrace (err != ENOENT); if (err) { + assert_backtrace (tdp->dn_stat.st_nlink > 0); + tdp->dn_stat.st_nlink--; + tdp->dn_set_ctime = 1; + if (diskfs_synchronous) + diskfs_node_update (tdp, 1); diskfs_drop_dirstat (fnp, tmpds); goto out; } @@ -168,7 +173,14 @@ diskfs_rename_dir (struct node *fdp, struct node *fnp, const char *fromname, if (diskfs_synchronous) diskfs_file_update (fnp, 1); if (err) - goto out; + { + assert_backtrace (tdp->dn_stat.st_nlink > 0); + tdp->dn_stat.st_nlink--; + tdp->dn_set_ctime = 1; + if (diskfs_synchronous) + diskfs_node_update (tdp, 1); + goto out; + } fdp->dn_stat.st_nlink--; fdp->dn_set_ctime = 1; @@ -213,7 +225,14 @@ diskfs_rename_dir (struct node *fdp, struct node *fnp, const char *fromname, } if (err) - goto out; + { + assert_backtrace (fnp->dn_stat.st_nlink > 0); + fnp->dn_stat.st_nlink--; + fnp->dn_set_ctime = 1; + if (diskfs_synchronous) + diskfs_node_update (fnp, 1); + goto out; + } /* 4: Remove the entry in fdp. */ ds = buf; -- 2.52.0
