Re: [PATCH] e2fsck: take flock(LOCK_EX) on whole-disk device during filesystem check

On Aug 14, 2026, at 23:29, Nandakumar Raghavan <[email protected]> wrote:
> 
> Hi,
> 
> Gentle ping on this patch.
> 
> I would appreciate any feedback when time permits.
> 
> On Mon, Jul 20, 2026 at 02:32:46PM +0000, Nandakumar Raghavan wrote:
>> 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.
>> 
>> 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.

>> ---
>> e2fsck/unix.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 105 insertions(+)
>> 
>> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
>> index 335ca377..12a9f50a 100644
>> --- a/e2fsck/unix.c
>> +++ b/e2fsck/unix.c
>> @@ -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.

>> #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:
>> +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.

>> +{
>> + 	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);

(minor) this could reuse `partition_attr` here?

>> +		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);

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

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

>> +	}
>> +
>> + 	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?

>> + 		}
>> + 		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);
>> -- 
>> 2.54.0
> 


Cheers, Andreas