Skip to content

Commit

Permalink
MFV r323535: 8585 improve batching done in zil_commit()
Browse files Browse the repository at this point in the history
FreeBSD notes:
- this MFV reverts FreeBSD commit r314549 to make the merge easier
- at present our emulation of cv_timedwait_hires is rather poor,
  so I elected to use cv_timedwait_sbt directly
Please see the differential revision for details.
Unfortunately, I did not get any positive reviews, so there could be
bugs in the FreeBSD-specific piece of the merge.
Hence, the long MFC timeout.

illumos/illumos-gate@1271e4b
illumos/illumos-gate@1271e4b

https://www.illumos.org/issues/8585
  The current implementation of zil_commit() can introduce significant
  latency, beyond what is inherent due to the latency of the underlying
  storage. The additional latency comes from two main problems:
  1. When there's outstanding ZIL blocks being written (i.e. there's
      already a "writer thread" in progress), then any new calls to
      zil_commit() will block waiting for the currently oustanding ZIL
      blocks to complete. The blocks written for each "writer thread" is
      coined a "batch", and there can only ever be a single "batch" being
      written at a time. When a batch is being written, any new ZIL
      transactions will have to wait for the next batch to be written,
      which won't occur until the current batch finishes.
  As a result, the underlying storage may not be used as efficiently
      as possible. While "new" threads enter zil_commit() and are blocked
      waiting for the next batch, it's possible that the underlying
      storage isn't fully utilized by the current batch of ZIL blocks. In
      that case, it'd be better to allow these new threads to generate
      (and issue) a new ZIL block, such that it could be serviced by the
      underlying storage concurrently with the other ZIL blocks that are
      being serviced.
  2. Any call to zil_commit() must wait for all ZIL blocks in its "batch"
      to complete, prior to zil_commit() returning. The size of any given
      batch is proportional to the number of ZIL transaction in the queue
      at the time that the batch starts processing the queue; which
      doesn't occur until the previous batch completes. Thus, if there's a
      lot of transactions in the queue, the batch could be composed of
      many ZIL blocks, and each call to zil_commit() will have to wait for
      all of these writes to complete (even if the thread calling
      zil_commit() only cared about one of the transactions in the batch).

Reviewed by: Brad Lewis <brad.lewis@delphix.com>
Reviewed by: Matt Ahrens <mahrens@delphix.com>
Reviewed by: George Wilson <george.wilson@delphix.com>
Approved by: Dan McDonald <danmcd@joyent.com>
Author: Prakash Surya <prakash.surya@delphix.com>

MFC after:	1 month
Differential Revision:	https://reviews.freebsd.org/D12355
  • Loading branch information
avg-I committed Sep 26, 2017
1 parent b5f772e commit 16b2010
Show file tree
Hide file tree
Showing 12 changed files with 1,383 additions and 292 deletions.
11 changes: 8 additions & 3 deletions cddl/contrib/opensolaris/cmd/ztest/ztest.c
Original file line number Diff line number Diff line change
Expand Up @@ -1825,13 +1825,14 @@ ztest_get_done(zgd_t *zgd, int error)
ztest_object_unlock(zd, object);

if (error == 0 && zgd->zgd_bp)
zil_add_block(zgd->zgd_zilog, zgd->zgd_bp);
zil_lwb_add_block(zgd->zgd_lwb, zgd->zgd_bp);

umem_free(zgd, sizeof (*zgd));
}

