Re: vmpage race and deadlock

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

Juergen Hannken-Illjes <hannken%eis.cs.tu-bs.de@localhost> wrote:
> Some weeks ago I started my fs stress test (5 x fsstress+fsx+dbench)
> on a log enabled ffs1 file system backed by md(4).
> 
> Usually within hours I get a deadlock where a thread is waiting on
> "genput" but the page in question is neither BUSY nor WANTED.  I suppose
> I tracked (*1) it down to three places, where we change page flags
> without holding the object lock.  With this diff (*2) in place the test
> runs for > 48 hours.

Cool!

> Using atomic ops here is not possible as flags is a 16bit value.
> 

You cannot use them at one place; in such case all uses i.e. protocol
would need to be converted to atomic ops.

> @@ -637,8 +639,9 @@ loopdone:
>                   npages << PAGE_SHIFT, 0, cred);
>               UVMHIST_LOG(ubchist, "gop_alloc off 0x%x/0x%x -> %d",
>                   startoffset, npages << PAGE_SHIFT, error,0);
>               if (!error) {
> +                     mutex_enter(&uobj->vmobjlock);
>                       for (i = 0; i < npages; i++) {
>                               struct vm_page *pg = pgs[i];
>  
>                               if (pg == NULL) {
> @@ -647,8 +650,9 @@ loopdone:
>                               pg->flags &= ~(PG_CLEAN|PG_RDONLY);
>                               UVMHIST_LOG(ubchist, "mark dirty pg %p",
>                                   pg,0,0,0);
>                       }
> +                     mutex_exit(&uobj->vmobjlock);
>               }
>       }
>       if (!glocked) {
>               genfs_node_unlock(vp);

I think this loop can actually be moved after mutex_enter(&uobj->vmobjlock)
at line 660 (with condition check, of course).

> Index: sys/uvm/uvm_bio.c
> ===================================================================
> RCS file: /cvsroot/src/sys/uvm/uvm_bio.c,v
> retrieving revision 1.70
> diff -p -u -4 -r1.70 uvm_bio.c
> --- sys/uvm/uvm_bio.c 22 Jun 2010 18:34:50 -0000      1.70
> +++ sys/uvm/uvm_bio.c 22 Nov 2010 09:11:18 -0000
> @@ -644,8 +644,9 @@ ubc_release(void *va, int flags)
>               if (zerolen) {
>                       memset((char *)umapva + endoff, 0, zerolen);
>               }
>               umap->flags &= ~UMAP_PAGES_LOCKED;
> +             mutex_enter(&uobj->vmobjlock);
>               mutex_enter(&uvm_pageqlock);
>               for (i = 0; i < npages; i++) {
>                       rv = pmap_extract(pmap_kernel(),
>                           umapva + slot_offset + (i << PAGE_SHIFT), &pa);
> @@ -657,9 +658,8 @@ ubc_release(void *va, int flags)
>               }
>               mutex_exit(&uvm_pageqlock);
>               pmap_kremove(umapva, ubc_winsize);
>               pmap_update(pmap_kernel());
> -             mutex_enter(&uobj->vmobjlock);
>               uvm_page_unbusy(pgs, npages);
>               mutex_exit(&uobj->vmobjlock);
>               unmapped = true;
>       } else {

This one is already made in rmind-uvplock branch.

Thanks for catching these!

-- 
Mindaugas