fix deadlock in clean up cache #1151

liuyongqing · 2019-09-11T09:07:38Z

@gaul ，the previous pull request is：#1146, still have some situations not fully considered, so I closed the previous pull request.

Deadlocks will appear in the following situations also：
thread A call s3fs_write, already get the fdent_data_lock and cache_cleanup_lock（for clean up cache dir）, after cleaning one file, and trying to get fd_manager_lock for Close the FdEntity.
thread B call s3fs_open for the same file，already get fd_manager_lock and fdent_lock, trying to get fdent_data_lock
the deadlock occured between thread A and thread B, because each thread wants to get the lock of another thread
so when cleanup a cache file, we should hold fd_manager_lock until it finished, if we cannot hold it, we can ignore it temporarily.

Another deadlock condition is in NoCacheLoadAndPost, because it trying to hold fd_manager_lock after hold fdent_data_lock

In short，We should lock in the order fd_manager_lock->fdent_lock->fdent_data_lock， if we trying to hold fd_manager_lock after hold fdent_lock or fdent_data_lock，we should use try_lock. The NoCacheLoadAndPost deadlock condition I didn't find a nice way to solve it，but it will only triggered if didn't have enough disk after cleaned up cache.

gaul

I am uneasy about the lack of testing for the cache cleanup code generally. Do you see a way to exercise this as a unit test?

gaul · 2019-09-12T00:20:06Z

src/fdcache.cpp

        continue;
      }
