From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from aserp1050.oracle.com ([141.146.126.70]:30106 "EHLO aserp1050.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755109AbcHBX2h (ORCPT ); Tue, 2 Aug 2016 19:28:37 -0400 Received: from aserp1040.oracle.com (aserp1040.oracle.com [141.146.126.69]) by aserp1050.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id u72KbUXT003744 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Tue, 2 Aug 2016 20:37:30 GMT Date: Tue, 2 Aug 2016 13:35:11 -0700 From: "Darrick J. Wong" To: Christoph Hellwig Cc: david@fromorbit.com, linux-fsdevel@vger.kernel.org, vishal.l.verma@intel.com, bfoster@redhat.com, xfs@oss.sgi.com Subject: Re: [PATCH 18/47] xfs: refactor redo intent item processing Message-ID: <20160802203511.GL8590@birch.djwong.org> References: <146907695530.25461.3225785294902719773.stgit@birch.djwong.org> <146907708421.25461.405239727630066080.stgit@birch.djwong.org> <20160801081023.GC30547@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160801081023.GC30547@infradead.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, Aug 01, 2016 at 01:10:23AM -0700, Christoph Hellwig wrote: > > +int > > +xfs_efi_recover( > > + struct xfs_mount *mp, > > + struct xfs_efi_log_item *efip) > > +{ > > + struct xfs_efd_log_item *efdp; > > + struct xfs_trans *tp; > > + int i; > > + int error = 0; > > + xfs_extent_t *extp; > > + xfs_fsblock_t startblock_fsb; > > + > > + ASSERT(!test_bit(XFS_EFI_RECOVERED, &efip->efi_flags)); > > + > > + /* > > + * First check the validity of the extents described by the > > + * EFI. If any are bad, then assume that all are bad and > > + * just toss the EFI. > > + */ > > + for (i = 0; i < efip->efi_format.efi_nextents; i++) { > > + extp = &(efip->efi_format.efi_extents[i]); > > + startblock_fsb = XFS_BB_TO_FSB(mp, > > + XFS_FSB_TO_DADDR(mp, extp->ext_start)); > > + if ((startblock_fsb == 0) || > > + (extp->ext_len == 0) || > > + (startblock_fsb >= mp->m_sb.sb_dblocks) || > > + (extp->ext_len >= mp->m_sb.sb_agblocks)) { > > + /* > > + * This will pull the EFI from the AIL and > > + * free the memory associated with it. > > + */ > > + set_bit(XFS_EFI_RECOVERED, &efip->efi_flags); > > + xfs_efi_release(efip); > > + return -EIO; > > + } > > + } > > I know it's just a code move, but there are lots of superflous braces > here, please remove them. Ok. It took me a while to figure out that you were referring to the parentheses on the if tests and not the curly braces around the code blocks. > > + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp); > > + if (error) > > + return error; > > + efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents); > > + > > + for (i = 0; i < efip->efi_format.efi_nextents; i++) { > > + extp = &(efip->efi_format.efi_extents[i]); > > and here.. > > > + ASSERT(XFS_LSN_CMP(last_lsn, lip->li_lsn) >= 0); > > This new check seems useful, but nothing in the changelog mentions > why it has been added. > > Otherwise this looks fine to me. I'm adding the following to the commit message: "Furthermore, ensure that log recovery only replays the redo items that were in the AIL prior to recovery by checking the item LSN against the largest LSN seen during log scanning. As written this should never happen, but we can be defensive anyway." --D