11820 upstream Nexenta iSCSI and COMSTAR fixes

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


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
  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. usr/src/uts/common/fs/doorfs/door_sys.c (Diff revision 2)

    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

  3. 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.

  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.

  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.

  6. stmf_worker_warn appears to be unused.

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

  7. 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.

  8. 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.

  9. 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.

  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.

  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.

  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. Another possible place to relese the itask_mutex.

  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.

  15. 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.

  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.

  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.

  18. usr/src/uts/common/io/idm/idm.c (Diff revision 2)

    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.

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

  2. 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.

Review request changed

Status: Closed (submitted)