So I played more and I think I answered my question myself. The default new behavior (no --rootfs passed) is not backward compatible. For example './scripts/build check fs=rofs' fails because test.py does not get/pass the proper value of --rootfs to run.py. I wonder if we have other cases/scripts when it breaks like that.
So I wonder if we tweak the 3rd patch in your series like so: - make --rootfs optional without default value of 'zfs' - if rootfs not specified like with test.py do old behavior - try rofs and then zfs I like ability to specify explicit root filesystem by I think we need to keep default behavior if a new parameter not specified. What do you think? Waldek On Thu, Aug 27, 2020 at 1:05 AM Waldek Kozaczuk <[email protected]> wrote: > I think all your patches look good (though I have not had a chance to test > it yet). > > I see you add new option '--rootfs' to explicitly tell which filesystem to > use. Now can you clarify if your changes to the mounting logic are backward > compatible? > > I other words when I do not specify '--rootfs', will it try to mounts > stuff in the same order it used to before your changes? I wonder if that > may break any existing scripts. > > Waldek > > On Wednesday, August 19, 2020 at 5:43:05 AM UTC-4 Fotis Xenakis wrote: > >> Signed-off-by: Fotis Xenakis <[email protected]> >> --- >> fs/vfs/main.cc | 52 +++++++++++++++++++++++++++++--------------------- >> loader.cc | 20 +++++++++---------- >> 2 files changed, 40 insertions(+), 32 deletions(-) >> >> diff --git a/fs/vfs/main.cc b/fs/vfs/main.cc >> index 3c8b327b..6cee319e 100644 >> --- a/fs/vfs/main.cc >> +++ b/fs/vfs/main.cc >> @@ -1593,7 +1593,7 @@ int faccessat(int dirfd, const char *pathname, int >> mode, int flags) >> return error; >> } >> >> -extern "C" >> +extern "C" >> int euidaccess(const char *pathname, int mode) >> { >> return access(pathname, mode); >> @@ -2375,45 +2375,53 @@ extern "C" void unmount_devfs() >> >> extern "C" int mount_rofs_rootfs(bool pivot_root) >> { >> - int ret; >> - >> - if (mkdir("/rofs", 0755) < 0) >> - kprintf("failed to create /rofs, error = %s\n", strerror(errno)); >> + constexpr char* mp = "/rofs"; >> >> - ret = sys_mount("/dev/vblk0.1", "/rofs", "rofs", MNT_RDONLY, 0); >> + if (mkdir(mp, 0755) < 0) { >> + int ret = errno; >> + kprintf("failed to create %s, error = %s\n", mp, strerror(errno)); >> + return ret; >> + } >> >> + int ret = sys_mount("/dev/vblk0.1", mp, "rofs", MNT_RDONLY, nullptr); >> if (ret) { >> - kprintf("failed to mount /rofs, error = %s\n", strerror(ret)); >> - rmdir("/rofs"); >> + kprintf("failed to mount %s, error = %s\n", mp, strerror(ret)); >> + rmdir(mp); >> return ret; >> } >> >> if (pivot_root) { >> - pivot_rootfs("/rofs"); >> + pivot_rootfs(mp); >> } >> >> return 0; >> } >> >> -extern "C" void mount_zfs_rootfs(bool pivot_root, bool extra_zfs_pools) >> +extern "C" int mount_zfs_rootfs(bool pivot_root, bool extra_zfs_pools) >> { >> - if (mkdir("/zfs", 0755) < 0) >> - kprintf("failed to create /zfs, error = %s\n", strerror(errno)); >> + constexpr char* mp = "/zfs"; >> >> - int ret = sys_mount("/dev/vblk0.1", "/zfs", "zfs", 0, (void >> *)"osv/zfs"); >> - >> - if (ret) >> - kprintf("failed to mount /zfs, error = %s\n", strerror(ret)); >> - >> - if (!pivot_root) { >> - return; >> + if (mkdir(mp, 0755) < 0) { >> + int ret = errno; >> + kprintf("failed to create %s, error = %s\n", mp, strerror(errno)); >> + return ret; >> } >> >> - pivot_rootfs("/zfs"); >> + int ret = sys_mount("/dev/vblk0.1", mp, "zfs", 0, (void *)"osv/zfs"); >> + if (ret) { >> + kprintf("failed to mount %s, error = %s\n", mp, strerror(ret)); >> + rmdir(mp); >> + return ret; >> + } >> >> - if (extra_zfs_pools) { >> - import_extra_zfs_pools(); >> + if (pivot_root) { >> + pivot_rootfs(mp); >> + if (extra_zfs_pools) { >> + import_extra_zfs_pools(); >> + } >> } >> + >> + return 0; >> } >> >> extern "C" void unmount_rootfs(void) >> diff --git a/loader.cc b/loader.cc >> index 66bfb52c..9d9d3173 100644 >> --- a/loader.cc >> +++ b/loader.cc >> @@ -87,7 +87,7 @@ extern "C" { >> void premain(); >> void vfs_init(void); >> void unmount_devfs(); >> - void mount_zfs_rootfs(bool,bool); >> + int mount_zfs_rootfs(bool, bool); >> int mount_rofs_rootfs(bool); >> void rofs_disable_cache(); >> } >> @@ -396,19 +396,20 @@ void* do_main_thread(void *_main_args) >> >> if (opt_mount) { >> unmount_devfs(); >> - // >> + >> // Try to mount rofs >> - if(mount_rofs_rootfs(opt_pivot) != 0) { >> - // >> + if (mount_rofs_rootfs(opt_pivot) != 0) { >> // Failed -> try to mount zfs >> zfsdev::zfsdev_init(); >> - mount_zfs_rootfs(opt_pivot, opt_extra_zfs_pools); >> + auto error = mount_zfs_rootfs(opt_pivot, opt_extra_zfs_pools); >> + if (error) { >> + debug("Could not mount zfs root filesystem.\n"); >> + } >> bsd_shrinker_init(); >> >> boot_time.event("ZFS mounted"); >> - } >> - else { >> - if(opt_disable_rofs_cache) { >> + } else { >> + if (opt_disable_rofs_cache) { >> debug("Disabling ROFS memory cache.\n"); >> rofs_disable_cache(); >> } >> @@ -491,8 +492,7 @@ void* do_main_thread(void *_main_args) >> >> if (opt_bootchart) { >> boot_time.print_chart(); >> - } >> - else { >> + } else { >> boot_time.print_total_time(); >> } >> >> -- >> 2.28.0 >> >> -- > You received this message because you are subscribed to the Google Groups > "OSv Development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To view this discussion on the web visit > https://groups.google.com/d/msgid/osv-dev/c64592fe-7f80-4661-8e25-a10d51ce44f6n%40googlegroups.com > <https://groups.google.com/d/msgid/osv-dev/c64592fe-7f80-4661-8e25-a10d51ce44f6n%40googlegroups.com?utm_medium=email&utm_source=footer> > . > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/CAL9cFfNtXC1hHK-XGgwJ3yL506no7MDzmj%2BUZgBz1Ekbr29GzA%40mail.gmail.com.
