Page MenuHomeFreeBSD

ufs/suspend: deny suspension if calliing process has file from mp opened for write
ClosedPublic

Authored by kib on Dec 28 2022, 11:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 18, 10:59 PM
Unknown Object (File)
Tue, Apr 16, 3:28 AM
Unknown Object (File)
Thu, Apr 11, 4:29 PM
Unknown Object (File)
Feb 5 2024, 2:13 AM
Unknown Object (File)
Jan 31 2024, 9:51 AM
Unknown Object (File)
Jan 12 2024, 2:52 PM
Unknown Object (File)
Jan 11 2024, 10:13 AM
Unknown Object (File)
Jan 3 2024, 3:05 PM
Subscribers

Details

Summary
Also deny suspension if we cannot check the above condition race-free
because there is more than one thread in the calling process.

PR:     267628, 267630
ffs_suspend.c: clean up includes

Order includes alphabetically.
Remove unneeded sys/param.h, it is already included by sys/systm.h.
Add descrip_check_write_mp() helper

... which verifies that given file table does not have file descriptors
referencing vnodes on the specified mount point.  It is up to the caller
to ensure that the check is not racy.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib requested review of this revision.Dec 28 2022, 11:38 PM

This approach will certainly fix the problem.

The issue is that growfs is often used to grow the only existing filesystem of a weekly-build image. Thus it is the only place that output can be saved (though a tmpfs could be mounted and then the result copied back when the growfs finished). As long as no writes are done while the filesystem is suspended it is safe to have the file open on the growfs partition. All writes done before the suspend will be sync'ed as part of the suspend. The filesystem is reloaded before the suspend is exited, so any further writes will happen to the newly grown filesystem. But one cannot depend on the application to avoid writes during the suspend and code to detect and error them out is complicated and in a hot path, so I think that this approach is better.

sys/ufs/ffs/ffs_suspend.c
308–309 ↗(On Diff #114620)

The check below does enforce single-thread curproc, so shouldn't this comment just say
/* Require single-thread curproc so that the check is not racey. */

This revision is now accepted and ready to land.Dec 29 2022, 12:42 AM
kib marked an inline comment as done.Dec 29 2022, 12:48 AM

This approach will certainly fix the problem.

It fixes this problem, and several others reported since live grow was introduced. Or rather, it does not allow the reported problems to occur, in a user-visible way. Unfortunately, I do not see a way to ensure that random termination of growfs does not wedge fs without also having this limitation.

The issue is that growfs is often used to grow the only existing filesystem of a weekly-build image. Thus it is the only place that output can be saved (though a tmpfs could be mounted and then the result copied back when the growfs finished). As long as no writes are done while the filesystem is suspended it is safe to have the file open on the growfs partition. All writes done before the suspend will be sync'ed as part of the suspend. The filesystem is reloaded before the suspend is exited, so any further writes will happen to the newly grown filesystem. But one cannot depend on the application to avoid writes during the suspend and code to detect and error them out is complicated and in a hot path, so I think that this approach is better.

Use wording suggested by Kirk, for part of the comment. Keep single-threading note as a placeholder for possible future improvement.

This revision now requires review to proceed.Dec 29 2022, 12:50 AM
This revision is now accepted and ready to land.Dec 29 2022, 5:20 AM