loader: implement mount/unmount rootfs

[tsoome]

Summary

We want to keep our root file system open to preserve bcache segment
between file accesses, thus reducing physical disk IO.

TODO: implement mount/unmount for cd9660/msdosfs/zfs.

[jhb]

FWIW, the original use case was floppies and that you had to have the bcache_free so that when you swapped floppies (for use with splitfs or kernel and mfsroot on separate floppies), the loader would always read data from the new floppy. However, that use case is no longer relevant.

[allanjude]

In D30848#693919, @jhb wrote:

FWIW, the original use case was floppies and that you had to have the bcache_free so that when you swapped floppies (for use with splitfs or kernel and mfsroot on separate floppies), the loader would always read data from the new floppy. However, that use case is no longer relevant.

That explains the behaviour, but I agree it doesn't make sense anymore

[allanjude]

Reviewed by: allanjude

[cperciva]

This also returns with pd->pd_blkio set to non-NULL. Is this intentional?

[cperciva]

Tested with HEAD in EC2 on arm64:

Booting in BIOS mode, this cuts the loader time from 5.38s to 3.66s.

Booting in EFI mode, this cuts the loader time from 0.962s to 0.634s.

The difference in time looks like it's almost all from I/Os being performed by open, which is consistent with the caching of disk and filesystem metadata.

[tsoome]

do not set pd_blkio to NULL.

We should either just leave it initialized or use CloseProtocol() call and
then set NULL.

[tsoome]

In D30848#694017, @cperciva wrote:

This also returns with pd->pd_blkio set to non-NULL. Is this intentional?

Yes. we do have disk list allocated, so we can just keep the protocol handles. The alternate would be to call CloseProtocol() and then set it NULL.

[imp]

stand/efi/libefi/efipart.c

955 ↗

(On Diff #91198)

I wonder if we shouldn't just bump pd->pd_open when we set currdev and decrement it when we clear it. would that be simpler?
That way, we'd flush the cache on change. As it is, the cache isn't flushed even when the open count reaches 0.

[imp]

stand/efi/libefi/efipart.c

955 ↗

(On Diff #91198)

maybe the simplest way to accomplish that is to call open on the new currdev and close on the old one when we change it.

[tsoome]

stand/efi/libefi/efipart.c

955 ↗

(On Diff #91198)

maybe the simplest way to accomplish that is to call open on the new currdev and close on the old one when we change it.

Yes, that was what I was starting to think too. And a bit more - this open call is not just open, but we should ad moun/unmount calls for filesystems and setting currdev should perform the unmount and mount. This will cache us the filesystem root handle and we do not have to reread superblock or call rpc mount and the bcache reference counting will behave as it should.

[tsoome]

This is complete rework. Instead of hacking into not releasing
bcache, we "mount" filesystem (if supported) while we set "currdev"
environment variable. By mounting, we create open disk device and we
do keep it open till this file system will get unmounted, and the
bcache segment for this devei is preserved.

At this iteration, only ufs implementation is provided.

[imp]

So I generally like this patch, despite what I fear is the negative impression that my detailed feedback might leave. Much of that detailed feedback is due, in part, to the pre-existing condition that malloc and friends do not set errno when returning NULL, which looks easy to remedy.

stand/efi/libefi/devicename.c

214–225

I'd make this a function because it's repeated a bunch of times.

218

When you make this a function, I'd suggest "Note we unmount any previously mounted fs only after successfully mounting the new because..." might be a better comment...

stand/i386/libi386/devicename.c

32

why include sys/mount.h? What's coming in from there? If it's just mount/umount prototypes, I'd suggest that maybe we should just put those in stand.h.

stand/libsa/mount.c

101

I would think we'd want to keep a mount count and only free things up in umount if that count, after decrementing, falls to 0. That would avoid cases where we umount twice because we mounted twice.
Alternatively, maybe we should make mounting a mounted filesystem something to warn about. It just feels weird to succeed here there's multiple mounts, but then at least whine in the unmount path.

108

The idiomatic way is to say if (fo->fo_mount == NULL) continue so that we can move the indentation back a level.

114

do we need to call fo_umount() here if add_mnt_info fails?

121

Why do we always succeed, even when we've failed to mount it?

133

We're not checking the return value here. ufs_umount doesn't return anything but 0, but others might in the future.

stand/libsa/ufs.c

921

In the mount code, we test against -1, but errno is going to be a small positive number so I think that's a mismatch.
Though I'm not sure where errno gets set for a calloc failure since znalloc doesn't seem to set it...

925

I'm not sure where strdup sets errno on failure. It should be in malloc, but that's the same code path as calloc so same question.

928

Same question here too. Sorry to be so tedious :)

930

At least for open I can fund where we set errno :)

[tsoome]

Addressing first round of comments and suggestions.

[tsoome]

stand/libsa/mount.c

121

It is temporary code there (I'll mark it with comment). In this version, the fo_mount/fo_unmount callbacks are only added for ufs, we definitely want them for most/all disk based file systems, but *maybe* not for some virtual file systems (pkgfs?). Therefore it just needs some work....

stand/libsa/ufs.c

921

malloc/calloc, memalign etc do get errno set from sbr() - when heap is full, sbrk() is called.

At this time, the mount() code is considering fo_mount() result 0 success and anything else fail.

[imp]

much better, and thanks for the malloc errno ptr.
one layering violation, and maybe a stale errno issue, but otherwise coming together nicely.

stand/libsa/mount.c

148

classically, I'd expect the ref count to start at 1, not 0, so this is confusing to me. The code is technically correct, but I worry it's more fragile since it uses an unfamiliar pattern.

169

I think this should live in common/main.c rather than here because it's /boot/loader specific and libsa isn't only for boot loader.

stand/libsa/stand.h

349

Likewise, I'd put this in common/bootstrap.h

stand/libsa/ufs.c

937

You should set errno = 0 at the top of this routine, otherwise you can get stale versions of errno and think there was an error when there wasn't.

[tsoome]

more comments.

And bugfix: setting up variable with env_setenv first time does
not call hooks. So we set up variable with empty value and call
with real value will call set hook.

[cperciva]

Confirmed that this latest version significantly reduces EC2 boot time.

[tsoome]

add zfs_mount and zfs_unmount.

While we do know, zfs pools are not closing open disk devices,
we still can save a bit from not doing IO.

[tsoome]

update cd9660 to have mount/unmount.

[tsoome]

implement mount/unmount for dosfs.

[cperciva]

Can this be committed now, with other mount/unmount capabilities added later?

[tsoome]

In D30848#713814, @cperciva wrote:

Can this be committed now, with other mount/unmount capabilities added later?

I have the needed bits done, just working with some testing. I do hope to be done soon enough.

[imp]

Looks good to me. Anything that we might have missed that didn't get caught in testing we can fix later if need be.

[tsoome]

added support for more file systems.