Re: [PATCH v3] Fix race condition in ext2fs when remounting
Not sure how I missed that; will grep for any others. James On 14 Aug 2015, at 13:16, Justus Winter 4win...@informatik.uni-hamburg.de wrote: Quoting James Clarke (2015-07-23 19:33:42) diff --git a/libdiskfs/disk-pager.c b/libdiskfs/disk-pager.c index 008aa2d..33b109c 100644 --- a/libdiskfs/disk-pager.c +++ b/libdiskfs/disk-pager.c @@ -24,6 +24,7 @@ __thread struct disk_image_user *diskfs_exception_diu; struct pager *diskfs_disk_pager; +struct requests *diskfs_disk_pager_requests; struct pager_requests Justus
Re: [PATCH v3] Fix race condition in ext2fs when remounting
Quoting James Clarke (2015-07-23 19:33:42) diff --git a/libdiskfs/disk-pager.c b/libdiskfs/disk-pager.c index 008aa2d..33b109c 100644 --- a/libdiskfs/disk-pager.c +++ b/libdiskfs/disk-pager.c @@ -24,6 +24,7 @@ __thread struct disk_image_user *diskfs_exception_diu; struct pager *diskfs_disk_pager; +struct requests *diskfs_disk_pager_requests; struct pager_requests Justus
Re: [PATCH v3] Fix race condition in ext2fs when remounting
Hi! First, thanks for working on this issue! On Sat, 25 Jul 2015 14:06:24 +0200, Justus Winter 4win...@informatik.uni-hamburg.de wrote: What happened to the corresponding fatfs change though? Isofs is read-only, and tmpfs' diskfs_reload_global_state is a nop too. So modulo the missing fatfs fix this patch looks good to me. Justus, thanks for working with James, reviewing his patch! Samuel, Thomas, is this still a smallish change that we can just merge, or do we need to do the paperwork now? I'm afraid, we'll have to... :-/ James, are you familiar with the FSF copyright assignment process? For example, see http://www.gnu.org/licenses/why-assign.en.html, and https://www.gnu.org/prep/maintain/html_node/Copyright-Papers.html. If that is OK for you, the simplest thing to do is to use the form to assign to the FSF all past and future contributions to the respective GNU packages. See the attached request-assign.future. After sending this information to ass...@gnu.org (please put me in CC so that I can track it), you'll receive a form to sign, and then mail this to the FSF, or send a scan of it via email, as applicable. For »program or package you're contributing to«, I suggest to specify all of: Mach, MIG, Hurd, glibc. This is to avoid having to repeat the whole procedure when touching those closely related packages, working on other parts of the Hurd code base later on. Grüße, Thomas Please email the following information to ass...@gnu.org, and we will send you the assignment form for your past and future changes. Please use your full legal name (in ASCII characters) as the subject line of the message. -- REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES [What is the name of the program or package you're contributing to?] [Did you copy any files or text written by someone else in these changes? Even if that material is free software, we need to know about it.] [Do you have an employer who might have a basis to claim to own your changes? Do you attend a school which might make such a claim?] [For the copyright registration, what country are you a citizen of?] [What year were you born?] [Please write your email address here.] [Please write your postal address here.] [Which files have you changed so far, and which new files have you written so far?] signature.asc Description: PGP signature
Re: [PATCH v3] Fix race condition in ext2fs when remounting
Quoting Justus Winter (2015-07-25 03:54:59) I'm merely wondering whether we shouldn't protect diskfs_reload_global_state with a lock. I'm not sure if concurrent remounts are a problem, but they could very well be. For the record, this is not a problem. There is the global diskfs_fsys_lock that prevents that. What happened to the corresponding fatfs change though? Isofs is read-only, and tmpfs' diskfs_reload_global_state is a nop too. So modulo the missing fatfs fix this patch looks good to me. Samuel, Thomas, is this still a smallish change that we can just merge, or do we need to do the paperwork now? Justus
Re: [PATCH v3] Fix race condition in ext2fs when remounting
Hi James :)
Quoting James Clarke (2015-07-23 19:33:42)
On some systems, ext2fs.static would regularly hang at startup, as a
race condition meant it would process paging requests while remounting.
To fix this, libpager has been altered to allow inhibiting and resuming
its worker threads, and ext2fs uses this to inhibit paging while
remounting.
I believe this is the correct solution. It's nicely written, and well
documented. Good work :)
I'm merely wondering...
diff --git a/ext2fs/ext2fs.c b/ext2fs/ext2fs.c
index d0fdfe7..03c9eed 100644
--- a/ext2fs/ext2fs.c
+++ b/ext2fs/ext2fs.c
@@ -207,10 +207,20 @@ main (int argc, char **argv)
error_t
diskfs_reload_global_state ()
{
... whether we shouldn't protect this function with a lock. I'm not
sure if concurrent remounts are a problem, but they could very well
be.
Justus
+ error_t err;
+
pokel_flush (global_pokel);
pager_flush (diskfs_disk_pager, 1);
- sblock = NULL;
+
+ /* libdiskfs is not responsible for inhibiting paging. */
+ err = inhibit_ext2_pager ();
+ if (err)
+return err;
+
get_hypermetadata ();
map_hypermetadata ();
+
+ resume_ext2_pager ();
+
return 0;
}
