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.
Tags
None
Referenced Files
F105637736: D23732.id68466.diff
Wed, Dec 18, 2:53 PM
Unknown Object (File)
Sat, Nov 30, 3:01 AM
Unknown Object (File)
Sat, Nov 30, 3:00 AM
Unknown Object (File)
Sat, Nov 30, 3:00 AM
Unknown Object (File)
Sat, Nov 30, 3:00 AM
Unknown Object (File)
Wed, Nov 27, 3:07 AM
Unknown Object (File)
Nov 17 2024, 9:38 AM
Unknown Object (File)
Oct 29 2024, 12:31 PM
Subscribers

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
Lint Not Applicable
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.