e2fsck: take flock(LOCK_EX) on whole-disk device during filesystem check
PATCH 1/1 e2fsck: take flock(LOCK_EX) on whole-disk device during filesystem check
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in /dev/disk/by-label/ and /dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.
udevd already serialises its own partition probes against whole-disk device access using flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiring flock(LOCK_EX) on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.
The whole-disk device is resolved via sysfs (/sys/dev/block/MAJ:MIN/partition) so that the lock covers the same device node that udevd locks. If sysfs resolution fails, a warning is emitted and the lock falls back to the partition device itself.
--- e2fsck/unix.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+)
e2fsck/unix.c +105 -0
@@ -36,6 +36,15 @@ extern int optind;#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef HAVE_SYS_FILE_H +#include <sys/file.h> +#endif +#ifdef HAVE_SYS_STAT_H +#include <sys/stat.h> +#endif +#ifdef HAVE_SYS_SYSMACROS_H +#include <sys/sysmacros.h> +#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif
@@ -1397,6 +1406,92 @@ err:return retval; } +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name) +{ +struct stat st; +char partition_attr[256 ];/* sysfs 'partition' attribute path */ +char parent_dev_path[256 ];/* sysfs '../dev' of the parent disk */ +char parent_devnum[32 ]; +FILE *f; +unsigned int maj, min; +int parent_resolved =0 ; +char lock_path[256 ]; +const char *opened_path; +int fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + maj =major (st.st_rdev); + min =minor (st.st_rdev); + +/* +* If dev_name is a partition (sysfs 'partition' attribute exists), +* resolve the parent whole-disk device so the flock covers the same +* device node that udevd locks before probing any partition on it. +*/ +snprintf (partition_attr,sizeof (partition_attr), +"/sys/dev/block/%u:%u/partition" , maj, min); + +if (access (partition_attr,F_OK ) ==0 ) { +snprintf (parent_dev_path,sizeof (parent_dev_path), +"/sys/dev/block/%u:%u/../dev" , maj, min); + + f =fopen (parent_dev_path,"r" ); +if (f) { +if (fscanf (f,"%31s" , parent_devnum) ==1 ) { +unsigned int pmaj, pmin; +if (sscanf (parent_devnum,"%u:%u" , + &pmaj, &pmin) ==2 ) { + maj = pmaj; + min = pmin; + parent_resolved =1 ; + } + } +fclose (f); + } +if (!parent_resolved) +log_err (ctx,_ ("Warning: could not resolve whole-disk " +"device for %s; lock may not prevent " +"udev races\n" ), dev_name); + } + +snprintf (lock_path,sizeof (lock_path), +"/dev/block/%u:%u" , maj, min); + + opened_path = lock_path; + fd =open (lock_path,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { + fd =open (dev_name,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) +return -1 ; + opened_path = dev_name; +if (parent_resolved) +log_err (ctx,_ ("Warning: %s not found; locking %s " +"instead, lock may not prevent " +"udev races\n" ), lock_path, dev_name); + } + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ; + } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), opened_path); +close (fd); +return -1 ; + } + +return fd; +} +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;
@@ -1413,6 +1508,7 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +int lock_fd =-1 ;__u32 features[3 ];char *cp;enum quota_type qtype;
@@ -1488,6 +1584,12 @@ int main (int argc, char *argv[])check_mount (ctx); + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name); +if (lock_fd <0 ) +log_err (ctx,_ ("Warning: could not lock %s; " +"proceeding without block device lock\n" ), + ctx->filesystem_name); +if (!(ctx->options &E2F_OPT_PREEN ) && !(ctx->options &E2F_OPT_NO ) && !(ctx->options &E2F_OPT_YES )) {
@@ -2169,6 +2271,9 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name); +if (lock_fd >=0 ) +close (lock_fd); +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);
Signature · 1 line
2.54.0
Hi,
Gentle ping on this patch.
I would appreciate any feedback when time permits.
Quoted 173 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.The whole-disk device is resolved via sysfs (/sys/dev/block/MAJ:MIN/partition) so that the lock covers the same device node that udevd locks. If sysfs resolution fails, a warning is emitted and the lock falls back to the partition device itself.
Signed-off-by Nandakumar Raghavan <[email protected]>--- e2fsck/unix.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+)e2fsck/unix.c +105
-0@@ -36,6 +36,15 @@ extern int optind;#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef HAVE_SYS_FILE_H +#include <sys/file.h> +#endif +#ifdef HAVE_SYS_STAT_H +#include <sys/stat.h> +#endif +#ifdef HAVE_SYS_SYSMACROS_H +#include <sys/sysmacros.h> +#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif @@ -1397,6 +1406,92 @@ err:return retval; } +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name) +{ +struct stat st; +char partition_attr[256 ];/* sysfs 'partition' attribute path */ +char parent_dev_path[256 ];/* sysfs '../dev' of the parent disk */ +char parent_devnum[32 ]; +FILE *f; +unsigned int maj, min; +int parent_resolved =0 ; +char lock_path[256 ]; +const char *opened_path; +int fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + maj =major (st.st_rdev); + min =minor (st.st_rdev); + +/* +* If dev_name is a partition (sysfs 'partition' attribute exists), +* resolve the parent whole-disk device so the flock covers the same +* device node that udevd locks before probing any partition on it. +*/ +snprintf (partition_attr,sizeof (partition_attr), +"/sys/dev/block/%u:%u/partition" , maj, min); + +if (access (partition_attr,F_OK ) ==0 ) { +snprintf (parent_dev_path,sizeof (parent_dev_path), +"/sys/dev/block/%u:%u/../dev" , maj, min); + + f =fopen (parent_dev_path,"r" ); +if (f) { +if (fscanf (f,"%31s" , parent_devnum) ==1 ) { +unsigned int pmaj, pmin; +if (sscanf (parent_devnum,"%u:%u" , + &pmaj, &pmin) ==2 ) { + maj = pmaj; + min = pmin; + parent_resolved =1 ; + } + } +fclose (f); + } +if (!parent_resolved) +log_err (ctx,_ ("Warning: could not resolve whole-disk " +"device for %s; lock may not prevent " +"udev races\n" ), dev_name); + } + +snprintf (lock_path,sizeof (lock_path), +"/dev/block/%u:%u" , maj, min); + + opened_path = lock_path; + fd =open (lock_path,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { + fd =open (dev_name,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) +return -1 ; + opened_path = dev_name; +if (parent_resolved) +log_err (ctx,_ ("Warning: %s not found; locking %s " +"instead, lock may not prevent " +"udev races\n" ), lock_path, dev_name); + } + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ; + } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), opened_path); +close (fd); +return -1 ; + } + +return fd; +} +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;@@ -1413,6 +1508,7 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +int lock_fd =-1 ;__u32 features[3 ];char *cp;enum quota_type qtype;@@ -1488,6 +1584,12 @@ int main (int argc, char *argv[])check_mount (ctx); + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name); +if (lock_fd <0 ) +log_err (ctx,_ ("Warning: could not lock %s; " +"proceeding without block device lock\n" ), + ctx->filesystem_name); +if (!(ctx->options &E2F_OPT_PREEN ) && !(ctx->options &E2F_OPT_NO ) && !(ctx->options &E2F_OPT_YES )) {@@ -2169,6 +2271,9 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name); +if (lock_fd >=0 ) +close (lock_fd); +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);Signature · 1 line
2.54.0
Quoted 31 lines · Nandakumar Raghavan
Hi,
Gentle ping on this patch.
I would appreciate any feedback when time permits.
Quoted 23 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.The whole-disk device is resolved via sysfs (/sys/dev/block/MAJ:MIN/partition) so that the lock covers the same device node that udevd locks. If sysfs resolution fails, a warning is emitted and the lock falls back to the partition device itself.
Signed-off-by Nandakumar Raghavan <[email protected]>
This looks like it would unnecessarily emit an error message if run on any non-Linux platform (e.g. MacOS, BSD), so it needs to be conditionally built.
Quoted 9 lines
e2fsck/unix.c extern int optind;
Quoted 9 lines
---
e2fsck/unix.c| 105 ++++++++++++++++++++++++++++++++++++++++++++++++++1 file changed, 105 insertions(+)e2fsck/unix.c +0
-0@@ -36,6 +36,15 @@ extern int optind;
Presumably this code is only useful on Linux, so the conditional headers are not needed, only a single #ifdef __linux__? Not really critical either way.
Quoted 9 lines
Quoted 9 lines
e2fsck/unix.c +6
-0#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef HAVE_SYS_FILE_H +#include <sys/file.h> +#endif +#ifdef HAVE_SYS_STAT_H +#include <sys/stat.h> +#endif
Quoted 8 lines
e2fsck/unix.c err: · line 1406
Quoted 8 lines
e2fsck/unix.c +4
-0+#ifdef HAVE_SYS_SYSMACROS_H +#include <sys/sysmacros.h> +#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif @@ -1397,6 +1406,92 @@ err: +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name)
This function should similarly be #ifdef'd out if not building on Linux.
Quoted 24 lines
Quoted 24 lines
e2fsck/unix.c +24
-0+{ +struct stat st; +char partition_attr[256 ];/* sysfs 'partition' attribute path */ +char parent_dev_path[256 ];/* sysfs '../dev' of the parent disk */ +char parent_devnum[32 ]; +FILE *f; +unsigned int maj, min; +int parent_resolved =0 ; +char lock_path[256 ]; +const char *opened_path; +int fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + maj =major (st.st_rdev); + min =minor (st.st_rdev); + +/* +* If dev_name is a partition (sysfs 'partition' attribute exists), +* resolve the parent whole-disk device so the flock covers the same
Quoted 8 lines
e2fsck/unix.c err: · line 1441
Quoted 8 lines
e2fsck/unix.c +8
-0+* device node that udevd locks before probing any partition on it. +*/ +snprintf (partition_attr,sizeof (partition_attr), +"/sys/dev/block/%u:%u/partition" , maj, min); + +if (access (partition_attr,F_OK ) ==0 ) { +snprintf (parent_dev_path,sizeof (parent_dev_path), +"/sys/dev/block/%u:%u/../dev" , maj, min);
(minor) this could reuse partition_attr here?
Quoted 13 lines
Quoted 13 lines
e2fsck/unix.c +13
-0+ f =fopen (parent_dev_path,"r" ); +if (f) { +if (fscanf (f,"%31s" , parent_devnum) ==1 ) { +unsigned int pmaj, pmin; +if (sscanf (parent_devnum,"%u:%u" , + &pmaj, &pmin) ==2 ) { + maj = pmaj; + min = pmin; + parent_resolved =1 ; + } + } +fclose (f); + }
Quoted 8 lines
e2fsck/unix.c err: · line 1463
Quoted 8 lines
e2fsck/unix.c +8
-0+if (!parent_resolved) +log_err (ctx,_ ("Warning: could not resolve whole-disk " +"device for %s; lock may not prevent " +"udev races\n" ), dev_name); + } + +snprintf (lock_path,sizeof (lock_path), +"/dev/block/%u:%u" , maj, min);
(style) could fit on a single line?
(style) could re-use partition_attr here (maybe renamed to dev_path or similar) to avoid having 3 single-use pathnames on the stack.
Quoted 4 lines
Quoted 4 lines
e2fsck/unix.c +4
-0+ + opened_path = lock_path; + fd =open (lock_path,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) {
Quoted 8 lines
e2fsck/unix.c err: · line 1475
Quoted 8 lines
e2fsck/unix.c +8
-0+ fd =open (dev_name,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) +return -1 ; + opened_path = dev_name; +if (parent_resolved) +log_err (ctx,_ ("Warning: %s not found; locking %s " +"instead, lock may not prevent " +"udev races\n" ), lock_path, dev_name);
(style) shouldn't split error messages across lines, even if > 80 columns
Quoted 9 lines
e2fsck/unix.c err: · line 1484
Quoted 9 lines
e2fsck/unix.c +9
-0+ } + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ;
Does this loop/hang forever if you try to kill it with CTRL-C?
Quoted 38 lines
Quoted 38 lines
e2fsck/unix.c +17
-0+ } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), opened_path); +close (fd); +return -1 ; + } + +return fd; +} +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;@@ -1413,6 +1508,7 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +int lock_fd =-1 ;__u32 features[3 ];char *cp;enum quota_type qtype;@@ -1488,6 +1584,12 @@ int main (int argc, char *argv[])check_mount (ctx); + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name); +if (lock_fd <0 ) +log_err (ctx,_ ("Warning: could not lock %s; " +"proceeding without block device lock\n" ), + ctx->filesystem_name); +if (!(ctx->options &
E2F_OPT_PREEN) &&
!(ctx->options &E2F_OPT_NO) &&
!(ctx->options &E2F_OPT_YES)) {e2fsck/unix.c +0
-0@@ -2169,6 +2271,9 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name);
Quoted 9 lines
Quoted 8 lines
e2fsck/unix.c +3
-0+if (lock_fd >=0 ) +close (lock_fd); +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);Signature · 1 line
2.54.0
Cheers, Andreas
Quoted 26 lines · Andreas Dilger
Quoted 25 lines · Nandakumar Raghavan
Hi,
Gentle ping on this patch.
I would appreciate any feedback when time permits.
Quoted 17 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.
Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
The whole-disk device is resolved via sysfs (/sys/dev/block/MAJ:MIN/partition) so that the lock covers the same device node that udevd locks. If sysfs resolution fails, a warning is emitted and the lock falls back to the partition device itself.
Signed-off-by Nandakumar Raghavan <[email protected]>
Thanks for the review, Andreas. Replies inline, v2 coming with all of these addressed.
Quoted 2 lines
This looks like it would unnecessarily emit an error message if run on any non-Linux platform (e.g. MacOS, BSD), so it needs to be conditionally built.
Agreed, fixed in v2 — lock_whole_disk(), its call site, the lock_fd variable, and the close() cleanup are all now wrapped in #ifdef __linux__. Nothing from this feature is compiled at all on non-Linux.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
---
e2fsck/unix.c| 105 ++++++++++++++++++++++++++++++++++++++++++++++++++1 file changed, 105 insertions(+)
Quoted 8 lines
Quoted 5 lines
e2fsck/unix.c extern int optind;
Quoted 5 lines
e2fsck/unix.c +0
-0@@ -36,6 +36,15 @@ extern int optind;Presumably this code is only useful on Linux, so the conditional headers are not needed, only a single
#ifdef __linux__? Not really critical either way.
Fixed. collapsed the three HAVE_SYS_*_H guards into one #ifdef __linux__ block around sys/file.h, sys/stat.h, sys/sysmacros.h.
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
e2fsck/unix.c +8
-0#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef HAVE_SYS_FILE_H +#include <sys/file.h> +#endif +#ifdef HAVE_SYS_STAT_H +#include <sys/stat.h> +#endif +#ifdef HAVE_SYS_SYSMACROS_H +#include <sys/sysmacros.h>
Quoted 8 lines
Quoted 6 lines
e2fsck/unix.c err: · line 1406
Quoted 6 lines
e2fsck/unix.c +2
-0+#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif @@ -1397,6 +1406,92 @@ err: +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name)This function should similarly be #ifdef'd out if not building on Linux.
Fixed. I have went with wrapping the whole function rather than guarding just the body, so it's fully absent from non-Linux builds.
Quoted 60 lines
Quoted 25 lines
Quoted 25 lines
e2fsck/unix.c +24
-0+{ +struct stat st; +char partition_attr[256 ];/* sysfs 'partition' attribute path */ +char parent_dev_path[256 ];/* sysfs '../dev' of the parent disk */ +char parent_devnum[32 ]; +FILE *f; +unsigned int maj, min; +int parent_resolved =0 ; +char lock_path[256 ]; +const char *opened_path; +int fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + maj =major (st.st_rdev); + min =minor (st.st_rdev); + +/* +* If dev_name is a partition (sysfs 'partition' attribute exists), +* resolve the parent whole-disk device so the flock covers the same Quoted 8 lines
e2fsck/unix.c err: · line 1441
Quoted 8 lines
e2fsck/unix.c +8
-0+* device node that udevd locks before probing any partition on it. +*/ +snprintf (partition_attr,sizeof (partition_attr), +"/sys/dev/block/%u:%u/partition" , maj, min); + +if (access (partition_attr,F_OK ) ==0 ) { +snprintf (parent_dev_path,sizeof (parent_dev_path), +"/sys/dev/block/%u:%u/../dev" , maj, min);(minor) this could reuse
partition_attrhere?Quoted 13 lines
Quoted 13 lines
e2fsck/unix.c +13
-0+ f =fopen (parent_dev_path,"r" ); +if (f) { +if (fscanf (f,"%31s" , parent_devnum) ==1 ) { +unsigned int pmaj, pmin; +if (sscanf (parent_devnum,"%u:%u" , + &pmaj, &pmin) ==2 ) { + maj = pmaj; + min = pmin; + parent_resolved =1 ; + } + } +fclose (f); + }Quoted 8 lines
e2fsck/unix.c err: · line 1463
Quoted 8 lines
e2fsck/unix.c +8
-0+if (!parent_resolved) +log_err (ctx,_ ("Warning: could not resolve whole-disk " +"device for %s; lock may not prevent " +"udev races\n" ), dev_name); + } + +snprintf (lock_path,sizeof (lock_path), +"/dev/block/%u:%u" , maj, min);(style) could fit on a single line?
Quoted 2 lines
(style) could re-use
partition_attrhere (maybe renamed todev_pathor similar) to avoid having 3 single-use pathnames on the stack.
Fixed — same dev_path buffer reused for all three paths, and the /dev/block/%u:%u snprintf is now a single line
Quoted 15 lines
Quoted 13 lines
e2fsck/unix.c err: · line 1475
Quoted 13 lines
e2fsck/unix.c +12
-0+ + opened_path = lock_path; + fd =open (lock_path,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { + fd =open (dev_name,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) +return -1 ; + opened_path = dev_name; +if (parent_resolved) +log_err (ctx,_ ("Warning: %s not found; locking %s " +"instead, lock may not prevent " +"udev races\n" ), lock_path, dev_name);(style) shouldn't split error messages across lines, even if > 80 columns
Done — both warning strings are now single-line literals.
Quoted 12 lines
Quoted 10 lines
e2fsck/unix.c err: · line 1484
Quoted 10 lines
e2fsck/unix.c +9
-0+ } + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ;Does this loop/hang forever if you try to kill it with CTRL-C?
No. I traced it. The SIGINT/SIGTERM handler is installed earlier in main(), before lock_whole_disk() runs, and isn't SA_RESTART (that's only applied to SIGUSR1/SIGUSR2 afterward). So a blocked flock() gets EINTR on Ctrl-C, E2F_FLAG_CANCEL is already set by the handler, and the existing EINTR branch in the retry loop breaks out and returns -1 cleanly. So there is no hang.
One more thing found while testing v2: the original patch printed "Warning:
could not lock %s; proceeding without block device lock" unconditionally whenever lock_whole_disk() returned -1 — including the very common case of running e2fsck against a regular file rather than a block device (which is how virtually every e2fsprogs test image is checked). This caused issues with test since the warning message went into stderr. v2 drops that blanket message entirely; lock_whole_disk() now only warns/errors at the point of a genuine failure
Quoted 55 lines
Quoted 39 lines
Quoted 39 lines
e2fsck/unix.c +17
-0+ } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), opened_path); +close (fd); +return -1 ; + } + +return fd; +} +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;@@ -1413,6 +1508,7 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +int lock_fd =-1 ;__u32 features[3 ];char *cp;enum quota_type qtype;@@ -1488,6 +1584,12 @@ int main (int argc, char *argv[])check_mount (ctx); + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name); +if (lock_fd <0 ) +log_err (ctx,_ ("Warning: could not lock %s; " +"proceeding without block device lock\n" ), + ctx->filesystem_name); +if (!(ctx->options &
E2F_OPT_PREEN) &&
!(ctx->options &E2F_OPT_NO) &&
!(ctx->options &E2F_OPT_YES)) {e2fsck/unix.c +0
-0@@ -2169,6 +2271,9 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name);Quoted 9 lines
Quoted 8 lines
e2fsck/unix.c +3
-0+if (lock_fd >=0 ) +close (lock_fd); +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);Signature · 1 line
2.54.0Cheers, Andreas
PATCH v2 1/1 e2fsck: take flock(LOCK_EX) on whole-disk device during filesystem check
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in /dev/disk/by-label/ and /dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.
udevd already serialises its own partition probes against whole-disk device access using flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiring flock(LOCK_EX) on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.
The whole-disk device is resolved via sysfs (/sys/dev/block/MAJ:MIN/partition) so that the lock covers the same device node that udevd locks. If sysfs resolution fails, a warning is emitted and the lock falls back to the partition device itself.
This mechanism relies on Linux-specific interfaces (sysfs, flock() semantics on block devices) and is compiled out entirely on non-Linux platforms.
---
Changes in v2:
- Guard the whole feature (function, call site, lock_fd, cleanup) with #ifdef __linux__ instead of just the function body, so it's fully absent from non-Linux builds
- Collapse HAVE_SYS_*_H header guards into a single #ifdef __linux__
- Reuse a single dev_path[] buffer instead of three separate stack buffers
- Keep translatable warning strings on one line each, even past 80 columns
- Drop the blanket "could not lock" warning that fired even when the target wasn't a block device at all (e.g. a regular file), which was causing issues with tests; lock_whole_disk() now only warns/errors at the point of a genuine failure
- No change needed for the Ctrl-C/EINTR concern — traced and confirmed the existing signal handling already unwinds cleanly
e2fsck/unix.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+)
e2fsck/unix.c +101 -0
@@ -36,6 +36,11 @@ extern int optind;#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef __linux__ +#include <sys/file.h> +#include <sys/stat.h> +#include <sys/sysmacros.h> +#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif
@@ -1397,6 +1402,90 @@ err:return retval; } +#ifdef __linux__ +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name) +{ +struct stat st; +char dev_path[256 ];/* reused for each sysfs/device path we build */ +char parent_devnum[32 ]; +FILE *f; +unsigned int maj, min; +int parent_resolved =0 ; +const char *opened_path; +int fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + maj =major (st.st_rdev); + min =minor (st.st_rdev); + +/* +* If dev_name is a partition (sysfs 'partition' attribute exists), +* resolve the parent whole-disk device so the flock covers the same +* device node that udevd locks before probing any partition on it. +*/ +snprintf (dev_path,sizeof (dev_path), +"/sys/dev/block/%u:%u/partition" , maj, min); + +if (access (dev_path,F_OK ) ==0 ) { +snprintf (dev_path,sizeof (dev_path), +"/sys/dev/block/%u:%u/../dev" , maj, min); + + f =fopen (dev_path,"r" ); +if (f) { +if (fscanf (f,"%31s" , parent_devnum) ==1 ) { +unsigned int pmaj, pmin; +if (sscanf (parent_devnum,"%u:%u" , + &pmaj, &pmin) ==2 ) { + maj = pmaj; + min = pmin; + parent_resolved =1 ; + } + } +fclose (f); + } +if (!parent_resolved) +log_err (ctx,_ ("Warning: could not resolve whole-disk device for %s; lock may not prevent udev races\n" ), dev_name); + } + +snprintf (dev_path,sizeof (dev_path),"/dev/block/%u:%u" , maj, min); + + opened_path = dev_path; + fd =open (dev_path,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { + fd =open (dev_name,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { +com_err (ctx->program_name, errno, +_ ("while trying to open %s for locking" ), dev_name); +return -1 ; + } + opened_path = dev_name; +if (parent_resolved) +log_err (ctx,_ ("Warning: %s not found; locking %s instead, lock may not prevent udev races\n" ), dev_path, dev_name); + } + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ; + } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), opened_path); +close (fd); +return -1 ; + } + +return fd; +} +#endif /* __linux__ */ +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;
@@ -1413,6 +1502,9 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +#ifdef __linux__ +int lock_fd =-1 ; +#endif __u32 features[3 ];char *cp;enum quota_type qtype;
@@ -1488,6 +1580,10 @@ int main (int argc, char *argv[])check_mount (ctx); +#ifdef __linux__ + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name); +#endif +if (!(ctx->options &E2F_OPT_PREEN ) && !(ctx->options &E2F_OPT_NO ) && !(ctx->options &E2F_OPT_YES )) {
@@ -2169,6 +2265,11 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name); +#ifdef __linux__ +if (lock_fd >=0 ) +close (lock_fd); +#endif +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);
Signature · 1 line
2.54.0
Quoted 23 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.The whole-disk device is resolved via sysfs (/sys/dev/block/MAJ:MIN/partition) so that the lock covers the same device node that udevd locks. If sysfs resolution fails, a warning is emitted and the lock falls back to the partition device itself.
Quoted 4 lines
This mechanism relies on Linux-specific interfaces (sysfs,
flock()semantics on block devices) and is compiled out entirely on non-Linux platforms.Signed-off-by Nandakumar Raghavan <[email protected]>
Looks much better, thanks.
Cheers, Andreas
Quoted 17 lines
---
Changes in v2:
- Guard the whole feature (function, call site,lock_fd, cleanup) with #ifdef__linux__instead of just the function body, so it's fully absent from non-Linux builds
- Collapse HAVE_SYS_*_H header guards into a single #ifdef__linux__
- Reuse a single dev_path[] buffer instead of three separate stack buffers
- Keep translatable warning strings on one line each, even past 80 columns
- Drop the blanket "could not lock" warning that fired even when the target wasn't a block device at all (e.g. a regular file), which was causing issues with tests;lock_whole_disk()now only warns/errors at the point of a genuine failure
- No change needed for the Ctrl-C/EINTR concern — traced and confirmed the existing signal handling already unwinds cleanly
Quoted 17 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.
Quoted 7 lines
The whole-disk device is resolved via sysfs (/sys/dev/block/MAJ:MIN/partition) so that the lock covers the same device node that udevd locks. If sysfs resolution fails, a warning is emitted and the lock falls back to the partition device itself.
This mechanism relies on Linux-specific interfaces (sysfs,
flock()semantics on block devices) and is compiled out entirely on non-Linux platforms.
You're clearly implementing the "Locking Block Device Access" protocol as documented by systemd:
https://systemd.io/BLOCK_DEVICE_LOCKING/
So why not use libsystemd like the example in that posting provides? sd_device_get_parent_with_subsystem_devtype is much less gross than open-coding the same logic here.
Quoted 140 lines
Signed-off-by Nandakumar Raghavan <[email protected]>---
Changes in v2:
- Guard the whole feature (function, call site,lock_fd, cleanup) with #ifdef__linux__instead of just the function body, so it's fully absent from non-Linux builds
- Collapse HAVE_SYS_*_H header guards into a single #ifdef__linux__
- Reuse a single dev_path[] buffer instead of three separate stack buffers
- Keep translatable warning strings on one line each, even past 80 columns
- Drop the blanket "could not lock" warning that fired even when the target wasn't a block device at all (e.g. a regular file), which was causing issues with tests;lock_whole_disk()now only warns/errors at the point of a genuine failure
- No change needed for the Ctrl-C/EINTR concern — traced and confirmed the existing signal handling already unwinds cleanlye2fsck/unix.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+)e2fsck/unix.c +92
-0@@ -36,6 +36,11 @@ extern int optind;#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef __linux__ +#include <sys/file.h> +#include <sys/stat.h> +#include <sys/sysmacros.h> +#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif @@ -1397,6 +1402,90 @@ err:return retval; } +#ifdef __linux__ +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name) +{ +struct stat st; +char dev_path[256 ];/* reused for each sysfs/device path we build */ +char parent_devnum[32 ]; +FILE *f; +unsigned int maj, min; +int parent_resolved =0 ; +const char *opened_path; +int fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + maj =major (st.st_rdev); + min =minor (st.st_rdev); + +/* +* If dev_name is a partition (sysfs 'partition' attribute exists), +* resolve the parent whole-disk device so the flock covers the same +* device node that udevd locks before probing any partition on it. +*/ +snprintf (dev_path,sizeof (dev_path), +"/sys/dev/block/%u:%u/partition" , maj, min); + +if (access (dev_path,F_OK ) ==0 ) { +snprintf (dev_path,sizeof (dev_path), +"/sys/dev/block/%u:%u/../dev" , maj, min); + + f =fopen (dev_path,"r" ); +if (f) { +if (fscanf (f,"%31s" , parent_devnum) ==1 ) { +unsigned int pmaj, pmin; +if (sscanf (parent_devnum,"%u:%u" , + &pmaj, &pmin) ==2 ) { + maj = pmaj; + min = pmin; + parent_resolved =1 ; + } + } +fclose (f); + } +if (!parent_resolved) +log_err (ctx,_ ("Warning: could not resolve whole-disk device for %s; lock may not prevent udev races\n" ), dev_name); + } + +snprintf (dev_path,sizeof (dev_path),"/dev/block/%u:%u" , maj, min); + + opened_path = dev_path; + fd =open (dev_path,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { + fd =open (dev_name,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { +com_err (ctx->program_name, errno, +_ ("while trying to open %s for locking" ), dev_name); +return -1 ; + } + opened_path = dev_name; +if (parent_resolved) +log_err (ctx,_ ("Warning: %s not found; locking %s instead, lock may not prevent udev races\n" ), dev_path, dev_name); + } + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ; + } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), opened_path); +close (fd); +return -1 ; + } + +return fd; +} +#endif /* __linux__ */ +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;@@ -1413,6 +1502,9 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +#ifdef __linux__ +int lock_fd =-1 ; +#endif __u32 features[3 ];char *cp;enum quota_type qtype;
e2fsck/unix.c int main (int argc, char *argv[]) · line 1584
Quoted 5 lines
e2fsck/unix.c +2
-0@@ -1488,6 +1580,10 @@ int main (int argc, char *argv[])check_mount (ctx); +#ifdef __linux__ + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name);
If the filesystem has an external journal device, doesn't that also need locking?
Why isn't this implemented as part of the unixio manager?
--D
Quoted 21 lines
e2fsck/unix.c +7
-0+#endif +if (!(ctx->options &E2F_OPT_PREEN ) && !(ctx->options &E2F_OPT_NO ) && !(ctx->options &E2F_OPT_YES )) {@@ -2169,6 +2265,11 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name); +#ifdef __linux__ +if (lock_fd >=0 ) +close (lock_fd); +#endif +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);Signature · 1 line
2.54.0
Quoted 26 lines · Darrick J. Wong
Quoted 17 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.Quoted 7 lines
The whole-disk device is resolved via sysfs (/sys/dev/block/MAJ:MIN/partition) so that the lock covers the same device node that udevd locks. If sysfs resolution fails, a warning is emitted and the lock falls back to the partition device itself.
This mechanism relies on Linux-specific interfaces (sysfs,
flock()semantics on block devices) and is compiled out entirely on non-Linux platforms.
Quoted 7 lines
You're clearly implementing the "Locking Block Device Access" protocol as documented by systemd:
https://systemd.io/BLOCK_DEVICE_LOCKING/So why not use libsystemd like the example in that posting provides?
sd_device_get_parent_with_subsystem_devtypeis much less gross than open-coding the same logic here.
This was on purpose and not something I missed. The sd_device_get_parent_with_subsystem_devtype requires libsystemd v251+, and e2fsck needs to keep working in contexts where that dependency isn't available or isn't guaranteed to be new enough - non-systemd distros, older LTS systems doing root-fs repair before userspace is fully up, embedded/recovery images, etc. Taking a hard runtime dependency on a versioned libsystemd API for a tool this fundamental felt like a much bigger cost than by parsing (/sys/dev/block/MAJ:MIN/partition, .../dev), which only relies on a stable kernel ABI with no library and no version floor. Happy to reconsider if the project wants to take the dependency, but wanted to flag the tradeoff explicitly rather than silently pick one side.
Quoted 142 lines
Quoted 142 lines
Signed-off-by Nandakumar Raghavan <[email protected]>---
Changes in v2:
- Guard the whole feature (function, call site,lock_fd, cleanup) with #ifdef__linux__instead of just the function body, so it's fully absent from non-Linux builds
- Collapse HAVE_SYS_*_H header guards into a single #ifdef__linux__
- Reuse a single dev_path[] buffer instead of three separate stack buffers
- Keep translatable warning strings on one line each, even past 80 columns
- Drop the blanket "could not lock" warning that fired even when the target wasn't a block device at all (e.g. a regular file), which was causing issues with tests;lock_whole_disk()now only warns/errors at the point of a genuine failure
- No change needed for the Ctrl-C/EINTR concern — traced and confirmed the existing signal handling already unwinds cleanlye2fsck/unix.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+)e2fsck/unix.c +92
-0@@ -36,6 +36,11 @@ extern int optind;#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef __linux__ +#include <sys/file.h> +#include <sys/stat.h> +#include <sys/sysmacros.h> +#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif @@ -1397,6 +1402,90 @@ err:return retval; } +#ifdef __linux__ +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name) +{ +struct stat st; +char dev_path[256 ];/* reused for each sysfs/device path we build */ +char parent_devnum[32 ]; +FILE *f; +unsigned int maj, min; +int parent_resolved =0 ; +const char *opened_path; +int fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + maj =major (st.st_rdev); + min =minor (st.st_rdev); + +/* +* If dev_name is a partition (sysfs 'partition' attribute exists), +* resolve the parent whole-disk device so the flock covers the same +* device node that udevd locks before probing any partition on it. +*/ +snprintf (dev_path,sizeof (dev_path), +"/sys/dev/block/%u:%u/partition" , maj, min); + +if (access (dev_path,F_OK ) ==0 ) { +snprintf (dev_path,sizeof (dev_path), +"/sys/dev/block/%u:%u/../dev" , maj, min); + + f =fopen (dev_path,"r" ); +if (f) { +if (fscanf (f,"%31s" , parent_devnum) ==1 ) { +unsigned int pmaj, pmin; +if (sscanf (parent_devnum,"%u:%u" , + &pmaj, &pmin) ==2 ) { + maj = pmaj; + min = pmin; + parent_resolved =1 ; + } + } +fclose (f); + } +if (!parent_resolved) +log_err (ctx,_ ("Warning: could not resolve whole-disk device for %s; lock may not prevent udev races\n" ), dev_name); + } + +snprintf (dev_path,sizeof (dev_path),"/dev/block/%u:%u" , maj, min); + + opened_path = dev_path; + fd =open (dev_path,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { + fd =open (dev_name,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { +com_err (ctx->program_name, errno, +_ ("while trying to open %s for locking" ), dev_name); +return -1 ; + } + opened_path = dev_name; +if (parent_resolved) +log_err (ctx,_ ("Warning: %s not found; locking %s instead, lock may not prevent udev races\n" ), dev_path, dev_name); + } + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ; + } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), opened_path); +close (fd); +return -1 ; + } + +return fd; +} +#endif /* __linux__ */ +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;@@ -1413,6 +1502,9 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +#ifdef __linux__ +int lock_fd =-1 ; +#endif __u32 features[3 ];char *cp;enum quota_type qtype;@@ -1488,6 +1580,10 @@ int main (int argc, char *argv[])
Quoted 7 lines
e2fsck/unix.c int main (int argc, char *argv[]) · line 1584
Quoted 4 lines
e2fsck/unix.c +2
-0check_mount (ctx); +#ifdef __linux__ + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name);If the filesystem has an external journal device, doesn't that also need locking?
Good point, and no, it doesn't currently - lock_whole_disk() is only called on ctx->filesystem_name. An external journal device (-O journal_dev) is a separate block device with its own by-uuid symlinks, and I don't have a strong argument that it's immune to the same class of race during replay. I'd like to treat this as a follow-up rather than block on it, since it needs a bit more digging into whether the external journal's own superblock write pattern actually creates the same transient-bad-checksum window - but I don't want to just wave it away either.
Quoted 2 lines
Why isn't this implemented as part of the unixio manager?
Agreed this is architecturally the more correct home for it - mke2fs, tune2fs, and resize2fs go through the same unix_io manager and do similar multi-step writes to live block devices, so they're exposed to the same race and get none of this protection today. I scoped this patch to e2fsck specifically because that's the most common/severe case in practice and wanted a narrow, reviewable first fix rather than a bigger refactor touching every tool at once. I can look at moving this into unix_io_open()/unix_io_close() as a follow-up so mke2fs/tune2fs/resize2fs pick it up automatically via ext2fs_open()/close()
Quoted 24 lines
--D
Quoted 21 lines
e2fsck/unix.c +7
-0+#endif +if (!(ctx->options &E2F_OPT_PREEN ) && !(ctx->options &E2F_OPT_NO ) && !(ctx->options &E2F_OPT_YES )) {@@ -2169,6 +2265,11 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name); +#ifdef __linux__ +if (lock_fd >=0 ) +close (lock_fd); +#endif +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);Signature · 1 line
2.54.0
Quoted 35 lines · Nandakumar Raghavan
Quoted 26 lines · Darrick J. Wong
Quoted 17 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.Quoted 7 lines
The whole-disk device is resolved via sysfs (/sys/dev/block/MAJ:MIN/partition) so that the lock covers the same device node that udevd locks. If sysfs resolution fails, a warning is emitted and the lock falls back to the partition device itself.
This mechanism relies on Linux-specific interfaces (sysfs,
flock()semantics on block devices) and is compiled out entirely on non-Linux platforms.Quoted 7 lines
You're clearly implementing the "Locking Block Device Access" protocol as documented by systemd:
https://systemd.io/BLOCK_DEVICE_LOCKING/So why not use libsystemd like the example in that posting provides?
sd_device_get_parent_with_subsystem_devtypeis much less gross than open-coding the same logic here.
Quoted 10 lines
This was on purpose and not something I missed. The
sd_device_get_parent_with_subsystem_devtype
requires libsystemd v251+, and e2fsck needs to keep working in contexts where
that dependency isn't available or isn't guaranteed to be new enough -
non-systemd distros, older LTS systems doing root-fs repair before userspace
is fully up, embedded/recovery images, etc.
Taking a hard runtime dependency on a versioned libsystemd API for a tool
this fundamental felt like a much bigger cost than by parsing (/sys/dev/block/MAJ:MIN/partition, .../dev), which only relies on a stable kernel ABI with no library and no version floor. Happy to reconsider if
the project wants to take the dependency, but wanted to flag the tradeoff
explicitly rather than silently pick one side.
I for one am not eager to have e2fsprogs depend on libsystemd to build.
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
Signed-off-by Nandakumar Raghavan <[email protected]>---
(quoted diff) +0
-0@@ -1488,6 +1580,10 @@ int main (int argc, char *argv[])Quoted 7 lines
e2fsck/unix.c int main (int argc, char *argv[]) · line 1584
Quoted 4 lines
e2fsck/unix.c +2
-0check_mount (ctx); +#ifdef __linux__ + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name);If the filesystem has an external journal device, doesn't that also need locking?
Quoted 8 lines
Good point, and no, it doesn't currently -
lock_whole_disk()is only called onctx->filesystem_name. An external journal device (-Ojournal_dev) is a separate block device with its own by-uuid symlinks, and I don't have a strong argument that it's immune to the same class of race during replay. I'd like to treat this as a follow-up rather than block on it, since it needs a bit more digging into whether the external journal's own superblock write pattern actually creates the same transient-bad-checksum window - but I don't want to just wave it away either.
At one point I was working on having jbd2 "mount" the journal device, and then ext4 would connect to the running journal as a "service", rather than having the journal device be mounted exclusively as subpart of the ext4 filesystem. I might have even posted patches for this to the list (maybe 15 years ago?)
This was intended to solve a number of issues:
- ensuring that the jbd2 block device was visibly "in use" to userspace
- allow a single jbd2 device (e.g. on NVMe) to be shared among multiple ext4 filesystems (on HDDs)
That shared device would have been much more useful when HDDs were prevalent and an NVMe device was harder to share (e.g. before LVM/DM). It had some other issues like what to do if there are journaled blocks to recover but one of the using filesystems does not mount and recover them? Maybe copy those blocks to the end of the journal and shrink the usable journal size?
It still has some appeal from the visibility POV, and for sharing it avoids stranding dedicated NVMe journal space on one inactive filesystem when other busy filesystems could be sharing the space.
Cheers, Andreas
Quoted 88 lines · Andreas Dilger
Quoted 35 lines · Nandakumar Raghavan
Quoted 26 lines · Darrick J. Wong
Quoted 17 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.Quoted 7 lines
The whole-disk device is resolved via sysfs (/sys/dev/block/MAJ:MIN/partition) so that the lock covers the same device node that udevd locks. If sysfs resolution fails, a warning is emitted and the lock falls back to the partition device itself.
This mechanism relies on Linux-specific interfaces (sysfs,
flock()semantics on block devices) and is compiled out entirely on non-Linux platforms.Quoted 7 lines
You're clearly implementing the "Locking Block Device Access" protocol as documented by systemd:
https://systemd.io/BLOCK_DEVICE_LOCKING/So why not use libsystemd like the example in that posting provides?
sd_device_get_parent_with_subsystem_devtypeis much less gross than open-coding the same logic here.Quoted 10 lines
This was on purpose and not something I missed. The
sd_device_get_parent_with_subsystem_devtype
requires libsystemd v251+, and e2fsck needs to keep working in contexts where
that dependency isn't available or isn't guaranteed to be new enough -
non-systemd distros, older LTS systems doing root-fs repair before userspace
is fully up, embedded/recovery images, etc.
Taking a hard runtime dependency on a versioned libsystemd API for a tool
this fundamental felt like a much bigger cost than by parsing (/sys/dev/block/MAJ:MIN/partition, .../dev), which only relies on a stable kernel ABI with no library and no version floor. Happy to reconsider if
the project wants to take the dependency, but wanted to flag the tradeoff
explicitly rather than silently pick one side.I for one am not eager to have e2fsprogs depend on libsystemd to build.
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
Signed-off-by Nandakumar Raghavan <[email protected]>---
(quoted diff) +0
-0@@ -1488,6 +1580,10 @@ int main (int argc, char *argv[])Quoted 7 lines
e2fsck/unix.c int main (int argc, char *argv[]) · line 1584
Quoted 4 lines
e2fsck/unix.c +2
-0check_mount (ctx); +#ifdef __linux__ + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name);If the filesystem has an external journal device, doesn't that also need locking?
Quoted 8 lines
Good point, and no, it doesn't currently -
lock_whole_disk()is only called onctx->filesystem_name. An external journal device (-Ojournal_dev) is a separate block device with its own by-uuid symlinks, and I don't have a strong argument that it's immune to the same class of race during replay. I'd like to treat this as a follow-up rather than block on it, since it needs a bit more digging into whether the external journal's own superblock write pattern actually creates the same transient-bad-checksum window - but I don't want to just wave it away either.At one point I was working on having jbd2 "mount" the journal device, and then ext4 would connect to the running journal as a "service", rather than having the journal device be mounted exclusively as subpart of the ext4 filesystem. I might have even posted patches for this to the list (maybe 15 years ago?)
This was intended to solve a number of issues:
- ensuring that the jbd2 block device was visibly "in use" to userspace
- allow a single jbd2 device (e.g. on NVMe) to be shared among multiple ext4 filesystems (on HDDs)That shared device would have been much more useful when HDDs were prevalent and an NVMe device was harder to share (e.g. before LVM/DM). It had some other issues like what to do if there are journaled blocks to recover but one of the using filesystems does not mount and recover them? Maybe copy those blocks to the end of the journal and shrink the usable journal size?
Quoted 5 lines
It still has some appeal from the visibility POV, and for sharing it avoids stranding dedicated NVMe journal space on one inactive filesystem when other busy filesystems could be sharing the space.
Cheers, Andreas
Thanks for the history, Andreas and I can see how it would solve the visibility problem more fundamentally than one-off locking external journal devices case by case.
That said, it sounds like a much bigger architectural change than what I'm trying to do here. I'll keep it in mind as background, but for this patch I'd still like to treat "should the external journal device also get locked" as a narrower, separate follow-up rather than pull in a jbd2 redesign.
Quoted 4 lines
Quoted 4 lines · Nandakumar Raghavan
This was on purpose and not something I missed. The
sd_device_get_parent_with_subsystem_devtyperequires libsystemd v251+, and e2fsck needs to keep working in contexts where that dependency isn't available or isn't guaranteed to be new enough - non-systemd distros, older LTS systems doing root-fs repair before userspace is fully up, embedded/recovery images, etc.
Cqn you name any other OS that has implemented
https://systemd.io/BLOCK_DEVICE_LOCKING/
*other* than systemd? So for non-systemd distros, who cares if we don't have the functionality? So what we could do is to use autoconf with something like this:
AC_CHECK_LIB(libsystemd, sd_device_get_parent_with_subsystem_devtype,
AC_DEFINE(HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE, 1,
[Define to 1 if libsystemd has sd_device_get_parent_with_subsystem_devtype]))
We would only do the flock(LOCK_EX) if and only if the function is present. If it isn't, say on MacOS, FreeBSD, or RHEL 7 we won't play the systemd locking game --- which is fine, because it doesn't matter on those platforms.
If you care about running e2fsck built on RHEL 8 on RHEL 7, that's an easy problem to solve. A executable linked using shared libraries won't work already since the shared libraries on RHEL 8 are too new. But we have a solution already, which is to use e2fsck.static, which is statically linked.
Quoted 5 lines
Taking a hard runtime dependency on a versioned libsystemd API for a tool this fundamental felt like a much bigger cost than by parsing (/sys/dev/block/MAJ:MIN/partition, .../dev), which only relies on a stable kernel ABI with no library and no version floor. Happy to reconsider if the project wants to take the dependency, but wanted to flag the tradeoff explicitly rather than silently pick one side.
If we really cared about taking a binary built on a system with libsystemd. and running on a system without libsystemd, another solution that we've used is dlopen.
For an example of that's done, see misc/create_inode_libarchve.c and the associated autoconf tests in configure.ac in the e2fsprogs sources. This is how "mke2fs -d embed_rootfs.tar.gz -t ext4 embed.img 1G" is handled without needing a hard dependency on libarchive being installed. If it's not installed then you'll just get the error message "you need libarchive to be able to process tarballs".
I don't think the dlopen() approach is needed here, though.
Cheers,
- Ted
Quoted 46 lines · Theodore Tso
Quoted 4 lines · Nandakumar Raghavan
This was on purpose and not something I missed. The
sd_device_get_parent_with_subsystem_devtyperequires libsystemd v251+, and e2fsck needs to keep working in contexts where that dependency isn't available or isn't guaranteed to be new enough - non-systemd distros, older LTS systems doing root-fs repair before userspace is fully up, embedded/recovery images, etc.Cqn you name any other OS that has implemented
https://systemd.io/BLOCK_DEVICE_LOCKING/
*other* than systemd? So for non-systemd distros, who cares if we don't have the functionality? So what we could do is to use autoconf with something like this:
AC_CHECK_LIB(libsystemd,
sd_device_get_parent_with_subsystem_devtype,
AC_DEFINE(HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE, 1,
[Define to 1 if libsystemd hassd_device_get_parent_with_subsystem_devtype]))We would only do the
flock(LOCK_EX)if and only if the function is present. If it isn't, say on MacOS, FreeBSD, or RHEL 7 we won't play the systemd locking game --- which is fine, because it doesn't matter on those platforms.If you care about running e2fsck built on RHEL 8 on RHEL 7, that's an easy problem to solve. A executable linked using shared libraries won't work already since the shared libraries on RHEL 8 are too new. But we have a solution already, which is to use e2fsck.static, which is statically linked.
Quoted 5 lines
Taking a hard runtime dependency on a versioned libsystemd API for a tool this fundamental felt like a much bigger cost than by parsing (/sys/dev/block/MAJ:MIN/partition, .../dev), which only relies on a stable kernel ABI with no library and no version floor. Happy to reconsider if the project wants to take the dependency, but wanted to flag the tradeoff explicitly rather than silently pick one side.
If we really cared about taking a binary built on a system with libsystemd. and running on a system without libsystemd, another solution that we've used is dlopen.
For an example of that's done, see
misc/create_inode_libarchve.cand the associated autoconf tests in configure.ac in the e2fsprogs sources. This is how "mke2fs -d embed_rootfs.tar.gz -t ext4 embed.img 1G" is handled without needing a hard dependency on libarchive being installed. If it's not installed then you'll just get the error message "you need libarchive to be able to process tarballs".
Quoted 5 lines
I don't think the
dlopen()approach is needed here, though.Cheers,
- Ted
Thank you, Ted for the comments. I agree on all points. I will rework this to use AC_CHECK_LIB as you described, switch to the sd-device API when libsystemd is present, and
drop the whole locking feature cleanly when it isn't.
Patch v3 to follow.
PATCH v3 1/1 e2fsck: take flock(LOCK_EX) on whole-disk device during filesystem check
In reply to Theodore TsoDuring journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in /dev/disk/by-label/ and /dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.
udevd already serialises its own partition probes against whole-disk device access using flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiring flock(LOCK_EX) on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.
The whole-disk device is resolved using libsystemd's sd-device API (sd_device_new_from_devnum(), sd_device_get_parent_with_subsystem_devtype()) rather than parsing sysfs, so the lock covers the same device node that udevd itself resolves and locks.
This locking is entirely optional at build time where configure checks for sd_device_get_parent_with_subsystem_devtype() via AC_CHECK_LIB, and e2fsck.c only compiles in the locking code when it is found. On platforms without libsystemd there is no udevd to race against in the first place, so nothing is lost by skipping it there.
---
Changes in v3:
- Resolve the parent whole-disk device using libsystemd's sd-device API (sd_device_new_from_devnum(), sd_device_get_parent_with_subsystem_devtype(), sd_device_get_devname()) instead of parsing sysfs paths
- Gate the feature on autoconf detecting sd_device_get_parent_with_subsystem_devtype() via AC_CHECK_LIB; compile it out entirely when libsystemd is absent, rather than falling back to another implementation
- Use sd_device_new_from_devnum() rather than the sd_device_new_from_devname() used by the systemd.io example, since the former has been available in libsystemd for much longer, keeping the version floor as low as possible
- Keep lock_whole_disk()'s signature unconditional and provide a stub returning -1 on the #else branch, so main()'s primary code path has no #ifdefs in it
Changes in v2:
- Guard the whole feature (function, call site, lock_fd, cleanup) rather than just the function body, so it is fully absent from non-Linux builds
- Collapse the three HAVE_SYS_*_H header guards into a single conditional
- Reuse a single dev_path[] buffer instead of three single-use stack buffers
- Keep translatable warning strings on one line each, even past 80 columns
- Drop the blanket "could not lock" warning that fired even when the target was not a block device at all (e.g. a regular file), which broke most of "make check"; lock_whole_disk() now only warns at the point of a genuine failure
- No change needed for the Ctrl-C/EINTR concern: the SIGINT handler is installed before lock_whole_disk() runs and is not SA_RESTART, so a blocked flock() is interrupted and the existing E2F_FLAG_CANCEL check unwinds cleanly
MCONFIG.in | 1 + configure | 71 +++++++++++++++++++++++++++++++++++ configure.ac | 14 +++++++ e2fsck/Makefile.in | 6 +-- e2fsck/unix.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++ lib/config.h.in | 6 +++ 6 files changed, 188 insertions(+), 3 deletions(-)
MCONFIG.in +1 -0
@@ -137,6 +137,7 @@ LIBE2P = $(LIB)/libe2p@LIB_EXT@ LIBEXT2FS = $(LIB)/libext2fs@LIB_EXT@ LIBUUID = @LIBUUID@ @SOCKET_LIB@ LIBMAGIC = @MAGIC_LIB@ +LIBSYSTEMD = @SYSTEMD_LIB@ LIBFUSE = @FUSE_LIB@ LIBSUPPORT = $(LIBINTL) $(LIB)/libsupport@STATIC_LIB_EXT@ LIBBLKID = @LIBBLKID@ @PRIVATE_LIBS_CMT@ $(LIBUUID)
configure +71 -0
83 lines not on this page · raw message
configure.ac +14 -0
@@ -1309,6 +1309,20 @@ if test "$ac_cv_func_dlopen" = yes ; then fi AC_SUBST(MAGIC_LIB) dnl +dnl See if libsystemd has sd-device APIs for whole-disk BSD locking +dnl +SYSTEMD_LIB= +AC_CHECK_LIB(systemd, sd_device_get_parent_with_subsystem_devtype, + [SYSTEMD_LIB=-lsystemd]) +if test -n "$SYSTEMD_LIB"; then + AC_CHECK_HEADERS([systemd/sd-device.h], , [SYSTEMD_LIB=]) +fi +if test -n "$SYSTEMD_LIB"; then + AC_DEFINE(HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE, 1, + [Define to 1 if libsystemd has sd-device APIs for whole-disk locking]) +fi +AC_SUBST(SYSTEMD_LIB) +dnl dnl libarchive dnl AC_ARG_WITH([libarchive],
e2fsck/Makefile.in +3 -3
@@ -17,20 +17,20 @@ MANPAGES= e2fsck.8 FMANPAGES= e2fsck.conf.5 LIBS= $(LIBSUPPORT) $(LIBEXT2FS) $(LIBCOM_ERR) $(LIBBLKID) $(LIBUUID) \ - $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(SYSLIBS) + $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) DEPLIBS= $(DEPLIBSUPPORT) $(LIBEXT2FS) $(DEPLIBCOM_ERR) $(DEPLIBBLKID) \ $(DEPLIBUUID) $(DEPLIBE2P) STATIC_LIBS= $(STATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) $(STATIC_LIBCOM_ERR) \ $(STATIC_LIBBLKID) $(STATIC_LIBUUID) $(LIBINTL) $(STATIC_LIBE2P) \ - $(LIBMAGIC) $(SYSLIBS) + $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) STATIC_DEPLIBS= $(DEPSTATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) \ $(DEPSTATIC_LIBCOM_ERR) $(DEPSTATIC_LIBBLKID) \ $(DEPSTATIC_LIBUUID) $(DEPSTATIC_LIBE2P) PROFILED_LIBS= $(PROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(PROFILED_LIBCOM_ERR) $(PROFILED_LIBBLKID) $(PROFILED_LIBUUID) \ - $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(SYSLIBS) + $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) PROFILED_DEPLIBS= $(DEPPROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(DEPPROFILED_LIBCOM_ERR) $(DEPPROFILED_LIBBLKID) \ $(DEPPROFILED_LIBUUID) $(DEPPROFILED_LIBE2P)
e2fsck/unix.c +93 -0
123 lines not on this page · raw message
lib/config.h.in +6 -0
@@ -352,6 +352,9 @@ /* Define to 1 if if struct sockaddr contains sa_len */ #undef HAVE_SA_LEN +/* Define to 1 if libsystemd has sd-device APIs for whole-disk locking */ +#undef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE + /* Define to 1 if you have the 'secure_getenv' function. */ #undef HAVE_SECURE_GETENV
@@ -430,6 +433,9 @@ /* Define to 1 if you have the 'sysconf' function. */ #undef HAVE_SYSCONF +/* Define to 1 if you have the <systemd/sd-device.h> header file. */ +#undef HAVE_SYSTEMD_SD_DEVICE_H + /* Define to 1 if you have the <sys/acl.h> header file. */ #undef HAVE_SYS_ACL_H
Signature · 1 line
2.55.0
Quoted 378 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.The whole-disk device is resolved using libsystemd's sd-device API (
sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype()) rather than parsing sysfs, so the lock covers the same device node that udevd itself resolves and locks.This locking is entirely optional at build time where configure checks for
sd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB, ande2fsck.conly compiles in the locking code when it is found. On platforms without libsystemd there is no udevd to race against in the first place, so nothing is lost by skipping it there.Signed-off-by Nandakumar Raghavan <[email protected]>---
Changes in v3:
- Resolve the parent whole-disk device using libsystemd's sd-device API (sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype(),sd_device_get_devname()) instead of parsing sysfs paths
- Gate the feature on autoconf detectingsd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB; compile it out entirely when libsystemd is absent, rather than falling back to another implementation
- Usesd_device_new_from_devnum()rather than thesd_device_new_from_devname()used by the systemd.io example, since the former has been available in libsystemd for much longer, keeping the version floor as low as possible
- Keep lock_whole_disk()'s signature unconditional and provide a stub returning -1 on the #else branch, so main()'s primary code path has no #ifdefs in itChanges in v2:
- Guard the whole feature (function, call site,lock_fd, cleanup) rather than just the function body, so it is fully absent from non-Linux builds
- Collapse the three HAVE_SYS_*_H header guards into a single conditional
- Reuse a single dev_path[] buffer instead of three single-use stack buffers
- Keep translatable warning strings on one line each, even past 80 columns
- Drop the blanket "could not lock" warning that fired even when the target was not a block device at all (e.g. a regular file), which broke most of "make check";lock_whole_disk()now only warns at the point of a genuine failure
- No change needed for the Ctrl-C/EINTR concern: the SIGINT handler is installed beforelock_whole_disk()runs and is notSA_RESTART, so a blockedflock()is interrupted and the existingE2F_FLAG_CANCELcheck unwinds cleanlyMCONFIG.in | 1 + configure | 71 +++++++++++++++++++++++++++++++++++ configure.ac | 14 +++++++ e2fsck/Makefile.in | 6 +-- e2fsck/unix.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++ lib/config.h.in | 6 +++ 6 files changed, 188 insertions(+), 3 deletions(-)MCONFIG.in +1
-0@@ -137,6 +137,7 @@ LIBE2P = $(LIB)/libe2p@LIB_EXT@ LIBEXT2FS = $(LIB)/libext2fs@LIB_EXT@ LIBUUID = @LIBUUID@ @SOCKET_LIB@ LIBMAGIC = @MAGIC_LIB@ +LIBSYSTEMD = @SYSTEMD_LIB@ LIBFUSE = @FUSE_LIB@ LIBSUPPORT = $(LIBINTL) $(LIB)/libsupport@STATIC_LIB_EXT@ LIBBLKID = @LIBBLKID@ @PRIVATE_LIBS_CMT@ $(LIBUUID)configure +71
-0@@ -702,6 +702,7 @@ fuse3_CFLAGS CLOCK_GETTIME_LIB ARCHIVE_LIBS ARCHIVE_CFLAGS +SYSTEMD_LIB MAGIC_LIB SOCKET_LIB SIZEOF_TIME_T@@ -14130,6 +14131,76 @@ if test "$ac_cv_func_dlopen" = yes ; then MAGIC_LIB=$DLOPEN_LIB fi +SYSTEMD_LIB= +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd" >&5 +printf %s "checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd... " >&6; } +if test ${ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype+y} +then : + printf %s "(cached) " >&6 +else case e in #( + e) ac_check_lib_save_LIBS=$LIBS +LIBS="-lsystemd $LIBS" +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ + +/* Override any GCC internal prototype to avoid an error. + Use char because int might match the return type of a GCC + builtin and then its argument prototype would still apply. + The 'extern "C"' is for builds by C++ compilers; + although this is not generally supported in C code supporting it here + has little cost and some practical benefit (sr 110532). */ +#ifdef __cplusplus +extern "C" +#endif +char sd_device_get_parent_with_subsystem_devtype (void); +int +main (void) +{ +return sd_device_get_parent_with_subsystem_devtype (); + ; + return 0; +} +_ACEOF +if ac_fn_c_try_link "$LINENO" +then : + ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=yes +else case e in #( + e) ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=no ;; +esac +fi +rm -f core conftest.err conftest.$ac_objext conftest.beam \ + conftest$ac_exeext conftest.$ac_ext +LIBS=$ac_check_lib_save_LIBS ;; +esac +fi +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&5 +printf "%s\n" "$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&6; } +if test "x$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" = xyes +then : + SYSTEMD_LIB=-lsystemd +fi + +if test -n "$SYSTEMD_LIB"; then + for ac_header in systemd/sd-device.h +do : + ac_fn_c_check_header_compile "$LINENO" "systemd/sd-device.h" "ac_cv_header_systemd_sd_device_h" "$ac_includes_default" +if test "x$ac_cv_header_systemd_sd_device_h" = xyes +then : + printf "%s\n" "#define HAVE_SYSTEMD_SD_DEVICE_H 1" >>confdefs.h + +else case e in #( + e) SYSTEMD_LIB= ;; +esac +fi + +done +fi +if test -n "$SYSTEMD_LIB"; then + +printf "%s\n" "#define HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE 1" >>confdefs.h + +fi + # Check whether --with-libarchive was given. if test ${with_libarchive+y}configure.ac +14
-0@@ -1309,6 +1309,20 @@ if test "$ac_cv_func_dlopen" = yes ; then fi AC_SUBST(MAGIC_LIB) dnl +dnl See if libsystemd has sd-device APIs for whole-disk BSD locking +dnl +SYSTEMD_LIB= +AC_CHECK_LIB(systemd, sd_device_get_parent_with_subsystem_devtype, + [SYSTEMD_LIB=-lsystemd]) +if test -n "$SYSTEMD_LIB"; then + AC_CHECK_HEADERS([systemd/sd-device.h], , [SYSTEMD_LIB=]) +fi +if test -n "$SYSTEMD_LIB"; then + AC_DEFINE(HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE, 1, + [Define to 1 if libsystemd has sd-device APIs for whole-disk locking]) +fi +AC_SUBST(SYSTEMD_LIB) +dnl dnl libarchive dnl AC_ARG_WITH([libarchive],e2fsck/Makefile.in +3
-3@@ -17,20 +17,20 @@ MANPAGES= e2fsck.8 FMANPAGES= e2fsck.conf.5 LIBS= $(LIBSUPPORT) $(LIBEXT2FS) $(LIBCOM_ERR) $(LIBBLKID) $(LIBUUID) \ - $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(SYSLIBS) + $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) DEPLIBS= $(DEPLIBSUPPORT) $(LIBEXT2FS) $(DEPLIBCOM_ERR) $(DEPLIBBLKID) \ $(DEPLIBUUID) $(DEPLIBE2P) STATIC_LIBS= $(STATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) $(STATIC_LIBCOM_ERR) \ $(STATIC_LIBBLKID) $(STATIC_LIBUUID) $(LIBINTL) $(STATIC_LIBE2P) \ - $(LIBMAGIC) $(SYSLIBS) + $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) STATIC_DEPLIBS= $(DEPSTATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) \ $(DEPSTATIC_LIBCOM_ERR) $(DEPSTATIC_LIBBLKID) \ $(DEPSTATIC_LIBUUID) $(DEPSTATIC_LIBE2P) PROFILED_LIBS= $(PROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(PROFILED_LIBCOM_ERR) $(PROFILED_LIBBLKID) $(PROFILED_LIBUUID) \ - $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(SYSLIBS) + $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) PROFILED_DEPLIBS= $(DEPPROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(DEPPROFILED_LIBCOM_ERR) $(DEPPROFILED_LIBBLKID) \ $(DEPPROFILED_LIBUUID) $(DEPPROFILED_LIBE2P)e2fsck/unix.c +93
-0@@ -36,6 +36,11 @@ extern int optind;#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +#include <sys/file.h> +#include <sys/stat.h> +#include <systemd/sd-device.h> +#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif @@ -1397,6 +1402,88 @@ err:return retval; } +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name) +{ +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +struct stat st; +sd_device *dev =NULL , *whole_dev; +const char *subsystem, *devtype, *whole_disk_devname; +int r, fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + r =sd_device_new_from_devnum (&dev,'b' , st.st_rdev); +if (r <0 ) +return -1 ; + + r =sd_device_get_subsystem (dev, &subsystem); +if (r <0 ||strcmp (subsystem,"block" ) !=0 ) { +sd_device_unref (dev); +return -1 ; + } + + r =sd_device_get_devtype (dev, &devtype); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + +/* +* If dev_name is a partition, resolve the parent whole-disk device +* so the flock covers the same device node that udevd locks before +* probing any partition on it. +*/ +if (strcmp (devtype,"disk" ) ==0 ) + whole_dev = dev; +else { + r =sd_device_get_parent_with_subsystem_devtype (dev,"block" , +"disk" , &whole_dev); +if (r <0 ) { +log_err (ctx,_ ("Warning: could not resolve whole-disk device for %s; lock may not prevent udev races\n" ), dev_name); +sd_device_unref (dev); +return -1 ; + } + } + + r =sd_device_get_devname (whole_dev, &whole_disk_devname); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + + fd =open (whole_disk_devname,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { +com_err (ctx->program_name, errno, +_ ("while trying to open %s for locking" ), whole_disk_devname); +sd_device_unref (dev); +return -1 ; + } +sd_device_unref (dev); + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ; + } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), whole_disk_devname); +close (fd); +return -1 ; + } + +return fd; +#else +return -1 ; +#endif /* HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE */ +} +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;@@ -1413,6 +1500,7 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +int lock_fd =-1 ;__u32 features[3 ];char *cp;enum quota_type qtype;@@ -1488,6 +1576,8 @@ int main (int argc, char *argv[])check_mount (ctx); + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name); +if (!(ctx->options &E2F_OPT_PREEN ) && !(ctx->options &E2F_OPT_NO ) && !(ctx->options &E2F_OPT_YES )) {@@ -2169,6 +2259,9 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name); +if (lock_fd >=0 ) +close (lock_fd); +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);lib/config.h.in +6
-0@@ -352,6 +352,9 @@ /* Define to 1 if if struct sockaddr contains sa_len */ #undef HAVE_SA_LEN +/* Define to 1 if libsystemd has sd-device APIs for whole-disk locking */ +#undef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE + /* Define to 1 if you have the 'secure_getenv' function. */ #undef HAVE_SECURE_GETENV@@ -430,6 +433,9 @@ /* Define to 1 if you have the 'sysconf' function. */ #undef HAVE_SYSCONF +/* Define to 1 if you have the <systemd/sd-device.h> header file. */ +#undef HAVE_SYSTEMD_SD_DEVICE_H + /* Define to 1 if you have the <sys/acl.h> header file. */ #undef HAVE_SYS_ACL_HSignature · 1 line
2.55.0
Hi,
Gentle ping on this patch.
I would appreciate any feedback.
Quoted 377 lines · Nandakumar Raghavan
Quoted 376 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.The whole-disk device is resolved using libsystemd's sd-device API (
sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype()) rather than parsing sysfs, so the lock covers the same device node that udevd itself resolves and locks.This locking is entirely optional at build time where configure checks for
sd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB, ande2fsck.conly compiles in the locking code when it is found. On platforms without libsystemd there is no udevd to race against in the first place, so nothing is lost by skipping it there.Signed-off-by Nandakumar Raghavan <[email protected]>---
Changes in v3:
- Resolve the parent whole-disk device using libsystemd's sd-device API (sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype(),sd_device_get_devname()) instead of parsing sysfs paths
- Gate the feature on autoconf detectingsd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB; compile it out entirely when libsystemd is absent, rather than falling back to another implementation
- Usesd_device_new_from_devnum()rather than thesd_device_new_from_devname()used by the systemd.io example, since the former has been available in libsystemd for much longer, keeping the version floor as low as possible
- Keep lock_whole_disk()'s signature unconditional and provide a stub returning -1 on the #else branch, so main()'s primary code path has no #ifdefs in itChanges in v2:
- Guard the whole feature (function, call site,lock_fd, cleanup) rather than just the function body, so it is fully absent from non-Linux builds
- Collapse the three HAVE_SYS_*_H header guards into a single conditional
- Reuse a single dev_path[] buffer instead of three single-use stack buffers
- Keep translatable warning strings on one line each, even past 80 columns
- Drop the blanket "could not lock" warning that fired even when the target was not a block device at all (e.g. a regular file), which broke most of "make check";lock_whole_disk()now only warns at the point of a genuine failure
- No change needed for the Ctrl-C/EINTR concern: the SIGINT handler is installed beforelock_whole_disk()runs and is notSA_RESTART, so a blockedflock()is interrupted and the existingE2F_FLAG_CANCELcheck unwinds cleanlyMCONFIG.in | 1 + configure | 71 +++++++++++++++++++++++++++++++++++ configure.ac | 14 +++++++ e2fsck/Makefile.in | 6 +-- e2fsck/unix.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++ lib/config.h.in | 6 +++ 6 files changed, 188 insertions(+), 3 deletions(-)MCONFIG.in +1
-0@@ -137,6 +137,7 @@ LIBE2P = $(LIB)/libe2p@LIB_EXT@ LIBEXT2FS = $(LIB)/libext2fs@LIB_EXT@ LIBUUID = @LIBUUID@ @SOCKET_LIB@ LIBMAGIC = @MAGIC_LIB@ +LIBSYSTEMD = @SYSTEMD_LIB@ LIBFUSE = @FUSE_LIB@ LIBSUPPORT = $(LIBINTL) $(LIB)/libsupport@STATIC_LIB_EXT@ LIBBLKID = @LIBBLKID@ @PRIVATE_LIBS_CMT@ $(LIBUUID)configure +71
-0@@ -702,6 +702,7 @@ fuse3_CFLAGS CLOCK_GETTIME_LIB ARCHIVE_LIBS ARCHIVE_CFLAGS +SYSTEMD_LIB MAGIC_LIB SOCKET_LIB SIZEOF_TIME_T@@ -14130,6 +14131,76 @@ if test "$ac_cv_func_dlopen" = yes ; then MAGIC_LIB=$DLOPEN_LIB fi +SYSTEMD_LIB= +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd" >&5 +printf %s "checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd... " >&6; } +if test ${ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype+y} +then : + printf %s "(cached) " >&6 +else case e in #( + e) ac_check_lib_save_LIBS=$LIBS +LIBS="-lsystemd $LIBS" +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ + +/* Override any GCC internal prototype to avoid an error. + Use char because int might match the return type of a GCC + builtin and then its argument prototype would still apply. + The 'extern "C"' is for builds by C++ compilers; + although this is not generally supported in C code supporting it here + has little cost and some practical benefit (sr 110532). */ +#ifdef __cplusplus +extern "C" +#endif +char sd_device_get_parent_with_subsystem_devtype (void); +int +main (void) +{ +return sd_device_get_parent_with_subsystem_devtype (); + ; + return 0; +} +_ACEOF +if ac_fn_c_try_link "$LINENO" +then : + ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=yes +else case e in #( + e) ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=no ;; +esac +fi +rm -f core conftest.err conftest.$ac_objext conftest.beam \ + conftest$ac_exeext conftest.$ac_ext +LIBS=$ac_check_lib_save_LIBS ;; +esac +fi +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&5 +printf "%s\n" "$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&6; } +if test "x$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" = xyes +then : + SYSTEMD_LIB=-lsystemd +fi + +if test -n "$SYSTEMD_LIB"; then + for ac_header in systemd/sd-device.h +do : + ac_fn_c_check_header_compile "$LINENO" "systemd/sd-device.h" "ac_cv_header_systemd_sd_device_h" "$ac_includes_default" +if test "x$ac_cv_header_systemd_sd_device_h" = xyes +then : + printf "%s\n" "#define HAVE_SYSTEMD_SD_DEVICE_H 1" >>confdefs.h + +else case e in #( + e) SYSTEMD_LIB= ;; +esac +fi + +done +fi +if test -n "$SYSTEMD_LIB"; then + +printf "%s\n" "#define HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE 1" >>confdefs.h + +fi + # Check whether --with-libarchive was given. if test ${with_libarchive+y}configure.ac +14
-0@@ -1309,6 +1309,20 @@ if test "$ac_cv_func_dlopen" = yes ; then fi AC_SUBST(MAGIC_LIB) dnl +dnl See if libsystemd has sd-device APIs for whole-disk BSD locking +dnl +SYSTEMD_LIB= +AC_CHECK_LIB(systemd, sd_device_get_parent_with_subsystem_devtype, + [SYSTEMD_LIB=-lsystemd]) +if test -n "$SYSTEMD_LIB"; then + AC_CHECK_HEADERS([systemd/sd-device.h], , [SYSTEMD_LIB=]) +fi +if test -n "$SYSTEMD_LIB"; then + AC_DEFINE(HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE, 1, + [Define to 1 if libsystemd has sd-device APIs for whole-disk locking]) +fi +AC_SUBST(SYSTEMD_LIB) +dnl dnl libarchive dnl AC_ARG_WITH([libarchive],e2fsck/Makefile.in +3
-3@@ -17,20 +17,20 @@ MANPAGES= e2fsck.8 FMANPAGES= e2fsck.conf.5 LIBS= $(LIBSUPPORT) $(LIBEXT2FS) $(LIBCOM_ERR) $(LIBBLKID) $(LIBUUID) \ - $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(SYSLIBS) + $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) DEPLIBS= $(DEPLIBSUPPORT) $(LIBEXT2FS) $(DEPLIBCOM_ERR) $(DEPLIBBLKID) \ $(DEPLIBUUID) $(DEPLIBE2P) STATIC_LIBS= $(STATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) $(STATIC_LIBCOM_ERR) \ $(STATIC_LIBBLKID) $(STATIC_LIBUUID) $(LIBINTL) $(STATIC_LIBE2P) \ - $(LIBMAGIC) $(SYSLIBS) + $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) STATIC_DEPLIBS= $(DEPSTATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) \ $(DEPSTATIC_LIBCOM_ERR) $(DEPSTATIC_LIBBLKID) \ $(DEPSTATIC_LIBUUID) $(DEPSTATIC_LIBE2P) PROFILED_LIBS= $(PROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(PROFILED_LIBCOM_ERR) $(PROFILED_LIBBLKID) $(PROFILED_LIBUUID) \ - $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(SYSLIBS) + $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) PROFILED_DEPLIBS= $(DEPPROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(DEPPROFILED_LIBCOM_ERR) $(DEPPROFILED_LIBBLKID) \ $(DEPPROFILED_LIBUUID) $(DEPPROFILED_LIBE2P)e2fsck/unix.c +93
-0@@ -36,6 +36,11 @@ extern int optind;#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +#include <sys/file.h> +#include <sys/stat.h> +#include <systemd/sd-device.h> +#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif @@ -1397,6 +1402,88 @@ err:return retval; } +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name) +{ +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +struct stat st; +sd_device *dev =NULL , *whole_dev; +const char *subsystem, *devtype, *whole_disk_devname; +int r, fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + r =sd_device_new_from_devnum (&dev,'b' , st.st_rdev); +if (r <0 ) +return -1 ; + + r =sd_device_get_subsystem (dev, &subsystem); +if (r <0 ||strcmp (subsystem,"block" ) !=0 ) { +sd_device_unref (dev); +return -1 ; + } + + r =sd_device_get_devtype (dev, &devtype); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + +/* +* If dev_name is a partition, resolve the parent whole-disk device +* so the flock covers the same device node that udevd locks before +* probing any partition on it. +*/ +if (strcmp (devtype,"disk" ) ==0 ) + whole_dev = dev; +else { + r =sd_device_get_parent_with_subsystem_devtype (dev,"block" , +"disk" , &whole_dev); +if (r <0 ) { +log_err (ctx,_ ("Warning: could not resolve whole-disk device for %s; lock may not prevent udev races\n" ), dev_name); +sd_device_unref (dev); +return -1 ; + } + } + + r =sd_device_get_devname (whole_dev, &whole_disk_devname); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + + fd =open (whole_disk_devname,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { +com_err (ctx->program_name, errno, +_ ("while trying to open %s for locking" ), whole_disk_devname); +sd_device_unref (dev); +return -1 ; + } +sd_device_unref (dev); + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ; + } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), whole_disk_devname); +close (fd); +return -1 ; + } + +return fd; +#else +return -1 ; +#endif /* HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE */ +} +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;@@ -1413,6 +1500,7 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +int lock_fd =-1 ;__u32 features[3 ];char *cp;enum quota_type qtype;@@ -1488,6 +1576,8 @@ int main (int argc, char *argv[])check_mount (ctx); + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name); +if (!(ctx->options &E2F_OPT_PREEN ) && !(ctx->options &E2F_OPT_NO ) && !(ctx->options &E2F_OPT_YES )) {@@ -2169,6 +2259,9 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name); +if (lock_fd >=0 ) +close (lock_fd); +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);lib/config.h.in +6
-0@@ -352,6 +352,9 @@ /* Define to 1 if if struct sockaddr contains sa_len */ #undef HAVE_SA_LEN +/* Define to 1 if libsystemd has sd-device APIs for whole-disk locking */ +#undef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE + /* Define to 1 if you have the 'secure_getenv' function. */ #undef HAVE_SECURE_GETENV@@ -430,6 +433,9 @@ /* Define to 1 if you have the 'sysconf' function. */ #undef HAVE_SYSCONF +/* Define to 1 if you have the <systemd/sd-device.h> header file. */ +#undef HAVE_SYSTEMD_SD_DEVICE_H + /* Define to 1 if you have the <sys/acl.h> header file. */ #undef HAVE_SYS_ACL_H
Quoted 9 lines
Quoted 2 lines
Signature · 1 line
2.55.0Hi,
Gentle ping on this patch.
I would appreciate any feedback.
Can you clarify why this should go into only fsck.ext4? Doesn't this problem exist for other filesystems too?
I sent an earlier email as well with links to:
1) fsck used to lock the device, but it surfaced a bug in udevd
https://bugs.freedesktop.org/show_bug.cgi?id=79576
2) because of the bug, fsck stopped locking the device
https://github.com/util-linux/util-linux/commit/3bbdae633f4a1dda5f95ee6c61f18a1c8ef12250
3) the systemd-udevd bug was fixed
https://github.com/systemd/systemd/commit/5d354e525a5
To me, it makes more sense for the locking that already exists in fsck to get updated (or reverted) to lock the entire device, using the existing -l param (or maybe a new param like --lock-device, --udevd-lock, etc., if util-linux maintainers don't want to change -l behavior).
Do you see an issue with doing the locking there instead of here in fsck.ext4?
Quoted 405 lines · Dan Streetman
Quoted 377 lines · Nandakumar Raghavan
Quoted 376 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.The whole-disk device is resolved using libsystemd's sd-device API (
sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype()) rather than parsing sysfs, so the lock covers the same device node that udevd itself resolves and locks.This locking is entirely optional at build time where configure checks for
sd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB, ande2fsck.conly compiles in the locking code when it is found. On platforms without libsystemd there is no udevd to race against in the first place, so nothing is lost by skipping it there.Signed-off-by Nandakumar Raghavan <[email protected]>---
Changes in v3:
- Resolve the parent whole-disk device using libsystemd's sd-device API (sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype(),sd_device_get_devname()) instead of parsing sysfs paths
- Gate the feature on autoconf detectingsd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB; compile it out entirely when libsystemd is absent, rather than falling back to another implementation
- Usesd_device_new_from_devnum()rather than thesd_device_new_from_devname()used by the systemd.io example, since the former has been available in libsystemd for much longer, keeping the version floor as low as possible
- Keep lock_whole_disk()'s signature unconditional and provide a stub returning -1 on the #else branch, so main()'s primary code path has no #ifdefs in itChanges in v2:
- Guard the whole feature (function, call site,lock_fd, cleanup) rather than just the function body, so it is fully absent from non-Linux builds
- Collapse the three HAVE_SYS_*_H header guards into a single conditional
- Reuse a single dev_path[] buffer instead of three single-use stack buffers
- Keep translatable warning strings on one line each, even past 80 columns
- Drop the blanket "could not lock" warning that fired even when the target was not a block device at all (e.g. a regular file), which broke most of "make check";lock_whole_disk()now only warns at the point of a genuine failure
- No change needed for the Ctrl-C/EINTR concern: the SIGINT handler is installed beforelock_whole_disk()runs and is notSA_RESTART, so a blockedflock()is interrupted and the existingE2F_FLAG_CANCELcheck unwinds cleanlyMCONFIG.in | 1 + configure | 71 +++++++++++++++++++++++++++++++++++ configure.ac | 14 +++++++ e2fsck/Makefile.in | 6 +-- e2fsck/unix.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++ lib/config.h.in | 6 +++ 6 files changed, 188 insertions(+), 3 deletions(-)MCONFIG.in +1
-0@@ -137,6 +137,7 @@ LIBE2P = $(LIB)/libe2p@LIB_EXT@ LIBEXT2FS = $(LIB)/libext2fs@LIB_EXT@ LIBUUID = @LIBUUID@ @SOCKET_LIB@ LIBMAGIC = @MAGIC_LIB@ +LIBSYSTEMD = @SYSTEMD_LIB@ LIBFUSE = @FUSE_LIB@ LIBSUPPORT = $(LIBINTL) $(LIB)/libsupport@STATIC_LIB_EXT@ LIBBLKID = @LIBBLKID@ @PRIVATE_LIBS_CMT@ $(LIBUUID)configure +71
-0@@ -702,6 +702,7 @@ fuse3_CFLAGS CLOCK_GETTIME_LIB ARCHIVE_LIBS ARCHIVE_CFLAGS +SYSTEMD_LIB MAGIC_LIB SOCKET_LIB SIZEOF_TIME_T@@ -14130,6 +14131,76 @@ if test "$ac_cv_func_dlopen" = yes ; then MAGIC_LIB=$DLOPEN_LIB fi +SYSTEMD_LIB= +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd" >&5 +printf %s "checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd... " >&6; } +if test ${ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype+y} +then : + printf %s "(cached) " >&6 +else case e in #( + e) ac_check_lib_save_LIBS=$LIBS +LIBS="-lsystemd $LIBS" +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ + +/* Override any GCC internal prototype to avoid an error. + Use char because int might match the return type of a GCC + builtin and then its argument prototype would still apply. + The 'extern "C"' is for builds by C++ compilers; + although this is not generally supported in C code supporting it here + has little cost and some practical benefit (sr 110532). */ +#ifdef __cplusplus +extern "C" +#endif +char sd_device_get_parent_with_subsystem_devtype (void); +int +main (void) +{ +return sd_device_get_parent_with_subsystem_devtype (); + ; + return 0; +} +_ACEOF +if ac_fn_c_try_link "$LINENO" +then : + ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=yes +else case e in #( + e) ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=no ;; +esac +fi +rm -f core conftest.err conftest.$ac_objext conftest.beam \ + conftest$ac_exeext conftest.$ac_ext +LIBS=$ac_check_lib_save_LIBS ;; +esac +fi +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&5 +printf "%s\n" "$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&6; } +if test "x$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" = xyes +then : + SYSTEMD_LIB=-lsystemd +fi + +if test -n "$SYSTEMD_LIB"; then + for ac_header in systemd/sd-device.h +do : + ac_fn_c_check_header_compile "$LINENO" "systemd/sd-device.h" "ac_cv_header_systemd_sd_device_h" "$ac_includes_default" +if test "x$ac_cv_header_systemd_sd_device_h" = xyes +then : + printf "%s\n" "#define HAVE_SYSTEMD_SD_DEVICE_H 1" >>confdefs.h + +else case e in #( + e) SYSTEMD_LIB= ;; +esac +fi + +done +fi +if test -n "$SYSTEMD_LIB"; then + +printf "%s\n" "#define HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE 1" >>confdefs.h + +fi + # Check whether --with-libarchive was given. if test ${with_libarchive+y}configure.ac +14
-0@@ -1309,6 +1309,20 @@ if test "$ac_cv_func_dlopen" = yes ; then fi AC_SUBST(MAGIC_LIB) dnl +dnl See if libsystemd has sd-device APIs for whole-disk BSD locking +dnl +SYSTEMD_LIB= +AC_CHECK_LIB(systemd, sd_device_get_parent_with_subsystem_devtype, + [SYSTEMD_LIB=-lsystemd]) +if test -n "$SYSTEMD_LIB"; then + AC_CHECK_HEADERS([systemd/sd-device.h], , [SYSTEMD_LIB=]) +fi +if test -n "$SYSTEMD_LIB"; then + AC_DEFINE(HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE, 1, + [Define to 1 if libsystemd has sd-device APIs for whole-disk locking]) +fi +AC_SUBST(SYSTEMD_LIB) +dnl dnl libarchive dnl AC_ARG_WITH([libarchive],e2fsck/Makefile.in +3
-3@@ -17,20 +17,20 @@ MANPAGES= e2fsck.8 FMANPAGES= e2fsck.conf.5 LIBS= $(LIBSUPPORT) $(LIBEXT2FS) $(LIBCOM_ERR) $(LIBBLKID) $(LIBUUID) \ - $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(SYSLIBS) + $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) DEPLIBS= $(DEPLIBSUPPORT) $(LIBEXT2FS) $(DEPLIBCOM_ERR) $(DEPLIBBLKID) \ $(DEPLIBUUID) $(DEPLIBE2P) STATIC_LIBS= $(STATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) $(STATIC_LIBCOM_ERR) \ $(STATIC_LIBBLKID) $(STATIC_LIBUUID) $(LIBINTL) $(STATIC_LIBE2P) \ - $(LIBMAGIC) $(SYSLIBS) + $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) STATIC_DEPLIBS= $(DEPSTATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) \ $(DEPSTATIC_LIBCOM_ERR) $(DEPSTATIC_LIBBLKID) \ $(DEPSTATIC_LIBUUID) $(DEPSTATIC_LIBE2P) PROFILED_LIBS= $(PROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(PROFILED_LIBCOM_ERR) $(PROFILED_LIBBLKID) $(PROFILED_LIBUUID) \ - $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(SYSLIBS) + $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) PROFILED_DEPLIBS= $(DEPPROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(DEPPROFILED_LIBCOM_ERR) $(DEPPROFILED_LIBBLKID) \ $(DEPPROFILED_LIBUUID) $(DEPPROFILED_LIBE2P)e2fsck/unix.c +93
-0@@ -36,6 +36,11 @@ extern int optind;#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +#include <sys/file.h> +#include <sys/stat.h> +#include <systemd/sd-device.h> +#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif @@ -1397,6 +1402,88 @@ err:return retval; } +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name) +{ +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +struct stat st; +sd_device *dev =NULL , *whole_dev; +const char *subsystem, *devtype, *whole_disk_devname; +int r, fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + r =sd_device_new_from_devnum (&dev,'b' , st.st_rdev); +if (r <0 ) +return -1 ; + + r =sd_device_get_subsystem (dev, &subsystem); +if (r <0 ||strcmp (subsystem,"block" ) !=0 ) { +sd_device_unref (dev); +return -1 ; + } + + r =sd_device_get_devtype (dev, &devtype); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + +/* +* If dev_name is a partition, resolve the parent whole-disk device +* so the flock covers the same device node that udevd locks before +* probing any partition on it. +*/ +if (strcmp (devtype,"disk" ) ==0 ) + whole_dev = dev; +else { + r =sd_device_get_parent_with_subsystem_devtype (dev,"block" , +"disk" , &whole_dev); +if (r <0 ) { +log_err (ctx,_ ("Warning: could not resolve whole-disk device for %s; lock may not prevent udev races\n" ), dev_name); +sd_device_unref (dev); +return -1 ; + } + } + + r =sd_device_get_devname (whole_dev, &whole_disk_devname); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + + fd =open (whole_disk_devname,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { +com_err (ctx->program_name, errno, +_ ("while trying to open %s for locking" ), whole_disk_devname); +sd_device_unref (dev); +return -1 ; + } +sd_device_unref (dev); + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ; + } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), whole_disk_devname); +close (fd); +return -1 ; + } + +return fd; +#else +return -1 ; +#endif /* HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE */ +} +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;@@ -1413,6 +1500,7 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +int lock_fd =-1 ;__u32 features[3 ];char *cp;enum quota_type qtype;@@ -1488,6 +1576,8 @@ int main (int argc, char *argv[])check_mount (ctx); + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name); +if (!(ctx->options &E2F_OPT_PREEN ) && !(ctx->options &E2F_OPT_NO ) && !(ctx->options &E2F_OPT_YES )) {@@ -2169,6 +2259,9 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name); +if (lock_fd >=0 ) +close (lock_fd); +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);lib/config.h.in +6
-0@@ -352,6 +352,9 @@ /* Define to 1 if if struct sockaddr contains sa_len */ #undef HAVE_SA_LEN +/* Define to 1 if libsystemd has sd-device APIs for whole-disk locking */ +#undef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE + /* Define to 1 if you have the 'secure_getenv' function. */ #undef HAVE_SECURE_GETENV@@ -430,6 +433,9 @@ /* Define to 1 if you have the 'sysconf' function. */ #undef HAVE_SYSCONF +/* Define to 1 if you have the <systemd/sd-device.h> header file. */ +#undef HAVE_SYSTEMD_SD_DEVICE_H + /* Define to 1 if you have the <sys/acl.h> header file. */ #undef HAVE_SYS_ACL_HQuoted 9 lines
Quoted 2 lines
Signature · 1 line
2.55.0Hi,
Gentle ping on this patch.
I would appreciate any feedback.
Can you clarify why this should go into only fsck.ext4? Doesn't this problem exist for other filesystems too?
I sent an earlier email as well with links to:
1) fsck used to lock the device, but it surfaced a bug in udevd
https://bugs.freedesktop.org/show_bug.cgi?id=795762) because of the bug, fsck stopped locking the device
https://github.com/util-linux/util-linux/commit/3bbdae633f4a1dda5f95ee6c61f18a1c8ef122503) the systemd-udevd bug was fixed
https://github.com/systemd/systemd/commit/5d354e525a5
Quoted 8 lines
To me, it makes more sense for the locking that already exists in fsck to get updated (or reverted) to lock the entire device, using the existing -l param (or maybe a new param like --lock-device, --udevd-lock, etc., if util-linux maintainers don't want to change -l behavior).
Do you see an issue with doing the locking there instead of here in fsck.ext4?
/sbin/fsck (aka the dispatch wrapper program) doesn't necessarily know which block device(s) are going to be opened by a the fsck.$FSTYP program that it creates. It might be able to infer that by opening any parameter and performing the udev locking protocol after checking if what it opened is a block device, but that wouldn't work for (say) a fsck.XXX program for a multi-device filesystem wherein you only need to specify one device and it will find the others.
That said, this patchset also doesn't handle multi-device ext4 filesystems (i.e. external jbd2 journal device) because the author doesn't want to do that. In their defense, the udev flock()ing protocol requires one to determine if an opened block device is a partition; if it is, then it requires opening and locking the parent bdev (e.g. sdf1 -> sdf) instead of locking the original device. This makes it way more complicated for multi-device filesystems because now the client has to detect multiple partitions coming from the same underlying device and handle that appropriately. I don't know why the protocol designers made that choice.
I can run "trace-cmd record -e 'flock*'" to observe the locking interactions with scsi disk partitions, but for whatever reason I don't see any flocking going on if I use kpartx to create the partitions with device-mapper. No idea why that is.
--D
Quoted 415 lines · Darrick J. Wong
Quoted 405 lines · Dan Streetman
Quoted 377 lines · Nandakumar Raghavan
Quoted 376 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.The whole-disk device is resolved using libsystemd's sd-device API (
sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype()) rather than parsing sysfs, so the lock covers the same device node that udevd itself resolves and locks.This locking is entirely optional at build time where configure checks for
sd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB, ande2fsck.conly compiles in the locking code when it is found. On platforms without libsystemd there is no udevd to race against in the first place, so nothing is lost by skipping it there.Signed-off-by Nandakumar Raghavan <[email protected]>---
Changes in v3:
- Resolve the parent whole-disk device using libsystemd's sd-device API (sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype(),sd_device_get_devname()) instead of parsing sysfs paths
- Gate the feature on autoconf detectingsd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB; compile it out entirely when libsystemd is absent, rather than falling back to another implementation
- Usesd_device_new_from_devnum()rather than thesd_device_new_from_devname()used by the systemd.io example, since the former has been available in libsystemd for much longer, keeping the version floor as low as possible
- Keep lock_whole_disk()'s signature unconditional and provide a stub returning -1 on the #else branch, so main()'s primary code path has no #ifdefs in itChanges in v2:
- Guard the whole feature (function, call site,lock_fd, cleanup) rather than just the function body, so it is fully absent from non-Linux builds
- Collapse the three HAVE_SYS_*_H header guards into a single conditional
- Reuse a single dev_path[] buffer instead of three single-use stack buffers
- Keep translatable warning strings on one line each, even past 80 columns
- Drop the blanket "could not lock" warning that fired even when the target was not a block device at all (e.g. a regular file), which broke most of "make check";lock_whole_disk()now only warns at the point of a genuine failure
- No change needed for the Ctrl-C/EINTR concern: the SIGINT handler is installed beforelock_whole_disk()runs and is notSA_RESTART, so a blockedflock()is interrupted and the existingE2F_FLAG_CANCELcheck unwinds cleanlyMCONFIG.in | 1 + configure | 71 +++++++++++++++++++++++++++++++++++ configure.ac | 14 +++++++ e2fsck/Makefile.in | 6 +-- e2fsck/unix.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++ lib/config.h.in | 6 +++ 6 files changed, 188 insertions(+), 3 deletions(-)MCONFIG.in +1
-0@@ -137,6 +137,7 @@ LIBE2P = $(LIB)/libe2p@LIB_EXT@ LIBEXT2FS = $(LIB)/libext2fs@LIB_EXT@ LIBUUID = @LIBUUID@ @SOCKET_LIB@ LIBMAGIC = @MAGIC_LIB@ +LIBSYSTEMD = @SYSTEMD_LIB@ LIBFUSE = @FUSE_LIB@ LIBSUPPORT = $(LIBINTL) $(LIB)/libsupport@STATIC_LIB_EXT@ LIBBLKID = @LIBBLKID@ @PRIVATE_LIBS_CMT@ $(LIBUUID)configure +71
-0@@ -702,6 +702,7 @@ fuse3_CFLAGS CLOCK_GETTIME_LIB ARCHIVE_LIBS ARCHIVE_CFLAGS +SYSTEMD_LIB MAGIC_LIB SOCKET_LIB SIZEOF_TIME_T@@ -14130,6 +14131,76 @@ if test "$ac_cv_func_dlopen" = yes ; then MAGIC_LIB=$DLOPEN_LIB fi +SYSTEMD_LIB= +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd" >&5 +printf %s "checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd... " >&6; } +if test ${ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype+y} +then : + printf %s "(cached) " >&6 +else case e in #( + e) ac_check_lib_save_LIBS=$LIBS +LIBS="-lsystemd $LIBS" +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ + +/* Override any GCC internal prototype to avoid an error. + Use char because int might match the return type of a GCC + builtin and then its argument prototype would still apply. + The 'extern "C"' is for builds by C++ compilers; + although this is not generally supported in C code supporting it here + has little cost and some practical benefit (sr 110532). */ +#ifdef __cplusplus +extern "C" +#endif +char sd_device_get_parent_with_subsystem_devtype (void); +int +main (void) +{ +return sd_device_get_parent_with_subsystem_devtype (); + ; + return 0; +} +_ACEOF +if ac_fn_c_try_link "$LINENO" +then : + ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=yes +else case e in #( + e) ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=no ;; +esac +fi +rm -f core conftest.err conftest.$ac_objext conftest.beam \ + conftest$ac_exeext conftest.$ac_ext +LIBS=$ac_check_lib_save_LIBS ;; +esac +fi +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&5 +printf "%s\n" "$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&6; } +if test "x$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" = xyes +then : + SYSTEMD_LIB=-lsystemd +fi + +if test -n "$SYSTEMD_LIB"; then + for ac_header in systemd/sd-device.h +do : + ac_fn_c_check_header_compile "$LINENO" "systemd/sd-device.h" "ac_cv_header_systemd_sd_device_h" "$ac_includes_default" +if test "x$ac_cv_header_systemd_sd_device_h" = xyes +then : + printf "%s\n" "#define HAVE_SYSTEMD_SD_DEVICE_H 1" >>confdefs.h + +else case e in #( + e) SYSTEMD_LIB= ;; +esac +fi + +done +fi +if test -n "$SYSTEMD_LIB"; then + +printf "%s\n" "#define HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE 1" >>confdefs.h + +fi + # Check whether --with-libarchive was given. if test ${with_libarchive+y}configure.ac +14
-0@@ -1309,6 +1309,20 @@ if test "$ac_cv_func_dlopen" = yes ; then fi AC_SUBST(MAGIC_LIB) dnl +dnl See if libsystemd has sd-device APIs for whole-disk BSD locking +dnl +SYSTEMD_LIB= +AC_CHECK_LIB(systemd, sd_device_get_parent_with_subsystem_devtype, + [SYSTEMD_LIB=-lsystemd]) +if test -n "$SYSTEMD_LIB"; then + AC_CHECK_HEADERS([systemd/sd-device.h], , [SYSTEMD_LIB=]) +fi +if test -n "$SYSTEMD_LIB"; then + AC_DEFINE(HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE, 1, + [Define to 1 if libsystemd has sd-device APIs for whole-disk locking]) +fi +AC_SUBST(SYSTEMD_LIB) +dnl dnl libarchive dnl AC_ARG_WITH([libarchive],e2fsck/Makefile.in +3
-3@@ -17,20 +17,20 @@ MANPAGES= e2fsck.8 FMANPAGES= e2fsck.conf.5 LIBS= $(LIBSUPPORT) $(LIBEXT2FS) $(LIBCOM_ERR) $(LIBBLKID) $(LIBUUID) \ - $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(SYSLIBS) + $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) DEPLIBS= $(DEPLIBSUPPORT) $(LIBEXT2FS) $(DEPLIBCOM_ERR) $(DEPLIBBLKID) \ $(DEPLIBUUID) $(DEPLIBE2P) STATIC_LIBS= $(STATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) $(STATIC_LIBCOM_ERR) \ $(STATIC_LIBBLKID) $(STATIC_LIBUUID) $(LIBINTL) $(STATIC_LIBE2P) \ - $(LIBMAGIC) $(SYSLIBS) + $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) STATIC_DEPLIBS= $(DEPSTATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) \ $(DEPSTATIC_LIBCOM_ERR) $(DEPSTATIC_LIBBLKID) \ $(DEPSTATIC_LIBUUID) $(DEPSTATIC_LIBE2P) PROFILED_LIBS= $(PROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(PROFILED_LIBCOM_ERR) $(PROFILED_LIBBLKID) $(PROFILED_LIBUUID) \ - $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(SYSLIBS) + $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) PROFILED_DEPLIBS= $(DEPPROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(DEPPROFILED_LIBCOM_ERR) $(DEPPROFILED_LIBBLKID) \ $(DEPPROFILED_LIBUUID) $(DEPPROFILED_LIBE2P)e2fsck/unix.c +93
-0@@ -36,6 +36,11 @@ extern int optind;#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +#include <sys/file.h> +#include <sys/stat.h> +#include <systemd/sd-device.h> +#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif @@ -1397,6 +1402,88 @@ err:return retval; } +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name) +{ +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +struct stat st; +sd_device *dev =NULL , *whole_dev; +const char *subsystem, *devtype, *whole_disk_devname; +int r, fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + r =sd_device_new_from_devnum (&dev,'b' , st.st_rdev); +if (r <0 ) +return -1 ; + + r =sd_device_get_subsystem (dev, &subsystem); +if (r <0 ||strcmp (subsystem,"block" ) !=0 ) { +sd_device_unref (dev); +return -1 ; + } + + r =sd_device_get_devtype (dev, &devtype); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + +/* +* If dev_name is a partition, resolve the parent whole-disk device +* so the flock covers the same device node that udevd locks before +* probing any partition on it. +*/ +if (strcmp (devtype,"disk" ) ==0 ) + whole_dev = dev; +else { + r =sd_device_get_parent_with_subsystem_devtype (dev,"block" , +"disk" , &whole_dev); +if (r <0 ) { +log_err (ctx,_ ("Warning: could not resolve whole-disk device for %s; lock may not prevent udev races\n" ), dev_name); +sd_device_unref (dev); +return -1 ; + } + } + + r =sd_device_get_devname (whole_dev, &whole_disk_devname); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + + fd =open (whole_disk_devname,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { +com_err (ctx->program_name, errno, +_ ("while trying to open %s for locking" ), whole_disk_devname); +sd_device_unref (dev); +return -1 ; + } +sd_device_unref (dev); + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ; + } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), whole_disk_devname); +close (fd); +return -1 ; + } + +return fd; +#else +return -1 ; +#endif /* HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE */ +} +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;@@ -1413,6 +1500,7 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +int lock_fd =-1 ;__u32 features[3 ];char *cp;enum quota_type qtype;@@ -1488,6 +1576,8 @@ int main (int argc, char *argv[])check_mount (ctx); + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name); +if (!(ctx->options &E2F_OPT_PREEN ) && !(ctx->options &E2F_OPT_NO ) && !(ctx->options &E2F_OPT_YES )) {@@ -2169,6 +2259,9 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name); +if (lock_fd >=0 ) +close (lock_fd); +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);lib/config.h.in +6
-0@@ -352,6 +352,9 @@ /* Define to 1 if if struct sockaddr contains sa_len */ #undef HAVE_SA_LEN +/* Define to 1 if libsystemd has sd-device APIs for whole-disk locking */ +#undef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE + /* Define to 1 if you have the 'secure_getenv' function. */ #undef HAVE_SECURE_GETENV@@ -430,6 +433,9 @@ /* Define to 1 if you have the 'sysconf' function. */ #undef HAVE_SYSCONF +/* Define to 1 if you have the <systemd/sd-device.h> header file. */ +#undef HAVE_SYSTEMD_SD_DEVICE_H + /* Define to 1 if you have the <sys/acl.h> header file. */ #undef HAVE_SYS_ACL_HQuoted 9 lines
Quoted 2 lines
Signature · 1 line
2.55.0Hi,
Gentle ping on this patch.
I would appreciate any feedback.
Can you clarify why this should go into only fsck.ext4? Doesn't this problem exist for other filesystems too?
I sent an earlier email as well with links to:
1) fsck used to lock the device, but it surfaced a bug in udevd
https://bugs.freedesktop.org/show_bug.cgi?id=795762) because of the bug, fsck stopped locking the device
https://github.com/util-linux/util-linux/commit/3bbdae633f4a1dda5f95ee6c61f18a1c8ef122503) the systemd-udevd bug was fixed
https://github.com/systemd/systemd/commit/5d354e525a5Quoted 8 lines
To me, it makes more sense for the locking that already exists in fsck to get updated (or reverted) to lock the entire device, using the existing -l param (or maybe a new param like --lock-device, --udevd-lock, etc., if util-linux maintainers don't want to change -l behavior).
Do you see an issue with doing the locking there instead of here in fsck.ext4?
Quoted 7 lines
/sbin/fsck(aka the dispatch wrapper program) doesn't necessarily know which block device(s) are going to be opened by a the fsck.$FSTYP program that it creates. It might be able to infer that by opening any parameter and performing the udev locking protocol after checking if what it opened is a block device, but that wouldn't work for (say) a fsck.XXX program for a multi-device filesystem wherein you only need to specify one device and it will find the others.
I'm not following how you would have an ext4 filesystem on multiple devices/partitions, unless you are talking about md or dm, in which case it's irrelevant for the purposes of systemd-udevd because md/dm create a single logical device (e.g. /dev/md127) that is what udevd (and fsck.ext4) actually cares about.
If I'm completely missing what you mean, can you give an example of what you're talking about?
Quoted 3 lines
That said, this patchset also doesn't handle multi-device ext4 filesystems (i.e. external jbd2 journal device) because the author
I don't think an external journal is relevant for the purposes of udevd locking, is it?
Quoted 16 lines
doesn't want to do that. In their defense, the udev flock()ing protocol requires one to determine if an opened block device is a partition; if it is, then it requires opening and locking the parent bdev (e.g. sdf1 -> sdf) instead of locking the original device. This makes it way more complicated for multi-device filesystems because now the client has to detect multiple partitions coming from the same underlying device and handle that appropriately. I don't know why the protocol designers made that choice.
I can run "trace-cmd record -e 'flock*'" to observe the locking interactions with scsi disk partitions, but for whatever reason I don't see any flocking going on if I use kpartx to create the partitions with device-mapper. No idea why that is.
--D
Quoted 427 lines · Dan Streetman
Quoted 415 lines · Darrick J. Wong
Quoted 405 lines · Dan Streetman
Quoted 377 lines · Nandakumar Raghavan
Quoted 376 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.The whole-disk device is resolved using libsystemd's sd-device API (
sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype()) rather than parsing sysfs, so the lock covers the same device node that udevd itself resolves and locks.This locking is entirely optional at build time where configure checks for
sd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB, ande2fsck.conly compiles in the locking code when it is found. On platforms without libsystemd there is no udevd to race against in the first place, so nothing is lost by skipping it there.Signed-off-by Nandakumar Raghavan <[email protected]>---
Changes in v3:
- Resolve the parent whole-disk device using libsystemd's sd-device API (sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype(),sd_device_get_devname()) instead of parsing sysfs paths
- Gate the feature on autoconf detectingsd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB; compile it out entirely when libsystemd is absent, rather than falling back to another implementation
- Usesd_device_new_from_devnum()rather than thesd_device_new_from_devname()used by the systemd.io example, since the former has been available in libsystemd for much longer, keeping the version floor as low as possible
- Keep lock_whole_disk()'s signature unconditional and provide a stub returning -1 on the #else branch, so main()'s primary code path has no #ifdefs in itChanges in v2:
- Guard the whole feature (function, call site,lock_fd, cleanup) rather than just the function body, so it is fully absent from non-Linux builds
- Collapse the three HAVE_SYS_*_H header guards into a single conditional
- Reuse a single dev_path[] buffer instead of three single-use stack buffers
- Keep translatable warning strings on one line each, even past 80 columns
- Drop the blanket "could not lock" warning that fired even when the target was not a block device at all (e.g. a regular file), which broke most of "make check";lock_whole_disk()now only warns at the point of a genuine failure
- No change needed for the Ctrl-C/EINTR concern: the SIGINT handler is installed beforelock_whole_disk()runs and is notSA_RESTART, so a blockedflock()is interrupted and the existingE2F_FLAG_CANCELcheck unwinds cleanlyMCONFIG.in | 1 + configure | 71 +++++++++++++++++++++++++++++++++++ configure.ac | 14 +++++++ e2fsck/Makefile.in | 6 +-- e2fsck/unix.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++ lib/config.h.in | 6 +++ 6 files changed, 188 insertions(+), 3 deletions(-)MCONFIG.in +1
-0@@ -137,6 +137,7 @@ LIBE2P = $(LIB)/libe2p@LIB_EXT@ LIBEXT2FS = $(LIB)/libext2fs@LIB_EXT@ LIBUUID = @LIBUUID@ @SOCKET_LIB@ LIBMAGIC = @MAGIC_LIB@ +LIBSYSTEMD = @SYSTEMD_LIB@ LIBFUSE = @FUSE_LIB@ LIBSUPPORT = $(LIBINTL) $(LIB)/libsupport@STATIC_LIB_EXT@ LIBBLKID = @LIBBLKID@ @PRIVATE_LIBS_CMT@ $(LIBUUID)configure +71
-0@@ -702,6 +702,7 @@ fuse3_CFLAGS CLOCK_GETTIME_LIB ARCHIVE_LIBS ARCHIVE_CFLAGS +SYSTEMD_LIB MAGIC_LIB SOCKET_LIB SIZEOF_TIME_T@@ -14130,6 +14131,76 @@ if test "$ac_cv_func_dlopen" = yes ; then MAGIC_LIB=$DLOPEN_LIB fi +SYSTEMD_LIB= +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd" >&5 +printf %s "checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd... " >&6; } +if test ${ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype+y} +then : + printf %s "(cached) " >&6 +else case e in #( + e) ac_check_lib_save_LIBS=$LIBS +LIBS="-lsystemd $LIBS" +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ + +/* Override any GCC internal prototype to avoid an error. + Use char because int might match the return type of a GCC + builtin and then its argument prototype would still apply. + The 'extern "C"' is for builds by C++ compilers; + although this is not generally supported in C code supporting it here + has little cost and some practical benefit (sr 110532). */ +#ifdef __cplusplus +extern "C" +#endif +char sd_device_get_parent_with_subsystem_devtype (void); +int +main (void) +{ +return sd_device_get_parent_with_subsystem_devtype (); + ; + return 0; +} +_ACEOF +if ac_fn_c_try_link "$LINENO" +then : + ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=yes +else case e in #( + e) ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=no ;; +esac +fi +rm -f core conftest.err conftest.$ac_objext conftest.beam \ + conftest$ac_exeext conftest.$ac_ext +LIBS=$ac_check_lib_save_LIBS ;; +esac +fi +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&5 +printf "%s\n" "$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&6; } +if test "x$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" = xyes +then : + SYSTEMD_LIB=-lsystemd +fi + +if test -n "$SYSTEMD_LIB"; then + for ac_header in systemd/sd-device.h +do : + ac_fn_c_check_header_compile "$LINENO" "systemd/sd-device.h" "ac_cv_header_systemd_sd_device_h" "$ac_includes_default" +if test "x$ac_cv_header_systemd_sd_device_h" = xyes +then : + printf "%s\n" "#define HAVE_SYSTEMD_SD_DEVICE_H 1" >>confdefs.h + +else case e in #( + e) SYSTEMD_LIB= ;; +esac +fi + +done +fi +if test -n "$SYSTEMD_LIB"; then + +printf "%s\n" "#define HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE 1" >>confdefs.h + +fi + # Check whether --with-libarchive was given. if test ${with_libarchive+y}configure.ac +14
-0@@ -1309,6 +1309,20 @@ if test "$ac_cv_func_dlopen" = yes ; then fi AC_SUBST(MAGIC_LIB) dnl +dnl See if libsystemd has sd-device APIs for whole-disk BSD locking +dnl +SYSTEMD_LIB= +AC_CHECK_LIB(systemd, sd_device_get_parent_with_subsystem_devtype, + [SYSTEMD_LIB=-lsystemd]) +if test -n "$SYSTEMD_LIB"; then + AC_CHECK_HEADERS([systemd/sd-device.h], , [SYSTEMD_LIB=]) +fi +if test -n "$SYSTEMD_LIB"; then + AC_DEFINE(HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE, 1, + [Define to 1 if libsystemd has sd-device APIs for whole-disk locking]) +fi +AC_SUBST(SYSTEMD_LIB) +dnl dnl libarchive dnl AC_ARG_WITH([libarchive],e2fsck/Makefile.in +3
-3@@ -17,20 +17,20 @@ MANPAGES= e2fsck.8 FMANPAGES= e2fsck.conf.5 LIBS= $(LIBSUPPORT) $(LIBEXT2FS) $(LIBCOM_ERR) $(LIBBLKID) $(LIBUUID) \ - $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(SYSLIBS) + $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) DEPLIBS= $(DEPLIBSUPPORT) $(LIBEXT2FS) $(DEPLIBCOM_ERR) $(DEPLIBBLKID) \ $(DEPLIBUUID) $(DEPLIBE2P) STATIC_LIBS= $(STATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) $(STATIC_LIBCOM_ERR) \ $(STATIC_LIBBLKID) $(STATIC_LIBUUID) $(LIBINTL) $(STATIC_LIBE2P) \ - $(LIBMAGIC) $(SYSLIBS) + $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) STATIC_DEPLIBS= $(DEPSTATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) \ $(DEPSTATIC_LIBCOM_ERR) $(DEPSTATIC_LIBBLKID) \ $(DEPSTATIC_LIBUUID) $(DEPSTATIC_LIBE2P) PROFILED_LIBS= $(PROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(PROFILED_LIBCOM_ERR) $(PROFILED_LIBBLKID) $(PROFILED_LIBUUID) \ - $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(SYSLIBS) + $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) PROFILED_DEPLIBS= $(DEPPROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(DEPPROFILED_LIBCOM_ERR) $(DEPPROFILED_LIBBLKID) \ $(DEPPROFILED_LIBUUID) $(DEPPROFILED_LIBE2P)e2fsck/unix.c +93
-0@@ -36,6 +36,11 @@ extern int optind;#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +#include <sys/file.h> +#include <sys/stat.h> +#include <systemd/sd-device.h> +#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif @@ -1397,6 +1402,88 @@ err:return retval; } +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name) +{ +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +struct stat st; +sd_device *dev =NULL , *whole_dev; +const char *subsystem, *devtype, *whole_disk_devname; +int r, fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + r =sd_device_new_from_devnum (&dev,'b' , st.st_rdev); +if (r <0 ) +return -1 ; + + r =sd_device_get_subsystem (dev, &subsystem); +if (r <0 ||strcmp (subsystem,"block" ) !=0 ) { +sd_device_unref (dev); +return -1 ; + } + + r =sd_device_get_devtype (dev, &devtype); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + +/* +* If dev_name is a partition, resolve the parent whole-disk device +* so the flock covers the same device node that udevd locks before +* probing any partition on it. +*/ +if (strcmp (devtype,"disk" ) ==0 ) + whole_dev = dev; +else { + r =sd_device_get_parent_with_subsystem_devtype (dev,"block" , +"disk" , &whole_dev); +if (r <0 ) { +log_err (ctx,_ ("Warning: could not resolve whole-disk device for %s; lock may not prevent udev races\n" ), dev_name); +sd_device_unref (dev); +return -1 ; + } + } + + r =sd_device_get_devname (whole_dev, &whole_disk_devname); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + + fd =open (whole_disk_devname,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { +com_err (ctx->program_name, errno, +_ ("while trying to open %s for locking" ), whole_disk_devname); +sd_device_unref (dev); +return -1 ; + } +sd_device_unref (dev); + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ; + } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), whole_disk_devname); +close (fd); +return -1 ; + } + +return fd; +#else +return -1 ; +#endif /* HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE */ +} +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;@@ -1413,6 +1500,7 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +int lock_fd =-1 ;__u32 features[3 ];char *cp;enum quota_type qtype;@@ -1488,6 +1576,8 @@ int main (int argc, char *argv[])check_mount (ctx); + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name); +if (!(ctx->options &E2F_OPT_PREEN ) && !(ctx->options &E2F_OPT_NO ) && !(ctx->options &E2F_OPT_YES )) {@@ -2169,6 +2259,9 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name); +if (lock_fd >=0 ) +close (lock_fd); +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);lib/config.h.in +6
-0@@ -352,6 +352,9 @@ /* Define to 1 if if struct sockaddr contains sa_len */ #undef HAVE_SA_LEN +/* Define to 1 if libsystemd has sd-device APIs for whole-disk locking */ +#undef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE + /* Define to 1 if you have the 'secure_getenv' function. */ #undef HAVE_SECURE_GETENV@@ -430,6 +433,9 @@ /* Define to 1 if you have the 'sysconf' function. */ #undef HAVE_SYSCONF +/* Define to 1 if you have the <systemd/sd-device.h> header file. */ +#undef HAVE_SYSTEMD_SD_DEVICE_H + /* Define to 1 if you have the <sys/acl.h> header file. */ #undef HAVE_SYS_ACL_HQuoted 9 lines
Quoted 2 lines
Signature · 1 line
2.55.0Hi,
Gentle ping on this patch.
I would appreciate any feedback.
Can you clarify why this should go into only fsck.ext4? Doesn't this problem exist for other filesystems too?
I sent an earlier email as well with links to:
1) fsck used to lock the device, but it surfaced a bug in udevd
https://bugs.freedesktop.org/show_bug.cgi?id=795762) because of the bug, fsck stopped locking the device
https://github.com/util-linux/util-linux/commit/3bbdae633f4a1dda5f95ee6c61f18a1c8ef122503) the systemd-udevd bug was fixed
https://github.com/systemd/systemd/commit/5d354e525a5Quoted 8 lines
To me, it makes more sense for the locking that already exists in fsck to get updated (or reverted) to lock the entire device, using the existing -l param (or maybe a new param like --lock-device, --udevd-lock, etc., if util-linux maintainers don't want to change -l behavior).
Do you see an issue with doing the locking there instead of here in fsck.ext4?
Quoted 7 lines
/sbin/fsck(aka the dispatch wrapper program) doesn't necessarily know which block device(s) are going to be opened by a the fsck.$FSTYP program that it creates. It might be able to infer that by opening any parameter and performing the udev locking protocol after checking if what it opened is a block device, but that wouldn't work for (say) a fsck.XXX program for a multi-device filesystem wherein you only need to specify one device and it will find the others.
Quoted 5 lines
I'm not following how you would have an ext4 filesystem on multiple devices/partitions, unless you are talking about md or dm, in which case it's irrelevant for the purposes of systemd-udevd because md/dm create a single logical device (e.g.
/dev/md127) that is what udevd (and fsck.ext4) actually cares about.
Many Linux filesystems can attach to multiple block devices and store data on them: XFS, btrfs, and bcachefs. ext4 doesn't store any of its own data on the journal device, but it still has to open the journal device.
Quoted 2 lines
If I'm completely missing what you mean, can you give an example of what you're talking about?
ext4 supports having a separate blockdev for a journal:
# mkfs.ext4 -O journal_dev /dev/sdb mke2fs 1.47.2 (1-Jan-2025) Discarding device blocks: done Creating filesystem with 2579968 4k blocks and 0 inodes Filesystem UUID: e11a1211-fdd7-408a-b342-a131692c2356 Superblock backups stored on blocks:
Zeroing journal device:
# mkfs.ext4 -J device=/dev/sdb -F /dev/sda
mke2fs 1.47.2 (1-Jan-2025)
Using journal device's blocksize: 4096
Discarding device blocks: done
Creating filesystem with 2579968 4k blocks and 645904 inodes
Filesystem UUID: a1789ae9-76e7-47e3-9972-5be0b8505517
Superblock backups stored on blocks:
32768, 98304, 163840, 229376, 294912, 819200, 884736, 1605632Allocating group tables: done
Writing inode tables: done
Adding journal to device /dev/sdb: done
Writing superblocks and filesystem accounting information: done
# blkid /dev/sd[ab] /dev/sda: UUID="a1789ae9-76e7-47e3-9972-5be0b8505517" EXT_JOURNAL="e11a1211-fdd7-408a-b342-a131692c2356" BLOCK_SIZE="4096" TYPE="ext4" /dev/sdb: UUID="e11a1211-fdd7-408a-b342-a131692c2356" BLOCK_SIZE="4096" LOGUUID="e11a1211-fdd7-408a-b342-a131692c2356" TYPE="jbd"
Note that you don't have to pass the journal to e2fsck:
# e2fsck -fn /dev/sda e2fsck 1.47.5~WIP-2026-03-08 (8-Mar-2026) Pass 1: Checking inodes, blocks, and sizes Pass 2: Checking directory structure Pass 3: Checking directory connectivity Pass 4: Checking reference counts Pass 5: Checking group summary information /dev/sda: 12/645904 files (0.0% non-contiguous), 50288/2579968 blocks
Quoted 5 lines
Quoted 2 lines
That said, this patchset also doesn't handle multi-device ext4 filesystems (i.e. external jbd2 journal device) because the author
I don't think an external journal is relevant for the purposes of udevd locking, is it?
Yes it is, e2fsck could need to replay the journal contents, and presumably you want the same protection for the secondary devices as the primary.
--D
Quoted 17 lines
Quoted 16 lines
doesn't want to do that. In their defense, the udev flock()ing protocol requires one to determine if an opened block device is a partition; if it is, then it requires opening and locking the parent bdev (e.g. sdf1 -> sdf) instead of locking the original device. This makes it way more complicated for multi-device filesystems because now the client has to detect multiple partitions coming from the same underlying device and handle that appropriately. I don't know why the protocol designers made that choice.
I can run "trace-cmd record -e 'flock*'" to observe the locking interactions with scsi disk partitions, but for whatever reason I don't see any flocking going on if I use kpartx to create the partitions with device-mapper. No idea why that is.
--D
Quoted 472 lines · Darrick J. Wong
Quoted 427 lines · Dan Streetman
Quoted 415 lines · Darrick J. Wong
Quoted 405 lines · Dan Streetman
Quoted 377 lines · Nandakumar Raghavan
Quoted 376 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.The whole-disk device is resolved using libsystemd's sd-device API (
sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype()) rather than parsing sysfs, so the lock covers the same device node that udevd itself resolves and locks.This locking is entirely optional at build time where configure checks for
sd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB, ande2fsck.conly compiles in the locking code when it is found. On platforms without libsystemd there is no udevd to race against in the first place, so nothing is lost by skipping it there.Signed-off-by Nandakumar Raghavan <[email protected]>---
Changes in v3:
- Resolve the parent whole-disk device using libsystemd's sd-device API (sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype(),sd_device_get_devname()) instead of parsing sysfs paths
- Gate the feature on autoconf detectingsd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB; compile it out entirely when libsystemd is absent, rather than falling back to another implementation
- Usesd_device_new_from_devnum()rather than thesd_device_new_from_devname()used by the systemd.io example, since the former has been available in libsystemd for much longer, keeping the version floor as low as possible
- Keep lock_whole_disk()'s signature unconditional and provide a stub returning -1 on the #else branch, so main()'s primary code path has no #ifdefs in itChanges in v2:
- Guard the whole feature (function, call site,lock_fd, cleanup) rather than just the function body, so it is fully absent from non-Linux builds
- Collapse the three HAVE_SYS_*_H header guards into a single conditional
- Reuse a single dev_path[] buffer instead of three single-use stack buffers
- Keep translatable warning strings on one line each, even past 80 columns
- Drop the blanket "could not lock" warning that fired even when the target was not a block device at all (e.g. a regular file), which broke most of "make check";lock_whole_disk()now only warns at the point of a genuine failure
- No change needed for the Ctrl-C/EINTR concern: the SIGINT handler is installed beforelock_whole_disk()runs and is notSA_RESTART, so a blockedflock()is interrupted and the existingE2F_FLAG_CANCELcheck unwinds cleanlyMCONFIG.in | 1 + configure | 71 +++++++++++++++++++++++++++++++++++ configure.ac | 14 +++++++ e2fsck/Makefile.in | 6 +-- e2fsck/unix.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++ lib/config.h.in | 6 +++ 6 files changed, 188 insertions(+), 3 deletions(-)MCONFIG.in +1
-0@@ -137,6 +137,7 @@ LIBE2P = $(LIB)/libe2p@LIB_EXT@ LIBEXT2FS = $(LIB)/libext2fs@LIB_EXT@ LIBUUID = @LIBUUID@ @SOCKET_LIB@ LIBMAGIC = @MAGIC_LIB@ +LIBSYSTEMD = @SYSTEMD_LIB@ LIBFUSE = @FUSE_LIB@ LIBSUPPORT = $(LIBINTL) $(LIB)/libsupport@STATIC_LIB_EXT@ LIBBLKID = @LIBBLKID@ @PRIVATE_LIBS_CMT@ $(LIBUUID)configure +71
-0@@ -702,6 +702,7 @@ fuse3_CFLAGS CLOCK_GETTIME_LIB ARCHIVE_LIBS ARCHIVE_CFLAGS +SYSTEMD_LIB MAGIC_LIB SOCKET_LIB SIZEOF_TIME_T@@ -14130,6 +14131,76 @@ if test "$ac_cv_func_dlopen" = yes ; then MAGIC_LIB=$DLOPEN_LIB fi +SYSTEMD_LIB= +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd" >&5 +printf %s "checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd... " >&6; } +if test ${ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype+y} +then : + printf %s "(cached) " >&6 +else case e in #( + e) ac_check_lib_save_LIBS=$LIBS +LIBS="-lsystemd $LIBS" +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ + +/* Override any GCC internal prototype to avoid an error. + Use char because int might match the return type of a GCC + builtin and then its argument prototype would still apply. + The 'extern "C"' is for builds by C++ compilers; + although this is not generally supported in C code supporting it here + has little cost and some practical benefit (sr 110532). */ +#ifdef __cplusplus +extern "C" +#endif +char sd_device_get_parent_with_subsystem_devtype (void); +int +main (void) +{ +return sd_device_get_parent_with_subsystem_devtype (); + ; + return 0; +} +_ACEOF +if ac_fn_c_try_link "$LINENO" +then : + ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=yes +else case e in #( + e) ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=no ;; +esac +fi +rm -f core conftest.err conftest.$ac_objext conftest.beam \ + conftest$ac_exeext conftest.$ac_ext +LIBS=$ac_check_lib_save_LIBS ;; +esac +fi +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&5 +printf "%s\n" "$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&6; } +if test "x$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" = xyes +then : + SYSTEMD_LIB=-lsystemd +fi + +if test -n "$SYSTEMD_LIB"; then + for ac_header in systemd/sd-device.h +do : + ac_fn_c_check_header_compile "$LINENO" "systemd/sd-device.h" "ac_cv_header_systemd_sd_device_h" "$ac_includes_default" +if test "x$ac_cv_header_systemd_sd_device_h" = xyes +then : + printf "%s\n" "#define HAVE_SYSTEMD_SD_DEVICE_H 1" >>confdefs.h + +else case e in #( + e) SYSTEMD_LIB= ;; +esac +fi + +done +fi +if test -n "$SYSTEMD_LIB"; then + +printf "%s\n" "#define HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE 1" >>confdefs.h + +fi + # Check whether --with-libarchive was given. if test ${with_libarchive+y}configure.ac +14
-0@@ -1309,6 +1309,20 @@ if test "$ac_cv_func_dlopen" = yes ; then fi AC_SUBST(MAGIC_LIB) dnl +dnl See if libsystemd has sd-device APIs for whole-disk BSD locking +dnl +SYSTEMD_LIB= +AC_CHECK_LIB(systemd, sd_device_get_parent_with_subsystem_devtype, + [SYSTEMD_LIB=-lsystemd]) +if test -n "$SYSTEMD_LIB"; then + AC_CHECK_HEADERS([systemd/sd-device.h], , [SYSTEMD_LIB=]) +fi +if test -n "$SYSTEMD_LIB"; then + AC_DEFINE(HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE, 1, + [Define to 1 if libsystemd has sd-device APIs for whole-disk locking]) +fi +AC_SUBST(SYSTEMD_LIB) +dnl dnl libarchive dnl AC_ARG_WITH([libarchive],e2fsck/Makefile.in +3
-3@@ -17,20 +17,20 @@ MANPAGES= e2fsck.8 FMANPAGES= e2fsck.conf.5 LIBS= $(LIBSUPPORT) $(LIBEXT2FS) $(LIBCOM_ERR) $(LIBBLKID) $(LIBUUID) \ - $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(SYSLIBS) + $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) DEPLIBS= $(DEPLIBSUPPORT) $(LIBEXT2FS) $(DEPLIBCOM_ERR) $(DEPLIBBLKID) \ $(DEPLIBUUID) $(DEPLIBE2P) STATIC_LIBS= $(STATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) $(STATIC_LIBCOM_ERR) \ $(STATIC_LIBBLKID) $(STATIC_LIBUUID) $(LIBINTL) $(STATIC_LIBE2P) \ - $(LIBMAGIC) $(SYSLIBS) + $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) STATIC_DEPLIBS= $(DEPSTATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) \ $(DEPSTATIC_LIBCOM_ERR) $(DEPSTATIC_LIBBLKID) \ $(DEPSTATIC_LIBUUID) $(DEPSTATIC_LIBE2P) PROFILED_LIBS= $(PROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(PROFILED_LIBCOM_ERR) $(PROFILED_LIBBLKID) $(PROFILED_LIBUUID) \ - $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(SYSLIBS) + $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) PROFILED_DEPLIBS= $(DEPPROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(DEPPROFILED_LIBCOM_ERR) $(DEPPROFILED_LIBBLKID) \ $(DEPPROFILED_LIBUUID) $(DEPPROFILED_LIBE2P)e2fsck/unix.c +93
-0@@ -36,6 +36,11 @@ extern int optind;#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +#include <sys/file.h> +#include <sys/stat.h> +#include <systemd/sd-device.h> +#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif @@ -1397,6 +1402,88 @@ err:return retval; } +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name) +{ +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +struct stat st; +sd_device *dev =NULL , *whole_dev; +const char *subsystem, *devtype, *whole_disk_devname; +int r, fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + r =sd_device_new_from_devnum (&dev,'b' , st.st_rdev); +if (r <0 ) +return -1 ; + + r =sd_device_get_subsystem (dev, &subsystem); +if (r <0 ||strcmp (subsystem,"block" ) !=0 ) { +sd_device_unref (dev); +return -1 ; + } + + r =sd_device_get_devtype (dev, &devtype); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + +/* +* If dev_name is a partition, resolve the parent whole-disk device +* so the flock covers the same device node that udevd locks before +* probing any partition on it. +*/ +if (strcmp (devtype,"disk" ) ==0 ) + whole_dev = dev; +else { + r =sd_device_get_parent_with_subsystem_devtype (dev,"block" , +"disk" , &whole_dev); +if (r <0 ) { +log_err (ctx,_ ("Warning: could not resolve whole-disk device for %s; lock may not prevent udev races\n" ), dev_name); +sd_device_unref (dev); +return -1 ; + } + } + + r =sd_device_get_devname (whole_dev, &whole_disk_devname); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + + fd =open (whole_disk_devname,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { +com_err (ctx->program_name, errno, +_ ("while trying to open %s for locking" ), whole_disk_devname); +sd_device_unref (dev); +return -1 ; + } +sd_device_unref (dev); + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ; + } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), whole_disk_devname); +close (fd); +return -1 ; + } + +return fd; +#else +return -1 ; +#endif /* HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE */ +} +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;@@ -1413,6 +1500,7 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +int lock_fd =-1 ;__u32 features[3 ];char *cp;enum quota_type qtype;@@ -1488,6 +1576,8 @@ int main (int argc, char *argv[])check_mount (ctx); + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name); +if (!(ctx->options &E2F_OPT_PREEN ) && !(ctx->options &E2F_OPT_NO ) && !(ctx->options &E2F_OPT_YES )) {@@ -2169,6 +2259,9 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name); +if (lock_fd >=0 ) +close (lock_fd); +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);lib/config.h.in +6
-0@@ -352,6 +352,9 @@ /* Define to 1 if if struct sockaddr contains sa_len */ #undef HAVE_SA_LEN +/* Define to 1 if libsystemd has sd-device APIs for whole-disk locking */ +#undef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE + /* Define to 1 if you have the 'secure_getenv' function. */ #undef HAVE_SECURE_GETENV@@ -430,6 +433,9 @@ /* Define to 1 if you have the 'sysconf' function. */ #undef HAVE_SYSCONF +/* Define to 1 if you have the <systemd/sd-device.h> header file. */ +#undef HAVE_SYSTEMD_SD_DEVICE_H + /* Define to 1 if you have the <sys/acl.h> header file. */ #undef HAVE_SYS_ACL_HQuoted 9 lines
Quoted 2 lines
Signature · 1 line
2.55.0Hi,
Gentle ping on this patch.
I would appreciate any feedback.
Can you clarify why this should go into only fsck.ext4? Doesn't this problem exist for other filesystems too?
I sent an earlier email as well with links to:
1) fsck used to lock the device, but it surfaced a bug in udevd
https://bugs.freedesktop.org/show_bug.cgi?id=795762) because of the bug, fsck stopped locking the device
https://github.com/util-linux/util-linux/commit/3bbdae633f4a1dda5f95ee6c61f18a1c8ef122503) the systemd-udevd bug was fixed
https://github.com/systemd/systemd/commit/5d354e525a5Quoted 8 lines
To me, it makes more sense for the locking that already exists in fsck to get updated (or reverted) to lock the entire device, using the existing -l param (or maybe a new param like --lock-device, --udevd-lock, etc., if util-linux maintainers don't want to change -l behavior).
Do you see an issue with doing the locking there instead of here in fsck.ext4?
Quoted 7 lines
/sbin/fsck(aka the dispatch wrapper program) doesn't necessarily know which block device(s) are going to be opened by a the fsck.$FSTYP program that it creates. It might be able to infer that by opening any parameter and performing the udev locking protocol after checking if what it opened is a block device, but that wouldn't work for (say) a fsck.XXX program for a multi-device filesystem wherein you only need to specify one device and it will find the others.Quoted 5 lines
I'm not following how you would have an ext4 filesystem on multiple devices/partitions, unless you are talking about md or dm, in which case it's irrelevant for the purposes of systemd-udevd because md/dm create a single logical device (e.g.
/dev/md127) that is what udevd (and fsck.ext4) actually cares about.Many Linux filesystems can attach to multiple block devices and store data on them: XFS, btrfs, and bcachefs. ext4 doesn't store any of its own data on the journal device, but it still has to open the journal device.
Quoted 2 lines
If I'm completely missing what you mean, can you give an example of what you're talking about?
ext4 supports having a separate blockdev for a journal:
# mkfs.ext4 -O journal_dev /dev/sdb mke2fs 1.47.2 (1-Jan-2025) Discarding device blocks: done Creating filesystem with 2579968 4k blocks and 0 inodes Filesystem UUID: e11a1211-fdd7-408a-b342-a131692c2356 Superblock backups stored on blocks:Zeroing journal device:
# mkfs.ext4 -J device=/dev/sdb -F /dev/sda mke2fs 1.47.2 (1-Jan-2025) Using journal device's blocksize: 4096 Discarding device blocks: done Creating filesystem with 2579968 4k blocks and 645904 inodes Filesystem UUID: a1789ae9-76e7-47e3-9972-5be0b8505517 Superblock backups stored on blocks: 32768, 98304, 163840, 229376, 294912, 819200, 884736, 1605632Allocating group tables: done
Writing inode tables: done
Adding journal to device/dev/sdb: done
Writing superblocks and filesystem accounting information: done# blkid /dev/sd[ab] /dev/sda: UUID="a1789ae9-76e7-47e3-9972-5be0b8505517" EXT_JOURNAL="e11a1211-fdd7-408a-b342-a131692c2356" BLOCK_SIZE="4096" TYPE="ext4" /dev/sdb: UUID="e11a1211-fdd7-408a-b342-a131692c2356" BLOCK_SIZE="4096" LOGUUID="e11a1211-fdd7-408a-b342-a131692c2356" TYPE="jbd"Note that you don't have to pass the journal to e2fsck:
Quoted 12 lines
# e2fsck -fn /dev/sda e2fsck 1.47.5~WIP-2026-03-08 (8-Mar-2026) Pass 1: Checking inodes, blocks, and sizes Pass 2: Checking directory structure Pass 3: Checking directory connectivity Pass 4: Checking reference counts Pass 5: Checking group summary information /dev/sda: 12/645904 files (0.0% non-contiguous), 50288/2579968 blocksQuoted 3 lines
Quoted 2 lines
That said, this patchset also doesn't handle multi-device ext4 filesystems (i.e. external jbd2 journal device) because the author
Not unwilling it just does not fit as a second lock_whole_disk() call. BLOCK_DEVICE_LOCKING.md does specify the multi-device case (lock in ascending major, then minor order), but if both devices
are partitions of the same disk they resolve to the same parent, and flock() locks are per open file description, so a second blocking LOCK_EX on its own fd would block against the lock this process already holds. It needs resolve, dedup, order plus check superblock first. I am happy to send it as a follow-up once this lands.
Quoted 6 lines
Quoted 2 lines
I don't think an external journal is relevant for the purposes of udevd locking, is it?
Yes it is, e2fsck could need to replay the journal contents, and presumably you want the same protection for the secondary devices as the primary.
Agreed, the journal device is written during replay just like the primary, so it wants the same lock. And as your example shows, the journal is not passed on the command line in the normal case
Quoted 20 lines
--D
Quoted 17 lines
Quoted 16 lines
doesn't want to do that. In their defense, the udev flock()ing protocol requires one to determine if an opened block device is a partition; if it is, then it requires opening and locking the parent bdev (e.g. sdf1 -> sdf) instead of locking the original device. This makes it way more complicated for multi-device filesystems because now the client has to detect multiple partitions coming from the same underlying device and handle that appropriately. I don't know why the protocol designers made that choice.
I can run "trace-cmd record -e 'flock*'" to observe the locking interactions with scsi disk partitions, but for whatever reason I don't see any flocking going on if I use kpartx to create the partitions with device-mapper. No idea why that is.
--D
Quoted 22 lines · Darrick J. Wong
Quoted 21 lines · Dan Streetman
Quoted 17 lines · Nandakumar Raghavan
Quoted 16 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.
...
Quoted 47 lines
Quoted 19 lines
Quoted 4 lines
Gentle ping on this patch.
I would appreciate any feedback.
Can you clarify why this should go into only fsck.ext4? Doesn't this problem exist for other filesystems too?
I sent an earlier email as well with links to:
1) fsck used to lock the device, but it surfaced a bug in udevd
https://bugs.freedesktop.org/show_bug.cgi?id=795762) because of the bug, fsck stopped locking the device
https://github.com/util-linux/util-linux/commit/3bbdae633f4a1dda5f95ee6c61f18a1c8ef122503) the systemd-udevd bug was fixed
https://github.com/systemd/systemd/commit/5d354e525a5Quoted 8 lines
To me, it makes more sense for the locking that already exists in fsck to get updated (or reverted) to lock the entire device, using the existing -l param (or maybe a new param like --lock-device, --udevd-lock, etc., if util-linux maintainers don't want to change -l behavior).
Do you see an issue with doing the locking there instead of here in fsck.ext4?
/sbin/fsck(aka the dispatch wrapper program) doesn't necessarily know which block device(s) are going to be opened by a the fsck.$FSTYP program that it creates. It might be able to infer that by opening any parameter and performing the udev locking protocol after checking if what it opened is a block device, but that wouldn't work for (say) a fsck.XXX program for a multi-device filesystem wherein you only need to specify one device and it will find the others.That said, this patchset also doesn't handle multi-device ext4 filesystems (i.e. external jbd2 journal device) because the author doesn't want to do that. In their defense, the udev flock()ing protocol requires one to determine if an opened block device is a partition; if it is, then it requires opening and locking the parent bdev (e.g. sdf1 -> sdf) instead of locking the original device. This makes it way more complicated for multi-device filesystems because now the client has to detect multiple partitions coming from the same underlying device and handle that appropriately. I don't know why the protocol designers made that choice.
Quoted 6 lines
I can run "trace-cmd record -e 'flock*'" to observe the locking interactions with scsi disk partitions, but for whatever reason I don't see any flocking going on if I use kpartx to create the partitions with device-mapper. No idea why that is.
--D
Systemd-udevd will not take its shared lock when the device in the uevent starts with "dm-", "md", or "drbd". See udev_get_whole_disk() in src/udev/udev-worker.c and how that's used by worker_lock_whole_disk(). Maybe that explains you not seeing the flocks in the second case. Would the partition device names (or what their symlinks expand to?) look like /^dm-*/?
Regards,
Mike Small
Quoted 77 lines · Mike Small
Quoted 22 lines · Darrick J. Wong
Quoted 21 lines · Dan Streetman
Quoted 17 lines · Nandakumar Raghavan
Quoted 16 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent....
Quoted 52 lines
Quoted 19 lines
Quoted 4 lines
Gentle ping on this patch.
I would appreciate any feedback.
Can you clarify why this should go into only fsck.ext4? Doesn't this problem exist for other filesystems too?
I sent an earlier email as well with links to:
1) fsck used to lock the device, but it surfaced a bug in udevd
https://bugs.freedesktop.org/show_bug.cgi?id=795762) because of the bug, fsck stopped locking the device
https://github.com/util-linux/util-linux/commit/3bbdae633f4a1dda5f95ee6c61f18a1c8ef122503) the systemd-udevd bug was fixed
https://github.com/systemd/systemd/commit/5d354e525a5Quoted 8 lines
To me, it makes more sense for the locking that already exists in fsck to get updated (or reverted) to lock the entire device, using the existing -l param (or maybe a new param like --lock-device, --udevd-lock, etc., if util-linux maintainers don't want to change -l behavior).
Do you see an issue with doing the locking there instead of here in fsck.ext4?
/sbin/fsck(aka the dispatch wrapper program) doesn't necessarily know which block device(s) are going to be opened by a the fsck.$FSTYP program that it creates. It might be able to infer that by opening any parameter and performing the udev locking protocol after checking if what it opened is a block device, but that wouldn't work for (say) a fsck.XXX program for a multi-device filesystem wherein you only need to specify one device and it will find the others.That said, this patchset also doesn't handle multi-device ext4 filesystems (i.e. external jbd2 journal device) because the author doesn't want to do that. In their defense, the udev flock()ing protocol requires one to determine if an opened block device is a partition; if it is, then it requires opening and locking the parent bdev (e.g. sdf1 -> sdf) instead of locking the original device. This makes it way more complicated for multi-device filesystems because now the client has to detect multiple partitions coming from the same underlying device and handle that appropriately. I don't know why the protocol designers made that choice.
I can run "trace-cmd record -e 'flock*'" to observe the locking interactions with scsi disk partitions, but for whatever reason I don't see any flocking going on if I use kpartx to create the partitions with device-mapper. No idea why that is.
Quoted 5 lines
Quoted 1 line
--D
Systemd-udevd will not take its shared lock when the device in the uevent starts with "dm-", "md", or "drbd". See
udev_get_whole_disk()insrc/udev/udev-worker.cand how that's used byworker_lock_whole_disk().
Doesn't that mean that blkid and e2fsck can still stomp on each other if the device is /dev/dm-0 ?
That's not in the specification, which means that for us, it's an undocumented implementation detail.
Quoted 3 lines
Maybe that explains you not seeing the flocks in the second case. Would the partition device names (or what their symlinks expand to?) look like /^dm-*/?
(For kpartx, yes it does since it uses dm to create partition devices)
But the fact that udev doesn't take the flock *at all* on dm/md/drbd devices makes this whole proposal feel pointless. Why would we add more code to e2fsprogs to satisfy a locking protocol that even the supposed benefactor doesn't follow consistently?
--D
Quoted 2 lines
Regards,
Mike Small
Quoted 386 lines · Dan Streetman
Quoted 382 lines · Nandakumar Raghavan
Quoted 378 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.The whole-disk device is resolved using libsystemd's sd-device API (
sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype()) rather than parsing sysfs, so the lock covers the same device node that udevd itself resolves and locks.This locking is entirely optional at build time where configure checks for
sd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB, ande2fsck.conly compiles in the locking code when it is found. On platforms without libsystemd there is no udevd to race against in the first place, so nothing is lost by skipping it there.Signed-off-by Nandakumar Raghavan <[email protected]>---
Changes in v3:
- Resolve the parent whole-disk device using libsystemd's sd-device API (sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype(),sd_device_get_devname()) instead of parsing sysfs paths
- Gate the feature on autoconf detectingsd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB; compile it out entirely when libsystemd is absent, rather than falling back to another implementation
- Usesd_device_new_from_devnum()rather than thesd_device_new_from_devname()used by the systemd.io example, since the former has been available in libsystemd for much longer, keeping the version floor as low as possible
- Keep lock_whole_disk()'s signature unconditional and provide a stub returning -1 on the #else branch, so main()'s primary code path has no #ifdefs in itChanges in v2:
- Guard the whole feature (function, call site,lock_fd, cleanup) rather than just the function body, so it is fully absent from non-Linux builds
- Collapse the three HAVE_SYS_*_H header guards into a single conditional
- Reuse a single dev_path[] buffer instead of three single-use stack buffers
- Keep translatable warning strings on one line each, even past 80 columns
- Drop the blanket "could not lock" warning that fired even when the target was not a block device at all (e.g. a regular file), which broke most of "make check";lock_whole_disk()now only warns at the point of a genuine failure
- No change needed for the Ctrl-C/EINTR concern: the SIGINT handler is installed beforelock_whole_disk()runs and is notSA_RESTART, so a blockedflock()is interrupted and the existingE2F_FLAG_CANCELcheck unwinds cleanlyMCONFIG.in | 1 + configure | 71 +++++++++++++++++++++++++++++++++++ configure.ac | 14 +++++++ e2fsck/Makefile.in | 6 +-- e2fsck/unix.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++ lib/config.h.in | 6 +++ 6 files changed, 188 insertions(+), 3 deletions(-)MCONFIG.in +1
-0@@ -137,6 +137,7 @@ LIBE2P = $(LIB)/libe2p@LIB_EXT@ LIBEXT2FS = $(LIB)/libext2fs@LIB_EXT@ LIBUUID = @LIBUUID@ @SOCKET_LIB@ LIBMAGIC = @MAGIC_LIB@ +LIBSYSTEMD = @SYSTEMD_LIB@ LIBFUSE = @FUSE_LIB@ LIBSUPPORT = $(LIBINTL) $(LIB)/libsupport@STATIC_LIB_EXT@ LIBBLKID = @LIBBLKID@ @PRIVATE_LIBS_CMT@ $(LIBUUID)configure +71
-0@@ -702,6 +702,7 @@ fuse3_CFLAGS CLOCK_GETTIME_LIB ARCHIVE_LIBS ARCHIVE_CFLAGS +SYSTEMD_LIB MAGIC_LIB SOCKET_LIB SIZEOF_TIME_T@@ -14130,6 +14131,76 @@ if test "$ac_cv_func_dlopen" = yes ; then MAGIC_LIB=$DLOPEN_LIB fi +SYSTEMD_LIB= +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd" >&5 +printf %s "checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd... " >&6; } +if test ${ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype+y} +then : + printf %s "(cached) " >&6 +else case e in #( + e) ac_check_lib_save_LIBS=$LIBS +LIBS="-lsystemd $LIBS" +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ + +/* Override any GCC internal prototype to avoid an error. + Use char because int might match the return type of a GCC + builtin and then its argument prototype would still apply. + The 'extern "C"' is for builds by C++ compilers; + although this is not generally supported in C code supporting it here + has little cost and some practical benefit (sr 110532). */ +#ifdef __cplusplus +extern "C" +#endif +char sd_device_get_parent_with_subsystem_devtype (void); +int +main (void) +{ +return sd_device_get_parent_with_subsystem_devtype (); + ; + return 0; +} +_ACEOF +if ac_fn_c_try_link "$LINENO" +then : + ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=yes +else case e in #( + e) ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=no ;; +esac +fi +rm -f core conftest.err conftest.$ac_objext conftest.beam \ + conftest$ac_exeext conftest.$ac_ext +LIBS=$ac_check_lib_save_LIBS ;; +esac +fi +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&5 +printf "%s\n" "$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&6; } +if test "x$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" = xyes +then : + SYSTEMD_LIB=-lsystemd +fi + +if test -n "$SYSTEMD_LIB"; then + for ac_header in systemd/sd-device.h +do : + ac_fn_c_check_header_compile "$LINENO" "systemd/sd-device.h" "ac_cv_header_systemd_sd_device_h" "$ac_includes_default" +if test "x$ac_cv_header_systemd_sd_device_h" = xyes +then : + printf "%s\n" "#define HAVE_SYSTEMD_SD_DEVICE_H 1" >>confdefs.h + +else case e in #( + e) SYSTEMD_LIB= ;; +esac +fi + +done +fi +if test -n "$SYSTEMD_LIB"; then + +printf "%s\n" "#define HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE 1" >>confdefs.h + +fi + # Check whether --with-libarchive was given. if test ${with_libarchive+y}configure.ac +14
-0@@ -1309,6 +1309,20 @@ if test "$ac_cv_func_dlopen" = yes ; then fi AC_SUBST(MAGIC_LIB) dnl +dnl See if libsystemd has sd-device APIs for whole-disk BSD locking +dnl +SYSTEMD_LIB= +AC_CHECK_LIB(systemd, sd_device_get_parent_with_subsystem_devtype, + [SYSTEMD_LIB=-lsystemd]) +if test -n "$SYSTEMD_LIB"; then + AC_CHECK_HEADERS([systemd/sd-device.h], , [SYSTEMD_LIB=]) +fi +if test -n "$SYSTEMD_LIB"; then + AC_DEFINE(HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE, 1, + [Define to 1 if libsystemd has sd-device APIs for whole-disk locking]) +fi +AC_SUBST(SYSTEMD_LIB) +dnl dnl libarchive dnl AC_ARG_WITH([libarchive],e2fsck/Makefile.in +3
-3@@ -17,20 +17,20 @@ MANPAGES= e2fsck.8 FMANPAGES= e2fsck.conf.5 LIBS= $(LIBSUPPORT) $(LIBEXT2FS) $(LIBCOM_ERR) $(LIBBLKID) $(LIBUUID) \ - $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(SYSLIBS) + $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) DEPLIBS= $(DEPLIBSUPPORT) $(LIBEXT2FS) $(DEPLIBCOM_ERR) $(DEPLIBBLKID) \ $(DEPLIBUUID) $(DEPLIBE2P) STATIC_LIBS= $(STATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) $(STATIC_LIBCOM_ERR) \ $(STATIC_LIBBLKID) $(STATIC_LIBUUID) $(LIBINTL) $(STATIC_LIBE2P) \ - $(LIBMAGIC) $(SYSLIBS) + $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) STATIC_DEPLIBS= $(DEPSTATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) \ $(DEPSTATIC_LIBCOM_ERR) $(DEPSTATIC_LIBBLKID) \ $(DEPSTATIC_LIBUUID) $(DEPSTATIC_LIBE2P) PROFILED_LIBS= $(PROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(PROFILED_LIBCOM_ERR) $(PROFILED_LIBBLKID) $(PROFILED_LIBUUID) \ - $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(SYSLIBS) + $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) PROFILED_DEPLIBS= $(DEPPROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(DEPPROFILED_LIBCOM_ERR) $(DEPPROFILED_LIBBLKID) \ $(DEPPROFILED_LIBUUID) $(DEPPROFILED_LIBE2P)e2fsck/unix.c +93
-0@@ -36,6 +36,11 @@ extern int optind;#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +#include <sys/file.h> +#include <sys/stat.h> +#include <systemd/sd-device.h> +#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif @@ -1397,6 +1402,88 @@ err:return retval; } +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name) +{ +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +struct stat st; +sd_device *dev =NULL , *whole_dev; +const char *subsystem, *devtype, *whole_disk_devname; +int r, fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + r =sd_device_new_from_devnum (&dev,'b' , st.st_rdev); +if (r <0 ) +return -1 ; + + r =sd_device_get_subsystem (dev, &subsystem); +if (r <0 ||strcmp (subsystem,"block" ) !=0 ) { +sd_device_unref (dev); +return -1 ; + } + + r =sd_device_get_devtype (dev, &devtype); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + +/* +* If dev_name is a partition, resolve the parent whole-disk device +* so the flock covers the same device node that udevd locks before +* probing any partition on it. +*/ +if (strcmp (devtype,"disk" ) ==0 ) + whole_dev = dev; +else { + r =sd_device_get_parent_with_subsystem_devtype (dev,"block" , +"disk" , &whole_dev); +if (r <0 ) { +log_err (ctx,_ ("Warning: could not resolve whole-disk device for %s; lock may not prevent udev races\n" ), dev_name); +sd_device_unref (dev); +return -1 ; + } + } + + r =sd_device_get_devname (whole_dev, &whole_disk_devname); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + + fd =open (whole_disk_devname,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { +com_err (ctx->program_name, errno, +_ ("while trying to open %s for locking" ), whole_disk_devname); +sd_device_unref (dev); +return -1 ; + } +sd_device_unref (dev); + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ; + } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), whole_disk_devname); +close (fd); +return -1 ; + } + +return fd; +#else +return -1 ; +#endif /* HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE */ +} +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;@@ -1413,6 +1500,7 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +int lock_fd =-1 ;__u32 features[3 ];char *cp;enum quota_type qtype;@@ -1488,6 +1576,8 @@ int main (int argc, char *argv[])check_mount (ctx); + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name); +if (!(ctx->options &E2F_OPT_PREEN ) && !(ctx->options &E2F_OPT_NO ) && !(ctx->options &E2F_OPT_YES )) {@@ -2169,6 +2259,9 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name); +if (lock_fd >=0 ) +close (lock_fd); +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);lib/config.h.in +6
-0@@ -352,6 +352,9 @@ /* Define to 1 if if struct sockaddr contains sa_len */ #undef HAVE_SA_LEN +/* Define to 1 if libsystemd has sd-device APIs for whole-disk locking */ +#undef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE + /* Define to 1 if you have the 'secure_getenv' function. */ #undef HAVE_SECURE_GETENV@@ -430,6 +433,9 @@ /* Define to 1 if you have the 'sysconf' function. */ #undef HAVE_SYSCONF +/* Define to 1 if you have the <systemd/sd-device.h> header file. */ +#undef HAVE_SYSTEMD_SD_DEVICE_H + /* Define to 1 if you have the <sys/acl.h> header file. */ #undef HAVE_SYS_ACL_HSignature · 1 line
2.55.0Hi,
Quoted 7 lines
Quoted 4 lines
Gentle ping on this patch.
I would appreciate any feedback.
Can you clarify why this should go into only fsck.ext4? Doesn't this problem exist for other filesystems too?
It can, yes - the race is not ext4-specific. Any fsck that writes metadata while udevd is probing the same disk can hit it. But there is no shared code path to fix it in each fsck opens its own devices and does its own I/O, so each one has to participate in the locking protocol itself. This patch covers ext2/ext3/ext4, which are all e2fsck. xfs_repair, btrfs check and so on would need equivalent changes in their own projects.
Quoted 19 lines
I sent an earlier email as well with links to:
1) fsck used to lock the device, but it surfaced a bug in udevd
https://bugs.freedesktop.org/show_bug.cgi?id=795762) because of the bug, fsck stopped locking the device
https://github.com/util-linux/util-linux/commit/3bbdae633f4a1dda5f95ee6c61f18a1c8ef122503) the systemd-udevd bug was fixed
https://github.com/systemd/systemd/commit/5d354e525a5To me, it makes more sense for the locking that already exists in fsck to get updated (or reverted) to lock the entire device, using the existing -l param (or maybe a new param like --lock-device, --udevd-lock, etc., if util-linux maintainers don't want to change -l behavior).
Quoted 2 lines
Do you see an issue with doing the locking there instead of here in fsck.ext4?
Darrick covered the main structural problem - /sbin/fsck doesn't know which devices the fsck.$FSTYP backend will actually open.
On top of that, today -l is not a weaker version of this patch, it is a different mechanism. From disk-utils/fsck.c on master:
- lock_disk() flocks /run/fsck/<diskname>.lock, a regular file. udevd flocks the device node. The two never interact.
- it returns early for is_irrotational_disk(), so -l does nothing at all.
If util-linux wants a --udevd-lock on the dispatcher later, this patch doesn't block it. But per systemd.io/BLOCK_DEVICE_LOCKING the lock is meant to be taken by the tool that actually modifies the device and it is recommended to take LOCK_EX BSD file locks when manipulating block devices in all tools that change file system block devices (mkfs, fsck etc) and doing it in e2fsck is what makes the guarantee hold regardless of who invoked it.
Quoted 378 lines · Nandakumar Raghavan
During journal replay, e2fsck writes the primary superblock back to disk in multiple I/O operations. The payload lands before the checksum, leaving a transient window where the on-disk superblock has a bad checksum.
If udevd processes a change uevent during this window, libblkid probes the primary superblock, finds a checksum mismatch, and concludes the partition has no recognisable filesystem. udev then fires a remove event, wiping all symlinks in
/dev/disk/by-label/and/dev/disk/by-uuid/. Any mount unit that depends on those symlinks will fail.udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.The whole-disk device is resolved using libsystemd's sd-device API (
sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype()) rather than parsing sysfs, so the lock covers the same device node that udevd itself resolves and locks.This locking is entirely optional at build time where configure checks for
sd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB, ande2fsck.conly compiles in the locking code when it is found. On platforms without libsystemd there is no udevd to race against in the first place, so nothing is lost by skipping it there.Signed-off-by Nandakumar Raghavan <[email protected]>---
Changes in v3:
- Resolve the parent whole-disk device using libsystemd's sd-device API (sd_device_new_from_devnum(),sd_device_get_parent_with_subsystem_devtype(),sd_device_get_devname()) instead of parsing sysfs paths
- Gate the feature on autoconf detectingsd_device_get_parent_with_subsystem_devtype()viaAC_CHECK_LIB; compile it out entirely when libsystemd is absent, rather than falling back to another implementation
- Usesd_device_new_from_devnum()rather than thesd_device_new_from_devname()used by the systemd.io example, since the former has been available in libsystemd for much longer, keeping the version floor as low as possible
- Keep lock_whole_disk()'s signature unconditional and provide a stub returning -1 on the #else branch, so main()'s primary code path has no #ifdefs in itChanges in v2:
- Guard the whole feature (function, call site,lock_fd, cleanup) rather than just the function body, so it is fully absent from non-Linux builds
- Collapse the three HAVE_SYS_*_H header guards into a single conditional
- Reuse a single dev_path[] buffer instead of three single-use stack buffers
- Keep translatable warning strings on one line each, even past 80 columns
- Drop the blanket "could not lock" warning that fired even when the target was not a block device at all (e.g. a regular file), which broke most of "make check";lock_whole_disk()now only warns at the point of a genuine failure
- No change needed for the Ctrl-C/EINTR concern: the SIGINT handler is installed beforelock_whole_disk()runs and is notSA_RESTART, so a blockedflock()is interrupted and the existingE2F_FLAG_CANCELcheck unwinds cleanlyMCONFIG.in | 1 + configure | 71 +++++++++++++++++++++++++++++++++++ configure.ac | 14 +++++++ e2fsck/Makefile.in | 6 +-- e2fsck/unix.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++ lib/config.h.in | 6 +++ 6 files changed, 188 insertions(+), 3 deletions(-)MCONFIG.in +1
-0@@ -137,6 +137,7 @@ LIBE2P = $(LIB)/libe2p@LIB_EXT@ LIBEXT2FS = $(LIB)/libext2fs@LIB_EXT@ LIBUUID = @LIBUUID@ @SOCKET_LIB@ LIBMAGIC = @MAGIC_LIB@ +LIBSYSTEMD = @SYSTEMD_LIB@ LIBFUSE = @FUSE_LIB@ LIBSUPPORT = $(LIBINTL) $(LIB)/libsupport@STATIC_LIB_EXT@ LIBBLKID = @LIBBLKID@ @PRIVATE_LIBS_CMT@ $(LIBUUID)configure +71
-0@@ -702,6 +702,7 @@ fuse3_CFLAGS CLOCK_GETTIME_LIB ARCHIVE_LIBS ARCHIVE_CFLAGS +SYSTEMD_LIB MAGIC_LIB SOCKET_LIB SIZEOF_TIME_T@@ -14130,6 +14131,76 @@ if test "$ac_cv_func_dlopen" = yes ; then MAGIC_LIB=$DLOPEN_LIB fi +SYSTEMD_LIB= +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd" >&5 +printf %s "checking for sd_device_get_parent_with_subsystem_devtype in -lsystemd... " >&6; } +if test ${ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype+y} +then : + printf %s "(cached) " >&6 +else case e in #( + e) ac_check_lib_save_LIBS=$LIBS +LIBS="-lsystemd $LIBS" +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ + +/* Override any GCC internal prototype to avoid an error. + Use char because int might match the return type of a GCC + builtin and then its argument prototype would still apply. + The 'extern "C"' is for builds by C++ compilers; + although this is not generally supported in C code supporting it here + has little cost and some practical benefit (sr 110532). */ +#ifdef __cplusplus +extern "C" +#endif +char sd_device_get_parent_with_subsystem_devtype (void); +int +main (void) +{ +return sd_device_get_parent_with_subsystem_devtype (); + ; + return 0; +} +_ACEOF +if ac_fn_c_try_link "$LINENO" +then : + ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=yes +else case e in #( + e) ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype=no ;; +esac +fi +rm -f core conftest.err conftest.$ac_objext conftest.beam \ + conftest$ac_exeext conftest.$ac_ext +LIBS=$ac_check_lib_save_LIBS ;; +esac +fi +{ printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&5 +printf "%s\n" "$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" >&6; } +if test "x$ac_cv_lib_systemd_sd_device_get_parent_with_subsystem_devtype" = xyes +then : + SYSTEMD_LIB=-lsystemd +fi + +if test -n "$SYSTEMD_LIB"; then + for ac_header in systemd/sd-device.h +do : + ac_fn_c_check_header_compile "$LINENO" "systemd/sd-device.h" "ac_cv_header_systemd_sd_device_h" "$ac_includes_default" +if test "x$ac_cv_header_systemd_sd_device_h" = xyes +then : + printf "%s\n" "#define HAVE_SYSTEMD_SD_DEVICE_H 1" >>confdefs.h + +else case e in #( + e) SYSTEMD_LIB= ;; +esac +fi + +done +fi +if test -n "$SYSTEMD_LIB"; then + +printf "%s\n" "#define HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE 1" >>confdefs.h + +fi + # Check whether --with-libarchive was given. if test ${with_libarchive+y}configure.ac +14
-0@@ -1309,6 +1309,20 @@ if test "$ac_cv_func_dlopen" = yes ; then fi AC_SUBST(MAGIC_LIB) dnl +dnl See if libsystemd has sd-device APIs for whole-disk BSD locking +dnl +SYSTEMD_LIB= +AC_CHECK_LIB(systemd, sd_device_get_parent_with_subsystem_devtype, + [SYSTEMD_LIB=-lsystemd]) +if test -n "$SYSTEMD_LIB"; then + AC_CHECK_HEADERS([systemd/sd-device.h], , [SYSTEMD_LIB=]) +fi +if test -n "$SYSTEMD_LIB"; then + AC_DEFINE(HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE, 1, + [Define to 1 if libsystemd has sd-device APIs for whole-disk locking]) +fi +AC_SUBST(SYSTEMD_LIB) +dnl dnl libarchive dnl AC_ARG_WITH([libarchive],e2fsck/Makefile.in +3
-3@@ -17,20 +17,20 @@ MANPAGES= e2fsck.8 FMANPAGES= e2fsck.conf.5 LIBS= $(LIBSUPPORT) $(LIBEXT2FS) $(LIBCOM_ERR) $(LIBBLKID) $(LIBUUID) \ - $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(SYSLIBS) + $(LIBINTL) $(LIBE2P) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) DEPLIBS= $(DEPLIBSUPPORT) $(LIBEXT2FS) $(DEPLIBCOM_ERR) $(DEPLIBBLKID) \ $(DEPLIBUUID) $(DEPLIBE2P) STATIC_LIBS= $(STATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) $(STATIC_LIBCOM_ERR) \ $(STATIC_LIBBLKID) $(STATIC_LIBUUID) $(LIBINTL) $(STATIC_LIBE2P) \ - $(LIBMAGIC) $(SYSLIBS) + $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) STATIC_DEPLIBS= $(DEPSTATIC_LIBSUPPORT) $(STATIC_LIBEXT2FS) \ $(DEPSTATIC_LIBCOM_ERR) $(DEPSTATIC_LIBBLKID) \ $(DEPSTATIC_LIBUUID) $(DEPSTATIC_LIBE2P) PROFILED_LIBS= $(PROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(PROFILED_LIBCOM_ERR) $(PROFILED_LIBBLKID) $(PROFILED_LIBUUID) \ - $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(SYSLIBS) + $(PROFILED_LIBE2P) $(LIBINTL) $(LIBMAGIC) $(LIBSYSTEMD) $(SYSLIBS) PROFILED_DEPLIBS= $(DEPPROFILED_LIBSUPPORT) $(PROFILED_LIBEXT2FS) \ $(DEPPROFILED_LIBCOM_ERR) $(DEPPROFILED_LIBBLKID) \ $(DEPPROFILED_LIBUUID) $(DEPPROFILED_LIBE2P)e2fsck/unix.c +93
-0@@ -36,6 +36,11 @@ extern int optind;#ifdef HAVE_SYS_IOCTL_H #include <sys/ioctl.h> #endif +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +#include <sys/file.h> +#include <sys/stat.h> +#include <systemd/sd-device.h> +#endif #ifdef HAVE_MALLOC_H #include <malloc.h> #endif @@ -1397,6 +1402,88 @@ err:return retval; } +static int lock_whole_disk (e2fsck_t ctx,const char *dev_name) +{ +#ifdef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE +struct stat st; +sd_device *dev =NULL , *whole_dev; +const char *subsystem, *devtype, *whole_disk_devname; +int r, fd; + +if (stat (dev_name, &st) <0 ) +return -1 ; + +if (!S_ISBLK (st.st_mode)) +return -1 ; + + r =sd_device_new_from_devnum (&dev,'b' , st.st_rdev); +if (r <0 ) +return -1 ; + + r =sd_device_get_subsystem (dev, &subsystem); +if (r <0 ||strcmp (subsystem,"block" ) !=0 ) { +sd_device_unref (dev); +return -1 ; + } + + r =sd_device_get_devtype (dev, &devtype); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + +/* +* If dev_name is a partition, resolve the parent whole-disk device +* so the flock covers the same device node that udevd locks before +* probing any partition on it. +*/ +if (strcmp (devtype,"disk" ) ==0 ) + whole_dev = dev; +else { + r =sd_device_get_parent_with_subsystem_devtype (dev,"block" , +"disk" , &whole_dev); +if (r <0 ) { +log_err (ctx,_ ("Warning: could not resolve whole-disk device for %s; lock may not prevent udev races\n" ), dev_name); +sd_device_unref (dev); +return -1 ; + } + } + + r =sd_device_get_devname (whole_dev, &whole_disk_devname); +if (r <0 ) { +sd_device_unref (dev); +return -1 ; + } + + fd =open (whole_disk_devname,O_RDONLY |O_CLOEXEC ,0 ); +if (fd <0 ) { +com_err (ctx->program_name, errno, +_ ("while trying to open %s for locking" ), whole_disk_devname); +sd_device_unref (dev); +return -1 ; + } +sd_device_unref (dev); + +while (flock (fd,LOCK_EX ) !=0 ) { +if (errno ==EINTR ) { +if (ctx->flags &E2F_FLAG_CANCEL ) { +close (fd); +return -1 ; + } +continue ; + } +com_err (ctx->program_name, errno, +_ ("while trying to lock %s" ), whole_disk_devname); +close (fd); +return -1 ; + } + +return fd; +#else +return -1 ; +#endif /* HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE */ +} +int main (int argc,char *argv[]) {errcode_t retval =0 , retval2 =0 , orig_retval =0 ;@@ -1413,6 +1500,7 @@ int main (int argc, char *argv[])int journal_size;int sysval, sys_page_size =4096 ;int old_bitmaps; +int lock_fd =-1 ;__u32 features[3 ];char *cp;enum quota_type qtype;@@ -1488,6 +1576,8 @@ int main (int argc, char *argv[])check_mount (ctx); + lock_fd =lock_whole_disk (ctx, ctx->filesystem_name); +if (!(ctx->options &E2F_OPT_PREEN ) && !(ctx->options &E2F_OPT_NO ) && !(ctx->options &E2F_OPT_YES )) {@@ -2169,6 +2259,9 @@ skip_write:ext2fs_close_free (&ctx->fs);free (ctx->journal_name); +if (lock_fd >=0 ) +close (lock_fd); +if (ctx->logf)fprintf (ctx->logf,"Exit status: %d\n" , exit_value);e2fsck_free_context (ctx);lib/config.h.in +6
-0@@ -352,6 +352,9 @@ /* Define to 1 if if struct sockaddr contains sa_len */ #undef HAVE_SA_LEN +/* Define to 1 if libsystemd has sd-device APIs for whole-disk locking */ +#undef HAVE_SD_DEVICE_GET_PARENT_WITH_SUBSYSTEM_DEVTYPE + /* Define to 1 if you have the 'secure_getenv' function. */ #undef HAVE_SECURE_GETENV@@ -430,6 +433,9 @@ /* Define to 1 if you have the 'sysconf' function. */ #undef HAVE_SYSCONF +/* Define to 1 if you have the <systemd/sd-device.h> header file. */ +#undef HAVE_SYSTEMD_SD_DEVICE_H + /* Define to 1 if you have the <sys/acl.h> header file. */ #undef HAVE_SYS_ACL_HSignature · 1 line
2.55.0
After discussing with Ted at LPC: It was discussed to use non-exclusive lock in e2fsck, but e2fsck cannot use a non-exclusive lock here, because of
how udevd backs off. It calls flock(fd, LOCK_SH | LOCK_NB) on the whole disk, and requeues the event only if that fails with EAGAIN (worker_lock_whole_disk() in udevd). Shared locks don't conflict, so a LOCK_SH held by e2fsck would not stop udevd from probing a half-written superblock. Only LOCK_EX does.
v4 changes:
Add a lock_whole_disk boolean to [options] in e2fsck.conf, default true, to disable the lock without rebuilding. It is documented in e2fsck.conf(5). The commit message now explains why LOCK_SH can't replace LOCK_EX, and describes the option. The libsystemd detection and the locking logic are unchanged from v3.
Quoted 6 lines · Nandakumar Raghavan
udevd already serialises its own partition probes against whole-disk device access using
flock(LOCK_SH|LOCK_NB); if EAGAIN is returned it requeues the event. Take advantage of this protocol by acquiringflock(LOCK_EX)on the whole-disk device before opening the filesystem. This forces udevd to defer all probes on that disk until e2fsck exits and the lock is released, by which point the filesystem is fully consistent.
I believe this was already part of fsck, and it conflicted with udevd and so was removed:
https://bugs.freedesktop.org/show_bug.cgi?id=79576
https://github.com/util-linux/util-linux/commit/3bbdae633f4a1dda5f95ee6c61f18a1c8ef12250
That particular problem was already fixed in systemd-udevd:
https://github.com/systemd/systemd/commit/5d354e525a5
But in general, does it really make sense for _only_ e2fsck to lock the device? Wouldn't it make more sense to add the device flock back into fsck itself? Or if the problem is during boot between systemd-fsck and systemd-udevd, wouldn't it be better to fix the issue in systemd-fsck?