7444 fs/xattr.c should be more transparent

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

Information
gwr
illumos-gate
7444
Reviewers
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
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. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




jjelinek




   

   





  

 



 

  


   

   

    
 
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. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




gwr




   

   







  

 



 

  


   

   

    
 
gwr


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/uts/common/fs/xattr.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    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.
      
 
      

      2. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




gwr




   

   

  

 



 















 





 

  

   




gwr




   

   





  

 



 

  


   

   

    
 
gwr


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/uts/common/fs/xattr.c
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




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

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




gwr




   

   

  

 



 















 





 

  

   




jjelinek




   

   





  

 



 

  


   

   

    
 
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. 



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

      3. 



    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!
      
 
      

      4. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




gwr




   

   

  

 



 















 



 

  

   




gwr




   

   

  

 



 















 



 

  

   




gwr




   

   

  

 



 















 



 

  

   




gwr




   

   

  

 



 















 





 

  

   




gwr




   

   





  

 



 

  


   

   

    
 
gwr


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  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.
      
 
      

      2. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




gwr




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...