Hadoop Common
  1. Hadoop Common
  2. HADOOP-4616

assertion makes fuse-dfs exit when reading incomplete data

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.18.2
    • Fix Version/s: 0.18.3, 0.19.1
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      When trying to read a file that is corrupt on HDFS (registered by the namenode, but part of the data is missing on the datanodes), some of the assertions in dfs_read fail, causing the program to abort. This makes it impossible to access the mounted partition until it is mounted again.

      A simple way to reproduce this bug is to remove enough datanodes to have part of the data missing, and to read each file listed in HDFS.

      this is the assertion that fails (fuse_dfs.c:903) : assert(bufferReadIndex >= 0 && bufferReadIndex < fh->bufferSize);

      The expected behaviour would be to return either no file or a corrupt file, but continue working afterward.

      removing the assertion seems to work for now, but a special behaviour is probably needed to handle this particular problem correctly.

      1. HADOOP-4616_0.19.txt
        7 kB
        Pete Wyckoff
      2. HADOOP-4616_0.19.txt
        7 kB
        Pete Wyckoff
      3. HADOOP-4616_0.18.2.txt
        6 kB
        Pete Wyckoff
      4. HADOOP-4616_0.18.2.txt
        6 kB
        Pete Wyckoff
      5. HADOOP-4616.txt
        12 kB
        Pete Wyckoff
      6. HADOOP-4616.txt
        2 kB
        Pete Wyckoff
      7. HADOOP-4616.txt
        2 kB
        Pete Wyckoff

        Issue Links

          Activity

          Hide
          Pete Wyckoff added a comment -

          is this causes because in the following, the "if(num_read < 0)" should be "if(num_read <= 0)"?

          This way, the error is caught and it should return an IO error and continue on. I think hdfs though should return -1 in this case, but regardless, the fuse code leaves this condition out and should catch it.

              while (dfs->rdbuffer_size  - total_read > 0 &&                                                                                                                                                         
                       (num_read = hdfsPread(fh->fs, fh->hdfsFH, offset + total_read, fh->buf + total_read, dfs->rdbuffer_size - total_read)) > 0) {                                                                   
                  total_read += num_read;                                                                                                                                                                              
                }                                                                                                                                                                                                      
                                                                                                                                                                                                                       
                if (num_read < 0) {                                                                                                                                                                                    
                  // invalidate the buffer                                                                                                                                                                             
                  fh->bufferSize = 0;                                                                                                                                                                                  
                  syslog(LOG_ERR, "Read error - pread failed for %s with return code %d %s:%d", path, (int)num_read, __FILE__, __LINE__);                                                                              
                  ret = -EIO;                                                                                                                                                                                          
                } else {                                                                                                                                                                                               
                  fh->bufferSize = total_read;                                                                                                                                                                         
                  fh->buffersStartOffset = offset;                                                                                                                                                                     
                                                                                                                                                                                                                       
                  if (dfs->rdbuffer_size - total_read > 0) {                                                                                                                                                           
                    isEOF = 1;                                                                                                                                                                                         
                  }                                                                                                                                                                                                    
                }                                           
          
          Show
          Pete Wyckoff added a comment - is this causes because in the following, the "if(num_read < 0)" should be "if(num_read <= 0)"? This way, the error is caught and it should return an IO error and continue on. I think hdfs though should return -1 in this case, but regardless, the fuse code leaves this condition out and should catch it. while (dfs->rdbuffer_size - total_read > 0 && (num_read = hdfsPread(fh->fs, fh->hdfsFH, offset + total_read, fh->buf + total_read, dfs->rdbuffer_size - total_read)) > 0) { total_read += num_read; } if (num_read < 0) { // invalidate the buffer fh->bufferSize = 0; syslog(LOG_ERR, "Read error - pread failed for %s with return code %d %s:%d" , path, ( int )num_read, __FILE__, __LINE__); ret = -EIO; } else { fh->bufferSize = total_read; fh->buffersStartOffset = offset; if (dfs->rdbuffer_size - total_read > 0) { isEOF = 1; } }
          Hide
          Pete Wyckoff added a comment -

          sorry, looking at it more carefully, this looks like the case where total read = 0, so the assertion is actually wrong as you say and should probably be instead:

              assert(bufferReadIndex >= 0 && bufferReadIndex <= fh->bufferSize);                                                                                                                                        
          

          But, if this is that case, the fact that it read 0 bytes should probably mean it should return an EIO to be more posix compliant. so, maybe the if should be

            if(num_read < 0 || (total_read == 0 && size > 0)) {
          

          and the assertion should also be as above to handle the case where size == 0.

          Show
          Pete Wyckoff added a comment - sorry, looking at it more carefully, this looks like the case where total read = 0, so the assertion is actually wrong as you say and should probably be instead: assert (bufferReadIndex >= 0 && bufferReadIndex <= fh->bufferSize); But, if this is that case, the fact that it read 0 bytes should probably mean it should return an EIO to be more posix compliant. so, maybe the if should be if (num_read < 0 || (total_read == 0 && size > 0)) { and the assertion should also be as above to handle the case where size == 0.
          Hide
          Pete Wyckoff added a comment -

          fixed by handling this case of the read immediately resulting in EOF with no bytes read.

          Show
          Pete Wyckoff added a comment - fixed by handling this case of the read immediately resulting in EOF with no bytes read.
          Hide
          Pete Wyckoff added a comment -

          Fixed, although it seems there may be a bug in hdfs as http://java.sun.com/j2se/1.5.0/docs/api/java/io/InputStream.html#read(byte[]) says the read should return -1 and not 0 in case of EOF or other error.

          Show
          Pete Wyckoff added a comment - Fixed, although it seems there may be a bug in hdfs as http://java.sun.com/j2se/1.5.0/docs/api/java/io/InputStream.html#read(byte[ ]) says the read should return -1 and not 0 in case of EOF or other error.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12393554/HADOOP-4616.txt
          against trunk revision 712344.

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

          +1 tests included. The patch appears to include 6 new or modified tests.

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

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

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3560/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3560/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3560/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3560/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/12393554/HADOOP-4616.txt against trunk revision 712344. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3560/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3560/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3560/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3560/console This message is automatically generated.
          Hide
          Pete Wyckoff added a comment -

          This is a more complete patch and also fixes the error codes.

          Show
          Pete Wyckoff added a comment - This is a more complete patch and also fixes the error codes.
          Hide
          Pete Wyckoff added a comment -

          re-submitting to hudson. Marc, can you try this patch and see if it fixes things for you?

          thanks, pete

          Show
          Pete Wyckoff added a comment - re-submitting to hudson. Marc, can you try this patch and see if it fixes things for you? thanks, pete
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12393676/HADOOP-4616.txt
          against trunk revision 712962.

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

          +1 tests included. The patch appears to include 6 new or modified tests.

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

          -1 javac. The applied patch generated 220 javac compiler warnings (more than the trunk's current 1011 warnings).

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3575/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3575/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3575/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3575/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/12393676/HADOOP-4616.txt against trunk revision 712962. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The applied patch generated 220 javac compiler warnings (more than the trunk's current 1011 warnings). +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3575/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3575/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3575/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3575/console This message is automatically generated.
          Hide
          Marc-Olivier Fleury added a comment -

          I tested the patch, it seems to work fine, at least on missing data (I could not find a file that was corrupt, those are a bit harder to get)

          I tested it by killing the datanodes, while keeping the namenode alive. When trying to copy a file to the local file system, I get the following error :

          cp : reading <file name> : : Unknown error 255

          Which is what we were supposed to get, I guess.

          A possible improvement would be to get a better error code, but I don't know what kind of possibility you have regarding the error codes...

          Thanks for the quick fix!

          Marc-O

          Show
          Marc-Olivier Fleury added a comment - I tested the patch, it seems to work fine, at least on missing data (I could not find a file that was corrupt, those are a bit harder to get) I tested it by killing the datanodes, while keeping the namenode alive. When trying to copy a file to the local file system, I get the following error : cp : reading <file name> : : Unknown error 255 Which is what we were supposed to get, I guess. A possible improvement would be to get a better error code, but I don't know what kind of possibility you have regarding the error codes... Thanks for the quick fix! Marc-O
          Hide
          Pete Wyckoff added a comment -

          hi Marc-O,

          Did you get a chance to look at the code? and if so, can you +1 it if you approve?

          thanks, pete

          Show
          Pete Wyckoff added a comment - hi Marc-O, Did you get a chance to look at the code? and if so, can you +1 it if you approve? thanks, pete
          Hide
          Pete Wyckoff added a comment -

          here is the 0.18.2 patch including generating a warning in libhdfs if it returns 0 from there and also fixing the memory leak of the mutex not being destroyed.
          if this is good, i can supply the 0.19/trunk patch which is almost identical.

          Show
          Pete Wyckoff added a comment - here is the 0.18.2 patch including generating a warning in libhdfs if it returns 0 from there and also fixing the memory leak of the mutex not being destroyed. if this is good, i can supply the 0.19/trunk patch which is almost identical.
          Hide
          Pete Wyckoff added a comment -

          had the ordering of the fprintf in libhdfs after setting noreadbytes=0 so always printing warning.

          Show
          Pete Wyckoff added a comment - had the ordering of the fprintf in libhdfs after setting noreadbytes=0 so always printing warning.
          Hide
          Pete Wyckoff added a comment -

          this is for 0.19 and trunk

          Show
          Pete Wyckoff added a comment - this is for 0.19 and trunk
          Hide
          Pete Wyckoff added a comment -

          re-submitting for hudson.

          Show
          Pete Wyckoff added a comment - re-submitting for hudson.
          Hide
          Pete Wyckoff added a comment -

          Dhruba and I looked at this more and did reproduce the test of 1) all blocks going missing for a file and 2) a block being corrupt for a file (i edited the block) and all seems ok here but could not hit the condition of getting a non error code back from dfs but reading 0 bytes. But I can see how it is possible.

          Note that this error does not happen for 0 size files normally as some higher level mechanism is returning EOF rather than fuse-dfs itself. Otherwise, this assertion would be failing all the time for 0 sized files.

          – pete

          Show
          Pete Wyckoff added a comment - Dhruba and I looked at this more and did reproduce the test of 1) all blocks going missing for a file and 2) a block being corrupt for a file (i edited the block) and all seems ok here but could not hit the condition of getting a non error code back from dfs but reading 0 bytes. But I can see how it is possible. Note that this error does not happen for 0 size files normally as some higher level mechanism is returning EOF rather than fuse-dfs itself. Otherwise, this assertion would be failing all the time for 0 sized files. – pete
          Hide
          Pete Wyckoff added a comment -

          changing priority to blocker as this needs to get fixed in 0.19.1 and 0.18.3 (if there will be such a thing).

          Show
          Pete Wyckoff added a comment - changing priority to blocker as this needs to get fixed in 0.19.1 and 0.18.3 (if there will be such a thing).
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12393751/HADOOP-4616_0.19.txt
          against trunk revision 713612.

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

          +1 tests included. The patch appears to include 6 new or modified tests.

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

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

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          -1 core tests. The patch failed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3581/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3581/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3581/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3581/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/12393751/HADOOP-4616_0.19.txt against trunk revision 713612. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3581/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3581/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3581/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3581/console This message is automatically generated.
          Hide
          Pete Wyckoff added a comment -

          re-submitting for hudson

          Show
          Pete Wyckoff added a comment - re-submitting for hudson
          Hide
          Pete Wyckoff added a comment -

          including the fix to the group freeing problem.

          Show
          Pete Wyckoff added a comment - including the fix to the group freeing problem.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12394123/HADOOP-4616_0.19.txt
          against trunk revision 719152.

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

          +1 tests included. The patch appears to include 6 new or modified tests.

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

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

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3607/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3607/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3607/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3607/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/12394123/HADOOP-4616_0.19.txt against trunk revision 719152. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3607/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3607/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3607/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3607/console This message is automatically generated.
          Hide
          Pete Wyckoff added a comment -

          ready to commit.

          Please apply :

          patch HADOOP-4616_0.19.txt to trunk and 0.19.1
          patch HADOOP-4616_0.18.2.txt to 0.18.2

          thanks, pete

          Show
          Pete Wyckoff added a comment - ready to commit. Please apply : patch HADOOP-4616 _0.19.txt to trunk and 0.19.1 patch HADOOP-4616 _0.18.2.txt to 0.18.2 thanks, pete
          Hide
          dhruba borthakur added a comment -

          This patch introduces a debug print into stderr in libhdfs:

          + if (noReadBytes == 0 || noReadBytes < -1)

          { + fprintf(stderr, "WARN: FSDataInputStream.read returned invalid return code - libhdfs returning EOF, i.e., 0: %d\n", noReadBytes); + }

          I am fine with it but would like to poll the user-community to see if this could break any application that is currently using libhdfs.

          Show
          dhruba borthakur added a comment - This patch introduces a debug print into stderr in libhdfs: + if (noReadBytes == 0 || noReadBytes < -1) { + fprintf(stderr, "WARN: FSDataInputStream.read returned invalid return code - libhdfs returning EOF, i.e., 0: %d\n", noReadBytes); + } I am fine with it but would like to poll the user-community to see if this could break any application that is currently using libhdfs.
          Hide
          dhruba borthakur added a comment -

          I just committed this. Thanks Pete!

          Show
          dhruba borthakur added a comment - I just committed this. Thanks Pete!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk #668 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/668/)
          . Fuse-dfs can handle bad values from FileSystem.read call.
          (Pete Wyckoff via dhruba)

          Show
          Hudson added a comment - Integrated in Hadoop-trunk #668 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/668/ ) . Fuse-dfs can handle bad values from FileSystem.read call. (Pete Wyckoff via dhruba)

            People

            • Assignee:
              Pete Wyckoff
              Reporter:
              Marc-Olivier Fleury
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development