svn commit: r277419 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs

[Matthew Ahrens]

On Fri, Aug 14, 2015 at 7:23 AM, Andriy Gapon <avg at freebsd.org> wrote:

> On 20/01/2015 15:09, Alexander Motin wrote:
> > Author: mav
> > Date: Tue Jan 20 13:09:12 2015
> > New Revision: 277419
> > URL: https://svnweb.freebsd.org/changeset/base/277419
> >
> > Log:
> >   Allow skipping dmu_buf_will_dirty() call in dsl_dir_transfer_space().
> >
> >   dsl_dir_transfer_space() is mostly called after dsl_dir_diduse_space(),
> >   which already calls dmu_buf_will_dirty() for the same dbuf and tx, so
> >   its duplicate call in those cases will change nothing, only spend time.
> >
> >   Skipping this call by four times reduces time spent in
> dbuf_write_done()
> >   and descendants, updating dataset statistics with several congested
> lock
> >   acquisitions.  When rewriting 8K zvol blocks at 1GB/s rate, this
> reduces
> >   CPU time spent inside dbuf_write_done(), according to profiling, from
> 45%
> >   of 683K samples to 18% of 422K.
>
> It probably would make sense to discuss such a change with the upstream
> folks.  Or, at the very least, let them know about it.
>

I have observed similar perf issues and have fixed (in a private,
not-fully-tested branch) in a similar way. These changes depend on the fact
that the dbuf is already dirty.  To ensure that this remains true in the
future, we should add assertions (which run on debug bits only) that it is
already dirty, and add comments explaining where it was dirtied, and add a
comment where it is dirtied explaining the other places that depend on the
dirtying.

--matt


>
>
> >   MFC after:  2 weeks
> >
> > Modified:
> >   head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dataset.c
> >   head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dir.c
> >
> > Modified:
> head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dataset.c
> >
> ==============================================================================
> > --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dataset.c
> Tue Jan 20 12:28:24 2015        (r277418)
> > +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dataset.c
> Tue Jan 20 13:09:12 2015        (r277419)
> > @@ -136,7 +136,7 @@ dsl_dataset_block_born(dsl_dataset_t *ds
> >       dsl_dir_diduse_space(ds->ds_dir, DD_USED_HEAD, delta,
> >           compressed, uncompressed, tx);
> >       dsl_dir_transfer_space(ds->ds_dir, used - delta,
> > -         DD_USED_REFRSRV, DD_USED_HEAD, tx);
> > +         DD_USED_REFRSRV, DD_USED_HEAD, NULL);
> >  }
> >
> >  int
> > @@ -179,7 +179,7 @@ dsl_dataset_block_kill(dsl_dataset_t *ds
> >               dsl_dir_diduse_space(ds->ds_dir, DD_USED_HEAD,
> >                   delta, -compressed, -uncompressed, tx);
> >               dsl_dir_transfer_space(ds->ds_dir, -used - delta,
> > -                 DD_USED_REFRSRV, DD_USED_HEAD, tx);
> > +                 DD_USED_REFRSRV, DD_USED_HEAD, NULL);
> >       } else {
> >               dprintf_bp(bp, "putting on dead list: %s", "");
> >               if (async) {
> > @@ -2837,7 +2837,7 @@ dsl_dataset_clone_swap_sync_impl(dsl_dat
> >                   origin_head->ds_dir->dd_origin_txg, UINT64_MAX,
> >                   &odl_used, &odl_comp, &odl_uncomp);
> >               dsl_dir_transfer_space(origin_head->ds_dir, cdl_used -
> odl_used,
> > -                 DD_USED_HEAD, DD_USED_SNAP, tx);
> > +                 DD_USED_HEAD, DD_USED_SNAP, NULL);
> >       }
> >
> >       /* swap ds_*_bytes */
> >
> > Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dir.c
> >
> ==============================================================================
> > --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dir.c
>  Tue Jan 20 12:28:24 2015        (r277418)
> > +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dir.c
>  Tue Jan 20 13:09:12 2015        (r277419)
> > @@ -1389,7 +1389,7 @@ dsl_dir_diduse_space(dsl_dir_t *dd, dd_u
> >                   accounted_delta, compressed, uncompressed, tx);
> >               dsl_dir_transfer_space(dd->dd_parent,
> >                   used - accounted_delta,
> > -                 DD_USED_CHILD_RSRV, DD_USED_CHILD, tx);
> > +                 DD_USED_CHILD_RSRV, DD_USED_CHILD, NULL);
> >       }
> >  }
> >
> > @@ -1397,7 +1397,7 @@ void
> >  dsl_dir_transfer_space(dsl_dir_t *dd, int64_t delta,
> >      dd_used_t oldtype, dd_used_t newtype, dmu_tx_t *tx)
> >  {
> > -     ASSERT(dmu_tx_is_syncing(tx));
> > +     ASSERT(tx == NULL || dmu_tx_is_syncing(tx));
> >       ASSERT(oldtype < DD_USED_NUM);
> >       ASSERT(newtype < DD_USED_NUM);
> >
> > @@ -1405,7 +1405,8 @@ dsl_dir_transfer_space(dsl_dir_t *dd, in
> >           !(dsl_dir_phys(dd)->dd_flags & DD_FLAG_USED_BREAKDOWN))
> >               return;
> >
> > -     dmu_buf_will_dirty(dd->dd_dbuf, tx);
> > +     if (tx != NULL)
> > +             dmu_buf_will_dirty(dd->dd_dbuf, tx);
> >       mutex_enter(&dd->dd_lock);
> >       ASSERT(delta > 0 ?
> >           dsl_dir_phys(dd)->dd_used_breakdown[oldtype] >= delta :
> >
>
>
> --
> Andriy Gapon
>
>