11820 upstream Nexenta iSCSI and COMSTAR fixes

Review Request #2412 — Created Oct. 23, 2019 and submitted

Information
jjelinek
illumos-gate
11820
Reviewers
general 
This is upstreaming from Nexenta of work in the iSCSI and COMSTAR code. The full list of Nexenta commits and the upstreaming status is described in the bug report.




 













 

 






 
 
 

  

   
  

  
    
   
  • 
    
    0
   
    • 
   
  • 
    
    0
   
    • 
   
  • 
    
    3
   
    • 
   
  • 
    
    6
   
    • 
   
  • 
    
    9
   
    • 
  

 

 

  

   
    

     Description
     From
     Last Updated
    

   
   


    


    


    


    


    


    


    


    


    

   
  

 












  

 








 














 



 

  

   




jjelinek




   

   

  

 



 















 





 

  

   




kkantor




   

   







  

 



 

  


   

   

    
 
kkantor


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Thanks for bringing this over. I have a few comments, but they're mostly just clarifying questions for things that looks a little strange to me. Let me know which of these comments aren't really relevant.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/uts/common/fs/doorfs/door_sys.c
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Does this change mean door calls can return up to 4M of data now? This seems a little out of scope for this port, but I don't know the context of this change.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Yes, it looks like it increases the amount of data we can transfer in one door upcall from 1MB to 4MB. The reason it is included here is that it was part of the following Nexenta commit which is one of the relevant changes that I am trying to upstream.
      
6fbbe56416a NEX-16625 Max amount of iSCSI targets is hard limited with doorfs core definitions
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Did this flip the conditional? I don't really understand why we would want something not to be a power of 2.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I don't think it flipped it. The old code was negating the result of the macro and the new code is doing the check directly to get a non-zero result.
      
 
      

      2. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    




    
    This conditional could be merged into the prior conditional block.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I'd like to leave things as they are in the Nexenta src unless there is a bug.
      
 
      

      2. 


 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    




    
    I would personally rather see an ASSERT to document and enforce that the lock is held. This comment pattern is repeated for many functions in this file.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      We could add ASSERTS as follow-on fixes if we want.
      
 
      

      2. 


 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    stmf_worker_warn appears to be unused.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I searched all the kernel src and it is unused. I removed it.
      
 
      

      2. 


 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Should we be holding itask_mutex when we enter this conditional?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      This matches the Nexenta src. I think the reason it is ok not to hold that mutex is that the itask_proxy_msg_id doesn't change once it is set.
      
 
      

      2. 


 
    


    

 
    

    8. 






  8. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Should we also destroy irport->irport_kstat_estat in this function?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I think you're right. I don't see any code to clean that up. I added that.
      
 
      

      2. 


 
    


    

 
    

    9. 






  9. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    We can turf this sort of lint directive now, right? If so I see a lot of ARGSUSED and CSTYLED notes in this file.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      That seems orthogonal to upstreaming this code, but we could do that cleanup as a separate change.
      
 
      

      2. 


 
    


    

 
    

    10. 






  10. 
 
    
  
  
    
  
 

   
    
    
    




    
    This conditional could be merged into the outer conditional as well.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Again, I'd like to leave things as they are in the Nexenta src unless there is a bug.
      
 
      

      2. 


 
    


    

 
    

    11. 






  11. 
 
    
  
  
    
  
 

   
    
    
    




    
    It seems to me that we should release the itask_mutex before we return here since it's release on line 5101 before returning.
    

    It looks like stmf_task_free needs the mutex to be held on entry. It then releases and re-acquires the mutex but does not release the mutex on exit.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      It seems like the fact that we're freeing the task is why we don't subsequently drop the mutex.
      
 
      

      2. 


 
    


    

 
    

    12. 






  12. 
 
    
  
  
    
  
 

   
    
    
    




    
    Release the itask_mutex here too? Maybe this is a copy/paste issue since this is the same as the line 5106 block.
    
  
    
 
    

 
    
  



 
    

    13. 






  13. 
 
    
  
  
    
  
 

   
    
    
    




    
    Another possible place to relese the itask_mutex.
    
  
    
 
    

 
    
  



 
    

    14. 






  14. 
 
    
  
  
    
  
 

   
    
    
    




    
    Ah, maybe all of that discussion of leaving the itask_mutex acquired when returning is explained by this comment.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I'm not sure that is right since it seems like the freeing is the real reason.
      
 
      

      2. 


 
    


    

 
    

    15. 






  15. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    It seems like the old stmf_min_nworkers global would be useful here. It looks to have been replaced by the stmf_worker_warn global, but stmf_worker_warn is unused.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      We currently match the Nexenta src here. If we want to enhance this in some way, we could do that separately.
      
 
      

      2. 


 
    


    

 
    

    16. 






  16. 
 
    
  
  
    
  
 

   
    
    
    




    
    Is this a time-sensitive set of operations? I don't really understand why a macro would be simpler to maintain than a separate function (which would give us another fbt probe) as the comment seems to indicate.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I don't know if it is time sensitive. I agree with the general sentiment about fbt probes though.
      
 
      

      2. 


 
    


    

 
    

    17. 






  17. 
 
    
  
  
    
  
 

   
    
    
    




    
    I like the sound of these kstats! A little nit: the white space seems to be different between these two structs.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      It is, but this matches the Nexenta src we're trying to upstream.
      
 
      

      2. 


 
    


    

 
    

    18. 






  18. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/uts/common/io/idm/idm.c
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    We don't want the refcnt->ir_mutex to be held by the caller, right? Would that skew the results of this function?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      It looks like calling idm_refcnt_hold would actually skew the intention of this function.
      
 
      

      2. 


 
    


    

 
    

    19. 




  19. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    20. 




  

 
















 



 

  

   




jjelinek




   

   

  

 



 















 





 

  

   




kkantor




   

   







  

 



 

  


   

   

    
 
kkantor


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Thanks for fixing these couple things and looking into the rest! Looks good to me.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




danmcd




   

   







  

 



 

  


   

   

    
 
danmcd


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Ummm, if I return right here, I'm still holding itask->itask_mutex.  Isn't this a bug?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Ooops, my bad.  Free function expects the lock held.
      
 
      

      2. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




jjelinek




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...