* Juan Quintela (quint...@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
> > * Juan Quintela (quint...@redhat.com) wrote:
> >> "Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
> >> > * Juan Quintela (quint...@redhat.com) wrote:
> >> >> We know quit closing the QIO.
> >> >
> >> > This patch does two different things; one of which I think I understand.
> >> >
> >> > The 'quit' has been removed - I think that makes sense because the
> >> > multifd threads terminate when either they come to the end of a stream
> >> > or hit an error; there's no case of asking them to quit explicitly.
> >> > The 'sem' was basically for kicking the recv-thread to quit; which again
> >> > isn't needed.
> >> >
> >> > Now, what about the object_unref on p->c ?
> >> > If I've got this right multifd_recv_terminate_threads is only called in
> >> > the error case; but doesn't multifd_load_cleanup also get called in that
> >> > case - it does the unref and p->c = NULL as well.
> >>
> >> Adding another comment O:-)
> >>
> >> In normal exit case: we don't care.
> >>
> >> In error case, we do a close of the p->c. Doing a close, means that the
> >> channel threads that are waiting on qio_channel_read_all_eof() will stop
> >> the waiting and return an error, so we are safe.
> >>
> >> ret = qio_channel_read_all_eof(p->c, (void *)p->packet,
> >> p->packet_len, &local_err);
> >> if (ret == 0) { /* EOF */
> >> break;
> >> }
> >> if (ret == -1) { /* Error */
> >> break;
> >> }
> >>
> >> We have to wait for three things:
> >> - we receive a synchronization packet
> >> - we receive EOF and finish
> >> - we got somehow an error and have to quit.
> >>
> >> Old versions have a semaphore, and we have three places where we set
> >> that semaphore "we are ready", when we receive one error, when we have
> >> data waiting to be read (it had a qio whatcher) or we just got a normal
> >> exit.
> >>
> >> Waiting on two things make code more complex that it used to be (and
> >> whatchers are really lame, because we know there are data ready, but not
> >> how much, dealing with non-whole packets/pages is a real mess). So we
> >> moved to:
> >> - quit is gone: we just close the channel, that makes the *_read_eof()
> >> to end. Notice that we don't care if the close is due to one error
> >> or because we have finished. It is just done.
> >> - Now reception channel only have to wait on _read_eof(), the three
> >> cases that we talked before are handled correctly.
> >>
> >> I understand the confusion, this only makes sense when you came from the
> >> previous version of the patchset.
> >
> > Two questions from that then:
> > a) Are you sure it's safe to close the qio_channel while another
> > thread is in qio_channel_read_all_eof? Is it really defined that it
> > causes the other thread to exit with an error; close() in some stuff
> > frees data structures that the other thread is still reading; that's
> > why I've used shutdown(2) in the past rather than close on fd's
>
> Dunno if it is safe (I think it is), but I agree that shutdown will also
> get what I need.
>
> > b) I don't think your answer explains why it's an object_unref?
>
> That is the standard way to closing qios to not have to take into
> account who have it oppened. See previous paragraph, it is better to
> use shutdown, done.
OK, great; I suspect it's unsafe because as soon as you do the unref
it could free the object; actually you should have a ref from each of
the threads to sotp it being freed while they use it.
Dave
> Thanks, Juan.
>
> > Dave
> >
> >> Later, Juan.
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
