Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NFS] Implement nfs timeout / advancedsettings #15686

Merged
merged 1 commit into from Jun 10, 2020
Merged

Conversation

peak3d
Copy link
Contributor

@peak3d peak3d commented Mar 6, 2019

Description

If NFS mount is configured in Kodi client, but NFS server is down, we currently run into many deadlock situations:

  • during play PAPlayer holds the context lock while trying to nfs_read
  • when refreshing ListItems the Jobworker holds the context lock during nfs_stat
  • on startup connection discovery locks the context lock, too.

Issue is, that our main thread, ProcessSlow() deadlocks the complete kodi application in that case.

Because of this the NFS timeout is with this PR limited to 5 seconds by default, but can be reconfigured using the advancedsettings (in seconds):

<network>
  <nfstimeout>2</nfstimeout>
</network>

The above examle sets the timeout to 2 sec.
A value of 0 does not set any timeout and NFS waits forever (default behaviour without this PR).

Valid nfstimeout values are between 0 and 3600 seconds.

Motivation and Context

Deadlocks here and there if my NFS server goes into sleep, or if I move with my phone away from home.

How Has This Been Tested?

  • play music over nfs
  • suspend the nfs server
  • navigate back

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

@peak3d peak3d added Type: Fix non-breaking change which fixes an issue v18 Leia Component: Network labels Mar 6, 2019
@peak3d peak3d added this to the Leia 18.2-rc1 milestone Mar 6, 2019
@peak3d
Copy link
Contributor Author

peak3d commented Mar 6, 2019

jenkins build this please

Rechi
Rechi previously requested changes Mar 6, 2019
{
uint32_t timeout = CServiceBroker::GetSettingsComponent()->GetAdvancedSettings()->m_nfsTimeout;
if (~timeout)
nfs_set_timeout(context, timeout);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break compiling on Ubuntu Xenial (16.04) with enabled nfs support.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the version does not support nfs_set_timeout ? If so, do we have the libnfs version somewhere during compile time?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nfs_set_timeout is available since 2.0.0 (sahlberg/libnfs@b01776c)
Ubuntu Xenial (16.04) provides 1.9.8 (https://packages.ubuntu.com/xenial/libnfs-dev)

@peak3d peak3d force-pushed the nfs branch 2 times, most recently from 3019596 to 03fb5fd Compare March 6, 2019 21:59
@@ -34,7 +34,10 @@ find_package_handle_standard_args(NFS
if(NFS_FOUND)
set(NFS_LIBRARIES ${NFS_LIBRARY})
set(NFS_INCLUDE_DIRS ${NFS_INCLUDE_DIR})
set(NFS_DEFINITIONS -DHAS_FILESYSTEM_NFS=1)

string(REPLACE "." ";" VERSION_LIST ${NFS_VERSION})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows this fails with the following error,

CMake Error at cmake/modules/FindNFS.cmake:38 (string):
  string sub-command REPLACE requires at least four arguments.
Call Stack (most recent call first):
  cmake/scripts/common/Macros.cmake:366 (find_package)
  cmake/scripts/common/Macros.cmake:432 (find_package_with_ver)
  CMakeLists.txt:177 (core_optional_dep)

@peak3d peak3d force-pushed the nfs branch 2 times, most recently from 6d567d7 to 711124c Compare March 7, 2019 19:01
@peak3d
Copy link
Contributor Author

peak3d commented Mar 7, 2019

@Rechi pls. review again, thx!

@Rechi Rechi dismissed their stale review March 7, 2019 19:39

Ubuntu and Windows issues fixed

cmake/modules/FindNFS.cmake Outdated Show resolved Hide resolved
@peak3d
Copy link
Contributor Author

peak3d commented Mar 7, 2019

Ok, thx for the fix, will add it.

@peak3d peak3d force-pushed the nfs branch 2 times, most recently from 49cce93 to 186d856 Compare March 14, 2019 14:04
@peak3d
Copy link
Contributor Author

peak3d commented Mar 14, 2019

jenkins build this please

@Rechi Rechi removed the v18 Leia label Apr 16, 2019
@Rechi Rechi removed this from the Leia 18.2-rc1 milestone Apr 16, 2019
@Kal42
Copy link

Kal42 commented Feb 9, 2020

Any update on this ?

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Feb 18, 2020
@phunkyfish
Copy link
Contributor

@peak3d do you want to progress this, close it or put it on the backburner?

@phunkyfish phunkyfish added the PR Cleanup: Ask Author PR appears to still be relevant and mergeable. Comment to author to check on status. label Mar 8, 2020
@peak3d
Copy link
Contributor Author

peak3d commented Mar 8, 2020

I'll check tomorrow

@pepeq2
Copy link

pepeq2 commented Apr 13, 2020

Any news on this?
Would be nice to have it implemented...

@phunkyfish
Copy link
Contributor

@peak3d did you ever check this?

:)

@peak3d
Copy link
Contributor Author

peak3d commented Jun 5, 2020

Jenkins build this please

fuzzard pushed a commit to fuzzard/xbmc that referenced this pull request Jun 9, 2020
Bump version to untagged commit hash to support features implemented in
xbmc#15686
@fuzzard fuzzard mentioned this pull request Jun 9, 2020
13 tasks
@peak3d
Copy link
Contributor Author

peak3d commented Jun 9, 2020

Jenkins build this please

fuzzard pushed a commit to fuzzard/xbmc that referenced this pull request Jun 9, 2020
Bump version to untagged commit hash to support features implemented in
xbmc#15686
fuzzard pushed a commit to fuzzard/xbmc that referenced this pull request Jun 9, 2020
Bump version to untagged commit hash to support features implemented in
xbmc#15686
fuzzard pushed a commit to fuzzard/xbmc that referenced this pull request Jun 10, 2020
Bump version to untagged commit hash to support features implemented in
xbmc#15686
@peak3d
Copy link
Contributor Author

peak3d commented Jun 10, 2020

Jenkins build this please

1 similar comment
@peak3d
Copy link
Contributor Author

peak3d commented Jun 10, 2020

Jenkins build this please

@peak3d peak3d merged commit 0aceba9 into xbmc:master Jun 10, 2020
@peak3d peak3d deleted the nfs branch June 10, 2020 15:20
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jun 14, 2020
[NFS] Implement nfs timeout / advancedsettings
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jun 15, 2020
[NFS] Implement nfs timeout / advancedsettings
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 3, 2020
[NFS] Implement nfs timeout / advancedsettings
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
[NFS] Implement nfs timeout / advancedsettings
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
[NFS] Implement nfs timeout / advancedsettings
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
[NFS] Implement nfs timeout / advancedsettings
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
[NFS] Implement nfs timeout / advancedsettings
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
[NFS] Implement nfs timeout / advancedsettings
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
[NFS] Implement nfs timeout / advancedsettings
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
[NFS] Implement nfs timeout / advancedsettings
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
[NFS] Implement nfs timeout / advancedsettings
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 7, 2020
[NFS] Implement nfs timeout / advancedsettings
@CvH CvH mentioned this pull request Nov 6, 2021
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Network Type: Feature non-breaking change which adds functionality v19 Matrix Wiki: Done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants