Uploaded image for project: 'Lustre'
  1. Lustre
  2. LU-2610

sanity test_103: ACL copy failed zfs - umask ignored

Details

    • Bug
    • Resolution: Fixed
    • Blocker
    • Lustre 2.7.0
    • Lustre 2.4.0
    • lustre-master #1142 OFED build ZFS
    • 3
    • 6094

    Description

      This issue was created by maloo for sarah <sarah@whamcloud.com>

      This issue relates to the following test suite run: https://maloo.whamcloud.com/test_sets/e2ede512-5ae4-11e2-8985-52540035b04c.

      The sub-test test_103 failed with the following error:

      == sanity test 103: acl test =========================================== 16:53:11 (1357692791)
      performing cp ...
      [3] $ umask 022 -- ok
      [4] $ mkdir d -- ok
      [5] $ cd d -- ok
      [6] $ touch f -- ok
      [7] $ setfacl -m u:bin:rw f -- ok
      [8] $ ls -l f | awk -- '{ print $1 }' -- failed
      -rw-rw-r--+                           ? -rw-rw-rw-+                            
      [11] $ cp f g -- ok
      [12] $ ls -l g | awk -- '{ print $1 }' -- failed
      -rw-r--r--                            ? -rw-rw-rw-                             
      [15] $ rm g -- ok
      [16] $ cp -p f g -- ok
      [17] $ ls -l f | awk -- '{ print $1 }' -- failed
      -rw-rw-r--+                           ? -rw-rw-rw-+                            
      [20] $ mkdir h -- ok
      [21] $ echo blubb > h/x -- ok
      [22] $ cp -rp h i -- ok
      [23] $ cat i/x -- ok
      [26] $ rm -r i -- ok
      [31] $ setfacl -R -m u:bin:rwx h -- ok
      [32] $ getfacl --omit-header h/x -- failed
      user::rw-                             | user::rw-                              
      user:bin:rwx                          | user:bin:rwx                           
      group::r--                            ? group::rw-                             
      mask::rwx                             | mask::rwx                              
      other::r--                            ? other::rw-                             
                                            |                                        
      [40] $ cp -rp h i -- ok
      [41] $ getfacl --omit-header i/x -- failed
      user::rw-                             | user::rw-                              
      user:bin:rwx                          | user:bin:rwx                           
      group::r--                            ? group::rw-                             
      mask::rwx                             | mask::rwx                              
      other::r--                            ? other::rw-                             
                                            |                                        
      [49] $ cd .. -- ok
      [50] $ rm -r d -- ok
      22 commands (17 passed, 5 failed)
       sanity test_103: @@@@@@ FAIL:  
      

      Attachments

        Issue Links

          Activity

            [LU-2610] sanity test_103: ACL copy failed zfs - umask ignored

            Patch landed to master (prior to 2.6.54)

            utopiabound Nathaniel Clark added a comment - Patch landed to master (prior to 2.6.54)
            utopiabound Nathaniel Clark added a comment - Re-enable test_40 http://review.whamcloud.com/12153

            sanity.sh test_40 is being skipped due to this bug. If the problem was fixed by LU-2804 then a patch is needed to re-enable this test.

            adilger Andreas Dilger added a comment - sanity.sh test_40 is being skipped due to this bug. If the problem was fixed by LU-2804 then a patch is needed to re-enable this test.
            pjones Peter Jones added a comment -

            Lai advises that this issue will be more completely addressed by LU-2804

            pjones Peter Jones added a comment - Lai advises that this issue will be more completely addressed by LU-2804
            laisiyao Lai Siyao added a comment -

            Actually for ldiskfs __mdd_acl_init() is redundant, because ACL is handled internally by ldiskfs in creation already. IMHO this function exists because in original design, ACL initialization should be done in MDD layer, and only the interfaces of get/set xattr are needed for OSD. (this is how ZFS OSD is implemented).

            If this is true, __mdd_acl_init() should act the same as ldiskfs_init_acl(), and also note __mdd_acl_init() is now inside #ifdef CONFIG_FS_POSIX_ACL and if (got_def_acl), this will not be true any more, because __mdd_acl_init() should initialise mode even when acl doesn't exist.

            laisiyao Lai Siyao added a comment - Actually for ldiskfs __mdd_acl_init() is redundant, because ACL is handled internally by ldiskfs in creation already. IMHO this function exists because in original design, ACL initialization should be done in MDD layer, and only the interfaces of get/set xattr are needed for OSD. (this is how ZFS OSD is implemented). If this is true, __mdd_acl_init() should act the same as ldiskfs_init_acl(), and also note __mdd_acl_init() is now inside #ifdef CONFIG_FS_POSIX_ACL and if (got_def_acl), this will not be true any more, because __mdd_acl_init() should initialise mode even when acl doesn't exist.

            My patch still has the OSD layer taking care of setting the mode. The mdd change just ensures that if mode+umask is different the the current computed node that the osd layer is called finish setting that mode. I was cognizant of LU-974 patch, and I didn't want to change what layer set the mode.

            I could update my patch so that osd-ldiskfs takes advantage of the cr_umask instead of passing it in via current->fs->umask from mdd, which would essentially just move where current->fs->umask gets set from mdd_internal.h::mdo_create_obj to osd_handler.c::__osd_object_create.

            utopiabound Nathaniel Clark added a comment - My patch still has the OSD layer taking care of setting the mode. The mdd change just ensures that if mode+umask is different the the current computed node that the osd layer is called finish setting that mode. I was cognizant of LU-974 patch, and I didn't want to change what layer set the mode. I could update my patch so that osd-ldiskfs takes advantage of the cr_umask instead of passing it in via current->fs->umask from mdd, which would essentially just move where current->fs->umask gets set from mdd_internal.h::mdo_create_obj to osd_handler.c::__osd_object_create.
            laisiyao Lai Siyao added a comment -

            Ldiskfs handles ACL and creation mode in low level, see ldiskfs_new_inode(), in contrast, ZFS handles this in ZPL layer, however lustre calls DMU directly to make new node, that's why we see different result in these two OSD backend filesystem.

            Nathaniel, now that you want to fix all this in MDD layer, I think you need check the patch for LU-974, because in that patch it's left to be done in OSD.

            laisiyao Lai Siyao added a comment - Ldiskfs handles ACL and creation mode in low level, see ldiskfs_new_inode(), in contrast, ZFS handles this in ZPL layer, however lustre calls DMU directly to make new node, that's why we see different result in these two OSD backend filesystem. Nathaniel, now that you want to fix all this in MDD layer, I think you need check the patch for LU-974 , because in that patch it's left to be done in OSD.

            ZFS does behave correctly as a local filesystem (in my limited testing). It honoured umask during file creation, I didn't do extensive umask vs ACL testing though.

            utopiabound Nathaniel Clark added a comment - ZFS does behave correctly as a local filesystem (in my limited testing). It honoured umask during file creation, I didn't do extensive umask vs ACL testing though.
            laisiyao Lai Siyao added a comment -

            Chris, it's designed to be so because the creation mode is decided by umask and parent default ACL, and it's not a good idea to hold parent UPDATE lock by client in creation. So finally it's decided to send calling thread umask to MDS and let server backend filesystem to calculate the correct mode.

            Linux kernel also lets bottom filesystem to do this instead of in VFS, so that ldiskfs can get the correct mode. I don't find ZFS does this at the first glance of code. Nathaniel, could you test this with ZFS as local filesystem?

            laisiyao Lai Siyao added a comment - Chris, it's designed to be so because the creation mode is decided by umask and parent default ACL, and it's not a good idea to hold parent UPDATE lock by client in creation. So finally it's decided to send calling thread umask to MDS and let server backend filesystem to calculate the correct mode. Linux kernel also lets bottom filesystem to do this instead of in VFS, so that ldiskfs can get the correct mode. I don't find ZFS does this at the first glance of code. Nathaniel, could you test this with ZFS as local filesystem?

            It's because POSIX ACLs and umask interact and it's up to the filesystem to work out the correct interaction.

            I think ACL + umask calculations are done in OSD because of the different way's ACLs are stored (that's my impression from working through the code), but I agree it isn't well encapsulated.

            utopiabound Nathaniel Clark added a comment - It's because POSIX ACLs and umask interact and it's up to the filesystem to work out the correct interaction. I think ACL + umask calculations are done in OSD because of the different way's ACLs are stored (that's my impression from working through the code), but I agree it isn't well encapsulated.

            People

              utopiabound Nathaniel Clark
              maloo Maloo
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: