On Wed, Mar 10, 2021 at 05:34:52PM -0800, Darrick J. Wong wrote: > On Fri, Mar 05, 2021 at 04:11:39PM +1100, Dave Chinner wrote: > > From: Dave Chinner <dchinner@xxxxxxxxxx> > > > > Adding a list_sort() call to the CIL push work while the xc_ctx_lock > > is held exclusively has resulted in fairly long lock hold times and > > that stops all front end transaction commits from making progress. > > Heh, nice solution. :) > > > We can move the sorting out of the xc_ctx_lock if we can transfer > > the ordering information to the log vectors as they are detached > > from the log items and then we can sort the log vectors. This > > requires log vectors to use a list_head rather than a single linked > > list > > Ergh, could pull out the list conversion into a separate piece? > Some of the lv_chain usage is ... not entirely textbook. Yes, I can probably do that. > > diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h > > index af54ea3f8c90..0445dd6acbce 100644 > > --- a/fs/xfs/xfs_log.h > > +++ b/fs/xfs/xfs_log.h > > @@ -9,7 +9,8 @@ > > struct xfs_cil_ctx; > > > > struct xfs_log_vec { > > - struct xfs_log_vec *lv_next; /* next lv in build list */ > > + struct list_head lv_chain; /* lv chain ptrs */ > > + int lv_order_id; /* chain ordering info */ > > uint32_t to match li_order_id? *nod* > > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c > > index 3d43a5088154..6dcc23829bef 100644 > > --- a/fs/xfs/xfs_log_cil.c > > +++ b/fs/xfs/xfs_log_cil.c > > @@ -72,6 +72,7 @@ xlog_cil_ctx_alloc(void) > > ctx = kmem_zalloc(sizeof(*ctx), KM_NOFS); > > INIT_LIST_HEAD(&ctx->committing); > > INIT_LIST_HEAD(&ctx->busy_extents); > > + INIT_LIST_HEAD(&ctx->lv_chain); > > INIT_WORK(&ctx->push_work, xlog_cil_push_work); > > return ctx; > > } > > @@ -237,6 +238,7 @@ xlog_cil_alloc_shadow_bufs( > > lv = kmem_alloc_large(buf_size, KM_NOFS); > > memset(lv, 0, xlog_cil_iovec_space(niovecs)); > > > > + INIT_LIST_HEAD(&lv->lv_chain); > > lv->lv_item = lip; > > lv->lv_size = buf_size; > > if (ordered) > > @@ -252,7 +254,6 @@ xlog_cil_alloc_shadow_bufs( > > else > > lv->lv_buf_len = 0; > > lv->lv_bytes = 0; > > - lv->lv_next = NULL; > > } > > > > /* Ensure the lv is set up according to ->iop_size */ > > @@ -379,8 +380,6 @@ xlog_cil_insert_format_items( > > if (lip->li_lv && shadow->lv_size <= lip->li_lv->lv_size) { > > /* same or smaller, optimise common overwrite case */ > > lv = lip->li_lv; > > - lv->lv_next = NULL; > > What /did/ these null assignments do? IIRC, at one point they ensured that the lv chain was correctly terminated when a lv was reused and added to the tail of an existing chain. I think that became redundant when we added the shadow buffers to allow allocation outside the CIL lock contexts... > > - list_del_init(&item->li_cil); > > - item->li_order_id = 0; > > - if (!ctx->lv_chain) > > - ctx->lv_chain = item->li_lv; > > - else > > - lv->lv_next = item->li_lv; > > + > > lv = item->li_lv; > > - item->li_lv = NULL; > > + lv->lv_order_id = item->li_order_id; > > num_iovecs += lv->lv_niovecs; > > - > > /* we don't write ordered log vectors */ > > if (lv->lv_buf_len != XFS_LOG_VEC_ORDERED) > > num_bytes += lv->lv_bytes; > > + list_add_tail(&lv->lv_chain, &ctx->lv_chain); > > + > > + list_del_init(&item->li_cil); > > Do the list manipulations need moving, or could they have stayed further > up in the loop body for a cleaner patch? I moved them so the code was structured as: <transfer item state to log vec> <manipulate lists> <clear item state> Because there was no clear separation between state and list manipulations. This will clean up if I separate the list manipulations into their own patch... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx