| Message ID | 20201118153947.3394530-5-saranyamohan@google.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Introduce parallel fsck to e2fsck pass1 | expand |
On Wed, Nov 18, 2020 at 07:38:50AM -0800, Saranya Muruganandam wrote: > From: Wang Shilong <wshilong@ddn.com> > > icache of fs will be rebuilt when needed, so after copying > fs, icache can be inited to NULL. > > Signed-off-by: Li Xi <lixi@ddn.com> > Signed-off-by: Wang Shilong <wshilong@ddn.com> > Signed-off-by: Saranya Muruganandam <saranyamohan@google.com> So this is part of the whole "how is the fs structure is shared question". It's going to be better if this is all in a single patch, instead of being spread out into separate patches. Certainly from a review perspective, it's going to be much simpler.... > +static void e2fsck_pass1_merge_fs(ext2_filsys dest, ext2_filsys src) > +{ > + struct ext2_inode_cache *icache = dest->icache; > + > + memcpy(dest, src, sizeof(struct struct_ext2_filsys)); > + if (dest->dblist) > + dest->dblist->fs = dest; > + if (dest->inode_map) > + dest->inode_map->fs = dest; > + if (dest->block_map) > + dest->block_map->fs = dest; > + dest->icache = icache; > + > + if (src->icache) { > + ext2fs_free_inode_cache(src->icache); > + src->icache = NULL; > + } > } I'm not sure how the merge_fs function is supposed to work. Right now it's not doing much in the way of merging; I'm guessing how it works is going to be done in a future patch, but this is going to make review a bit tricky. .... peaking ahead in the patch series. Yeah, I think we need to think very carefully about how we do stuff here. It looks very ad hoc, with some aspects of the merge functionality in e2fsck/pass1.c, and some in libext2fs. I wonder if we wouldn't be better off with the concept of a top-level fs structure, and child fs structures which point back to the top-level fs structure --- much like what is currently being done with the e2fsck_t structure. So if fs->parent_fs is non-NULL, we know that this is a child fs structure, and there are very clear rules about how things like io_managers are shared --- or not. Note that some io_managers, such as undo io manager, fundamentally *have* to be shared, which implies that we need to have some kind of flag indicating whether or not an io_manager is thread-aware or not. And if an io_manager is not thread-aware, then it wouldn't be safe to use it in multi-threading mode. I'm going to stop doing a fine-grained review of this patch series, and it seems pretty clear to me that we need to have some fundamental architectural discussions about how to make multi-threading work. An ad-hoc approach is great for a prototype, as this surfaces all of these sorts of questions --- but I think we need to now stop, and think very carefully about how to make multi-threading support something which can be a long-term maintainable code base. And that means figuring out what the semantics should be about various shared data sructures, and then documenting how things are supposed to work very explicitly. (I note that many of these functions don't have much in the way of documentation, which is unfortunate from the prespective of some future developer who is wishing to further extend this work.) - Ted
diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c index 5b4947b0..ba513d91 100644 --- a/e2fsck/pass1.c +++ b/e2fsck/pass1.c @@ -2086,8 +2086,10 @@ endit: ctx->invalid_bitmaps++; } -static void e2fsck_pass1_copy_fs(ext2_filsys dest, ext2_filsys src) +static errcode_t e2fsck_pass1_copy_fs(ext2_filsys dest, ext2_filsys src) { + errcode_t retval; + memcpy(dest, src, sizeof(struct struct_ext2_filsys)); if (dest->dblist) dest->dblist->fs = dest; @@ -2095,6 +2097,29 @@ static void e2fsck_pass1_copy_fs(ext2_filsys dest, ext2_filsys src) dest->inode_map->fs = dest; if (dest->block_map) dest->block_map->fs = dest; + + /* icache will be rebuilt if needed, so do not copy from @src */ + src->icache = NULL; + return 0; +} + +static void e2fsck_pass1_merge_fs(ext2_filsys dest, ext2_filsys src) +{ + struct ext2_inode_cache *icache = dest->icache; + + memcpy(dest, src, sizeof(struct struct_ext2_filsys)); + if (dest->dblist) + dest->dblist->fs = dest; + if (dest->inode_map) + dest->inode_map->fs = dest; + if (dest->block_map) + dest->block_map->fs = dest; + dest->icache = icache; + + if (src->icache) { + ext2fs_free_inode_cache(src->icache); + src->icache = NULL; + } } static errcode_t e2fsck_pass1_thread_prepare(e2fsck_t global_ctx, e2fsck_t *thread_ctx) @@ -2118,12 +2143,18 @@ static errcode_t e2fsck_pass1_thread_prepare(e2fsck_t global_ctx, e2fsck_t *thre goto out_context; } - e2fsck_pass1_copy_fs(thread_fs, global_fs); + retval = e2fsck_pass1_copy_fs(thread_fs, global_fs); + if (retval) { + com_err(global_ctx->program_name, retval, "while copying fs"); + goto out_fs; + } thread_fs->priv_data = thread_context; thread_context->fs = thread_fs; *thread_ctx = thread_context; return 0; +out_fs: + ext2fs_free_mem(&thread_fs); out_context: ext2fs_free_mem(&thread_context); return retval; @@ -2147,7 +2178,7 @@ static int e2fsck_pass1_thread_join(e2fsck_t global_ctx, e2fsck_t thread_ctx) global_ctx->flags |= (flags & E2F_FLAG_SIGNAL_MASK) | (global_ctx->flags & E2F_FLAG_SIGNAL_MASK); - e2fsck_pass1_copy_fs(global_fs, thread_fs); + e2fsck_pass1_merge_fs(global_fs, thread_fs); global_fs->priv_data = global_ctx; global_ctx->fs = global_fs;