kern/178997: Heavy disk I/O may hang system

[Klaus Weber]

The following reply was made to PR kern/178997; it has been noted by GNATS.

From: Klaus Weber <fbsd-bugs-2013-1 at unix-admin.de>
To: Bruce Evans <brde at optusnet.com.au>, freebsd-gnats-submit at FreeBSD.org
Cc: Klaus Weber <fbsd-bugs-2013-1 at unix-admin.de>
Subject: Re: kern/178997: Heavy disk I/O may hang system
Date: Wed, 31 Jul 2013 18:18:59 +0200

 --OgqxwSJOaUobr8KG
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline
 
 After studying style(9), I have now made a revised version of the
 patch, relative to 9/STABLE of about 3 weeks ago:
 
 Index: sys/kern/vfs_bio.c
 ===================================================================
 --- sys/kern/vfs_bio.c  (revision 253339)
 +++ sys/kern/vfs_bio.c  (working copy)
 @@ -1146,7 +1146,7 @@
         if (bo->bo_dirty.bv_cnt > dirtybufthresh + 10) {
                 (void) VOP_FSYNC(bp->b_vp, MNT_NOWAIT, curthread);
                 altbufferflushes++;
 -       } else if (bo->bo_dirty.bv_cnt > dirtybufthresh) {
 +       } else if (bdflush_required(bo)) {
                 BO_LOCK(bo);
                 /*
                  * Try to find a buffer to flush.
 @@ -1438,6 +1438,24 @@
  }
 
  /*
 + *     Flushing is deemed necessary if at least one of the following is true:
 + *     - bo is hogging more than dirtybufthresh buffers (previous behavior)
 + *     - numdirtybuffers exceeds dirtybufthresh
 + *     - we are or act as bufdaemon (TDP_NORUNNINGBUF)
 + */
 +int
 +bdflush_required(struct bufobj *bo)
 +{
 +       struct thread *td = curthread;
 +
 +       KASSERT(bo != NULL, ("bdflush_required: NULL bo"));
 +
 +       return ((bo->bo_dirty.bv_cnt > dirtybufthresh) ||
 +               (numdirtybuffers > dirtybufthresh) ||
 +               (td && (td->td_pflags & TDP_NORUNNINGBUF)));
 +}
 +
 +/*
   *     brelse:
   *
   *     Release a busy buffer and, if requested, free its resources.  The
 Index: sys/sys/buf.h
 ===================================================================
 --- sys/sys/buf.h       (revision 253339)
 +++ sys/sys/buf.h       (working copy)
 @@ -486,6 +486,7 @@
  void   bdata2bio(struct buf *bp, struct bio *bip);
  void   bwillwrite(void);
  int    buf_dirty_count_severe(void);
 +int    bdflush_required(struct bufobj *bo);
  void   bremfree(struct buf *);
  void   bremfreef(struct buf *);        /* XXX Force bremfree, only for nfs. */
  int    bread(struct vnode *, daddr_t, int, struct ucred *, struct buf **);
 Index: sys/ufs/ffs/ffs_snapshot.c
 ===================================================================
 --- sys/ufs/ffs/ffs_snapshot.c  (revision 253339)
 +++ sys/ufs/ffs/ffs_snapshot.c  (working copy)
 @@ -2161,7 +2161,7 @@
         struct buf *nbp;
         int bp_bdskip;
 
 -       if (bo->bo_dirty.bv_cnt <= dirtybufthresh)
 +       if (!bdflush_required(bo))
                 return;
 
         td = curthread;
 
 [End of patch]
 
 Besides being more concise, I hope this patch no longer has
 style-issues, although I was not sure regarding the indention of the
 second and third line of the return statement. I chose to left-align
 the condtions.
 
 I have also tested limiting numdirtybuffers to hidirtybuffers (instead
 of dirtybufthresh), but that does not prevent the system from hanging.
 
 I would appreciate if someone could review and eventually commit this
 patch (or a variant thereof), and then close this bug.
 
 Thanks, Klaus
 
 --OgqxwSJOaUobr8KG
 Content-Type: text/x-diff; charset=us-ascii
 Content-Disposition: attachment; filename="bo_dirty.bv_cnt_FIX_FINAL.patch"
 
 Index: sys/kern/vfs_bio.c
 ===================================================================
 --- sys/kern/vfs_bio.c	(revision 253339)
 +++ sys/kern/vfs_bio.c	(working copy)
 @@ -1146,7 +1146,7 @@
  	if (bo->bo_dirty.bv_cnt > dirtybufthresh + 10) {
  		(void) VOP_FSYNC(bp->b_vp, MNT_NOWAIT, curthread);
  		altbufferflushes++;
 -	} else if (bo->bo_dirty.bv_cnt > dirtybufthresh) {
 +	} else if (bdflush_required(bo)) {
  		BO_LOCK(bo);
  		/*
  		 * Try to find a buffer to flush.
 @@ -1438,6 +1438,24 @@
  }
  
  /*
 + *	Flushing is deemed necessary if at least one of the following is true:
 + *	- bo is hogging more than dirtybufthresh buffers (previous behavior)
 + *	- numdirtybuffers exceeds dirtybufthresh
 + *	- we are or act as bufdaemon (TDP_NORUNNINGBUF)
 + */
 +int
 +bdflush_required(struct bufobj *bo)
 +{
 +	struct thread *td = curthread;
 +
 +	KASSERT(bo != NULL, ("bdflush_required: NULL bo"));
 +
 +	return ((bo->bo_dirty.bv_cnt > dirtybufthresh) ||
 +	        (numdirtybuffers > dirtybufthresh) ||
 +	        (td && (td->td_pflags & TDP_NORUNNINGBUF)));
 +}
 +
 +/*
   *	brelse:
   *
   *	Release a busy buffer and, if requested, free its resources.  The
 Index: sys/sys/buf.h
 ===================================================================
 --- sys/sys/buf.h	(revision 253339)
 +++ sys/sys/buf.h	(working copy)
 @@ -486,6 +486,7 @@
  void	bdata2bio(struct buf *bp, struct bio *bip);
  void	bwillwrite(void);
  int	buf_dirty_count_severe(void);
 +int	bdflush_required(struct bufobj *bo);
  void	bremfree(struct buf *);
  void	bremfreef(struct buf *);	/* XXX Force bremfree, only for nfs. */
  int	bread(struct vnode *, daddr_t, int, struct ucred *, struct buf **);
 Index: sys/ufs/ffs/ffs_snapshot.c
 ===================================================================
 --- sys/ufs/ffs/ffs_snapshot.c	(revision 253339)
 +++ sys/ufs/ffs/ffs_snapshot.c	(working copy)
 @@ -2161,7 +2161,7 @@
  	struct buf *nbp;
  	int bp_bdskip;
  
 -	if (bo->bo_dirty.bv_cnt <= dirtybufthresh)
 +	if (!bdflush_required(bo))
  		return;
  
  	td = curthread;
 
 --OgqxwSJOaUobr8KG--