[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] 9pfs: Fix potential deadlock of QEMU mainloop
From: |
Greg Kurz |
Subject: |
Re: [PATCH] 9pfs: Fix potential deadlock of QEMU mainloop |
Date: |
Thu, 7 May 2020 18:35:36 +0200 |
On Thu, 07 May 2020 17:03:46 +0200
Christian Schoenebeck <address@hidden> wrote:
> On Donnerstag, 7. Mai 2020 16:33:28 CEST Greg Kurz wrote:
> > > I also haven't reviewed QEMU's lock implementations in very detail, but
> > > IIRC CoMutexes are completely handled in user space, while QemuMutex uses
> > > regular OS mutexes and hence might cost context switches.
> >
> > ... since the locking would only been exercised with an hypothetical
> > client doing stupid things, this is beginning to look like bike-shedding
> > to me. :)
>
> Aha, keep that in mind when you're doing your next review. ;-)
>
Fair enough. :)
> No seriously, like I said, I don't really care too much about Mutex vs.
> CoMutex in you patch here. It was actually more about wide-picture thinking,
> i.e. other places of (co)mutexes being used or other potential changes that
> would make this or other uses more relevant one day.
>
Then I agree with you that it would be better to use CoMutex if we were
experiencing thread pool exhaustion indeed.
> > > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > > > > index 9e046f7acb51..ac84ae804496 100644
> > > > > > --- a/hw/9pfs/9p.c
> > > > > > +++ b/hw/9pfs/9p.c
> > > > > > @@ -2170,7 +2170,7 @@ static int coroutine_fn
> > > > > > v9fs_do_readdir_with_stat(V9fsPDU *pdu, int32_t count = 0;
> > > > > >
> > > > > > struct stat stbuf;
> > > > > > off_t saved_dir_pos;
> > > > > >
> > > > > > - struct dirent *dent;
> > > > > > + struct dirent dent;
> > > > > >
> > > > > > /* save the directory position */
> > > > > > saved_dir_pos = v9fs_co_telldir(pdu, fidp);
> > > > > >
> > > > > > @@ -2181,13 +2181,11 @@ static int coroutine_fn
> > > > > > v9fs_do_readdir_with_stat(V9fsPDU *pdu, while (1) {
> > > > > >
> > > > > > v9fs_path_init(&path);
> > > > > >
> > > > > > - v9fs_readdir_lock(&fidp->fs.dir);
> > > > > > -
> > > > >
> > > > > That's the deadlock fix, but ...
> > > > >
> > > > > > err = v9fs_co_readdir(pdu, fidp, &dent);
> > > > > >
> > > > > > - if (err || !dent) {
> > > > > > + if (err <= 0) {
> > > > > >
> > > > > > break;
> > > > > >
> > > > > > }
> > > > >
> > > > > ... even though this code simplification might make sense, I don't
> > > > > think
> > > > > it
> > > > > should be mixed with the deadlock fix together in one patch. They are
> > > > > not
> > > >
> > > > I could possibly split this in two patches, one for returning a copy
> > > > and one for moving the locking around, but...
> > > >
> > > > > related with each other, nor is the code simplification you are aiming
> > > > > trivial
> > > >
> > > > ... this assertion is somewhat wrong: moving the locking to
> > > > v9fs_co_readdir() really requires it returns a copy.
> > >
> > > Yeah, I am also not sure whether a split would make it more trivial enough
> > > in this case to be worth the hassle. If you find an acceptable solution,
> > > good, if not then leave it one patch.
> >
> > Another option would be to g_malloc() the dirent in v9fs_co_readdir() and
> > g_free() in the callers. This would cause less churn since we could keep
> > the same function signature.
>
> I was actually just going to suggest the same. So yes, looks like a less
> invasive change to me.
>
I'll just do that then :)
> Best regards,
> Christian Schoenebeck
>
>