From 4f50af70efeaef2c7305719df7bc070be604793d Mon Sep 17 00:00:00 2001 From: thexai <58434170+thexai@users.noreply.github.com> Date: Thu, 2 Mar 2023 20:22:12 +0100 Subject: [PATCH 1/4] NFSFile: adapt timeouts for compatibility with NFSv4 --- xbmc/filesystem/NFSFile.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/xbmc/filesystem/NFSFile.cpp b/xbmc/filesystem/NFSFile.cpp index e7179c11007b3..c32e79ff5d69a 100644 --- a/xbmc/filesystem/NFSFile.cpp +++ b/xbmc/filesystem/NFSFile.cpp @@ -45,16 +45,16 @@ using namespace std::chrono_literals; namespace { +// Default "lease_time" on most Linux NFSv4 servers are 90s. +// See: https://linux-nfs.org/wiki/index.php/NFS_lock_recovery_notes +// Keep alive interval should be always less than lease_time to avoid client session expires -constexpr auto CONTEXT_TIMEOUT = 6min; - -constexpr auto KEEP_ALIVE_TIMEOUT = 3min; - -constexpr auto IDLE_TIMEOUT = 3min; +constexpr auto CONTEXT_TIMEOUT = 60s; // 2/3 parts of lease_time +constexpr auto KEEP_ALIVE_TIMEOUT = 45s; // half of lease_time +constexpr auto IDLE_TIMEOUT = 30s; // close fast unused contexts when no active connections constexpr auto SETTING_NFS_VERSION = "nfs.version"; - -} // namespace +} // unnamed namespace CNfsConnection::CNfsConnection() : m_pNfsContext(NULL), From b581bb8dd23eff1da4b7368adc9bf6c9a1797809 Mon Sep 17 00:00:00 2001 From: thexai <58434170+thexai@users.noreply.github.com> Date: Thu, 2 Mar 2023 20:41:36 +0100 Subject: [PATCH 2/4] NFSFile: Log errors on keep alive reads In case an NFSv4 server are configured with low "lese_time" i.e: 30s the session will expire before receiving the first keep alive and we will realize it by nfs_read errors here --- xbmc/filesystem/NFSFile.cpp | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/xbmc/filesystem/NFSFile.cpp b/xbmc/filesystem/NFSFile.cpp index c32e79ff5d69a..f669c282d0ffc 100644 --- a/xbmc/filesystem/NFSFile.cpp +++ b/xbmc/filesystem/NFSFile.cpp @@ -432,11 +432,20 @@ void CNfsConnection::keepAlive(const std::string& _exportPath, struct nfsfh* _pF if (!pContext)// this should normally never happen - paranoia pContext = m_pNfsContext; - CLog::Log(LOGINFO, "NFS: sending keep alive after {} s.", - std::chrono::duration_cast(KEEP_ALIVE_TIMEOUT).count()); + CLog::LogF(LOGDEBUG, "sending keep alive after {}s.", + std::chrono::duration_cast(KEEP_ALIVE_TIMEOUT).count()); + std::unique_lock lock(*this); + nfs_lseek(pContext, _pFileHandle, 0, SEEK_CUR, &offset); - nfs_read(pContext, _pFileHandle, 32, buffer); + + int bytes = nfs_read(pContext, _pFileHandle, 32, buffer); + if (bytes < 0) + { + CLog::LogF(LOGERROR, "nfs_read - Error ({}, {})", bytes, nfs_get_error(pContext)); + return; + } + nfs_lseek(pContext, _pFileHandle, offset, SEEK_SET, &offset); } From 1c3671e0866b617c3578cd234c758ad3d3d293af Mon Sep 17 00:00:00 2001 From: thexai <58434170+thexai@users.noreply.github.com> Date: Thu, 23 Feb 2023 10:13:09 +0100 Subject: [PATCH 3/4] NFSFile: Only retry nfs_open when error is NFS4ERR_EXPIRED Retrying with other error codes and force deinit connection may cause crash, specifically when a file does not exist. Anyway, NFS4ERR_EXPIRED should not occur now that contexts expiration and keep alives are handled correctly. --- xbmc/filesystem/NFSFile.cpp | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/xbmc/filesystem/NFSFile.cpp b/xbmc/filesystem/NFSFile.cpp index f669c282d0ffc..b22bba8030ae2 100644 --- a/xbmc/filesystem/NFSFile.cpp +++ b/xbmc/filesystem/NFSFile.cpp @@ -53,6 +53,8 @@ constexpr auto CONTEXT_TIMEOUT = 60s; // 2/3 parts of lease_time constexpr auto KEEP_ALIVE_TIMEOUT = 45s; // half of lease_time constexpr auto IDLE_TIMEOUT = 30s; // close fast unused contexts when no active connections +constexpr int NFS4ERR_EXPIRED = -11; // client session expired due idle time greater than lease_time + constexpr auto SETTING_NFS_VERSION = "nfs.version"; } // unnamed namespace @@ -594,34 +596,34 @@ bool CNFSFile::Open(const CURL& url) std::unique_lock lock(gNfsConnection); - auto NfsOpen = [this](const CURL& url, std::string& filename) -> bool - { - if (!gNfsConnection.Connect(url, filename)) - return false; + if (!gNfsConnection.Connect(url, filename)) + return false; - m_pNfsContext = gNfsConnection.GetNfsContext(); - m_exportPath = gNfsConnection.GetContextMapId(); + m_pNfsContext = gNfsConnection.GetNfsContext(); + m_exportPath = gNfsConnection.GetContextMapId(); - return nfs_open(m_pNfsContext, filename.c_str(), O_RDONLY, &m_pFileHandle) == 0; - }; + int ret = nfs_open(m_pNfsContext, filename.c_str(), O_RDONLY, &m_pFileHandle); - if (!NfsOpen(url, filename)) + if (ret == NFS4ERR_EXPIRED) // client session expired due no activity/keep alive { CLog::Log(LOGERROR, "CNFSFile::Open: Unable to open file - trying again with a new context: error: '{}'", nfs_get_error(m_pNfsContext)); gNfsConnection.Deinit(); + m_pNfsContext = gNfsConnection.GetNfsContext(); + m_exportPath = gNfsConnection.GetContextMapId(); + ret = nfs_open(m_pNfsContext, filename.c_str(), O_RDONLY, &m_pFileHandle); + } - if (!NfsOpen(url, filename)) - { - CLog::Log(LOGERROR, "CNFSFile::Open: Unable to open file: '{}' error: '{}'", - url.GetFileName(), nfs_get_error(m_pNfsContext)); + if (ret != 0) + { + CLog::Log(LOGERROR, "CNFSFile::Open: Unable to open file: '{}' error: '{}'", url.GetFileName(), + nfs_get_error(m_pNfsContext)); - m_pNfsContext = nullptr; - m_exportPath.clear(); - return false; - } + m_pNfsContext = nullptr; + m_exportPath.clear(); + return false; } CLog::Log(LOGDEBUG, "CNFSFile::Open - opened {}", url.GetFileName()); From 3459df385e4ec0f9161177a6271bc05b51cc505c Mon Sep 17 00:00:00 2001 From: thexai <58434170+thexai@users.noreply.github.com> Date: Sat, 4 Mar 2023 11:53:50 +0100 Subject: [PATCH 4/4] NFSFile: use 128K read/write chunk size in NFSv4 NFSv4 not has maximun chunk size limitation at server side (used TCP vs UDP in NFSv3, etc.) then 'nfs_get_readmax' returns zero or not implemented. Clients must set the chunk size they want to use. --- xbmc/filesystem/NFSFile.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/xbmc/filesystem/NFSFile.cpp b/xbmc/filesystem/NFSFile.cpp index b22bba8030ae2..5e0c148770ca2 100644 --- a/xbmc/filesystem/NFSFile.cpp +++ b/xbmc/filesystem/NFSFile.cpp @@ -329,10 +329,22 @@ bool CNfsConnection::Connect(const CURL& url, std::string &relativePath) } m_exportPath = exportPath; m_hostName = url.GetHostName(); - //read chunksize only works after mount + + // read chunksize only works after mount m_readChunkSize = nfs_get_readmax(m_pNfsContext); m_writeChunkSize = nfs_get_writemax(m_pNfsContext); + if (m_readChunkSize == 0) + { + CLog::Log(LOGDEBUG, "NFS Server did not return max read chunksize - Using 128K default"); + m_readChunkSize = 128 * 1024; // 128K + } + if (m_writeChunkSize == 0) + { + CLog::Log(LOGDEBUG, "NFS Server did not return max write chunksize - Using 128K default"); + m_writeChunkSize = 128 * 1024; // 128K + } + if (contextRet == CNfsConnection::ContextStatus::NEW) { CLog::Log(LOGDEBUG, "NFS: chunks: r/w {}/{}", (int)m_readChunkSize, (int)m_writeChunkSize);