Re: Closing all FDs

On 04/22/2014 03:01 AM, Michal Luscon wrote:

> I am wondering whether it would be possible to replace SQUID_MAXFD by
> Biggest_FD in the following code snippet from ipc.cc:

There are several problems with using Biggest_FD. In short, it is not
100% safe to use it now, and it is probably the wrong long-term
solution. Details below.

> /* Make sure all other filedescriptors are closed */
> for (x = 3; x < SQUID_MAXFD; ++x)
> close(x);

Biggest_FD tracks descriptors opened through the Comm API (comm_open and
friends, all calling fd_open at the end). Biggest_FD does not account
descriptors opened without calling fd_open, unfortunately. There are
probably many such cases. Here is a partial list of such out-of-comm
open call candidates (some are missing and some are not applicable, I am
sure):

> src/comm/ModDevPoll.cc: devpoll_fd = open("/dev/poll", O_RDWR);
> src/ip/Intercept.cc: natfd = open(IPNAT_NAME, O_RDONLY, 0);
> src/ip/Intercept.cc: natfd = open(IPL_NAT, O_RDONLY, 0);
> src/ip/Intercept.cc: pffd = open("/dev/pf", O_RDONLY);
> src/fs/rock/RockSwapDir.cc: const int swap = open(filePath, O_WRONLY|O_CREAT|O_TRUNC|O_BINARY, 0600);
> src/disk.cc: fd = open(path, mode, 0644);
> src/DiskIO/DiskDaemon/diskd.cc: fd = open(buf, r->offset, 0600);
> src/DiskIO/DiskThreads/aiops_win32.cc: requestp->ret = open(requestp->path, requestp->oflag, requestp->mode);
> src/DiskIO/DiskThreads/aiops.cc: requestp->ret = open(requestp->path, requestp->oflag, requestp->mode);
> src/ssl/certificate_db.cc: fd = open(filename.c_str(), 0);
> src/main.cc: if ((i = open("/dev/tty", O_RDWR | O_TEXT)) >= 0) {
> src/main.cc: nullfd = open(_PATH_DEVNULL, O_RDWR | O_TEXT);

Ideally, all such calls should be found and, if needed, adjusted to call
fd_open or equivalent before you can use Biggest_FD reliably, but it
would be difficult to make sure we find all of them.

Also, for this specific loop, on platforms that support FD_CLOEXEC, we
only need to close descriptors that do not have COMM_NOCLOEXEC flag set
but should have it. A better approach for those platforms would be to
make sure all descriptors have appropriate FD_CLOEXEC setting. If we do
that, the loop itself can probably be removed for those platforms!

Finally, please note that Biggest_FD should be within 50% of SQUID_MAXFD
on a fully loaded and properly configured Squid, so using Biggest_FD
here is not going to help much with performance in the
interesting/important case. Removing the loop as discussed above seems a
far better solution AFAICT.

> The motivation behind this request is to speed up reloading Squid in
> paravirtualized environment.

FWIW, the above loop also causes a ton of bogus valgrind warnings
because many of the descriptors it is trying to close are invalid.

HTH,

Alex.
Received on Tue Apr 22 2014 - 15:16:25 MDT