On Mon, Aug 30, 2021 at 10:45:47AM +0200, Martin Pieuchot wrote:
> On 06/08/21(Fri) 08:08, Theo Buehler wrote:
> > > The diff below fixes this by setting the "source" amap lock to the newly
> > > allocated one. This is not strictly necessary on OpenBSD since the amap
> > > is only inserted on the global list at the end of amap copy, but this
> > > satisfies the locking requirement of amap_wipeout() and is IMHO the
> > > simplest solution. This has also the advantage of reducing the
> > > differences with NetBSD.
> >
> > This diff causes a reproducible hang with mount_mfs(8) on my laptop. I
> > use an mfs noperm partition as described in
> > https://www.openbsd.org/faq/faq5.html#Release
> >
> > $ tail -1 /etc/fstab
> > swap /dest mfs rw,nosuid,noperm,-P/var/dest,-s1.5G,noauto 0 0
> >
> > If I run 'top -S -s.1' on an otherwise idle system and do 'mount /dest',
> > I see two mount_mfs(8) processes spin in fltamap and the pagedaemon is
> > spinning in pgdaemon before the system locks up completely (this takes
> > something between 1 and 20 seconds to happen).
>
> I couldn't reproduce this hang here. Do you also see it with the
> smaller fix below?
This appears to work fine. I can't reproduce the problem with this diff
across a few reboots.
The issue remains perfectly reproducible with the first version of the
diff, so if you want to poke at it, we can...
>
> Index: uvm/uvm_amap.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_amap.c,v
> retrieving revision 1.89
> diff -u -p -r1.89 uvm_amap.c
> --- uvm/uvm_amap.c 26 Mar 2021 13:40:05 -0000 1.89
> +++ uvm/uvm_amap.c 30 Aug 2021 08:31:21 -0000
> @@ -618,6 +618,13 @@ amap_copy(struct vm_map *map, struct vm_
> return;
> srcamap = entry->aref.ar_amap;
>
> + /*
> + * Make the new amap share the source amap's lock, and then lock
> + * both.
> + */
> + amap->am_lock = srcamap->am_lock;
> + rw_obj_hold(amap->am_lock);
> +
> amap_lock(srcamap);
>
> /*
> @@ -655,7 +662,7 @@ amap_copy(struct vm_map *map, struct vm_
>
> chunk = amap_chunk_get(amap, lcv, 1, PR_NOWAIT);
> if (chunk == NULL) {
> - amap_unlock(srcamap);
> + /* amap_wipeout() releases the lock. */
> amap->am_ref = 0;
> amap_wipeout(amap);
> return;
> @@ -695,10 +702,10 @@ amap_copy(struct vm_map *map, struct vm_
> * If we referenced any anons, then share the source amap's lock.
> * Otherwise, we have nothing in common, so allocate a new one.
> */
> - KASSERT(amap->am_lock == NULL);
> - if (amap->am_nused != 0) {
> - amap->am_lock = srcamap->am_lock;
> - rw_obj_hold(amap->am_lock);
> + KASSERT(amap->am_lock == srcamap->am_lock);
> + if (amap->am_nused == 0) {
> + rw_obj_free(amap->am_lock);
> + amap->am_lock = NULL;
> }
> amap_unlock(srcamap);
>
