Page MenuHomeFreeBSD

loader: open file list should be dynamic
ClosedPublic

Authored by tsoome on Jul 31 2021, 8:11 AM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 23 2024, 7:13 PM
Unknown Object (File)
Jan 5 2024, 3:47 AM
Unknown Object (File)
Dec 20 2023, 3:37 AM
Unknown Object (File)
Sep 17 2023, 3:16 AM
Unknown Object (File)
Sep 15 2023, 5:33 PM
Unknown Object (File)
Aug 25 2023, 7:07 AM
Unknown Object (File)
Aug 17 2023, 1:19 PM
Unknown Object (File)
Jul 3 2023, 5:07 PM
Subscribers

Details

Summary

Open file list is currently created as statically allocated array (64 items).
Once this array is filled up, loader will not be able to operate with files.
In most cases, this mechanism is good enough, but the problem appears, when
we have many disks with zfs pool(s). In current loader implementation, all
discovered zfs pool configurations are kept in memory and disk devices open -
consuming the open file array. Rewrite the open file mechanism to use
dynamically allocated list.

Diff Detail

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

Event Timeline

stand/libsa/close.c
74–75

This should be a function fd2open_file(fd) or something. It's repeated a lot.

stand/libsa/closeall.c
44

nit: casts like this usually don't have a space.

stand/libsa/lseek.c
34–35

This file has a lot of white space changes that makes it a little harder to review.... If possible, can you split the changes between functional and whitespace?

stand/libsa/netif.c
306

same comment for the socket list as I had for the file list: this lookup code is repeated 3 times, which is my usual threshold...
Even so, having similar patterns would help people read the code more easily.

this is coming along nicely. I have only a couple of questions around closing files.

stand/libsa/close.c
95

why do you free them only from the tail?

stand/libsa/closeall.c
47

This is a total re-write of this function / file. Whomever wrote it (you or your client) should likely remove the old licenses and put a standard BSD-2-Clause license in its place.

stand/libsa/netif.c
374

Same question as I had for the other close routine: Why only free from the end?

stand/libsa/open.c
125

I think this is my answer to the questions. But you should document the requirement that the list be ordered by fd,.
The reason I say "I think" is because I don't think we need to keep the freed internal entries if we want to preserve the behavior that the last open_file* on the list has the highest f_id

stand/libsa/close.c
95

The reason is actually pretty simple. We do need unique ID for files (and sockets). If we switch to use list, we have three alternates:

  1. keep list ID's ordered (as this implementation does).
  2. Implement ID search by walking the list
  3. implement ID space with allocate and free.

I did pick first option because in most cases, the fd is really short lived and with one exception, we practically do not keep multiple files open. This one exception is zfs pools - pools are discovered, and "imported", with disk devices kept open and practically never closed...

tsoome added inline comments.
stand/libsa/close.c
95

comment added to open.c

stand/libsa/netif.c
374

comment added (next to list declaration).

stand/libsa/open.c
125

Yea, we can use different strategies to handle ID. But it is not just about how we manage ID, but also combination with entry search for lookup/remove/add... anyhow, since both open file and sockets lists do have special properties there, I think this approach is reasonably good... :)

tsoome marked 2 inline comments as done.

Suggestions from imp.

I'll note that we could remove from the middle of the list and maintain the current preconditions for the code. However, there's so few files open that this optimization is optional and likely not worth making.

This revision is now accepted and ready to land.Aug 11 2021, 5:20 PM
This revision was automatically updated to reflect the committed changes.