From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ipmail06.adl6.internode.on.net ([150.101.137.145]:65298 "EHLO ipmail06.adl6.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965783AbeBMXK3 (ORCPT ); Tue, 13 Feb 2018 18:10:29 -0500 Date: Wed, 14 Feb 2018 09:02:20 +1100 From: Dave Chinner Subject: Re: [PATCH v2] xfs: byte range buffer dirty region tracking Message-ID: <20180213220220.GF6778@dastard> References: <20180201010514.30233-1-david@fromorbit.com> <20180205003415.dn6elcqb4kae3xle@destitution> <20180206162141.GA3862@bfoster.bfoster> <20180212024138.GB6778@dastard> <20180212142619.GA33694@bfoster.bfoster> <20180212211824.GC6778@dastard> <20180213131525.GA38210@bfoster.bfoster> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180213131525.GA38210@bfoster.bfoster> Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: Brian Foster Cc: linux-xfs@vger.kernel.org On Tue, Feb 13, 2018 at 08:15:26AM -0500, Brian Foster wrote: > On Tue, Feb 13, 2018 at 08:18:24AM +1100, Dave Chinner wrote: > > On Mon, Feb 12, 2018 at 09:26:19AM -0500, Brian Foster wrote: > > > :/ So it seems to > > > me this breaks a technically valid case in weird/subtle ways. For > > > example, why assert about last == 0, but then go on to add the range > > > anyways, explicitly not size it correctly, but then format it as if > > > nothing is wrong? If it were really wrong/invalid (which I don't think > > > it is), why not put the check in the log side and skip adding the range > > > rather than add it, skip sizing it, and then format it. > > > > So what you're really concerned about is that I put asserts into the > > code to catch broken development code, but then allow production > > systems through without caring whether it works correctly because > > that boundary condition will never occur during runtime on > > production systems? > > No. As already mentioned in my previous mail, I care little about the > asserts. Asserts can easily be removed if they turn out to be bogus. > Wrong asserts tend to have little negative effect on production users > because along with only affecting debug kernels, they'd have to be > fairly rare to slip through our testing. So I'm perfectly _happy_ to be > cautious with regard to asserts. > > What I care much more about is not leaving latent bugs around in the > code. IMO, there is very rarely good enough justification to knowingly > commit buggy/fragile code to the kernel, Hold on a minute! I'm not asking anyone to commit buggy or fragile code. I've already fixed the off-by-one problems you've pointed out, and all I was trying to do was understand what you saw wrong with the asserts to catch a "should never happen" condition so I could change it in a way that you'd find acceptible. There's no need to shout and rant at me.... > ... having said all that and having already wasted more time on this > than it would have taken for you to just fix the patch, I'll end my rant > with this splat[1]. It demonstrates the "boundary condition" that "will > never occur during runtime on production systems" (production system > level output included for extra fun ;P). This is a pre-existing bug in xlog_cil_insert_format_items() that my change has exposed: /* Skip items that do not have any vectors for writing */ if (!shadow->lv_niovecs && !ordered) continue; The code I added triggers this (niovecs == 0), and that now gives us the case where we have a dirty log item descriptor (XFS_LID_DIRTY) without a log vector attached to item->li_lv. Then in xlog_cil_insert_items(): /* Skip items which aren't dirty in this transaction. */ if (!(lidp->lid_flags & XFS_LID_DIRTY)) continue; /* * Only move the item if it isn't already at the tail. This is * to prevent a transient list_empty() state when reinserting * an item that is already the only item in the CIL. */ if (!list_is_last(&lip->li_cil, &cil->xc_cil)) list_move_tail(&lip->li_cil, &cil->xc_cil); We put that "clean" log item on the CIL because XFS_LID_DIRTY is set, and then when we push the CIL in xlog_cil_push(), we trip over a dirty log item without a log vector when chaining log vectors to pass to the log writing code here: while (!list_empty(&cil->xc_cil)) { struct xfs_log_item *item; item = list_first_entry(&cil->xc_cil, struct xfs_log_item, li_cil); list_del_init(&item->li_cil); 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; num_iovecs += lv->lv_niovecs; } i.e. lv ends up null part way through the log item chain we are processing and the next loop iteration fails. IOWs, the bug isn't in the patch I wrote - it has uncovered a latent bug added years ago for a condition that had never, ever been exercised until now. Brian, can you now give me all the details of what you were doing to produce this and turn on CONFIG_XFS_DEBUG so that it catches the zero length buffer that was logged when it happens? That way I can test a fix for this bug and that the buffer range logging exercises this case properly... Cheers, Dave. -- Dave Chinner david@fromorbit.com