-
-      if(ent->IsMultiOpened()){


Can we remove the definition of IsMultiOpened?

gaul · 2019-09-12T00:22:44Z

src/fdcache.cpp

        S3FS_PRN_DBG("cleaned up: %s", next_path.c_str());
+        FdManager::DeleteCacheFile(next_path.c_str());


If I understand this correct, we do not need to check FdEntity::pagelist.IsModified since there are no opens and thus no modified data?

gaul

This is looking good!

gaul · 2019-09-14T23:08:27Z

test/test-utils.sh

@@ -258,6 +258,11 @@ function check_content_type() {
    fi
 }

+function get_disk_avail_size() {
+    DISK_AVAIL_SIZE=`df $1 --output=avail |tail -n 1|tr -dc '0-9'`


This does not work on macOS. Instead try:

BLOCK_SIZE=$((1024 * 1024)) df $1 | awk '{print $4}' | tail -n 1

gaul · 2019-09-14T23:12:39Z

test/integration-test-main.sh

+        dd if=/dev/urandom of=$dir/file-$x bs=1048576 count=1
+    done
+
+    file_cnt=$(ls -1 $dir | wc -l)


The -1 is unnecessary in non-interactive shells/pipelines.

gaul · 2019-09-14T23:15:34Z

test/integration-test-main.sh

@@ -597,6 +621,10 @@ function add_all_tests {
    add_tests test_concurrency
    add_tests test_concurrent_writes
    add_tests test_open_second_fd
+    ENSURE_DISKFREE_SIZE=`ps -ef|grep  ensure_diskfree|awk '{print $NF}' |tr -dc '0-9'`
+    if [ ! -z "$ENSURE_DISKFREE_SIZE" ]; then


Could you simplify this to:

if pidof s3fs | grep -q ensure_diskfree; then

gaul · 2019-09-15T17:29:30Z

Please add pidof to the brew install section of .travis.yml to address macOS failures.

gaul · 2019-09-16T05:04:12Z

test/integration-test-main.sh

@@ -597,6 +621,9 @@ function add_all_tests {
    add_tests test_concurrency
    add_tests test_concurrent_writes
    add_tests test_open_second_fd
+    if pidof s3fs | xargs -I {} ps -o cmd -fp {} | grep -q ensure_diskfree; then


macOS Travis complains that cmd is not found -- is there another way to do this?

https://travis-ci.org/s3fs-fuse/s3fs-fuse/jobs/585372211#L3704

ggtakec · 2019-10-14T12:42:32Z

@liuyongqing #1169 will solve the build error on OSX. Please try it.

ggtakec · 2019-10-14T15:06:46Z

@liuyongqing I'm sorry that #1171 issue makes build error now, please wait for fixing it.

liuyongqing · 2019-10-14T15:21:21Z

@ggtakec ,the -oensure_diskfree param seems have some problem in mac os，the simplest ensure_diskfree mount test code can't pass test case：https://github.com/s3fs-fuse/s3fs-fuse/pull/1170/commits

liuyongqing · 2019-10-15T11:44:48Z

@ggtakec ，do you know how to fix ensure_diskfree mount can't pass test case in MacOS?

gaul · 2020-01-13T11:37:20Z

@liuyongqing do we have a path forward on this pull request?

liuyongqing · 2020-01-13T13:06:03Z

@gaul ，can we ignore the check for mac os temporarily，according to this test pull request：https://github.com/s3fs-fuse/s3fs-fuse/pull/1170/commits，the failed test case not related to this commit.

ggtakec

@liuyongqing Sorry for my late review.

One of reason for this test failure is:

DISK_AVAIL_SIZE=`BLOCK_SIZE=$((1024 * 1024)) df $1 | awk '{print $4}' | tail -n 1`

"BLOCK_SIZE" is not valid in Mac OS df.
But we can use "BLOCKSIZE" instead.
The environment variable "BLOCKSIZE" can be used on Mac OS and other OS.
In Mac OS df, the only acceptable environment variable name is "BLOCKSIZE".

And the other is the following part.

ENSURE_DISKFREE_SIZE=$((CACHE_DISK_AVAIL_SIZE - 256))

200MB is defined as free space, but the test_clean_up_cache test uses 256MB.
So this part seems to be correct for "-256".

Please confirm these points in the conversation of the source code.
(I made the same fix to master as you and confirmed that TravisCI was no longer a problem.)
Please try rebasing this PR code on the latest master.

Thanks in advance for your help.

ggtakec · 2020-01-16T13:42:53Z

test/integration-test-main.sh

+    file_cnt=$(ls $dir | wc -l)
+    if [ $file_cnt != $count ]; then
+        echo "Expected $count files but got $file_cnt"
+        return 1


It is better to call "rm -rf $dir" before returning, because of next testing.

ggtakec · 2020-01-16T13:43:27Z

test/integration-test-main.sh

+    fi
+    CACHE_DISK_AVAIL_SIZE=`get_disk_avail_size $CACHE_DIR`
+    if [ "$CACHE_DISK_AVAIL_SIZE" -lt "$ENSURE_DISKFREE_SIZE" ];then
+        echo "Cache disk avail size:$CACHE_DISK_AVAIL_SIZE less than ensure_diskfree size:$ENSURE_DISKFREE_SIZE"


It is better to call "rm -rf $dir" before returning, because of next testing, too.

ggtakec · 2020-01-16T13:45:10Z

test/small-integration-test.sh

+#reserve 200MB for data cache
+source test-utils.sh
+CACHE_DISK_AVAIL_SIZE=`get_disk_avail_size $CACHE_DIR`
+ENSURE_DISKFREE_SIZE=$((CACHE_DISK_AVAIL_SIZE - 200))


test_clean_up_cache function uses 256MB, so here I think it should be "- 256".

ggtakec · 2020-01-16T13:47:12Z

test/test-utils.sh

@@ -258,6 +258,11 @@ function check_content_type() {
    fi
 }

+function get_disk_avail_size() {
+    DISK_AVAIL_SIZE=`BLOCK_SIZE=$((1024 * 1024)) df $1 | awk '{print $4}' | tail -n 1`


Please change the environment variable name to "BLOCKSIZE" instead of "BLOCK_SIZE".
This is the environment variable name common to MacOS and other OS.

liuyongqing · 2020-01-19T12:38:00Z

@ggtakec ，hi，I changed the code according to the above advice, but it still failed in macos

ggtakec · 2020-01-19T15:05:29Z

@liuyongqing Thanks for your help.
When I checked it correctly again, I got the same your error.

I think the reason which is that we run some tests and fter that we run "use_cache = $ {CACHE_DIR} -o ensure_diskfree = $ {ENSURE_DISKFREE_SIZE}" final test.
And it seems that s3fs got the following error at that time.

"There is no enough disk space for used as cache(or temporary) directory by s3fs."

This is because we determined the ensure_diskfree size before the test started, but after some tests it seems that the disk usage is different.
(For example, disk space is used in s3proxy, cache directory, etc.)
So I think we should clear the disk before running the ensure_diskfree test or run this test right after calculating ENSURE_DISKFREE_SIZE.

What about running the ensure_diskfree test first to solve it?
To do this, change FLAGS in the small-integration-test.sh file as follows:

FLAGS=(
    "use_cache=${CACHE_DIR} -o ensure_diskfree=${ENSURE_DISKFREE_SIZE}"
    enable_content_md5
    enable_noobj_cache
    nocopyapi
    nomultipart
    notsup_compat_dir
    sigv2
    singlepart_copy_limit=$((10 * 1024))  # limit size to exercise multipart code paths
    #use_sse  # TODO: S3Proxy does not support SSE
)

I have confirmed that this procedure does not cause any errors.

Please retry to change your code and run test again.
Thanks in advance for your help.

liuyongqing · 2020-01-19T16:19:48Z

@ggtakec ，the test still failed, complain:The job exceeded the maximum log length, and has been terminated.

ggtakec · 2020-01-20T13:15:59Z

@liuyongqing Thanks for your reports.
My test (the source codes probably are the same) did not put any error, but the source code probably is as same as yours.

I found following error in the TravisCI logs.

./integration-test-main.sh: line 677: [: : integer expression expected
test_clean_up_cache passed

The test_clean_up_cache test passed, but an error occured in that test script.
Due to this error, it seems that 256 files have not been deleted and there is not enough disk space for s3fs.
So it seems that some tests(maybe writing files) have failed since then.

I think we need to find out why this script error is occurring.

ggtakec · 2020-01-26T15:53:09Z

@liuyongqing I found the reason of following error message.

./integration-test-main.sh: line 677: [: : integer expression expected

The reason is not clear, but on MacOS, the variables CACHE_DIR and
ENSURE_DISKFREE_SIZE is not found in the function of integration-test-main.sh.
So it seems that you can work around this problem by exporting them in the definition of each variable in small-integration-test.sh.

In addition to this problem, some test units(functions) forgot to delete test files.
It has been merged into master in #1230.

Even after solving the above two problems, the test after test_chown still seems to fail.
I will continue to investigate, but I'm sorry because it is not in time for the release of v1.86.

ggtakec · 2020-01-28T12:01:47Z

@liuyongqing I merged #1232.
Please export CACHE_DIR and ENSURE_DISKFREE_SIZE variables at where these are defined(please see in my previous comment), and try PR build again.
I hope that you should have a successful build of this PR.

Thanks in advance for your help.

liuyongqing · 2020-01-29T04:26:23Z

@ggtakec ，hi，the macos run test success, but ppc64Ie platform run test case failed

ggtakec · 2020-01-29T11:40:51Z

@liuyongqing It was "The build has been terminated".
Then I would try to rebuild ppc64le now.
Please wait, and if the build on travis would success, please squash the PR code.
After that, I will merge this PR.

liuyongqing · 2020-01-30T09:16:34Z

@ggtakec ，all test pass，commits are squashed to one commit.

ggtakec · 2020-01-30T13:17:42Z

@liuyongqing Thank you for your cooperation.
I'll merge this PR.
We apologize for taking a long time.
But thanks to it, s3fs has gotten better.

gaul requested changes Sep 12, 2019

View reviewed changes

gaul requested changes Sep 14, 2019

View reviewed changes

gaul reviewed Sep 16, 2019

View reviewed changes

ggtakec reviewed Jan 16, 2020

View reviewed changes

ggtakec mentioned this pull request Jan 24, 2020

1.86 release #1050

Closed

4 tasks

fix deadlock due to fdmanager_lock and fdent_data_lock

b762a0a

ggtakec merged commit ee1d3a9 into s3fs-fuse:master Jan 30, 2020

This was referenced Jan 30, 2020

just test ensure_diskfree param #1170

Closed

Avoid test errors on MacOS #1242

Merged

Roay mentioned this pull request Mar 28, 2024

Deadlock for FdManager::fd_manager_lock && FdEntity::fdent_lock when write data concurrently #2438

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix deadlock in clean up cache #1151

fix deadlock in clean up cache #1151

liuyongqing commented Sep 11, 2019

gaul left a comment •

edited

Loading

gaul Sep 12, 2019

gaul Sep 12, 2019

gaul left a comment

gaul Sep 14, 2019

gaul Sep 14, 2019

gaul Sep 14, 2019

gaul commented Sep 15, 2019

gaul Sep 16, 2019

ggtakec commented Oct 14, 2019

ggtakec commented Oct 14, 2019

liuyongqing commented Oct 14, 2019

liuyongqing commented Oct 15, 2019

gaul commented Jan 13, 2020

liuyongqing commented Jan 13, 2020

ggtakec left a comment

ggtakec Jan 16, 2020

ggtakec Jan 16, 2020

ggtakec Jan 16, 2020

ggtakec Jan 16, 2020

liuyongqing commented Jan 19, 2020

ggtakec commented Jan 19, 2020

liuyongqing commented Jan 19, 2020

ggtakec commented Jan 20, 2020

ggtakec commented Jan 26, 2020

ggtakec commented Jan 28, 2020

liuyongqing commented Jan 29, 2020

ggtakec commented Jan 29, 2020

liuyongqing commented Jan 30, 2020

ggtakec commented Jan 30, 2020

		S3FS_PRN_DBG("cleaned up: %s", next_path.c_str());
		FdManager::DeleteCacheFile(next_path.c_str());

fix deadlock in clean up cache #1151

fix deadlock in clean up cache #1151

Conversation

liuyongqing commented Sep 11, 2019

gaul left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaul commented Sep 15, 2019

Choose a reason for hiding this comment

ggtakec commented Oct 14, 2019

ggtakec commented Oct 14, 2019

liuyongqing commented Oct 14, 2019

liuyongqing commented Oct 15, 2019

gaul commented Jan 13, 2020

liuyongqing commented Jan 13, 2020

ggtakec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuyongqing commented Jan 19, 2020

ggtakec commented Jan 19, 2020

liuyongqing commented Jan 19, 2020

ggtakec commented Jan 20, 2020

ggtakec commented Jan 26, 2020

ggtakec commented Jan 28, 2020

liuyongqing commented Jan 29, 2020

ggtakec commented Jan 29, 2020

liuyongqing commented Jan 30, 2020

ggtakec commented Jan 30, 2020

gaul left a comment •

edited

Loading