On Mittwoch, 6. Mai 2020 19:49:10 CEST Greg Kurz wrote:
> > Ok, but why not both? Moving locks to worker thread and QemuMutex ->
> > CoMutex?
> Using CoMutex would be mandatory if we leave the locking where it sits
> today, so that the main thread can switch to other coroutines instead
> of blocking. We don't have the same requirement with the worker thread:
> it just needs to do the actual readdir() and then it goes back to the
> thread pool, waiting to be summoned again for some other work.
Yes, I know.
> So I'd
> rather use standard mutexes to keep things simple... why would you
> want to use a CoMutex here ?
Like you said, it would not be mandatory, nor a big deal, the idea was just if
a lock takes longer than expected then a worker thread could already continue
with another task. I mean the amount of worker threads are limited they are
not growing on demand, are they?
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.
> > > 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.
> > enough to justify squashing. The deadlock fix should make it through the
> > stable branches, while the code simplification should not. So that's
> > better
> > off as a separate cleanup patch.
>
> The issue has been there for such a long time without causing any
> trouble. Not worth adding churn in stable for a bug that is impossible
> to hit with a regular linux guest.
Who knows. There are also other clients out there. A potential deadlock is
still a serious issue after all.
> > > @@ -32,13 +32,20 @@ int coroutine_fn v9fs_co_readdir(V9fsPDU *pdu,
> > > V9fsFidState *fidp, struct dirent *entry;
> > >
> > > errno = 0;
> > >
> > > +
> > > + v9fs_readdir_lock(&fidp->fs.dir);
> > > +
> > >
> > > entry = s->ops->readdir(&s->ctx, &fidp->fs);
> > > if (!entry && errno) {
> > >
> > > err = -errno;
> > >
> > > + } else if (entry) {
> > > + memcpy(dent, entry, sizeof(*dent));
> > > + err = 1;
> >
> > I find using sizeof(*dent) a bit dangerous considering potential type
> > changes in future. I would rather use sizeof(struct dirent). It is also
> > more human friendly to read IMO.
>
> Hmm... I believe it's the opposite actually: with sizeof(*dent), memcpy
> will always copy the number of bytes that are expected to fit in *dent,
> no matter the type.
Yes, but what you intend is to flat copy a structure, not pointers. So no
matter how the type is going to be changed you always actually wanted
(semantically)
copy(sizeof(struct dirent), nelements)
Now it is nelements=1, in future it might also be nelements>1, but what you
certainly don't ever want here is
copy(sizeof(void*), nelements)
> But yes, since memcpy() doesn't do any type checking for us, I think
> I'll just turn this into:
>
> *dent = *entry;
Ok
Best regards,
Christian Schoenebeck
