Re: [Virtio-fs] [PATCH] virtiofsd: Fix lo_destroy crash in g_hash_table_foreach_remove

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Wed, Aug 07, 2019 at 09:29:55AM -0400, Vivek Goyal wrote:
> > On Wed, Aug 07, 2019 at 02:17:15PM +0100, Dr. David Alan Gilbert wrote:
> > > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > > On Wed, Aug 07, 2019 at 10:31:52AM +0100, Dr. David Alan Gilbert (git) wrote:
> > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > > > 
> > > > > This fixes a crash in lo_destroy since g_hash_table_foreach_remove
> > > > > doesn't like the hashtable changing as it iterates, and unref_inode
> > > > > will remove entries.
> > > > 
> > > > Hi David,
> > > > 
> > > > Got two questions.
> > > > 
> > > > - Shouldn't we take a lo->mutex to make sure parallel hash table
> > > >   modifications are not happening.
> > > 
> > > See Stefan's big comment (which I just changed the function name in)
> > > which explains that we can't take lo->mutex
> > 
> > That comment says that unref_inode() takes the lock and that's why
> > we can't take it.
> > 
> > But that's easy to fix. We just have to come up with another helper which
> > does not take lock and asssumes lock has already been taken.
> > 
> > /* This assumes lo->lock is already held */
> > __unref_inode()
> > {
> > }
> 
> Nice idea.  It would be cleaner to drop the comments and introduce a
> unref_inode_locked() function so we can follow the usual locking regime.
> 
> (Linux uses __foo() but double underscore identifiers are reserved in
> the C language standard, so IMO it's better to avoid them).

OK< yeh that's easy enough to do.

> 
> > > 
> > > > - Also before destroying lo, should we sever connection so that any
> > > >   requests which come after lo_destroy() are not entertained.
> > > 
> > > No, because in some situations lo_destroy does not happen at the end;
> > > e.g. it happens during a umount and we still have the connection
> > > to be able to remount it.
> > 
> > Ok, so we don't sever the connection completely. But we don't expect to
> > process further requests till a new INIT has been done, isn't it?
> > 
> > IOW, once lo_destroy() is received, we cleanup any pending state from
> > the mount and if any requests are received from client, we error them
> > out.
> > 
> > And start processing requests normally when a new INIT has been
> > received.
> > 
> > Atleast that was my understanding of the design.
> 
> There is a state machine that does:
> 
> 1. Drain in-flight requests when DESTROY is received.

Hmm does that drain happen in the reboot case where there wasn't
actually a destroy message? (fuse_session_process_buf_int 
where it calls se->op.destroy explicitly)

Dave

> 2. Process DESTROY in a serialized fashion.
> 3. Forbid any requests until the next INIT.  <-- requests queued after
> 						 DESTROY are rejected
> 						 here
> 4. Process INIT in a serialized fashion.
> 
> Stefan


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK