Page MenuHomeFreeBSD

Fix poor performance of ftp(1) due to small SO_SNDBUF and SO_RCVBUF
ClosedPublic

Authored by hrs on Feb 17 2020, 7:07 PM.

Details

Summary

ftp(1) from vendor/tnftp always tries the following for
every TCP connection:

  1. Get the current buffer length of SO_SNDBUF and SO_RCVBUF by getsockopt(2).
  1. Invoke setsockopt(2) to set them to the same values after checking if they are in a range between 8 KiB to 8 MiB.

This behavior breaks dynamic buffer sizing enabled by
default (see net.inet.tcp.{recv,send}buf_auto sysctls)
and leads to a very poor transfer rate. fetch(1) does
not have this problem. See also Bug 240827.

This change prevents SO_SNDBUF and SO_RCVBUF from configuring
when the auto buffer sizing is enabled unless they are
explicitly specified.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

donner added inline comments.
tnftp/dist/src/util.c
1118–1120 ↗(On Diff #68466)

Such out of scope results of the preprocessor opens a can or worms for other. Consider a patch which modifies the following line by inserting an additional command. This extra command will be commented out depending on conditional variables (FreeBSD), causing complete different behaviour.

tnftp/dist/src/util.c
1118–1120 ↗(On Diff #68466)

I could not understand your point. Can you provide your suggested change?

Using #ifdef makes the readability worse but does not affect the intended behavior. I do not want to change the original lines wherever possible because this file is not maintained in the FreeBSD tree.

tnftp/dist/src/util.c
1118–1120 ↗(On Diff #68466)

Okay, that's a valid reason and a good solution.

I'd prefer unconditionally visible variable "auto_sndbuf", with has a FreeBSD specific initialization only. But you point is valid, too.

So let me suggest the following approach to avoid code duplication and prevent interactin with foreign logic chains:

#ifdef __FreeBSD__
        DPRINTF("auto_rcvbuf = %d\n", auto_rcvbuf);
        if (auto_rcvbuf == 0) {
#endif
        if (setsockopt ... SO_SNDBUF ...
        if (setsockopt ... SO_RCVBUF ...
#ifdef __FreeBSD__
        }
#endif

Fix the lines pointed out in the review.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 27 2020, 7:50 PM
This revision was automatically updated to reflect the committed changes.