static int
ztest_get_data(void *arg, lr_write_t *lr, char *buf, zio_t *zio)
ztest_get_data(void *arg, lr_write_t *lr, char *buf, struct lwb *lwb,
zio_t *zio)
{
ztest_ds_t *zd = arg;
objset_t *os = zd->zd_os;
Expand All @@ -1845,6 +1846,10 @@ ztest_get_data(void *arg, lr_write_t *lr, char *buf, zio_t *zio)
zgd_t *zgd;
int error;

ASSERT3P(lwb, !=, NULL);
ASSERT3P(zio, !=, NULL);
ASSERT3U(size, !=, 0);

ztest_object_lock(zd, object, RL_READER);
error = dmu_bonus_hold(os, object, FTAG, &db);
if (error) {
Expand All @@ -1865,7 +1870,7 @@ ztest_get_data(void *arg, lr_write_t *lr, char *buf, zio_t *zio)
db = NULL;

zgd = umem_zalloc(sizeof (*zgd), UMEM_NOFAIL);
zgd->zgd_zilog = zd->zd_zilog;
zgd->zgd_lwb = lwb;
zgd->zgd_private = zd;

if (buf != NULL) { /* immediate write */
Expand Down
7 changes: 7 additions & 0 deletions sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dmu.c
Original file line number Diff line number Diff line change
Expand Up @@ -1728,6 +1728,13 @@ dmu_sync_late_arrival(zio_t *pio, objset_t *os, dmu_sync_cb_t *done, zgd_t *zgd,
return (SET_ERROR(EIO));
}

/*
* In order to prevent the zgd's lwb from being free'd prior to
* dmu_sync_late_arrival_done() being called, we have to ensure
* the lwb's "max txg" takes this tx's txg into account.
*/
zil_lwb_add_txg(zgd->zgd_lwb, dmu_tx_get_txg(tx));

dsa = kmem_alloc(sizeof (dmu_sync_arg_t), KM_SLEEP);
dsa->dsa_dr = NULL;
dsa->dsa_done = done;
Expand Down
2 changes: 1 addition & 1 deletion sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/dmu.h
Original file line number Diff line number Diff line change
Expand Up @@ -920,7 +920,7 @@ uint64_t dmu_tx_get_txg(dmu_tx_t *tx);
* {zfs,zvol,ztest}_get_done() args
*/
typedef struct zgd {
struct zilog *zgd_zilog;
struct lwb *zgd_lwb;
struct blkptr *zgd_bp;
dmu_buf_t *zgd_db;
struct rl *zgd_rl;
Expand Down
8 changes: 6 additions & 2 deletions sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/zil.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ extern "C" {

struct dsl_pool;
struct dsl_dataset;
struct lwb;

/*
* Intent log format:
Expand Down Expand Up @@ -140,6 +141,7 @@ typedef enum zil_create {
/*
* Intent log transaction types and record structures
*/
#define TX_COMMIT 0 /* Commit marker (no on-disk state) */
#define TX_CREATE 1 /* Create file */
#define TX_MKDIR 2 /* Make directory */
#define TX_MKXATTR 3 /* Make XATTR directory */
Expand Down Expand Up @@ -388,7 +390,8 @@ typedef int zil_parse_blk_func_t(zilog_t *zilog, blkptr_t *bp, void *arg,
typedef int zil_parse_lr_func_t(zilog_t *zilog, lr_t *lr, void *arg,
uint64_t txg);
typedef int zil_replay_func_t();
typedef int zil_get_data_t(void *arg, lr_write_t *lr, char *dbuf, zio_t *zio);
typedef int zil_get_data_t(void *arg, lr_write_t *lr, char *dbuf,
struct lwb *lwb, zio_t *zio);

extern int zil_parse(zilog_t *zilog, zil_parse_blk_func_t *parse_blk_func,
zil_parse_lr_func_t *parse_lr_func, void *arg, uint64_t txg);
Expand Down Expand Up @@ -427,7 +430,8 @@ extern void zil_clean(zilog_t *zilog, uint64_t synced_txg);
extern int zil_suspend(const char *osname, void **cookiep);
extern void zil_resume(void *cookie);

extern void zil_add_block(zilog_t *zilog, const blkptr_t *bp);
extern void zil_lwb_add_block(struct lwb *lwb, const blkptr_t *bp);
extern void zil_lwb_add_txg(struct lwb *lwb, uint64_t txg);
extern int zil_bp_tree_add(zilog_t *zilog, const blkptr_t *bp);

extern void zil_set_sync(zilog_t *zilog, uint64_t syncval);
Expand Down
72 changes: 61 additions & 11 deletions sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/zil_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
*/
/*
* Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012 by Delphix. All rights reserved.
* Copyright (c) 2012, 2017 by Delphix. All rights reserved.
* Copyright (c) 2014 Integros [integros.com]
*/

Expand All @@ -37,21 +37,75 @@ extern "C" {
#endif

/*
* Log write buffer.
* Possbile states for a given lwb structure. An lwb will start out in
* the "closed" state, and then transition to the "opened" state via a
* call to zil_lwb_write_open(). After the lwb is "open", it can
* transition into the "issued" state via zil_lwb_write_issue(). After
* the lwb's zio completes, and the vdev's are flushed, the lwb will
* transition into the "done" state via zil_lwb_write_done(), and the
* structure eventually freed.
*/
typedef enum {
LWB_STATE_CLOSED,
LWB_STATE_OPENED,
LWB_STATE_ISSUED,
LWB_STATE_DONE,
LWB_NUM_STATES
} lwb_state_t;

/*
* Log write block (lwb)
*
* Prior to an lwb being issued to disk via zil_lwb_write_issue(), it
* will be protected by the zilog's "zl_writer_lock". Basically, prior
* to it being issued, it will only be accessed by the thread that's
* holding the "zl_writer_lock". After the lwb is issued, the zilog's
* "zl_lock" is used to protect the lwb against concurrent access.
*/
typedef struct lwb {
zilog_t *lwb_zilog; /* back pointer to log struct */
blkptr_t lwb_blk; /* on disk address of this log blk */
boolean_t lwb_slog; /* lwb_blk is on SLOG device */
int lwb_nused; /* # used bytes in buffer */
int lwb_sz; /* size of block and buffer */
lwb_state_t lwb_state; /* the state of this lwb */
char *lwb_buf; /* log write buffer */
zio_t *lwb_zio; /* zio for this buffer */
zio_t *lwb_write_zio; /* zio for the lwb buffer */
zio_t *lwb_root_zio; /* root zio for lwb write and flushes */
dmu_tx_t *lwb_tx; /* tx for log block allocation */
uint64_t lwb_max_txg; /* highest txg in this lwb */
list_node_t lwb_node; /* zilog->zl_lwb_list linkage */
list_t lwb_waiters; /* list of zil_commit_waiter's */
avl_tree_t lwb_vdev_tree; /* vdevs to flush after lwb write */
kmutex_t lwb_vdev_lock; /* protects lwb_vdev_tree */
hrtime_t lwb_issued_timestamp; /* when was the lwb issued? */
} lwb_t;

/*
* ZIL commit waiter.
*
* This structure is allocated each time zil_commit() is called, and is
* used by zil_commit() to communicate with other parts of the ZIL, such
* that zil_commit() can know when it safe for it return. For more
* details, see the comment above zil_commit().
*
* The "zcw_lock" field is used to protect the commit waiter against
* concurrent access. This lock is often acquired while already holding
* the zilog's "zl_writer_lock" or "zl_lock"; see the functions
* zil_process_commit_list() and zil_lwb_flush_vdevs_done() as examples
* of this. Thus, one must be careful not to acquire the
* "zl_writer_lock" or "zl_lock" when already holding the "zcw_lock";
* e.g. see the zil_commit_waiter_timeout() function.
*/
typedef struct zil_commit_waiter {
kcondvar_t zcw_cv; /* signalled when "done" */
kmutex_t zcw_lock; /* protects fields of this struct */
list_node_t zcw_node; /* linkage in lwb_t:lwb_waiter list */
lwb_t *zcw_lwb; /* back pointer to lwb when linked */
boolean_t zcw_done; /* B_TRUE when "done", else B_FALSE */
int zcw_zio_error; /* contains the zio io_error value */
} zil_commit_waiter_t;

/*
* Intent log transaction lists
*/
Expand Down Expand Up @@ -94,43 +148,39 @@ struct zilog {
const zil_header_t *zl_header; /* log header buffer */
objset_t *zl_os; /* object set we're logging */
zil_get_data_t *zl_get_data; /* callback to get object content */
zio_t *zl_root_zio; /* log writer root zio */
lwb_t *zl_last_lwb_opened; /* most recent lwb opened */
hrtime_t zl_last_lwb_latency; /* zio latency of last lwb done */
uint64_t zl_lr_seq; /* on-disk log record sequence number */
uint64_t zl_commit_lr_seq; /* last committed on-disk lr seq */
uint64_t zl_destroy_txg; /* txg of last zil_destroy() */
uint64_t zl_replayed_seq[TXG_SIZE]; /* last replayed rec seq */
uint64_t zl_replaying_seq; /* current replay seq number */
uint32_t zl_suspend; /* log suspend count */
kcondvar_t zl_cv_writer; /* log writer thread completion */
kcondvar_t zl_cv_suspend; /* log suspend completion */
uint8_t zl_suspending; /* log is currently suspending */
uint8_t zl_keep_first; /* keep first log block in destroy */
uint8_t zl_replay; /* replaying records while set */
uint8_t zl_stop_sync; /* for debugging */
uint8_t zl_writer; /* boolean: write setup in progress */
kmutex_t zl_writer_lock; /* single writer, per ZIL, at a time */
uint8_t zl_logbias; /* latency or throughput */
uint8_t zl_sync; /* synchronous or asynchronous */
int zl_parse_error; /* last zil_parse() error */
uint64_t zl_parse_blk_seq; /* highest blk seq on last parse */
uint64_t zl_parse_lr_seq; /* highest lr seq on last parse */
uint64_t zl_parse_blk_count; /* number of blocks parsed */
uint64_t zl_parse_lr_count; /* number of log records parsed */
uint64_t zl_next_batch; /* next batch number */
uint64_t zl_com_batch; /* committed batch number */
kcondvar_t zl_cv_batch[2]; /* batch condition variables */
itxg_t zl_itxg[TXG_SIZE]; /* intent log txg chains */
list_t zl_itx_commit_list; /* itx list to be committed */
uint64_t zl_cur_used; /* current commit log size used */
list_t zl_lwb_list; /* in-flight log write list */
kmutex_t zl_vdev_lock; /* protects zl_vdev_tree */
avl_tree_t zl_vdev_tree; /* vdevs to flush in zil_commit() */
avl_tree_t zl_bp_tree; /* track bps during log parse */
clock_t zl_replay_time; /* lbolt of when replay started */
uint64_t zl_replay_blks; /* number of log blocks replayed */
zil_header_t zl_old_header; /* debugging aid */
uint_t zl_prev_blks[ZIL_PREV_BLKS]; /* size - sector rounded */
uint_t zl_prev_rotor; /* rotor for zl_prev[] */
txg_node_t zl_dirty_link; /* protected by dp_dirty_zilogs list */
uint64_t zl_dirty_max_txg; /* highest txg used to dirty zilog */
};

typedef struct zil_bp_node {
Expand Down
1 change: 1 addition & 0 deletions sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/zio.h
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,7 @@ extern enum zio_checksum zio_checksum_dedup_select(spa_t *spa,
extern enum zio_compress zio_compress_select(spa_t *spa,
enum zio_compress child, enum zio_compress parent);

extern void zio_cancel(zio_t *zio);
extern void zio_suspend(spa_t *spa, zio_t *zio);
extern int zio_resume(spa_t *spa);
extern void zio_resume_wait(spa_t *spa);
Expand Down
12 changes: 6 additions & 6 deletions sys/cddl/contrib/opensolaris/uts/common/fs/zfs/txg.c
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ txg_fini(dsl_pool_t *dp)
tx_state_t *tx = &dp->dp_tx;
int c;

ASSERT(tx->tx_threads == 0);
ASSERT0(tx->tx_threads);

mutex_destroy(&tx->tx_sync_lock);

Expand Down Expand Up @@ -204,7 +204,7 @@ txg_sync_start(dsl_pool_t *dp)

dprintf("pool %p\n", dp);

ASSERT(tx->tx_threads == 0);
ASSERT0(tx->tx_threads);

tx->tx_threads = 2;

Expand Down Expand Up @@ -265,7 +265,7 @@ txg_sync_stop(dsl_pool_t *dp)
/*
* Finish off any work in progress.
*/
ASSERT(tx->tx_threads == 2);
ASSERT3U(tx->tx_threads, ==, 2);

/*
* We need to ensure that we've vacated the deferred space_maps.
Expand All @@ -277,7 +277,7 @@ txg_sync_stop(dsl_pool_t *dp)
*/
mutex_enter(&tx->tx_sync_lock);

ASSERT(tx->tx_threads == 2);
ASSERT3U(tx->tx_threads, ==, 2);

tx->tx_exiting = 1;

Expand Down Expand Up @@ -616,7 +616,7 @@ txg_wait_synced(dsl_pool_t *dp, uint64_t txg)
ASSERT(!dsl_pool_config_held(dp));

mutex_enter(&tx->tx_sync_lock);
ASSERT(tx->tx_threads == 2);
ASSERT3U(tx->tx_threads, ==, 2);
if (txg == 0)
txg = tx->tx_open_txg + TXG_DEFER_SIZE;
if (tx->tx_sync_txg_waiting < txg)
Expand All @@ -641,7 +641,7 @@ txg_wait_open(dsl_pool_t *dp, uint64_t txg)
ASSERT(!dsl_pool_config_held(dp));

mutex_enter(&tx->tx_sync_lock);
ASSERT(tx->tx_threads == 2);
ASSERT3U(tx->tx_threads, ==, 2);
if (txg == 0)
txg = tx->tx_open_txg + 1;
if (tx->tx_quiesce_txg_waiting < txg)
Expand Down
12 changes: 7 additions & 5 deletions sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
#include <sys/acl.h>
#include <sys/vmmeter.h>
#include <vm/vm_param.h>
#include <sys/zil.h>

/*
* Programming rules.
Expand Down Expand Up @@ -1276,7 +1277,7 @@ zfs_get_done(zgd_t *zgd, int error)
VN_RELE_ASYNC(ZTOV(zp), dsl_pool_vnrele_taskq(dmu_objset_pool(os)));

if (error == 0 && zgd->zgd_bp)
zil_add_block(zgd->zgd_zilog, zgd->zgd_bp);
zil_lwb_add_block(zgd->zgd_lwb, zgd->zgd_bp);

kmem_free(zgd, sizeof (zgd_t));
}
Expand All @@ -1289,7 +1290,7 @@ static int zil_fault_io = 0;
* Get data to generate a TX_WRITE intent log record.
*/
int
zfs_get_data(void *arg, lr_write_t *lr, char *buf, zio_t *zio)
zfs_get_data(void *arg, lr_write_t *lr, char *buf, struct lwb *lwb, zio_t *zio)
{
zfsvfs_t *zfsvfs = arg;
objset_t *os = zfsvfs->z_os;
Expand All @@ -1301,8 +1302,9 @@ zfs_get_data(void *arg, lr_write_t *lr, char *buf, zio_t *zio)
zgd_t *zgd;
int error = 0;

ASSERT(zio != NULL);
ASSERT(size != 0);
ASSERT3P(lwb, !=, NULL);
ASSERT3P(zio, !=, NULL);
ASSERT3U(size, !=, 0);

/*
* Nothing to do if the file has been removed
Expand All @@ -1320,7 +1322,7 @@ zfs_get_data(void *arg, lr_write_t *lr, char *buf, zio_t *zio)
}

zgd = (zgd_t *)kmem_zalloc(sizeof (zgd_t), KM_SLEEP);
zgd->zgd_zilog = zfsvfs->z_log;
zgd->zgd_lwb = lwb;
zgd->zgd_private = zp;

/*
Expand Down
Loading

0 comments on commit 16b2010

Please sign in to comment.