Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-3808

fuse_dfs: postpone libhdfs intialization until after fork

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.2-alpha
    • Fix Version/s: 2.0.2-alpha
    • Component/s: fuse-dfs
    • Labels:
      None

      Description

      libhdfs may create threads or initialize internal JNI data structures when it is used. So we should be careful to daemonize() before initializing libhdfs, not after.

      Unfortunately, fuse_dfs is doing just this – initializing libhdfs in its main function, prior to calling fuse_main which does a daemonize.

      daemonize() does not preserve threads, because it is implemented by fork()}}ing and then continuing execution in the child and allowing the parent to {{exit().

      1. HDFS-3808.001.patch
        4 kB
        Colin Patrick McCabe
      2. HDFS-3808.002.patch
        5 kB
        Colin Patrick McCabe
      3. HDFS-3808.003.patch
        5 kB
        Colin Patrick McCabe

        Activity

        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12541148/HDFS-3808.001.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.hdfs.TestDatanodeBlockScanner

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3017//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3017//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12541148/HDFS-3808.001.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDatanodeBlockScanner +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3017//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3017//console This message is automatically generated.
        Hide
        Andy Isaacson added a comment -

        It's not accurate to say "we should not call fork", because forking to exec a child process should work just fine. The problem here is the daemonize() pattern, where we fork and continue execution in the child process in order to fiddle parent/child relationships and pgrp mappings.

        I'll update the description accordingly.

        Reviewing the patch now.

        Show
        Andy Isaacson added a comment - It's not accurate to say "we should not call fork", because forking to exec a child process should work just fine. The problem here is the daemonize() pattern, where we fork and continue execution in the child process in order to fiddle parent/child relationships and pgrp mappings. I'll update the description accordingly. Reviewing the patch now.
        Hide
        Andy Isaacson added a comment -
        +  fprintf(stderr, "LD_LIBRARY_PATH=%s",ld == NULL ? "NULL" : ld);
        +  fprintf(stderr, "CLASSPATH=%s",cp == NULL ? "NULL" : cp);
        

        Tiny cleanup: let's add \n to these format strings so the debugging output is slightly easier to read.

        @@ -76,6 +86,21 @@ void init_protectedpaths(dfs_context *dfs) {
             j++;
           }
           dfs->protectedpaths[j] = NULL;
        +
        +  ret = fuseConnectInit(options.nn_uri, options.nn_port);
        

        This call doesn't belong inside of init_protectedpaths. Instead, please move it out to right after the invocation of init_protectedpaths in dfs_init. And while you're here, you can make init_protectedpaths static.

        +  char attrTimeoutBuf[512], entryTimeoutBuf[512];
        ...
        -  {
        -    char buf[1024];
        -
        -    snprintf(buf, sizeof buf, "-oattr_timeout=%d",options.attribute_timeout);
        -    fuse_opt_add_arg(&args, buf);
        +  snprintf(attrTimeoutBuf, sizeof(attrTimeoutBuf),
        +           "-oattr_timeout=%d",options.attribute_timeout);
        +  fuse_opt_add_arg(&args, attrTimeoutBuf);
         
        -    snprintf(buf, sizeof buf, "-oentry_timeout=%d",options.entry_timeout);
        -    fuse_opt_add_arg(&args, buf);
        -  }
        +  snprintf(entryTimeoutBuf, sizeof(entryTimeoutBuf),
        +           "-oentry_timeout=%d",options.entry_timeout);
        +  fuse_opt_add_arg(&args, entryTimeoutBuf);
        

        No need to add two separate bufs here; fuse_opt_add_arg must be making a copy of the string anyways since we're passing in automatic arrays, so please preserve just one buf[]. However, 1024 is absurdly overlarge. %d has a max expansion of 1+ceil(ln(2**64)/ln(10)) = 21, so make the buffer 40 bytes or 80 bytes and call it a day.

        Show
        Andy Isaacson added a comment - + fprintf(stderr, "LD_LIBRARY_PATH=%s" ,ld == NULL ? "NULL" : ld); + fprintf(stderr, "CLASSPATH=%s" ,cp == NULL ? "NULL" : cp); Tiny cleanup: let's add \n to these format strings so the debugging output is slightly easier to read. @@ -76,6 +86,21 @@ void init_protectedpaths(dfs_context *dfs) { j++; } dfs->protectedpaths[j] = NULL; + + ret = fuseConnectInit(options.nn_uri, options.nn_port); This call doesn't belong inside of init_protectedpaths . Instead, please move it out to right after the invocation of init_protectedpaths in dfs_init . And while you're here, you can make init_protectedpaths static. + char attrTimeoutBuf[512], entryTimeoutBuf[512]; ... - { - char buf[1024]; - - snprintf(buf, sizeof buf, "-oattr_timeout=%d" ,options.attribute_timeout); - fuse_opt_add_arg(&args, buf); + snprintf(attrTimeoutBuf, sizeof(attrTimeoutBuf), + "-oattr_timeout=%d" ,options.attribute_timeout); + fuse_opt_add_arg(&args, attrTimeoutBuf); - snprintf(buf, sizeof buf, "-oentry_timeout=%d" ,options.entry_timeout); - fuse_opt_add_arg(&args, buf); - } + snprintf(entryTimeoutBuf, sizeof(entryTimeoutBuf), + "-oentry_timeout=%d" ,options.entry_timeout); + fuse_opt_add_arg(&args, entryTimeoutBuf); No need to add two separate bufs here; fuse_opt_add_arg must be making a copy of the string anyways since we're passing in automatic arrays, so please preserve just one buf[] . However, 1024 is absurdly overlarge. %d has a max expansion of 1+ceil(ln(2**64)/ln(10)) = 21, so make the buffer 40 bytes or 80 bytes and call it a day.
        Hide
        Aaron T. Myers added a comment -

        Thanks a lot for reviewing the patch, Andy.

        Colin, can you also comment on what testing you've done of this change?

        Show
        Aaron T. Myers added a comment - Thanks a lot for reviewing the patch, Andy. Colin, can you also comment on what testing you've done of this change?
        Hide
        Colin Patrick McCabe added a comment -

        Thanks for the review, Andy. I'm probably going to change the LD_LIBRARY_PATH, etc. printouts to ERROR directives, which will both send the message to syslog and also add a '\n' at the end.

        I tested this by running test_fuse_dfs, and also by manually running fuse without -d or -f and verifying that the results were as expected.

        Show
        Colin Patrick McCabe added a comment - Thanks for the review, Andy. I'm probably going to change the LD_LIBRARY_PATH, etc. printouts to ERROR directives, which will both send the message to syslog and also add a '\n' at the end. I tested this by running test_fuse_dfs, and also by manually running fuse without -d or -f and verifying that the results were as expected.
        Hide
        Andy Isaacson added a comment -
        -    DEBUG("dfs->rdbuffersize <= 0 = %ld", dfs->rdbuffer_size);
        +    DEBUG("dfs->rdbuffersize <= 0 = %Zd", dfs->rdbuffer_size);
        

        Please use standard z not libc5's Z. Looks like there is one other Zd in dfsPrintOptions as well, please fix it while you're here.

        Other than that, this looks great! Thanks!

        Show
        Andy Isaacson added a comment - - DEBUG( "dfs->rdbuffersize <= 0 = %ld" , dfs->rdbuffer_size); + DEBUG( "dfs->rdbuffersize <= 0 = %Zd" , dfs->rdbuffer_size); Please use standard z not libc5's Z. Looks like there is one other Zd in dfsPrintOptions as well, please fix it while you're here. Other than that, this looks great! Thanks!
        Hide
        Colin Patrick McCabe added a comment -

        %Z -> %z

        Show
        Colin Patrick McCabe added a comment - %Z -> %z
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12541283/HDFS-3808.003.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3034//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3034//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12541283/HDFS-3808.003.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3034//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3034//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12541276/HDFS-3808.002.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.hdfs.web.TestWebHDFS
        org.apache.hadoop.hdfs.TestPersistBlocks

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3033//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3033//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12541276/HDFS-3808.002.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.web.TestWebHDFS org.apache.hadoop.hdfs.TestPersistBlocks +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3033//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3033//console This message is automatically generated.
        Hide
        Aaron T. Myers added a comment -

        +1, the latest patch looks good to me. I'm confident that the test failures are unrelated.

        Show
        Aaron T. Myers added a comment - +1, the latest patch looks good to me. I'm confident that the test failures are unrelated.
        Hide
        Aaron T. Myers added a comment -

        I've just committed this to trunk and branch-2.

        Thanks a lot for the contribution, Colin. Thanks a lot for the reviews, Andy.

        Show
        Aaron T. Myers added a comment - I've just committed this to trunk and branch-2. Thanks a lot for the contribution, Colin. Thanks a lot for the reviews, Andy.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk-Commit #2591 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2591/)
        HDFS-3808. fuse_dfs: postpone libhdfs intialization until after fork. Contributed by Colin Patrick McCabe. (Revision 1374106)

        Result = SUCCESS
        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1374106
        Files :

        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_dfs.c
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_init.c
        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2591 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2591/ ) HDFS-3808 . fuse_dfs: postpone libhdfs intialization until after fork. Contributed by Colin Patrick McCabe. (Revision 1374106) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1374106 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_dfs.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_init.c
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #2656 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2656/)
        HDFS-3808. fuse_dfs: postpone libhdfs intialization until after fork. Contributed by Colin Patrick McCabe. (Revision 1374106)

        Result = SUCCESS
        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1374106
        Files :

        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_dfs.c
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_init.c
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2656 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2656/ ) HDFS-3808 . fuse_dfs: postpone libhdfs intialization until after fork. Contributed by Colin Patrick McCabe. (Revision 1374106) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1374106 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_dfs.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_init.c
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #2621 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2621/)
        HDFS-3808. fuse_dfs: postpone libhdfs intialization until after fork. Contributed by Colin Patrick McCabe. (Revision 1374106)

        Result = FAILURE
        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1374106
        Files :

        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_dfs.c
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_init.c
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2621 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2621/ ) HDFS-3808 . fuse_dfs: postpone libhdfs intialization until after fork. Contributed by Colin Patrick McCabe. (Revision 1374106) Result = FAILURE atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1374106 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_dfs.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_init.c
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #1137 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1137/)
        HDFS-3808. fuse_dfs: postpone libhdfs intialization until after fork. Contributed by Colin Patrick McCabe. (Revision 1374106)

        Result = FAILURE
        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1374106
        Files :

        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_dfs.c
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_init.c
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1137 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1137/ ) HDFS-3808 . fuse_dfs: postpone libhdfs intialization until after fork. Contributed by Colin Patrick McCabe. (Revision 1374106) Result = FAILURE atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1374106 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_dfs.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_init.c
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #1169 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1169/)
        HDFS-3808. fuse_dfs: postpone libhdfs intialization until after fork. Contributed by Colin Patrick McCabe. (Revision 1374106)

        Result = FAILURE
        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1374106
        Files :

        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_dfs.c
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_init.c
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1169 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1169/ ) HDFS-3808 . fuse_dfs: postpone libhdfs intialization until after fork. Contributed by Colin Patrick McCabe. (Revision 1374106) Result = FAILURE atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1374106 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_dfs.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/fuse-dfs/fuse_init.c

          People

          • Assignee:
            Colin Patrick McCabe
            Reporter:
            Colin Patrick McCabe
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development