Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-566

INode.permissions should be marked as volatile to avoid synchronization problems

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.21.0
    • Fix Version/s: None
    • Component/s: namenode
    • Labels:
      None

      Description

      Looking at INode, I can see that the long permissions field is updated in the synchronized updatePermissions, read in other non-synchronized contexts

      I believe that to avoid race conditions and other synchronisation problems, the field should be marked volatile

      1. The Java language specification declares that long and double can be written as two 32-bit writes, unless the field is volatile http://java.sun.com/docs/books/jls/second_edition/html/memory.doc.html#28733
      2. The JVM is free to re-order accesses to non-volatile data; reads of the permission may pick up out of date values
      3. Volatile data may be cached/optimised out, changes may not propagate

      I don't think its enough to make the write operation synchronised, as the reads are still unsynchronized, and in other threads can pick up values midway through the update, or cache the value.

      It's a simple fix: declare permissions as volatile.

      private volatile long permissions;
      

        Issue Links

          Activity

          Hide
          steve_l added a comment -

          I should add that a 64-bit server VM will use atomic 64 bit read/write operations; the risk of a partial write being visible is one for 32 bit machines.

          The language spec section 17.4 says

          "The load, store, read, and write actions on volatile variables are atomic, even if the type of the variable is double or long."

          Show
          steve_l added a comment - I should add that a 64-bit server VM will use atomic 64 bit read/write operations; the risk of a partial write being visible is one for 32 bit machines. The language spec section 17.4 says "The load, store, read, and write actions on volatile variables are atomic, even if the type of the variable is double or long."
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I agree that both read and write of fields should be synchronized in general.

          However, in the current NameNode implementation, all accesses (read/write) to INode have to first obtain the global lock, rootDir, Therefore, there is no synchronization problem.

          For this issue, we could possibly remove "synchronized" from INode.updatePermissionStatus(..) to avoid confusion.

          Show
          Tsz Wo Nicholas Sze added a comment - I agree that both read and write of fields should be synchronized in general. However, in the current NameNode implementation, all accesses (read/write) to INode have to first obtain the global lock, rootDir, Therefore, there is no synchronization problem. For this issue, we could possibly remove "synchronized" from INode.updatePermissionStatus(..) to avoid confusion.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I am going to remove "synchronized" from INode.updatePermissionStatus(..) via HDFS-3350.

          Show
          Tsz Wo Nicholas Sze added a comment - I am going to remove "synchronized" from INode.updatePermissionStatus(..) via HDFS-3350 .
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Just have committed HDFS-3350.

          Show
          Tsz Wo Nicholas Sze added a comment - Just have committed HDFS-3350 .

            People

            • Assignee:
              Tsz Wo Nicholas Sze
              Reporter:
              Steve Loughran
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development