Re: CVS commit: src/sys/ufs/lfs

[Antti Kantee <pooka%cs.hut.fi@localhost>]

On Mon Dec 07 2009 at 04:12:10 +0000, Eduardo Horvath wrote:
> Module Name:  src
> Committed By: eeh
> Date:         Mon Dec  7 04:12:10 UTC 2009
> 
> Modified Files:
>       src/sys/ufs/lfs: lfs_bio.c lfs_vfsops.c lfs_vnops.c
> 
> Log Message:
> Fix some more hangs and deadlocks.

How can replacing reference counting by adjusting reclaim order fix
anything?

> @@ -318,11 +318,9 @@
>        * vref vnodes here so that cleaner doesn't try to reuse them.
>        * (see XXX comment in lfs_reserveavail)
>        */
> -     mutex_enter(&vp->v_interlock);
> -     lfs_vref(vp);
> +     VHOLD(vp);
>       if (vp2 != NULL) {
> -             mutex_enter(&vp2->v_interlock);
> -             lfs_vref(vp2);
> +             VHOLD(vp2);
>       }
>  
>       error = lfs_reserveavail(fs, vp, vp2, fsb);
> @@ -338,9 +336,9 @@
>               lfs_reserveavail(fs, vp, vp2, -fsb);
>  
>  done:
> -     lfs_vunref(vp);
> +     HOLDRELE(vp);
>       if (vp2 != NULL) {
> -             lfs_vunref(vp2);
> +             HOLDRELE(vp2);
>       }
>  
>       return error;

Postponing sync flushing doesn't smell like it preserves the correct
semantics.  Can you explain why it works in a bit more detail than in
the commit message?  It would be good to explain it in the comments also
so that someone trying to later read the code doesn't have to go "huh??".

> Index: src/sys/ufs/lfs/lfs_vfsops.c
> diff -u src/sys/ufs/lfs/lfs_vfsops.c:1.280 src/sys/ufs/lfs/lfs_vfsops.c:1.281
> --- src/sys/ufs/lfs/lfs_vfsops.c:1.280        Tue Nov 17 17:08:57 2009
> +++ src/sys/ufs/lfs/lfs_vfsops.c      Mon Dec  7 04:12:10 2009
> @@ -1,4 +1,4 @@
> -/*   $NetBSD: lfs_vfsops.c,v 1.280 2009/11/17 17:08:57 pooka Exp $   */
> +/*   $NetBSD: lfs_vfsops.c,v 1.281 2009/12/07 04:12:10 eeh Exp $     */
>  
>  /*-
>   * Copyright (c) 1999, 2000, 2001, 2002, 2003, 2007, 2007
> @@ -61,7 +61,7 @@
>   */
>  
>  #include <sys/cdefs.h>
> -__KERNEL_RCSID(0, "$NetBSD: lfs_vfsops.c,v 1.280 2009/11/17 17:08:57 pooka 
> Exp $");
> +__KERNEL_RCSID(0, "$NetBSD: lfs_vfsops.c,v 1.281 2009/12/07 04:12:10 eeh Exp 
> $");
>  
>  #if defined(_KERNEL_OPT)
>  #include "opt_lfs.h"
> @@ -1549,6 +1549,7 @@
>       vaddr_t kva;
>       off_t eof, offset, startoffset = 0;
>       size_t bytes, iobytes, skipbytes;
> +     bool async = (flags & PGO_SYNCIO) == 0;
>       daddr_t lbn, blkno;
>       struct vm_page *pg;
>       struct buf *mbp, *bp;
> @@ -1778,6 +1779,14 @@
>               UVMHIST_LOG(ubchist, "skipbytes %d", skipbytes, 0,0,0);
>       }
>       UVMHIST_LOG(ubchist, "returning 0", 0,0,0,0);
> +
> +     if (!async) {
> +             /* Start a segment write. */
> +             UVMHIST_LOG(ubchist, "flushing", 0,0,0,0);
> +             mutex_enter(&lfs_lock);
> +             lfs_flush(fs, 0, 1);
> +             mutex_exit(&lfs_lock);
> +     }
>       return (0);
>  
>      tryagain:
> 
> Index: src/sys/ufs/lfs/lfs_vnops.c
> diff -u src/sys/ufs/lfs/lfs_vnops.c:1.225 src/sys/ufs/lfs/lfs_vnops.c:1.226
> --- src/sys/ufs/lfs/lfs_vnops.c:1.225 Tue Nov 17 22:49:24 2009
> +++ src/sys/ufs/lfs/lfs_vnops.c       Mon Dec  7 04:12:10 2009
> @@ -1,4 +1,4 @@
> -/*   $NetBSD: lfs_vnops.c,v 1.225 2009/11/17 22:49:24 eeh Exp $      */
> +/*   $NetBSD: lfs_vnops.c,v 1.226 2009/12/07 04:12:10 eeh Exp $      */
>  
>  /*-
>   * Copyright (c) 1999, 2000, 2001, 2002, 2003 The NetBSD Foundation, Inc.
> @@ -60,7 +60,7 @@
>   */
>  
>  #include <sys/cdefs.h>
> -__KERNEL_RCSID(0, "$NetBSD: lfs_vnops.c,v 1.225 2009/11/17 22:49:24 eeh Exp 
> $");
> +__KERNEL_RCSID(0, "$NetBSD: lfs_vnops.c,v 1.226 2009/12/07 04:12:10 eeh Exp 
> $");
>  
>  #ifdef _KERNEL_OPT
>  #include "opt_compat_netbsd.h"
> @@ -1768,7 +1768,8 @@
>       if (pg == NULL)
>               return;
>  
> -     while (pg->flags & PG_BUSY) {
> +     while (pg->flags & PG_BUSY &&
> +         pg->uobject == &vp->v_uobj) {
>               mutex_exit(&vp->v_interlock);
>               if (sp->cbpp - sp->bpp > 1) {
>                       /* Write gathered pages */
> @@ -2157,7 +2158,7 @@
>                */
>               ip->i_lfs_iflags |= LFSI_NO_GOP_WRITE;
>               r = genfs_do_putpages(vp, startoffset, endoffset,
> -                                    ap->a_flags, &busypg);
> +                                    ap->a_flags & ~PGO_SYNCIO, &busypg);
>               ip->i_lfs_iflags &= ~LFSI_NO_GOP_WRITE;
>               if (r != EDEADLK)
>                       return r;
>