7444 fs/xattr.c should be more transparent

Review Request #226 — Created Oct. 2, 2016 and submitted

gwr
illumos-gate
7444
general

7444 fs/xattr.c should be more transparent

extended attribute (SUNWattr_rw, SUNWattr_ro, etc)
on smbfs, zfs

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
estibi
  1. Ship It!
    1. Thanks. By any chance, do you have time to look at this too?
      https://www.illumos.org/rb/r/227/

    2. I had to do a little more work on this after noticing that things weren't quite right with an FS that supports SYSATTR but not XATTR.
      The fix actually makes these changes a little smaller. Mind having another look? Thanks!

  2. 
      
jjelinek
  1. This looks good to me. My only request is that you add a short comment in xattr_dir_inactive at line 1339 explaining the rele and how this interacts with the overall vnode holding.

    1. I added some comments above xattr_dir_inactive.

      I also added a "Big Theory Statement" at the top.
      (Wish it were there when I started:)
      If you can, please have a look at that additon.

  2. 
      
gwr
  1. 
      
  2. usr/src/uts/common/fs/xattr.c (Diff revision 1)
     
     

    Jerry Jelinek asked:
    ...add a short comment [here] explaining the rele and how this interacts with the overall vnode holding.

    1. Oh... I didn't see Jerry's note above, and so thought he mailed me unicast...
      when all I need to do was refresh this page. oh well. Ignore this comment.

  3. 
      
gwr
gwr
  1. 
      
  2. usr/src/uts/common/fs/xattr.c (Diff revision 2)
     
     

    Yeah, cstyle... I'll fix before I push.

  3. 
      
gwr
jjelinek
  1. I'm not sure if the copyright date is correct but other than that, this looks good to me.

    1. Yeah, this work was done ages ago...

    2. Thanks. By any chance, do you have time to look at this too?
      https://www.illumos.org/rb/r/227/

    3. I had to do a little more work on this after noticing that things weren't quite right with an FS that supports SYSATTR but not XATTR.
      The fix actually makes these changes a little smaller. Mind having another look? Thanks!

  2. 
      
gwr
gwr
gwr
gwr
gwr
  1. 
      
  2. usr/src/uts/common/fs/xattr.c (Diff revision 7)
     
     

    Just to explain why this function went back to how it was before: While it would "make sense" to try to get the "real" XATTR directory here (immediately after we get the GFS XATTR directory), getting the real XATTR directory can legitimately fail, i.e. if the real FS supports SYSATTR but not XATTR, or if the real FS chooses to create the XATTR directory only when the CREATE_XATTRDIR flag is set.
    If the real FS declines to give us the XATTR directory for any reason, we still need to create the GFS directory because at this point we know we support (at least) SYSATTR, and the GFS XATTR dir. is needed for those.

    Given all that, there's not really any point trying to get the real XATTR dir here. That can just as well wait until we really need it, in xattr_dir_realdir.

    So that part of the code is like it was before.
    The remaining (important) differences is that: once we obtain the real XATTR directory, we "keep" it (VN_HOLD) via the reference saved in the GFS dir xattrdir_realvp.
    That fixes the problem where the real FS saw an artificially short lifetime of the XATTR directory.

    1. The latest looks good, thanks for comments.

  3. 
      
gwr
Review request changed

Status: Closed (submitted)

Loading...