On Mon, Jan 04, 2021 at 11:23:53AM -0500, Brian Foster wrote: > On Thu, Dec 31, 2020 at 09:16:11AM +1100, Dave Chinner wrote: > > On Wed, Dec 30, 2020 at 12:56:27AM +0100, Donald Buczek wrote: > > > If the value goes below the limit while some threads are > > > already waiting but before the push worker gets to it, these threads are > > > not woken. > > > > > > Always wake all CIL push waiters. Test with waitqueue_active() as an > > > optimization. This is possible, because we hold the xc_push_lock > > > spinlock, which prevents additions to the waitqueue. > > > > > > Signed-off-by: Donald Buczek <buczek@xxxxxxxxxxxxx> > > > --- > > > fs/xfs/xfs_log_cil.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c > > > index b0ef071b3cb5..d620de8e217c 100644 > > > --- a/fs/xfs/xfs_log_cil.c > > > +++ b/fs/xfs/xfs_log_cil.c > > > @@ -670,7 +670,7 @@ xlog_cil_push_work( > > > /* > > > * Wake up any background push waiters now this context is being pushed. > > > */ > > > - if (ctx->space_used >= XLOG_CIL_BLOCKING_SPACE_LIMIT(log)) > > > + if (waitqueue_active(&cil->xc_push_wait)) > > > wake_up_all(&cil->xc_push_wait); > > > > That just smells wrong to me. It *might* be correct, but this > > condition should pair with the sleep condition, as space used by a > > CIL context should never actually decrease.... > > > > ... but I'm a little confused by this assertion. The shadow buffer > allocation code refers to the possibility of shadow buffers falling out > that are smaller than currently allocated buffers. Further, the > _insert_format_items() code appears to explicitly optimize for this > possibility by reusing the active buffer, subtracting the old size/count > values from the diff variables and then reformatting the latest > (presumably smaller) item to the lv. Individual items might shrink, but the overall transaction should grow. Think of a extent to btree conversion of an inode fork. THe data in the inode fork decreases from a list of extents to a btree root block pointer, so the inode item shrinks. But then we add a new btree root block that contains all the extents + the btree block header, and it gets rounded up to ithe 128 byte buffer logging chunk size. IOWs, while the inode item has decreased in size, the overall space consumed by the transaction has gone up and so the CIL ctx used_space should increase. Hence we can't just look at individual log items and whether they have decreased in size - we have to look at all the items in the transaction to understand how the space used in that transaction has changed. i.e. it's the aggregation of all items in the transaction that matter here, not so much the individual items. > Of course this could just be implementation detail. I haven't dug into > the details in the remainder of this thread and I don't have specific > examples off the top of my head, but perhaps based on the ability of > various structures to change formats and the ability of log vectors to > shrink in size, shouldn't we expect the possibility of a CIL context to > shrink in size as well? Just from poking around the CIL it seems like > the surrounding code supports it (xlog_cil_insert_items() checks len > 0 > for recalculating split res as well)... Yes, there may be situations where it decreases. It may be this is fine, but the assumption *I've made* in lots of the CIL push code is that ctx->used_space rarely, if ever, will go backwards. e.g. we run the first transaction into the CIL, it steals the sapce needed for the cil checkpoint headers for the transaciton. Then if the space returned by the item formatting is negative (because it is in the AIL and being relogged), the CIL checkpoint now doesn't have the space reserved it needs to run a checkpoint. That transaction is a sync transaction, so it forces the log, and now we push the CIL without sufficient reservation to write out the log headers and the items we just formatted.... So, yeah, shrinking transaction space usage definitely violates some of the assumptions the code makes about how relogging works. It's entirely possible the assumptions I've made are not entirely correct in some corner cases - those particular cases are what we need to ferret out here, and then decide if they are correct or not and deal with it from there... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
