From faf2a8416a1ab933918e3c5091c905194126b60c Mon Sep 17 00:00:00 2001 From: Andy Pan Date: Thu, 19 Oct 2023 15:55:56 +0800 Subject: [PATCH] internal/poll: revise the determination about [handled] and improve the code readability for SendFile There were a bit too many conditional branches in the old code, resulting in a poor readability. It could be more concise by reducing and consolidating some of the conditions. Furthermore, how we've determined whether or not the data transimission was handled by sendfile(2) seems inappropriate, because it marked the operation as unhandled whenever any non-retryable error occurs from calling sendfile(2), it doesn't look like a right approach, at least this is an inconsistent behavior with what we've done in Splice. Related to #64044 Change-Id: Ieb65e0879a8841654d0e64a1263a4e43179df1ba Reviewed-on: https://go-review.googlesource.com/c/go/+/537275 TryBot-Result: Gopher Robot Commit-Queue: Ian Lance Taylor Run-TryBot: Andy Pan Auto-Submit: Ian Lance Taylor Reviewed-by: Damien Neil Reviewed-by: Bryan Mills LUCI-TryBot-Result: Go LUCI --- src/internal/poll/sendfile_bsd.go | 35 ++++++++++--------------- src/internal/poll/sendfile_linux.go | 35 ++++++++++--------------- src/internal/poll/sendfile_solaris.go | 37 +++++++++++---------------- 3 files changed, 43 insertions(+), 64 deletions(-) diff --git a/src/internal/poll/sendfile_bsd.go b/src/internal/poll/sendfile_bsd.go index 0f55cad73d..8fcdb1c22e 100644 --- a/src/internal/poll/sendfile_bsd.go +++ b/src/internal/poll/sendfile_bsd.go @@ -13,51 +13,44 @@ import "syscall" const maxSendfileSize int = 4 << 20 // SendFile wraps the sendfile system call. -func SendFile(dstFD *FD, src int, pos, remain int64) (int64, error, bool) { +func SendFile(dstFD *FD, src int, pos, remain int64) (written int64, err error, handled bool) { if err := dstFD.writeLock(); err != nil { return 0, err, false } defer dstFD.writeUnlock() + if err := dstFD.pd.prepareWrite(dstFD.isFile); err != nil { return 0, err, false } dst := dstFD.Sysfd - var ( - written int64 - err error - handled = true - ) for remain > 0 { n := maxSendfileSize if int64(n) > remain { n = int(remain) } pos1 := pos - n, err1 := syscall.Sendfile(dst, src, &pos1, n) + n, err = syscall.Sendfile(dst, src, &pos1, n) if n > 0 { pos += int64(n) written += int64(n) remain -= int64(n) - } else if n == 0 && err1 == nil { - break } - if err1 == syscall.EINTR { + if err == syscall.EINTR { continue } - if err1 == syscall.EAGAIN { - if err1 = dstFD.pd.waitWrite(dstFD.isFile); err1 == nil { - continue - } + // This includes syscall.ENOSYS (no kernel + // support) and syscall.EINVAL (fd types which + // don't implement sendfile), and other errors. + // We should end the loop when there is no error + // returned from sendfile(2) or it is not a retryable error. + if err != syscall.EAGAIN { + break } - if err1 != nil { - // This includes syscall.ENOSYS (no kernel - // support) and syscall.EINVAL (fd types which - // don't implement sendfile) - err = err1 - handled = false + if err = dstFD.pd.waitWrite(dstFD.isFile); err != nil { break } } - return written, err, handled + handled = written != 0 || (err != syscall.ENOSYS && err != syscall.EINVAL) + return } diff --git a/src/internal/poll/sendfile_linux.go b/src/internal/poll/sendfile_linux.go index cc31969a43..c2a0653294 100644 --- a/src/internal/poll/sendfile_linux.go +++ b/src/internal/poll/sendfile_linux.go @@ -11,49 +11,42 @@ import "syscall" const maxSendfileSize int = 4 << 20 // SendFile wraps the sendfile system call. -func SendFile(dstFD *FD, src int, remain int64) (int64, error, bool) { +func SendFile(dstFD *FD, src int, remain int64) (written int64, err error, handled bool) { if err := dstFD.writeLock(); err != nil { return 0, err, false } defer dstFD.writeUnlock() + if err := dstFD.pd.prepareWrite(dstFD.isFile); err != nil { return 0, err, false } dst := dstFD.Sysfd - var ( - written int64 - err error - handled = true - ) for remain > 0 { n := maxSendfileSize if int64(n) > remain { n = int(remain) } - n, err1 := syscall.Sendfile(dst, src, nil, n) + n, err = syscall.Sendfile(dst, src, nil, n) if n > 0 { written += int64(n) remain -= int64(n) - } else if n == 0 && err1 == nil { + continue + } else if err != syscall.EAGAIN && err != syscall.EINTR { + // This includes syscall.ENOSYS (no kernel + // support) and syscall.EINVAL (fd types which + // don't implement sendfile), and other errors. + // We should end the loop when there is no error + // returned from sendfile(2) or it is not a retryable error. break } - if err1 == syscall.EINTR { + if err == syscall.EINTR { continue } - if err1 == syscall.EAGAIN { - if err1 = dstFD.pd.waitWrite(dstFD.isFile); err1 == nil { - continue - } - } - if err1 != nil { - // This includes syscall.ENOSYS (no kernel - // support) and syscall.EINVAL (fd types which - // don't implement sendfile) - err = err1 - handled = false + if err = dstFD.pd.waitWrite(dstFD.isFile); err != nil { break } } - return written, err, handled + handled = written != 0 || (err != syscall.ENOSYS && err != syscall.EINVAL) + return } diff --git a/src/internal/poll/sendfile_solaris.go b/src/internal/poll/sendfile_solaris.go index f9f685c64a..1ba0c8d064 100644 --- a/src/internal/poll/sendfile_solaris.go +++ b/src/internal/poll/sendfile_solaris.go @@ -16,29 +16,25 @@ import "syscall" const maxSendfileSize int = 4 << 20 // SendFile wraps the sendfile system call. -func SendFile(dstFD *FD, src int, pos, remain int64) (int64, error, bool) { +func SendFile(dstFD *FD, src int, pos, remain int64) (written int64, err error, handled bool) { if err := dstFD.writeLock(); err != nil { return 0, err, false } defer dstFD.writeUnlock() + if err := dstFD.pd.prepareWrite(dstFD.isFile); err != nil { return 0, err, false } dst := dstFD.Sysfd - var ( - written int64 - err error - handled = true - ) for remain > 0 { n := maxSendfileSize if int64(n) > remain { n = int(remain) } pos1 := pos - n, err1 := syscall.Sendfile(dst, src, &pos1, n) - if err1 == syscall.EAGAIN || err1 == syscall.EINTR { + n, err = syscall.Sendfile(dst, src, &pos1, n) + if err == syscall.EAGAIN || err == syscall.EINTR { // partial write may have occurred n = int(pos1 - pos) } @@ -46,25 +42,22 @@ func SendFile(dstFD *FD, src int, pos, remain int64) (int64, error, bool) { pos += int64(n) written += int64(n) remain -= int64(n) - } else if n == 0 && err1 == nil { + continue + } else if err != syscall.EAGAIN && err != syscall.EINTR { + // This includes syscall.ENOSYS (no kernel + // support) and syscall.EINVAL (fd types which + // don't implement sendfile), and other errors. + // We should end the loop when there is no error + // returned from sendfile(2) or it is not a retryable error. break } - if err1 == syscall.EAGAIN { - if err1 = dstFD.pd.waitWrite(dstFD.isFile); err1 == nil { - continue - } - } - if err1 == syscall.EINTR { + if err == syscall.EINTR { continue } - if err1 != nil { - // This includes syscall.ENOSYS (no kernel - // support) and syscall.EINVAL (fd types which - // don't implement sendfile) - err = err1 - handled = false + if err = dstFD.pd.waitWrite(dstFD.isFile); err != nil { break } } - return written, err, handled + handled = written != 0 || (err != syscall.ENOSYS && err != syscall.EINVAL) + return } -- 2.48.1