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. ;-) 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. > > > > > 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. Best regards, Christian Schoenebeck