[illumos-Developer] webrev: 1039 /usr/lib/smbsrv/smbd prevents suspend

[Gordon Ross]

On Thu, May 26, 2011 at 12:59 PM, Eric Schrock <eric.schrock at delphix.com> wrote:
> This definitely seems to be an improvement, but I'm having a hard time
> understanding the code flow.  Perhaps it's beyond the scope of this fix, but
> can you explain the cases in which this cv is signaled and how the userland
> component works?  Looking at the code, there seem to be two cases we signal
> this cv:
> 1. When transitioning out of the SMB_SERVER_STATE_RUNNING state
> 2. When adding a FID to the spool list in smb_com_close_print_file()
> The latter doesn't appear to be protected by any kind of mutex.  More

The list has an rwlock.  But the cv_broadcast is supposed to hold the
mutex protecting that cv.  I meant to fix that, so thanks for the reminder.
(Interestingly, the kernel lets us get away with not holding that mutex:)

> importantly, the code in smb_server_spooldoc() does an unconditional
> cv_wait_sig() first without checking the FID list.  So there can be
> perfectly valid data waiting for us, but we'll sleep forever (or at least
> until the server stops running or another print file is closed) before

smb_close_print_file adds a fid to the list and then signals sp_cv
(via cv_broadcast) so the spooldoc thread should get its wakeup.

> checking it.  We then implement a retry loop in userland with an arbitrary
> timeout (5 retries) with no explanation of why or what the behavior is in
> this case (the thread just exits?).  You've now added two new error codes,
> EINTR and EAGAIN, but haven't touched the userland code; what are the
> ramifications of receiving these new errors?

It could affect that (bogus) retry count thing.
I've changed it so these new error codes
will not affect the retry count.

> If the intent is to just have it block in the kernel until something's
> available, it seems like it could be much simpler.

Yeah...

I've updated the webrev based on your feedback.
Thanks for taking a look.

Gordon